Skip to content

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

Merged
dhruva-vapi merged 2 commits into
engine-consolidation/01-shared-utilsfrom
engine-consolidation/02-reconcile-statekey
May 15, 2026
Merged

dhruva-vapi merged 2 commits into
engine-consolidation/01-shared-utilsfrom
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

@dhruva-vapi
dhruva-vapi force-pushed the engine-consolidation/01-shared-utils branch from 64e75ec to eae50eb Compare May 15, 2026 03:19
#35)

Consolidates 4+ duplicated helpers that had accumulated across the
gitops engine as symptom-fixes piled up. Pure factoring, zero
behavior change at every reachable call site.

Duplications collapsed:
  - `slugify` — 4 byte-identical copies (pull.ts, dep-dedup.ts,
    audit.ts, setup.ts) → 1 in src/slug-utils.ts
  - `extractBaseSlug` — 2 byte-identical copies → 1
  - `FOLDER_MAP` — 2 byte-identical copies (pull.ts, resources.ts) → 1
  - `UUID_SUFFIX_RE` — open-coded in 3 places → 1 constant
  - recanonicalize's inlined precondition-2 check (UUID prefix match)
    → extracted as `isEngineSuffixedSlug`

src/slug-utils.ts is config-free by design (no `./config.ts` import,
no side effects at load) so it's safely importable from any test
without priming process.argv / VAPI_TOKEN. This is the testability
property the prior dep-dedup.ts comment claimed for its local
duplicates but didn't actually enforce.

Regex tightening: shared `UUID_SUFFIX_RE` uses `^(.+)-([0-9a-f]{8})$`
(non-empty base) where the prior pull/dep-dedup copies used
`^(.*)-...` (allowed empty base). Strict improvement — engine-
generated keys always have a non-empty base, and the only input
class affected is the synthetic `-<8hex>` shape which is never
produced by `generateResourceId`. Pinned by a regression test in
tests/slug-utils.test.ts.

Back-compat: dep-dedup.ts re-exports slugify/extractBaseSlug so
existing tests importing them via that path keep working.

Tests: 228/228 pass (208 prior + 20 new slug-utils cases covering
slugify behavior, UUID_SUFFIX_RE boundaries, extractBaseSlug loose
form, isEngineSuffixedSlug strict form).
…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

Copy link
Copy Markdown
Contributor Author

Superseded by #36 (merged 2026-05-15). GitHub auto-closed this PR when the stack-parent base branch was deleted on #35's merge. Same diff, retargeted at main.

