refactor(push): extract reconcileStateKeyForResource — fold two ensure-fns into one generic helper - #36
Merged
Conversation
…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).
4 tasks done
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
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):
`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:
Cost: production call sites pass `VAPI_ENV` and `formatApiError` explicitly. Tests pass `"test-env"` and an inline formatter.
Files changed
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"`:
Test plan
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.