diff --git a/.github/workflows/promotion.yml b/.github/workflows/promotion.yml index 350f8f6..d5be0b7 100644 --- a/.github/workflows/promotion.yml +++ b/.github/workflows/promotion.yml @@ -99,6 +99,13 @@ jobs: paths=(':(glob).vapi-state.*.json') if [[ "$PROMOTION_OUTCOME" == "success" ]]; then paths+=(resources) + elif [[ -s tmp/promotion-applied.txt ]]; then + # A later transition failed: keep the files of the transitions + # that finished applying, so git matches what reached the + # platform. The failed transition's rewrites are discarded below. + while IFS= read -r applied; do + [[ -n "$applied" ]] && paths+=(":(literal)$applied") + done < tmp/promotion-applied.txt fi if [[ -z "$(git status --porcelain -- "${paths[@]}")" ]]; then @@ -110,6 +117,10 @@ jobs: git config user.email "vapi-gitops[bot]@users.noreply.github.com" git add -A -- "${paths[@]}" git commit -m "chore: record promoted Vapi state [skip promotion]" + # Unstaged rewrites from a failed transition would make the rebase + # refuse to run, and then nothing would be pushed, state included. + git reset --hard HEAD + git clean -fd -- resources for attempt in 1 2 3; do git pull --rebase origin main if git push; then diff --git a/improvements.md b/improvements.md index 5cc60ad..b4219c0 100644 --- a/improvements.md +++ b/improvements.md @@ -84,6 +84,7 @@ you which stack PR closes the row.** | 30 | Tool-linking pass could PATCH a raw assistant slug | Mid-push 400 naming the wrong resource | None | RESOLVED 2026-08-03 (#51) | | 31 | Unresolved references handled 3 inconsistent ways, no dangling-ref check | Same authoring mistake, three different failure modes | None | Open | | 32 | Test suite never ran in CI; 20 tests rotted after the hash store | Regression guards for #22/#23 silently stopped running | None | RESOLVED 2026-09-30 (#56) | +| 35 | A failed promotion pushed nothing, not even state | git lost track of resources already on the platform | None | RESOLVED 2026-10-01 | **Active backlog after cleanup:** `#2`, `#6`, `#8`, `#12`, `#20`, `#24–#26`, `#31`, and the open remainder of `#27` (wiring the listing-completeness verdict into push/delete/audit, and moving `cleanup.ts` onto the shared pager). Resolved entries stay in this file as historical incident notes per the maintenance directive; stale superseded backlog rows are not duplicated. @@ -1711,6 +1712,61 @@ errors today; widening `include` is a separate change. --- +## 35. A failed promotion pushed nothing, not even state + +**[RESOLVED 2026-10-01]** + +**Discovered:** 2026-10-01, while planning the promotion check gate (TEST-141). + +### Problem + +When a multi-transition promotion failed partway, the workflow meant to +commit the UUID state and leave resource files alone. But the failing +transition had already rewritten tracked files in its target org, so the +commit step's `git pull --rebase` refused ("You have unstaged changes") and +the job pushed nothing: not the state, and not the files of the transitions +that had already reached the platform. + +### Current behavior (Verified, before the fix) + +- `.github/workflows/promotion.yml` "Commit reconciled files and UUID state" + staged only `.vapi-state.*.json` on a non-success outcome, then ran + `git pull --rebase origin main` with the failed transition's rewrites + still in the working tree. +- `promotionPlanApply` writes target files before `apply.ts` runs, so any + update to an existing target file left a tracked modification behind. +- Reproduced in a scratch repo: transitions a→b (applies) then b→c (fails + with an existing file in c) → `error: cannot pull with rebase`, exit 128, + origin unchanged. + +### Risk + +Git and the platform disagree after any partial failure: b's resources are +live but not in git, and the next promotion plans from stale files. + +### Current mitigation + +None needed once the fix below lands. + +### Possible fix (landed) + +- `src/promote-cmd.ts` truncates `tmp/promotion-applied.txt` at the start of + each `--apply` run and, after each successful `apply.ts`, appends the paths + `git status` reports under `resources//` — read from git rather + than the plan, because apply's own pull and push can rewrite other files. +- On a non-success outcome the commit step adds the state files plus exactly + those paths, commits, then `git reset --hard HEAD && git clean -fd -- + resources` before rebasing, so the failed transition's rewrites can't block + the push. +- `promotionCommandRun(args, deps)` takes an injectable child runner for + tests (`tests/promote-cmd.test.ts`). + +### Status + +**RESOLVED 2026-10-01.** + +--- + ## Out of scope (intentionally not improvements) - **State file is identity-only and not git-ignored.** It's intentionally diff --git a/src/promote-cmd.ts b/src/promote-cmd.ts index 7888191..d78ecdc 100644 --- a/src/promote-cmd.ts +++ b/src/promote-cmd.ts @@ -1,7 +1,7 @@ import { resolveApiKey } from "./api-key.ts"; -import { spawnSync } from "node:child_process"; -import { existsSync, readFileSync } from "node:fs"; -import { resolve } from "node:path"; +import { execFileSync, spawnSync } from "node:child_process"; +import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; +import { dirname, resolve } from "node:path"; import { fileURLToPath } from "node:url"; import type { PromotionConfig, PromotionPipeline } from "./promotion.ts"; import { @@ -36,6 +36,16 @@ const ROOT_DIR = resolve( process.env.VAPI_GITOPS_ROOT ?? fileURLToPath(new URL("..", import.meta.url)), ); +// Files that transitions which finished applying left changed, one path per +// line. When a later transition fails, the promotion workflow commits exactly +// these (plus state) and discards the failed transition's rewrites, so what +// reached the platform is still recorded in git. +export const APPLIED_PATHS_FILE = "tmp/promotion-applied.txt"; + +export interface PromotionDeps { + childRun: typeof childRun; +} + function argumentsParse(args: string[]): PromotionArguments { const parsed: PromotionArguments = { all: false, apply: false }; for (let index = 0; index < args.length; index++) { @@ -189,6 +199,51 @@ function childRun( if (result.status !== 0) throw new Error(`${script} failed for ${org}`); } +// The paths git reports changed under resources//, including new and +// deleted files. Read from git rather than the plan because apply's pull and +// push can rewrite files the plan didn't name. +function changedPathsRead(org: string): string[] { + const output = execFileSync( + "git", + [ + "status", + "--porcelain", + "-z", + "--untracked-files=all", + "--", + `resources/${org}`, + ], + { cwd: ROOT_DIR, encoding: "utf8" }, + ); + const entries = output.split("\0").filter(Boolean); + const paths: string[] = []; + for (let index = 0; index < entries.length; index++) { + const entry = entries[index]!; + paths.push(entry.slice(3)); + // A rename or copy is followed by its original path. + if (entry[0] === "R" || entry[0] === "C") index++; + } + return paths; +} + +function appliedPathsRecord(org: string): void { + const file = resolve(ROOT_DIR, APPLIED_PATHS_FILE); + const recorded = existsSync(file) + ? readFileSync(file, "utf8").split("\n").filter(Boolean) + : []; + let changed: string[]; + try { + changed = changedPathsRead(org); + } catch (error) { + console.warn( + `⚠️ Could not record applied paths for ${org}: ${error instanceof Error ? error.message : String(error)}`, + ); + return; + } + const paths = [...new Set([...recorded, ...changed])]; + writeFileSync(file, paths.length > 0 ? `${paths.join("\n")}\n` : ""); +} + function stateLoad(org: string) { const path = resolve(ROOT_DIR, `.vapi-state.${org}.json`); if (!existsSync(path)) @@ -204,7 +259,9 @@ async function transitionRun( apply: boolean, tokens: Map, allowEmptySourceDeletion: boolean, + deps: PromotionDeps, ): Promise { + const { childRun } = deps; if (apply) { childRun( "src/pull.ts", @@ -246,11 +303,13 @@ async function transitionRun( connectionLoad(config, transition.target, tokens), ["--force", "--allow-new-files", "--resolve=ours", ...changedPaths], ); + appliedPathsRecord(transition.target); return plan.changes.some((change) => change.kind === "delete"); } export async function promotionCommandRun( args = process.argv.slice(2), + deps: PromotionDeps = { childRun }, ): Promise { const parsed = argumentsParse(args); const configPath = resolve(ROOT_DIR, "promotion.yml"); @@ -259,6 +318,11 @@ export async function promotionCommandRun( const config = promotionConfigParse(readFileSync(configPath, "utf8")); const tokens = parsed.apply ? tokensParse() : new Map(); delete process.env.VAPI_PROMOTION_TOKENS; + if (parsed.apply) { + const file = resolve(ROOT_DIR, APPLIED_PATHS_FILE); + mkdirSync(dirname(file), { recursive: true }); + writeFileSync(file, ""); + } // Applying a deletion removes the intermediate org's state entry. Carry the // reviewed authorization forward so the same deletion can reach later orgs. const deletionAuthorizedSources = new Set(); @@ -270,6 +334,7 @@ export async function promotionCommandRun( parsed.apply, tokens, deletionAuthorizedSources.has(sourceKey), + deps, ); if (deleted) deletionAuthorizedSources.add( diff --git a/tests/promote-cmd.test.ts b/tests/promote-cmd.test.ts new file mode 100644 index 0000000..a5cf1be --- /dev/null +++ b/tests/promote-cmd.test.ts @@ -0,0 +1,189 @@ +import assert from "node:assert/strict"; +import { execFileSync } from "node:child_process"; +import { + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import test from "node:test"; + +// promote-cmd.ts binds its root at import, so one fixture repo serves the +// file; each test resets it. +const ROOT = mkdtempSync(join(tmpdir(), "promote-cmd-")); +process.env.VAPI_GITOPS_ROOT = ROOT; +const { APPLIED_PATHS_FILE, promotionCommandRun } = + await import("../src/promote-cmd.ts"); + +function git(...args: string[]): string { + return execFileSync( + "git", + ["-c", "user.name=t", "-c", "user.email=t@example.com", ...args], + { cwd: ROOT, encoding: "utf8" }, + ); +} + +function write(path: string, content: string): void { + mkdirSync(dirname(join(ROOT, path)), { recursive: true }); + writeFileSync(join(ROOT, path), content); +} + +function fixtureReset(): void { + rmSync(ROOT, { recursive: true, force: true }); + mkdirSync(ROOT, { recursive: true }); + write( + "promotion.yml", + "version: 1\norgs:\n a: {}\n b: {}\n c: {}\npipelines:\n release:\n orgs: [a, b, c]\n resources: ['**/*']\n", + ); + write("resources/a/assistants/intake.yml", "name: Intake\n"); + for (const org of ["a", "b", "c"]) write(`.vapi-state.${org}.json`, "{}\n"); + write(".gitignore", "tmp/\n.env.*\n"); + git("init", "-q", "-b", "main"); + git("add", "-A"); + git("commit", "-qm", "base"); +} + +interface ChildCall { + script: string; + org: string; +} + +// Stands in for pull.ts and apply.ts: apply into `failOrg` fails, and every +// other apply also rewrites one file the plan didn't name, as apply's own +// pull can. +function childRunFake(calls: ChildCall[], failOrg?: string) { + return (script: string, org: string) => { + calls.push({ script, org }); + if (script !== "src/apply.ts") return; + if (org === failOrg) throw new Error(`${script} failed for ${org}`); + write(`resources/${org}/assistants/pulled-by-apply.yml`, "name: Pulled\n"); + }; +} + +function appliedPaths(): string[] { + const file = join(ROOT, APPLIED_PATHS_FILE); + return existsSync(file) + ? readFileSync(file, "utf8").split("\n").filter(Boolean) + : []; +} + +process.env.VAPI_PROMOTION_TOKENS = JSON.stringify({ a: "t", b: "t", c: "t" }); + +test("after one transition applies and the next fails, only the applied transition's files are recorded", async () => { + fixtureReset(); + process.env.VAPI_PROMOTION_TOKENS = JSON.stringify({ + a: "t", + b: "t", + c: "t", + }); + const calls: ChildCall[] = []; + const log = console.log; + console.log = () => {}; + let failure: unknown; + try { + await promotionCommandRun(["--all", "--apply"], { + childRun: childRunFake(calls, "c"), + }); + } catch (error) { + failure = error; + } finally { + console.log = log; + } + assert.deepEqual( + { + failure: (failure as Error | undefined)?.message, + applies: calls + .filter((c) => c.script === "src/apply.ts") + .map((c) => c.org), + recorded: appliedPaths(), + // The failed transition's rewrites are on disk but not recorded. + cRewritten: existsSync(join(ROOT, "resources/c/assistants/intake.yml")), + }, + { + failure: "src/apply.ts failed for c", + applies: ["b", "c"], + recorded: [ + "resources/b/assistants/intake.yml", + "resources/b/assistants/pulled-by-apply.yml", + ], + cRewritten: true, + }, + ); +}); + +test("each --apply run starts a fresh record; a plan-only run leaves it alone", async () => { + fixtureReset(); + write(APPLIED_PATHS_FILE, "resources/stale/assistants/old.yml\n"); + const log = console.log; + console.log = () => {}; + try { + await promotionCommandRun(["--all"], { childRun: childRunFake([]) }); + const afterPlan = appliedPaths(); + process.env.VAPI_PROMOTION_TOKENS = JSON.stringify({ + a: "t", + b: "t", + c: "t", + }); + await promotionCommandRun( + ["--pipeline", "release", "--from", "a", "--to", "b", "--apply"], + { + childRun: childRunFake([]), + }, + ); + assert.deepEqual( + [afterPlan, appliedPaths()], + [ + ["resources/stale/assistants/old.yml"], + [ + "resources/b/assistants/intake.yml", + "resources/b/assistants/pulled-by-apply.yml", + ], + ], + ); + } finally { + console.log = log; + } +}); + +test("a deleted or renamed file is recorded by its current path", async () => { + fixtureReset(); + write("resources/b/assistants/old-name.yml", "name: Old\n"); + write("resources/b/assistants/gone.yml", "name: Gone\n"); + git("add", "-A"); + git("commit", "-qm", "b files"); + git( + "mv", + "resources/b/assistants/old-name.yml", + "resources/b/assistants/new-name.yml", + ); + rmSync(join(ROOT, "resources/b/assistants/gone.yml")); + process.env.VAPI_PROMOTION_TOKENS = JSON.stringify({ + a: "t", + b: "t", + c: "t", + }); + const log = console.log; + console.log = () => {}; + try { + await promotionCommandRun( + ["--pipeline", "release", "--from", "a", "--to", "b", "--apply"], + { + childRun: childRunFake([]), + }, + ); + } finally { + console.log = log; + } + assert.deepEqual(appliedPaths().sort(), [ + "resources/b/assistants/gone.yml", + "resources/b/assistants/intake.yml", + "resources/b/assistants/new-name.yml", + "resources/b/assistants/pulled-by-apply.yml", + ]); +}); + +test.after(() => rmSync(ROOT, { recursive: true, force: true }));