@dhruva-vapi
dhruva-vapi merged commit 41c47c8 into engine-consolidation/01-shared-utils May 15, 2026
mhar-andal pushed a commit that referenced this pull request Jun 3, 2026
…e-fns into one generic helper (#34) [skip ci]

* refactor(engine): extract shared slug + folder helpers into slug-utils (#35)

Consolidates 4+ duplicated helpers that had accumulated across the
gitops engine as symptom-fixes piled up. Pure factoring, zero
behavior change at every reachable call site.

Duplications collapsed:
  - `slugify` — 4 byte-identical copies (pull.ts, dep-dedup.ts,
    audit.ts, setup.ts) → 1 in src/slug-utils.ts
  - `extractBaseSlug` — 2 byte-identical copies → 1
  - `FOLDER_MAP` — 2 byte-identical copies (pull.ts, resources.ts) → 1
  - `UUID_SUFFIX_RE` — open-coded in 3 places → 1 constant
  - recanonicalize's inlined precondition-2 check (UUID prefix match)
    → extracted as `isEngineSuffixedSlug`

src/slug-utils.ts is config-free by design (no `./config.ts` import,
no side effects at load) so it's safely importable from any test
without priming process.argv / VAPI_TOKEN. This is the testability
property the prior dep-dedup.ts comment claimed for its local
duplicates but didn't actually enforce.

Regex tightening: shared `UUID_SUFFIX_RE` uses `^(.+)-([0-9a-f]{8})$`
(non-empty base) where the prior pull/dep-dedup copies used
`^(.*)-...` (allowed empty base). Strict improvement — engine-
generated keys always have a non-empty base, and the only input
class affected is the synthetic `-<8hex>` shape which is never
produced by `generateResourceId`. Pinned by a regression test in
tests/slug-utils.test.ts.

Back-compat: dep-dedup.ts re-exports slugify/extractBaseSlug so
existing tests importing them via that path keep working.

Tests: 228/228 pass (208 prior + 20 new slug-utils cases covering
slugify behavior, UUID_SUFFIX_RE boundaries, extractBaseSlug loose
form, isEngineSuffixedSlug strict form).

* refactor(push): extract reconcileStateKeyForResource — fold two 94-line 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).
scott-lowe-vapi added a commit that referenced this pull request Oct 3, 2026
.github/workflows/vapi-checks.yml runs `npm run check` on pull requests
(opened, synchronize, reopened, ready_for_review) and on manual dispatch,
only when the repository variable VAPI_CHECKS_ENABLED is 'true' and a
vapi-checks.yml exists — so on the upstream template it is dormant.

- Live runs only for same-repository, non-Dependabot PRs and dispatch;
  forks and Dependabot get a keyless dry run, and secrets are passed only
  to live runs. Never pull_request_target.
- Checks out the head SHA with full history (for --changed-since against
  origin/<base>) and persist-credentials: false.
- --all posts the aggregate `Vapi Evals` status; dispatching one named
  check doesn't. The run step execs node so GitHub's cancel reaches it,
  inside a 22-minute budget under a 30-minute job timeout; concurrency
  cancels a superseded push's runs.
- permissions: contents: read, statuses: write.

Docs: a README "PR Checks" section (setup from test files to required
status, the build-failure table, fork/Dependabot handling, CI orgs, cost
and what stays real), the AGENTS.md simulations step, a "Inline PR
Checks" section in docs/learnings/simulations.md, and improvements.md #34.

Refs TEST-141

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
scott-lowe-vapi added a commit that referenced this pull request Oct 3, 2026
## Value

**V.A.L.U.E. tier:** project — PR 8 of 10 for inline simulation PR checks ([TEST-141](https://linear.app/vapi/issue/TEST-141/gitops-run-simulation-suites-against-pr-changes-inline-as-ci-checks)). This PR turns `npm run check` into a PR check and documents setup end to end.

- **Problem:** `npm run check` (PR 7) gives a verdict locally, but a reviewer needs it on the PR: a `Vapi Evals` status whose **Details** link opens the exact run (PAL-608's contract), run on every affected push, safe for forks and Dependabot.
- **Who it affects:** gitops users, single-org and CI-org alike, who get a PR check by adding one variable and one secret; and the maintainers of forks like Hoag's and Mudflap's, who hand-maintain shell workflows for this today (PAL-608).
- **What changes:**
  - **`.github/workflows/vapi-checks.yml`** (opt-in):
    - **Triggers:** `pull_request` (opened, synchronize, reopened, ready_for_review), plus `workflow_dispatch` with an optional `check` input. Never `pull_request_target`.
    - **Opt-in:** it runs only when `vars.VAPI_CHECKS_ENABLED == 'true'`, and does nothing without a `vapi-checks.yml`, so on the upstream template it stays dormant.
    - **Live vs dry run:**
      - live only for same-repository, non-Dependabot PRs (checking both the actor and the PR author) and for dispatch;
      - everything else gets a keyless dry run, and secrets are passed only to live runs.
    - **Checkout:** the head SHA, with `fetch-depth: 0` so `--changed-since origin/<base>` diffs from the merge base, and `persist-credentials: false`.
    - **Statuses:** `--all` posts the aggregate `Vapi Evals`; dispatching one named check never changes it.
    - **Cancellation:** concurrency is keyed per PR with `cancel-in-progress`, and the run step `exec`s node, so GitHub's cancel reaches it and it cancels the superseded runs.
    - **Time:** `--budget-minutes 22` under `timeout-minutes: 30`.
    - **Permissions:** `contents: read` and `statuses: write`.
  - **README "PR Checks":** the end-user guide, covering:
    - test files with a judge example, and the check config;
    - the dry run, plus the build-failure table (what fails and how to fix it);
    - live runs, and turning on the workflow;
    - what a PR shows, making `Vapi Evals` required, and fork/Dependabot unblocking;
    - dedicated CI orgs;
    - cost, and what stays real in the run org.

    The project tree gains `vapi-checks.example.yml` and `check-cmd.ts`.
  - **AGENTS.md:** the "Testing with Simulations" step 5 now points at `npm run sim` vs `npm run check`.
  - **`docs/learnings/simulations.md` "Inline PR Checks":**
    - the parity results and the two known differences (handoff names, tool order);
    - mock behaviour, and hook-fired tools bypassing mocks;
    - why servers are replaced rather than deleted;
    - transfers;
    - what still reaches real systems;
    - the chat-mode limits;
    - where run items keep tool results.
  - **Indexes:** the learnings index row, and `improvements.md` #34 (RESOLVED). No `docs/changelog.md` edit.

## Evidence of value

- **Dormant on the template:** this PR adds the workflow, and `VapiAI/gitops` has no `VAPI_CHECKS_ENABLED` variable, so the `vapi-checks` job is **skipped** in this PR's own checks ([run 36941407785](https://github.com/VapiAI/gitops/actions/runs/36941407785): `completed / skipped`). That also shows GitHub parsed the workflow and evaluated its `if:`. Customers who haven't opted in see no change and no spend.
- **The command the workflow runs** (`--all`, `--changed-since`, live and dry run, statuses, job summary, JSON) is covered end to end by PR 7's tests against a stub of the simulations and GitHub APIs. It was also run live in the owner's test org: innocuous exit 0, degraded exit 1, no resources created, every tool result a mock.
- `npm test`: 474 passing (docs and workflow only; no code change).

## Testing plan

- The workflow YAML parses. `actionlint` isn't available in this environment, so it isn't linted. To check by hand: `if:` expressions, the `secrets` ternaries in step `env`, and the `exec` line.
- **Not yet tested — needs the owner,** because it needs a repository secret holding an API key:
  1. Create a private dogfood repo from this branch, with `resources/<test-org>/` holding the parity squad from `tests/fixtures/check-parity/resources/parity/`, renamed, and that fixture's `vapi-checks.yml`.
  2. Add the `VAPI_PRIVATE_API_KEY` secret and `VAPI_CHECKS_ENABLED=true`.
  3. Open a degraded-prompt PR and an innocuous one. Expect red and green `Vapi Evals` statuses whose **Details** links land on the runs, plus the job summaries (screenshots to add here).
  4. Open a PR from a fork. Expect a dry run and no status, because the token is read-only.
  5. Push a `package.json` change as Dependabot would. Expect `Vapi Evals` = `error`.
  6. Push twice quickly. Expect the first run canceled (`itemCounts.canceled > 0`).

Stacked on #63.

Refs TEST-141

🤖 Generated with [Claude Code](https://claude.com/claude-code)
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