Skip to content

fix(promotion): commit the files of transitions that applied when a later one fails - #65

Merged
scott-lowe-vapi merged 1 commit into
mainfrom
fix/promotion-commit-applied-transitions
Oct 7, 2026
Merged

scott-lowe-vapi merged 1 commit into
mainfrom
fix/promotion-commit-applied-transitions

Conversation

@scott-lowe-vapi

@scott-lowe-vapi scott-lowe-vapi commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Value

V.A.L.U.E. tier: project — PR 9 of 10 for inline simulation PR checks (TEST-141). This is a fix to promotion, independent of the check stack (cut from main), and the promotion gate (PR 10) depends on it.

  • Problem: when a multi-transition promotion fails partway, the "Commit reconciled files and UUID state" step pushes nothing: not the UUID state, and not the files of transitions that already reached the platform.
    • Why: on failure the step stages only .vapi-state.*.json, but the failed transition's promotionPlanApply has already rewritten tracked files in its target org, so git pull --rebase refuses with "You have unstaged changes".
    • Result: git and the platform disagree, and the next promotion plans from stale files.
  • Who it affects: teams using promotion.yml with two or more transitions (dev → staging → prod), and anyone who adds the promotion gate in PR 10, where a failing gate is exactly a mid-run failure.
  • What changes:
    • src/promote-cmd.ts:

      • each --apply run truncates tmp/promotion-applied.txt (gitignored);
      • after each successful apply.ts, it records each path git status --porcelain -z --untracked-files=all -- resources/<target> reports (new, modified, deleted and renamed files, by current path), with its content at that moment written to git's object store (git hash-object -w; - for a deletion). These are read from git, not the plan, because apply's own pull and push can rewrite files the plan didn't name;
      • promotionCommandRun(args, deps = { childRun }) gains a test seam.
    • .github/workflows/promotion.yml: on a non-success outcome, the commit step:

      1. stages the state files plus exactly the recorded blobs (git update-index --cacheinfo; deletions with git rm --cached), and commits. Staging the recorded content, not the working tree, means a later failed transition that rewrote the same file can't replace what was applied;
      2. runs git reset --hard HEAD && git clean -fd -- resources, so the failed transition's rewrites are discarded;
      3. then rebases and pushes as before.

      On success it's unchanged: it commits all of resources.

    • improvements.md: refactor(engine): extract shared slug + folder helpers into slug-utils #35, RESOLVED.

Evidence of value

Red/green in a scratch repo.

  • Setup: a bare origin plus a clone; the pipeline is a → b → c, and c starts with a tracked assistants/intake.yml.
  • The run: promote --all --apply runs this branch's promote-cmd with a fake child runner. Apply into b succeeds (writing state and one extra file, as apply's pull can). Apply into c writes state, then fails.
  • Then: the workflow's real commit step, extracted from each ref's promotion.yml, runs with PROMOTION_OUTCOME=failure.
main (69c7e83) This branch
Commit step error: cannot pull with rebase: You have unstaged changes. exit 128 pushed, exit 0
origin/main unchanged (only base) chore: record promoted Vapi state [skip promotion]
.vapi-state.b.json / .c.json on origin {} / {} (state lost) uuid-b / uuid-c (both recorded)
resources/b/assistants/intake.yml, pulled-by-apply.yml missing committed
resources/c/assistants/intake.yml old content old content (the failed rewrite is discarded)
resources/c/assistants/pulled-by-apply.yml — removed by git clean

tmp/promotion-applied.txt after the run held exactly the two resources/b/... paths.

Same-org case: red before the fix, green after. Two pipelines promote into the same org in one --all run. The first applies resources/c/assistants/intake.yml with a's content, and the second rewrites that file with b's content and then fails. Before the fix, the committed file was b's (the failed rewrite); after it, the commit holds a's, the content that was actually applied.

Tests: on top of #68, npm test goes from 476 to 484 passing.

