refactor(push): extract reconcileStateKeyForResource — fold two ensure-fns into one generic helper - #34
Merged
dhruva-vapi merged 2 commits intoMay 15, 2026
Conversation
dhruva-vapi
force-pushed
the
engine-consolidation/01-shared-utils
branch
from
May 15, 2026 03:19
64e75ec to
eae50eb
Compare
#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
force-pushed
the
engine-consolidation/02-reconcile-statekey
branch
from
May 15, 2026 03:20
d3c198a to
4d952a4
Compare
4 tasks done
Contributor
Author
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).
This was referenced Oct 1, 2026
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)
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