Skip to content

refactor(push): extract reconcileStateKeyForResource — fold two ensure-fns into one generic helper - #36

Merged
dhruva-vapi merged 1 commit into
mainfrom
engine-consolidation/02-reconcile-statekey
May 15, 2026
Merged

dhruva-vapi merged 1 commit into
mainfrom
engine-consolidation/02-reconcile-statekey

Conversation

@dhruva-vapi

Copy link
Copy Markdown
Contributor

Summary

Tier 2 of 2 in the engine-consolidation series. Stacks on #33 (slug-utils).

`ensureToolExists` (`push.ts:936-1029`) and `ensureStructuredOutputExists` (`push.ts:1031-1124`) were structurally identical 94-line functions. Both implemented the same dedup-then-apply flow: look up existing dashboard/state match by canonical name, adopt with the orphan-deletion guard, mark `touched` for `mergeScoped`, call the apply function.

They differed only in:

  • Resource type label (`tools` / `structuredOutputs`)
  • Apply function (`applyTool` / `applyStructuredOutput`)
  • State section (`state.tools` / `state.structuredOutputs`)
  • Remote-list cache (`existingRemoteTools` / `existingRemoteStructuredOutputs`)
  • Per-type bookkeeping array (`autoAppliedTools` / `autoAppliedStructuredOutputs`)
  • Ambiguous-warning singular/plural label

All five differences parameterize cleanly. `reconcileStateKeyForResource` is the generic helper; both ensure-functions become ~14-line wrappers.

Behavior preservation is the contract

This refactor is behavior-equivalent. Verified path-by-path against pre-refactor (code-reviewer ran the full diff):

Path Verdict
Ambiguous warning string Byte-identical (`Multiple ${plural} share the name "${name}" — adopting ${uuid}...`)
Reuse log line Byte-identical
Auto-apply log line Byte-identical
`autoApplied` key format (`${resourceType}:${resourceId}`) Identical
Orphan-deletion scope (adopted UUID only; `duplicateUuids` left for `npm run cleanup`) Preserved
`autoApplied.add` BEFORE `if (!uuid) return` Preserved — critical for dry-run / drift-halt semantics
`applied[type]++` / `pushToAutoAppliedList` / `touched.add` AFTER null check Preserved
try/catch boundary Identical
Match-path `upsertState` BEFORE orphan-deletion Preserved

`tests/push-dry-run.test.ts` passes unchanged — the integration test pins the behavior contract end-to-end.

API discipline

Both `vapiEnv` and `formatError` are required parameters (not optional with placeholder defaults). Reason: a future caller (e.g. a sims auto-apply path) must not accidentally:

  • Emit `npm run cleanup -- ` verbatim to a confused operator, or
  • Render a degraded error message that loses the VapiApiError 3-line breakout

Cost: production call sites pass `VAPI_ENV` and `formatApiError` explicitly. Tests pass `"test-env"` and an inline formatter.

Files changed

File Change LOC
`src/reconcile-state-key.ts` NEW (generic helper) +205
`tests/reconcile-state-key.test.ts` NEW (16 cases — 8 scenarios × 2 resource types) +444
`src/push.ts` Two 94-line ensure-fns → two ~14-line wrappers -129

Net: +674 / -154 → +520. Production code shrinks: source-only net is +76 / -154 = -78 lines while collapsing two functions into one.

What the 16 tests pin down

Each runs for BOTH `resourceType: "tools"` and `resourceType: "structuredOutputs"`:

  1. State hit, same UUID, redundant alias → drops alias, marks deletion touched, calls applyFn
  2. State hit, different UUID under base-slug-match key → adopts UUID, rekeys, deletes suffixed entry
  3. Dashboard hit only → reconciles to dashboard UUID via name match
  4. Ambiguous dashboard match → logs warning, picks lex-smallest, surfaces `duplicateUuids`
  5. No match → pure create path (state populated, `applied` incremented, `autoApplied` set, callback fired)
  6. Idempotency → second call short-circuits via `autoApplied`
  7. `applyFn` returns null → state NOT updated, `applied` does NOT increment, `autoApplied` STILL records the key (preserves dry-run semantics)
  8. Orphan-deletion guard scope → only adopted-UUID keys dropped; `duplicateUuids` keys preserved

Test plan

  • `npm run build` (tsc --noEmit) — clean
  • `npm test` — 244/244 pass (228 prior + 16 new)
  • `npx @biomejs/biome check --write` — clean
  • `tests/push-dry-run.test.ts` passes UNCHANGED (the contract pin)

