fix(gitops): restore 7 P0 regressions surfaced in feat/optimization-gitops-flow audit; add regression tests#10
Merged
dhruva-reddy merged 6 commits intomainfrom Apr 21, 2026
Conversation
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
10 tasks
dhruva-reddy
changed the title
dhruva-reddy
force-pushed
the
fix/p0-gitops-regressions
branch
from
2f63e7c to
a1da236
Compare
Contributor
Author
Merge activity
|
dhruva-reddy
changed the base branch from
feat/optimization-gitops-flow
to
graphite-base/10
…ow audit
A multi-agent audit of feat/optimization-gitops-flow found seven P0-class
regressions that would cause data loss, silent failure, or API rejection
in real workflows. Each is restored or fixed below.
P0-1 (src/credentials.ts, src/pull.ts, src/push.ts):
Restore the scoped `replaceCredentialRefs` walker. The branch had
replaced it with a generic `deepReplaceValues` string-walk that swapped
every value matching a credential slug. Combined with `pullCredentials`
auto-slugifying provider-named credentials (`openai`, `11labs`,
`langfuse`, etc.), this rewrote `voice.provider`, `model.provider`, and
`observabilityPlan.provider` enum values into UUIDs on push, which the
Vapi API rejects. The scoped walker matches only at exactly
`credentialId` / `credentialIds` keys and uses a WeakSet for cycle
safety. The deleted warning comment is restored.
P0-2 (src/push.ts):
Wrap the apply phases in try / finally so `await saveState(state)`
always flushes even on partial failure. Without it, a 5xx mid-push
leaves API-issued UUIDs unrecorded, and the next run creates duplicates
on the platform. Save errors are caught and surfaced with a recovery
hint pointing at `npm run pull -- <org> --bootstrap`.
P0-3 (src/pull.ts):
Restore null preservation in `cleanResource`. The Vapi API uses `null`
to represent intentionally cleared fields (`voicemailMessage`,
`endCallMessage`, etc.). Stripping null on pull caused the next push to
drop the clear and the platform silently re-applied the prior value.
P0-4 (src/cleanup.ts, src/interactive.ts):
Restore both pre-existing safety gates that the branch removed:
`--confirm <slug>` double-gate (so a stray `--force` from another
command can't go destructive) and the empty-state refusal (so a fresh
clone or corrupted state file can't be misread as 'all remote
resources are orphaned' and wipe the org). The interactive cleanup
wrapper now passes both `--force` and `--confirm <slug>` since the
user's confirm() prompt is the explicit consent.
P0-5 (src/interactive.ts):
Drop the unconditional `--force` in interactive pull (both the All
flow and the per-resource picker). Default is now local-first; an
explicit `Overwrite locally modified files?` confirm gates the
`--force` flag. This restores Section 4 of the improvements doc that
the interactive flow was bypassing.
P0-6 (src/pull.ts):
Use `git status --porcelain --untracked-files=all -z` so untracked
files are listed individually rather than as a collapsed `?? dir/`
entry, and gate the mtime fallback on `(!changedFiles ||
changedFiles.size === 0)` so it fires when git has no useful info.
Also include `.yaml` (in addition to `.md` and `.yml`) when checking
the git-changed-files preservation set, to avoid duplicate-resourceId
errors on the next push for assistants stored as `.yaml`.
P0-7 (src/push.ts, src/config.ts):
Extract `pathMatchesFolder` and broaden the match: short-form paths
like `assistants/foo.yml` (the form documented in AGENTS.md) now
correctly match the assistants type instead of silently no-op'ing.
In `parseFlags`, refuse unrecognized bare positional args (e.g.
`npm run push -- <org> foo`) instead of silently dropping them and
triggering a full apply with full orphan-deletion check. Allow
unrecognized long flags (starting with `--`) to pass through so
command-specific flags like `--confirm` reach their consumer.
P1 cleanups in the same files:
- `src/config.ts`: hardcoded `.env.dev` in the missing-token error
message becomes `.env.${VAPI_ENV}` so the user is told to create the
correct env file for their slug.
- `src/config.ts`: `--confirm <slug>` is now consumed by `parseFlags`
so cleanup.ts's flag forwarding doesn't trip the new strict-arg
refusal.
Locks in the P0 behaviors restored in the previous commit so future refactors can't silently regress them. Tests use Node's built-in test runner (no new dependencies): - tests/credentials.test.ts: 8 unit tests for replaceCredentialRefs, including the key P0-1 case that asserts provider enums are NOT swapped even when the credential slug exactly matches a provider name like `openai`. Also covers cycle safety, non-plain-object pass-through (Date, Buffer), and symmetric round-trip behavior. - tests/clean-resource.test.ts: 4 unit tests pinning that `cleanResource` strips EXCLUDED_FIELDS and undefined values but preserves null (P0-3). - tests/path-matching.test.ts: 11 unit tests for the new `pathMatchesFolder` helper covering long-form, short-form, absolute, Windows-style, nested, and the segment-boundary edge cases (P0-7). - tests/cleanup-safety.test.ts: 4 spawn-based integration tests asserting that `cleanup --force` refuses without `--confirm <slug>`, refuses with the wrong slug, refuses on empty state, and does NOT refuse a default dry-run (P0-4). - tests/cli-arg-parsing.test.ts: 6 spawn-based integration tests covering bare-id refusal, misspelled-type refusal, and that valid args (positional types, file paths, `--confirm` slug consumption) are all accepted at parse time (P0-7). Run with `npm test`. All 33 tests pass under tsx.
`requested improvements.md` is a local audit log used during the P0 review of feat/optimization-gitops-flow. Keep it out of the upstream repo so it doesn't get pushed accidentally; reviewers who want it can fetch the audit summary from the related conversation.
The README still listed the old cleanup direct command without `--confirm <slug>`, didn't mention the new interactive `Overwrite locally modified files?` prompt, omitted the new keyboard shortcuts (Ctrl+G, ←/→) that searchableCheckbox actually supports, and pointed both AGENTS.md and the project-structure tree at the now-stale `docs/environment-scoped-resources.md`. Sweep: - README commands table: cleanup direct form is now `--force --confirm <org>`; add `npm test` row. - README Interactive Mode: document the new pull and cleanup prompts; flesh out the keyboard navigation list (Ctrl+G group toggle, arrow-key collapse/expand). - README "Pulling Without Losing Local Work": explain the two-layer detection (git + mtime fallback) so the fresh-clone case is covered in the docs as well as the engine. - README "Selective Push": call out that short-form paths (`assistants/foo.yml`) are supported, and that bare resource ids are rejected explicitly with a hint. - README troubleshooting: add entries for "Refusing to run destructive cleanup" and "Unrecognized argument / push appears to do nothing". - README project structure: drop the stale `docs/environment-scoped-resources.md` line; add `docs/learnings/` and `tests/` with brief annotations. - AGENTS.md: drop the stale env-scoped-resources line from the docs tree. - Delete `docs/environment-scoped-resources.md` entirely. The whole file walked users through `push:dev` / `pull:dev` / `push:stg` / `cp resources/dev/...` workflows that no longer exist in this repo; keeping it would actively misroute anyone who landed on it. No source changes; npm run build and npm test still pass.
…lder
Two small cleanups surfaced during the merge from origin/main:
.vapi-ignore example
main added three identical .vapi-ignore.example files in
resources/{dev,stg,prod}/. Those directories exist as dead artifacts
from before the org-slug refactor — nothing in this branch's code
reads them, and the example content still referenced the deleted
`pull:<env>` command form.
Replaced with a single canonical resources/.vapi-ignore.example that:
- Tells the user to copy it to `resources/<org>/.vapi-ignore`
(matching how config.ts actually resolves the ignore file).
- Uses `npm run pull -- <org>` in the description instead of
`pull:<env>` (which no longer exists).
- Adds `evals/` to the list of known folder paths (was missing).
scripts/mock-vapi-webhook-server.ts removed
The mock webhook receiver has not been used in any real pilot and
lived in a dedicated folder that was otherwise empty. Removed:
- scripts/mock-vapi-webhook-server.ts
- `mock:webhook` npm script
- README commands-table row, project-structure entry, and the
"Webhook Local Testing" section
- AGENTS.md "When to use" row, commands list entry, project-
structure entry, and the "Mock Server Testing" subsection
Build + tests still pass (tsc --noEmit clean, 33/33 tests).
Sweep of the lowest-cost, highest-value items from the pre-push review:
P1-3 src/push.ts
Append `"evals"` to ALL_RESOURCE_TYPES and add a comment anchoring it to
VALID_RESOURCE_TYPES in types.ts. Without this, hasAnyLoadedResources,
getTargetedResourceTypes, getMissingCredentialNames, and
getInvalidStateMappings all skipped evals — so `npm run push -- <org>
evals` on a fresh clone would bypass the bootstrap pre-flight check and
proceed with uninitialized state.
P1-2 src/cleanup.ts
Add evals to the orphan-scan resourceTypes array. cleanup.ts already
counted evals in stateIds, but the scan never fetched /eval from the
platform, so eval orphans were silently un-detectable via direct
cleanup. (Orphans during `push --force` were already handled by
delete.ts.)
P1-4 src/eval.ts
Rewrite the empty-state remediation text and the printUsage examples to
match the org-slug command surface. Was:
- "Eval files go in: resources/evals/" (pre-org-scoped path)
- "tsx src/eval.ts dev -s ..." (direct-tsx invocation; no longer
documented)
Now:
- "Eval files go in: resources/<org>/evals/" with config.env
interpolation for the actual org
- "npm run eval -- <org> -s ..." canonical command form
P1-5 AGENTS.md
.vapi-ignore section pointed at `resources/<env>/.vapi-ignore` and
`resources/<env>/.vapi-ignore.example`. Both are wrong for this repo:
the env placeholder should be <org>, and the canonical example lives at
`resources/.vapi-ignore.example` (no subfolder) after the consolidation
in 8b0fc88. Fixed both references.
P2-2 src/config.ts
`loadEnvFile` comments still cited `.env.dev`/`.env.stg`/`.env.prod` as
examples. Replaced with `.env.my-org` / `.env.my-org.local` so the
inline doc matches the org-slug model the surrounding code uses.
P2-3 src/resources.ts
`resourceId` computation comment showed example paths without the
`<org>` segment. Updated to the current layout.
P2-5 src/call.ts
Mic-permission hint mixed "System Preferences" (pre-Ventura) and
"System Settings" (current) in adjacent log lines. Normalized to
"System Settings" everywhere.
Deferred to follow-up PRs (non-trivial):
- P1-1 applyEval needs upsertResourceWithStateRecovery pattern
- P1-6 interactive pull classify-and-surface 5xx/network errors
- P1-7 automated coverage for interactive flows via @inquirer/testing
Build + tests still pass (tsc --noEmit clean, 33/33).
dhruva-reddy
force-pushed
the
fix/p0-gitops-regressions
branch
from
a1da236 to
1f226b9
Compare
vtkovapi
approved these changes
Apr 21, 2026
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
2 participants
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.
Describe your changes
Stacked on top of #7. Restores seven P0-class correctness/safety behaviors that
feat/optimization-gitops-flowregressed, locks them in with 33 regression tests, sweeps docs and CLI text to the org-slug model, and integratesorigin/main's parallel fix-set (a1e3228) +.vapi-ignorefeature (eb29042).P0 fixes (see
requested improvements.mdaudit for details; also mostly shipped in parallel onmainasa1e3228):credentialId/credentialIds— thefeatbranch's generic string-walk rewroteprovider: openai/11labs/langfuseenums into UUIDs on push, which the Vapi API rejects. Restored the scoped walker with cycle-safeWeakSetandisPlainObjectguard.saveStatewrapped intry { ... } finallyso a mid-push 5xx no longer leaves API-issued UUIDs unrecorded and creates duplicates on retry.cleanResourcepreservesnull— the Vapi API usesnullto mean "intentionally cleared". Stripping it caused pull→push round-trip drift that silently re-applied prior values.--forcealone is no longer enough;--confirm <slug>is also required, plus empty-state refusal. Prevents a fresh clone or corrupted state from being misread as "everything is orphaned" and wiping the org. Interactive cleanup passes both flags since theconfirm()prompt is the user's explicit consent.--forcein both the "All" and per-resource flows; adds an explicitOverwrite locally modified files?confirm (defaultNo).git status --porcelain --untracked-files=all -z(was just-z, causing untracked dirs to collapse). Mtime fallback now fires whenchangedFilesis an empty Set, not justundefined..yamlextension now included alongside.md/.ymlin the preservation check.assistants/foo.yml(the form documented inAGENTS.md) used to silently no-op. ExtractedpathMatchesFolderhelper and broadened the match. Bare resource ids (npm run push -- <org> foo) are now refused explicitly instead of triggering a full apply with orphan-deletion.Other changes in this PR:
tests/using Node's built-innode:testrunner (no new dependencies);npm testadded..env.<org>,resources/<org>/...,npm run <cmd> -- <org>);docs/environment-scoped-resources.mddeleted as fully stale.origin/main's parallel fix (a1e3228) +.vapi-ignorefeature (eb29042) cleanly — full conflict-resolution rationale in the merge commit message..vapi-ignore.exampleto a single canonicalresources/.vapi-ignore.example(main's merge added three duplicates in dead env-named folders).scripts/mock-vapi-webhook-server.tsand its references in README / AGENTS /package.json.cleanup.tsorphan scan andALL_RESOURCE_TYPESin push;eval.tsremediation text updated to org-slug paths; AGENTS.md.vapi-ignoresection fixed.Deferred to follow-up PRs (flagged, not blocking):
applyEvaldoesn't yet useupsertResourceWithStateRecovery— will 404-crash on stale state mappings instead of recovering like the other resource types.src/interactive.ts,src/searchableCheckbox.ts,src/setup.ts) have no automated coverage; the--forceconfirm prompt and cleanup's confirm spawn are only protected by code review.Relevant Context (linear ticket, slack link, etc)
Surfaced during a multi-agent audit of
feat/optimization-gitops-flow(parent PR #7) before it shipped to customer-facing deployments. The audit's findings and a status-per-requested-improvement table live inrequested improvements.mdat the repo root (gitignored — local audit log only).API Changes
Is this changing the public API?
If yes, is it backward‐compatible?
N/A. This repo is an internal CLI that consumes the Vapi API; it does not expose any public API surface. The only user-visible CLI changes are additive (
npm test) or correctness-restoring (cleanup--confirm <slug>now required for destructive runs, bare-id positional args refused explicitly). Behaviors that were silent no-ops or silent overwrites now error or prompt loudly.Non backward-compatible changes might break customers' agents. Please proceed with care and notify the team.
How did you test this?
Automated (33 tests, all passing under
npm test):tests/credentials.test.ts(8) —replaceCredentialRefsscoping, including the specificprovider: openairegression that motivated the walker revert; cycle safety; Date/Buffer pass-through; symmetric round-trip.tests/clean-resource.test.ts(4) —nullpreservation,undefinedstripping,EXCLUDED_FIELDShandling, nested structures.tests/path-matching.test.ts(11) —pathMatchesFolderacross long-form, short-form, absolute, Windows-style, nested-folder, and segment-boundary cases.tests/cleanup-safety.test.ts(4) — spawn-based integration:cleanup --forcerefuses without--confirm <slug>/ with wrong slug / with empty state; dry-run allowed without the gates.tests/cli-arg-parsing.test.ts(6) — spawn-based integration: bare-id refusal, misspelled-type refusal, valid positional types / file paths /--confirmslug consumption all accepted.Verified locally (no network):
npm run build(tsc --noEmit) clean at every commit.speakerandmicload undercreateRequire(import.meta.url)in ESM (per therequested improvements.mdsmoke test command).git status --porcelain --untracked-files=all -zexpands untracked dirs to individual files on macOS/APFS (the mechanism the P0-6 fix relies on).Manual smoke test flagged for reviewer (requires real terminal / real org):
Overwrite locally modified files?prompt defaults to No and only forwards--forcewhen user explicitly answers Yes.--force --confirm <slug>so the destructive subprocess proceeds.resources/<org>/and.vapi-state.<org>.json, runnpm run pull -- <org> --bootstrap, thennpm run pull -- <org>, edit one assistant, run pull again and confirm✏️ <id> (locally modified, preserving)fires (P0-6 end-to-end).