Testing plan

  • tests/promote-cmd.test.ts (3 tests, a real temp git repo, injected child runner):

    • one transition applies and the next fails: only the applied transition's files are recorded, including a file apply rewrote that the plan didn't name;
    • every --apply run starts a fresh record, and a plan-only run leaves it alone;
    • deleted and renamed files are recorded by their current path.
  • tests/promote-cmd.test.ts also checks that each applied file is recorded with its content at apply time (git cat-file of the recorded blob, after a later rewrite), and a deletion as -.

  • New tests/promotion-workflow-commit.test.ts (4 tests) runs the workflow's real commit step, read from promotion.yml and run with bash -e, against a clone of a bare origin, after a promotion driven through promote-cmd:

    • partial failure: state and the applied transition's files are pushed, and the failed rewrite is not;
    • the same-org case above;
    • success commits every change;
    • no changes pushes nothing.

    A later edit to the YAML that brings either bug back fails here.

  • The existing tests/promotion.test.ts, and test(promotion): pin the exact files promoting an assistant writes to the target #68's golden promotion test, pass unchanged.

  • Not tested:

    • a real GitHub Actions promotion run (the step runs locally under bash, read from the YAML) — manual QA with two non-production orgs before merge.

Stacked on #68.

Refs TEST-141

After review

  • Dhruva: if recording an applied transition's files fails, the run now stops with an error naming the org and how to reconcile it (npm run pull -- <org>), instead of warning and letting the commit step save that org's state without its files. Test: promote-cmd.test.ts breaks git after the first apply and expects the run to stop there.
  • Chris (commit per transition): replied on the PR. The blob snapshot gives the same guarantee, that what's committed is what reached the platform, without the CLI making git commits mid-run.

🤖 Generated with Claude Code

@scott-lowe-vapi
scott-lowe-vapi force-pushed the fix/promotion-commit-applied-transitions branch from 308b8d5 to ef619e1 Compare October 3, 2026 06:18
@scott-lowe-vapi
scott-lowe-vapi marked this pull request as ready for review October 3, 2026 06:19
@scott-lowe-vapi
scott-lowe-vapi changed the base branch from main to graphite-base/65 October 3, 2026 06:28
@scott-lowe-vapi
scott-lowe-vapi force-pushed the fix/promotion-commit-applied-transitions branch from ef619e1 to c07fed1 Compare October 3, 2026 06:28
@scott-lowe-vapi
scott-lowe-vapi changed the base branch from graphite-base/65 to test/promotion-golden-baseline October 3, 2026 06:28

@chris-garber-vapi chris-garber-vapi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little wary on this one. It seems to me like if we're making git blobs for the files that we could instead just be doing some type of commit operation, so we could have like: test -> apply -> commit or something like that, rather than having a kinda intermediate half state that we commit after the fact. I think this would make our system more like terraform (which I think we're kinda taking inspiration from).

Don't wanna block here, I'm far from an expert. I think this will likely get the job done, but I don't know if it's the best option

@dhruva-vapi dhruva-vapi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, snapshotting the blob after each apply is a nice touch since a later failed transition into the same org can't overwrite what actually reached the platform. tests pass locally too

one non-blocking thing: if appliedPathsRecord fails it only warns, so when a later transition fails we'd commit that org's state without its files, which is the same git vs platform drift this PR is fixing. can we make that fail loudly so someone knows to reconcile that org by hand

@scott-lowe-vapi
scott-lowe-vapi force-pushed the test/promotion-golden-baseline branch from 7283c24 to e5ba491 Compare October 6, 2026 22:50
@scott-lowe-vapi
scott-lowe-vapi force-pushed the fix/promotion-commit-applied-transitions branch from c07fed1 to 4e7a5e0 Compare October 6, 2026 22:50
@scott-lowe-vapi

Copy link
Copy Markdown
Contributor Author

Thanks both.

@dhruva-vapi: done. If recording an applied transition's files fails, the run now stops with an error that names the org and how to reconcile it (npm run pull -- <org>), instead of warning and letting the commit step save that org's state without its files. promote-cmd.test.ts covers it.

@chris-garber-vapi on committing per transition: I looked at it. It would mean promote making git commits, and pushing with auth, between transitions inside the CI run, and a crash between an apply and its commit would still leave the same gap. Snapshotting exactly what each apply left, then committing those blobs in the always() step, gives the same guarantee (what's committed is what reached the platform) without the CLI touching git history mid-run. Happy to revisit if we move toward a broader plan/apply/commit model.

🤖 Generated with Claude Code

scott-lowe-vapi commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor Author

Merge activity

  • Oct 7, 5:47 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Oct 7, 5:49 PM UTC: Graphite rebased this pull request as part of a merge.
  • Oct 7, 5:49 PM UTC: @scott-lowe-vapi merged this pull request with Graphite.

@scott-lowe-vapi
scott-lowe-vapi changed the base branch from test/promotion-golden-baseline to graphite-base/65 October 7, 2026 17:47
scott-lowe-vapi added a commit that referenced this pull request Oct 7, 2026
… the target (#68)

## Value

**V.A.L.U.E. tier:** small — a tests-only baseline under the remaining promotion changes for TEST-141 ([TEST-141](https://linear.app/vapi/issue/TEST-141/gitops-run-simulation-suites-against-pr-changes-inline-as-ci-checks)). It changes no behaviour. It's small rather than micro because it guards a blast-radius path: promotion into production.

- **Problem:** the code that moves assistants from a lower org into production is the riskiest path in gitops. The existing tests check pieces of it (canonicalization, bindings, deletions) but never the full set of files a promotion writes. So a refactor could change what lands in prod without any test failing.
- **Who it affects:** teams that promote resources between orgs (dev → staging → prod). It's a baseline for future changes to promotion's plan/apply core. The stacked PRs don't touch that core: #65's commit behaviour is covered by its own `promote-cmd` and workflow-commit tests, and #66 only adds the `check` field.
- **What changes:** new `tests/promotion-golden.test.ts`. It runs one staging → prod promotion through `promotionPlanBuild` and `promotionPlanApply`, then asserts the plan and every file left in the target org:
  - source UUIDs canonicalized to names (`toolIds`, a handoff destination);
  - referenced tools and assistants pulled in as dependencies, and an unreferenced staging-only tool left behind;
  - a credential bound by name, and a phone number bound to the target's;
  - a target file the source no longer has deleted;
  - an unrelated target file left untouched;
  - the markdown prompt body preserved;
  - the source org never written.

  A second test re-plans right after the apply and expects no changes.

## Evidence of value

**Mutation check:** each deliberate break of `src/promotion.ts` turns the test red, and it's green again once the break is reverted.

| Break | Golden test |
|---|---|
| Phone binding returns the source phone | ❌ fails |
| Credential binding returns the source UUID | ❌ fails |
| UUID → name canonicalization disabled | ❌ fails |
| None (`main`) | ✅ passes |

The expected output was read line by line against promotion's documented behaviour before it was pinned.

**Tests:** `npm test` goes from 474 to 476 passing.

## Testing plan

- `tests/promotion-golden.test.ts`: the golden promotion, and idempotence on re-plan.
- **Not tested:**
  - the child `pull`/`apply` processes (this exercises the file-writing core, not the CLI);
  - a real two-org promotion (manual QA before #65 and #66 merge).

Base of the promotion stack: #68 ← #65 ← #66.

Refs TEST-141

## After review

Chris's mutation-tested suggestions, each verified to fail the golden when the code it guards is broken:

- `.vapi-ignore`d prod files are left alone (an ignored, prod-only `assistants/hotline.yml`).
- The whole repository is snapshotted, so a stray write to `.env.*` or a state file fails it.
- Billing arrives only as a dependency of a dependency (intake → `handoff-to-billing` → billing).
- A file promotion doesn't rewrite is copied byte for byte (it keeps a comment).
- The prompt body has a heading, blank lines and a `---` separator.

Not taken: the `omit` phone case (`promotion.test.ts` already covers it), the unused state entries, and the input/output formatting pass, which is a follow-up.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
@scott-lowe-vapi
scott-lowe-vapi changed the base branch from graphite-base/65 to main October 7, 2026 17:47
…ater one fails

When a promotion failed partway, the workflow's commit step staged only
the UUID state, but the failing transition had already rewritten tracked
files in its target org, so `git pull --rebase` refused and nothing was
pushed — not the state, and not the files of transitions that had
already reached the platform.

- promote-cmd truncates tmp/promotion-applied.txt at the start of each
  --apply run and, after each successful apply.ts, records every path git
  reports changed under resources/<target>/ (from git, not the plan:
  apply's own pull and push can rewrite other files) with its content at
  that moment, written to git's object store (`-` for a deletion).
- On a non-success outcome, the commit step stages the state files plus
  exactly those recorded blobs, commits, then resets and cleans
  resources/ so the failed transition's rewrites can't block the rebase.
  Staging the recorded content rather than the working tree means a later
  failed transition that rewrote the same file (two pipelines promoting
  into one org in one --all run) can't replace what was applied.
- promotionCommandRun(args, deps) takes an injectable child runner.
- tests/promotion-workflow-commit.test.ts runs the workflow's real commit
  step (read from promotion.yml, run with bash -e) against a bare origin:
  partial failure, the same-org case, success, and no changes.

Refs TEST-141

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@scott-lowe-vapi
scott-lowe-vapi force-pushed the fix/promotion-commit-applied-transitions branch from 4e7a5e0 to 26465d7 Compare October 7, 2026 17:48
@scott-lowe-vapi
scott-lowe-vapi merged commit 3c1e10e into main Oct 7, 2026
4 checks passed
scott-lowe-vapi added a commit that referenced this pull request Oct 7, 2026
## Value

**V.A.L.U.E. tier:** project — PR 10 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)), the "check before deploy" step in promotion.

> Stacked on #65 (the promotion partial-failure fix), now that #57–#64 have merged. This PR is the single gate commit.

- **Problem:** promotion copies staging's reviewed files into production, but nothing checks that staging's agents still behave before they move on. Teams promoting dev → staging → prod need a behaviour gate between orgs, without new infrastructure.
- **Who it affects:** multi-org gitops users (the promotion pipeline), who get a "check before deploy" step with one line of `promotion.yml`. Single-org users are unaffected.
- **What changes:**
  - **`promotion.yml`** accepts `orgs.<slug>.check: <name>` (a slug), naming a `vapi-checks.yml` check.
  - **New `src/promotion-gate.ts`:**
    - **Validation before any transition:** the check must exist, and its `org` and `runOrg` must be the gated org, otherwise the run errors. `vapi-checks.yml` is required once any org is gated.
    - **Plan line:** what the gate would run, built offline.
    - **Live gate:** the check runs live and reduces to the worst target result.
  - **`src/promote-cmd.ts`:** in each transition, after the plan is built:
    - no changes skips the gate;
    - plan-only prints `check  would run <name> in <org> (<n> simulations × <t> targets)`;
    - `--apply` runs the check (after the bindings refresh, before `promotionPlanApply` writes anything). Any non-pass throws `Promotion out of <org> blocked: check <name> <outcome> (<run url>)`.
    - A pass is cached per source org and dropped once a transition applies into that org.
    - `promotionCommandRun(args, overrides)` now takes `Partial<PromotionDeps>` (`childRun`, `checkRun`).
  - **`.github/workflows/promotion.yml`:** `timeout-minutes: 90` on the "Reconcile configured promotions" **step**, not the job, so the `if: always()` commit step (fixed in #65) still runs after a blocked or slow gate.
  - **Docs:** `promotion.example.yml` (a commented `check:`), a README "Check before promoting" section, and a pointer from "PR Checks".

## Evidence of value

**The real gate, run live** in the owner's test org on the TEST-141 parity squad.
- **Setup:** a scratch repo whose `promotion.yml` gates `parity` on check `core`, with pipeline `parity → parity-prod`.
- **The run:** `promote --pipeline release --from parity --to parity-prod --apply`.
- **The fake:** the child runner was faked, so bindings pulls were no-ops and the downstream `apply.ts` was recorded but not run. No second org was needed or touched.

| Variant | Gate run | Result | Downstream apply | `resources/parity-prod/` |
|---|---|---|---|---|
| Degraded scheduler prompt | [7ed19587](https://dashboard.vapi.ai/simulations/run/7ed19587-d1ca-4d44-9232-7cdd15a50d67): 2 of 3 failed | `Promotion out of parity blocked: check core failed (https://dashboard.vapi.ai/simulations/run/7ed19587-…)` | **none** | **empty** (nothing written) |
| Fixture as-is | [95470670](https://dashboard.vapi.ai/simulations/run/95470670-2861-45d3-a483-7a220fc3591a): 3 of 3 passed | promoted | `["parity-prod"]` | written; 20 applied paths recorded |

The test org's resource counts were identical before and after both gate runs.

**Tests:** `npm test` goes from 484 (#65) to 492 passing, and #68's golden promotion test passes unchanged.

## Testing plan

- **`tests/promotion-gate.test.ts`** (6 tests, real git fixture, injected `childRun` / `checkRun`):
  - a pass applies;
  - failed and incomplete both block with the exact message, with no apply and the target untouched;
  - plan-only prints the line and runs nothing;
  - no changes skips the gate;
  - the three config errors (no `vapi-checks.yml`, unknown check, check in another org) stop before anything applies;
  - the pass cache: reused for two pipelines out of one org, and re-run after a transition applies into the gated org.
- **No gate configured, no change:** with no `check:` in `promotion.yml` and an **invalid** `vapi-checks.yml` present, plan and `--apply` both succeed, `checkRun` is never called, and the plan output equals a pinned string. That string is exactly what #65's code (before the gate existed) prints for the same fixture, which I confirmed by running #65's `promote-cmd` on it. So the gate is invisible unless someone opts in.
- **`tests/promotion.test.ts`:** `orgs.<slug>.check` is parsed, and a non-slug is rejected.
- **Not tested:**
  - **A real two-org promotion:** only one test org was available. The downstream apply was faked, so the blocked case shows nothing written, and the pass case shows the apply was called.
  - **A GitHub Actions promotion run with a gate**, including the step timeout firing.
- **Found while testing (pre-existing, out of scope):** promotion's dependency check rejects simulations that reference a **stock personality by UUID**, with "Referenced managed dependency is missing from source: personalities/a0000000-…". So a gated org's tests need local personality files until that's fixed.

Stacked on #65.

Refs TEST-141

## After review

The gate now refuses, when the config loads and before anything applies:

- an unknown key under an org in `promotion.yml`, so a misspelled `check:` can't silently drop the gate;
- `toolMocks: off` and `stripWebhooks: false`, because a gate runs in the real org, never a CI org;
- a check `baseUrl` that differs from the org's `baseUrl` in `promotion.yml`, so the org's key only goes to the host promotion uses (the gate always uses that host);
- a gate on an org that is last in every pipeline, where it would never run;
- gated checks whose combined budget is over 300 minutes.

It also fixes:

- **Deadline:** each batch of 3 targets gets a full `timeoutMinutes`, so a check with more than 3 targets is no longer falsely blocked as incomplete.
- **Step timeout:** raised from 90 to 330 minutes, as a safety net that no longer cuts short long ungated promotions.
- **Tests:** the deadline and the worst-target rule are pure helpers with their own tests.

The guide changes (blocks stop the whole run, simulation cost, the stock-personality limitation, accurate wording) are in #71. The `promote` User-Agent for gate runs is in #78. The block-report detail, fetch-stubbed gate test and deduplication are follow-ups.

🤖 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.

3 participants