Code review

In-branch review by code-reviewer subagent. No blocking findings. Behavior contract preserved path-by-path (verified by reading pre-refactor against the new helper).

Two LOW findings addressed in-branch (`vapiEnv` and `formatError` upgraded from optional-with-default to required, per L1+L2).

One non-blocking residual: `ensureAssistantDepsExist` was NOT consolidated into this helper. Reason is sound — assistant adoption has additional steps (squad-ref propagation, deps-first-pass coordination) that the generic helper doesn't model. Tier 3 candidate, not blocking this PR.

Stack

Reviewable alone given the byte-equivalent behavior contract. Merging this without merging #33 is technically possible (no direct slug-utils import) but the recommended order is base-first.

Related


Note: this PR supersedes #34, which was auto-closed by GitHub when its base branch (the stack parent) was deleted on merge.

…ne ensure-fns into one generic helper

`ensureToolExists` and `ensureStructuredOutputExists` were structurally
identical 94-line functions differing only in: resource type label,
apply function, state section, remote-list cache, and the per-type
bookkeeping array. Both implemented the same dedup-then-apply flow:
look up existing dashboard/state match by canonical name, adopt with
orphan-deletion guard, mark `touched` for `mergeScoped`, call apply.

Behavioral contract preserved exactly:
  - Log strings byte-identical (verified path-by-path against
    pre-refactor)
  - `autoApplied.add` BEFORE the `if (!uuid) return` early-exit
  - `applied[type]++`, `pushToAutoAppliedList`, `touched.add` AFTER
    the null check — preserves dry-run / drift-halt semantics
  - Orphan-deletion guard scope unchanged: deletes state keys
    pointing at the adopted UUID, leaves `duplicateUuids` alone for
    `npm run cleanup` to handle
  - try/catch boundary identical
  - All 228 prior tests pass unchanged, including the integration
    test in tests/push-dry-run.test.ts

push.ts shrunk -129 net LOC (two 94-line functions collapsed to
~14-line wrappers). Helper is `tools | structuredOutputs` narrow
today; adding a future type requires a deliberate union widening
+ LABELS map entry, not a config flag.

`vapiEnv` and `formatError` parameters are required (not optional
with placeholder defaults) so a future caller can't accidentally
emit a degraded warning or error message.

Tests: 244/244 pass (228 prior + 16 new — 8 scenarios × 2 resource
types covering happy path, ambiguous match, null applyFn ordering
contract, orphan-deletion guard scope, run-scoped idempotency,
state-hit/dashboard-hit/no-match branches).

Closes the symptom-fix pattern documented in improvements.md #10
(now handled by the generic helper instead of the two hardcoded
functions).
@dhruva-vapi
dhruva-vapi merged commit fd8c23e into main May 15, 2026
scott-lowe-vapi added a commit that referenced this pull request Oct 3, 2026
AGENTS.md was 55 KB. Codex reads it only up to project_doc_max_bytes
(32 KiB by default), so it silently lost everything after the tools
section. It also taught agents things that break: UUIDs in assistant_ids,
scenario judges and credentialId (they only work in one org and break
promotion), a deprecated endCallFunctionEnabled, a cleanup command that
is refused without --confirm, and that every command is interactive.

- AGENTS.md is rewritten as an 19 KB core: which repository you're in
  (template vs customer deployment, so the changelog rule reaches every
  agent), safety rules (ask before changing a live org or deleting, never
  handle keys, reference by name, the PATCH-replaces-nested-objects rule
  that only Claude saw before, .ts executes, don't bypass safety checks),
  setup without a terminal, the change loop, a corrected quick reference
  and reference table, naming and renames, simulations and PR checks,
  promotion, learnings routing, and engine conventions.
- Reference material moves to docs/guides/: resource-reference.md (every
  setting, with the UUID and deprecation fixes), writing-prompts.md, sync
  internals into how-it-works.md, test-call output into commands.md.
- CLAUDE.md imports AGENTS.md instead of copying it; the Cursor changelog
  rule folds into AGENTS.md; the learnings rule matches the two indexes.
- tests/agent-docs.test.ts keeps AGENTS.md under 30 KB, CLAUDE.md
  importing it, every learnings file routed, and no UUIDs in agent-facing
  examples.
- Found while checking: npm run cleanup doesn't read .vapi-ignore, so a
  destructive cleanup deletes resources a team excluded. Documented in
  AGENTS.md and the workflows guide; logged as improvements.md #36.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant