From 69f0fe181aa19ffda85591e8a765f2c0b3e6cdfd Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Tue, 3 Nov 2026 18:58:09 -0800 Subject: [PATCH] fix(release-gate): a missing report artifact or a sha-less failure fails closed (GST-1263) (#3982) * fix(release-gate): a missing report artifact or a sha-less failure fails closed (GST-1263) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: pre-landing review fixes Review-Finding: p1/red-team#1 reader=red-team Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- .github/workflows/release-gate.yml | 7 ++-- .../capy-gst-1263-release-gate-fail-closed.md | 1 + docs/ship-policy.md | 13 ++++++-- lib/release-gate.ts | 4 +-- test/release-gate-workflow.test.ts | 33 +++++++++++++++++-- test/release-gate.test.ts | 19 +++++++++++ 6 files changed, 66 insertions(+), 11 deletions(-) create mode 100644 changelog.d/capy-gst-1263-release-gate-fail-closed.md diff --git a/.github/workflows/release-gate.yml b/.github/workflows/release-gate.yml index c696e2793..4ca8104c3 100644 --- a/.github/workflows/release-gate.yml +++ b/.github/workflows/release-gate.yml @@ -84,14 +84,15 @@ jobs: GSTACK_STATE_ROOT: ${{ runner.temp }}/gstack-state DRY_RUN: ${{ inputs.dry_run && '1' || '' }} IGNORE_CADENCE: ${{ inputs.ignore_cadence && '1' || '' }} - # held and failed are verdicts the release-check job publishes on the gated commit. Failing this job would + # held and failed on a gated commit are verdicts the release-check job publishes there. Failing this job would # leave a failed check on main's head, which release-cut reads as red even after that head gates green, so - # only a run that recorded no action (a crash) fails it. + # those leave it green. A failure with no gated commit (a refused policy, unreadable notes, an unresolved + # branch) publishes no check run, so it fails the job, as does a run that recorded no action (a crash). run: | rc=0 ./bin/gstack-release-gate run --repo "$GITHUB_REPOSITORY" --budget-minutes 330 \ --summary "$GITHUB_STEP_SUMMARY" --outputs "$GITHUB_OUTPUT" ${DRY_RUN:+--dry-run} ${IGNORE_CADENCE:+--ignore-cadence} || rc=$? - if [ "$rc" != 0 ] && ! grep -qE '^action=(held|failed)$' "$GITHUB_OUTPUT"; then exit "$rc"; fi + if [ "$rc" != 0 ] && ! { grep -qE '^action=(held|failed)$' "$GITHUB_OUTPUT" && grep -qE '^sha=[0-9a-f]{40}$' "$GITHUB_OUTPUT"; }; then exit "$rc"; fi # The gated commit's check run (release_gate.check_name, main-backstop), the # evidence scripts/release-cut.ts reads. Its own job with checks: write and diff --git a/changelog.d/capy-gst-1263-release-gate-fail-closed.md b/changelog.d/capy-gst-1263-release-gate-fail-closed.md new file mode 100644 index 000000000..239204131 --- /dev/null +++ b/changelog.d/capy-gst-1263-release-gate-fail-closed.md @@ -0,0 +1 @@ +- **The release gate fails closed.** A gate run that concluded success but left no report-verdict artifact was recorded GREEN; it is now INCOMPLETE and re-dispatched once like any other incomplete gate. A release-gate run that failed before it had a commit to gate (a refused policy, unreadable notes, an unresolved branch) left the workflow job green with no check run anywhere; that job now fails. (GST-1263) diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 6473c7e48..1ba1ccf28 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -1166,8 +1166,10 @@ policy read, so no paid work starts and nothing is written. With the block it ru 4. dispatches the full gate on that sha and reads its verdict and flake records. A case that failed once and passed its one rule-16 rerun is green and is listed as flaky; -5. records GREEN, RED or INCOMPLETE. An INCOMPLETE gate is re-dispatched once - by the next run, then left for a person; +5. records GREEN, RED or INCOMPLETE. GREEN needs the run's success and a + report that says GREEN, so a missing report artifact is INCOMPLETE. An + INCOMPLETE gate is re-dispatched once by the next run, then left for a + person; 6. on RED, bisects the red cases over main's first-parent commits since the last green gate (at most 4 rounds, within the spend cap) and records the first bad commit; @@ -1175,7 +1177,12 @@ policy read, so no paid work starts and nothing is written. With the block it ru `checks: write` and runs no repository code: success for a green gate, failure for a red one, and neutral when the gate has no verdict yet (still running at the budget, or incomplete), so a later green on the same commit - is not undercut by an earlier failure. + is not undercut by an earlier failure. A run that fails before it has a + commit to gate (a refused policy, unreadable notes, an unresolved branch) + publishes no check run, so it fails the workflow job instead. That failed + job is a red check run on main's head, so release-cut refuses that commit + until a later release-gate run on it succeeds (release-cut reads the + latest check run of each name). **What release-cut reads.** A GREEN release record on a commit counts as gated, the same as a successful `main-backstop` check run. A RED record diff --git a/lib/release-gate.ts b/lib/release-gate.ts index af586a1df..9e5b61bfb 100644 --- a/lib/release-gate.ts +++ b/lib/release-gate.ts @@ -114,8 +114,8 @@ export function parseReport(text: string): ReportFacts { export function classifyRun(run: Pick, report: ReportFacts | null): { verdict: ReleaseRecord['verdict']; red: string[]; detail: string } { if (run.status !== 'completed') return { verdict: 'PENDING', red: [], detail: `run is ${run.status}` }; if (run.conclusion === 'success') { - if (report?.verdict === 'RED') return { verdict: 'INCOMPLETE', red: [], detail: 'the run concluded success but its report says RED' }; - return { verdict: 'GREEN', red: [], detail: report ? 'run and report green' : 'run concluded success (no report artifact)' }; + if (report?.verdict === 'GREEN') return { verdict: 'GREEN', red: [], detail: 'run and report green' }; + return { verdict: 'INCOMPLETE', red: [], detail: report ? `the run concluded success but its report says ${report.verdict}` : 'the run concluded success but its report artifact is missing' }; } if (report?.verdict === 'RED' && report.red.length) { return { verdict: 'RED', red: report.red, detail: `${report.red.length} red case(s)${report.other.length ? `; also not green: ${report.other.join(', ')}` : ''}` }; diff --git a/test/release-gate-workflow.test.ts b/test/release-gate-workflow.test.ts index cc8ce50e9..bed0021c5 100644 --- a/test/release-gate-workflow.test.ts +++ b/test/release-gate-workflow.test.ts @@ -59,8 +59,8 @@ const TRAIN = { gate_workflow: 'evals.yml', record_artifact: 'gstack-train-recor describe('release-gate.yml', () => { // Value: protects=the schedule fires every RELEASE_GATE_SCHEDULE_HOURS (the cadence floor the policy validates), one gate at a time, only from the // default branch, with contents and actions write but no checks write, and runs the bin without --sha (main's head from the remote, so an open gate on an older commit - // is finished first) with its outputs wired, and a held or failed verdict leaves the job green (only a crash fails it, so main's head never carries a - // failed native check release-cut would read as red); the policy step + // is finished first) with its outputs wired, and a held or failed verdict on a gated commit leaves the job green (so main's head never carries a + // failed native check release-cut would read as red; the exit is exercised below); the policy step // uses git and jq only and is inert without a release_gate block; fails_when=the cron and the cadence drift apart, two gates overlap, the job that // runs repository code can write check runs, or an inert policy reaches bun; why_new=new workflow; seam=none; no_claim=GitHub's cron jitter test('schedule, concurrency, permissions and the gate step', () => { @@ -84,11 +84,38 @@ describe('release-gate.yml', () => { expect(gate.run).toContain('./bin/gstack-release-gate run --repo "$GITHUB_REPOSITORY" --budget-minutes'); expect(gate.run).not.toContain('--sha'); expect(gate.run).toContain("|| rc=$?"); - expect(gate.run).toContain(`if [ "$rc" != 0 ] && ! grep -qE '^action=(held|failed)$' "$GITHUB_OUTPUT"; then exit "$rc"; fi`); + expect(gate.run).toContain(`if [ "$rc" != 0 ] && ! { grep -qE '^action=(held|failed)$' "$GITHUB_OUTPUT" && grep -qE '^sha=[0-9a-f]{40}$' "$GITHUB_OUTPUT"; }; then exit "$rc"; fi`); expect(gate.run).toContain('--outputs "$GITHUB_OUTPUT"'); expect(job.outputs).toEqual({ action: '${{ steps.gate.outputs.action }}', sha: '${{ steps.gate.outputs.sha }}', check: '${{ steps.gate.outputs.check }}' }); }); + // Value: protects=the gate step's exit: a held or failed verdict on a gated commit leaves the job green (release-check publishes its check run), but a + // failed verdict with no commit (a refused policy, unreadable notes, an unresolved branch) or no recorded action fails the job, since no check run + // follows it; fails_when=a sha-less failure leaves the job green while the backstop stands down (GST-1263); why_new=reader note on #3917; + // seam=a stub bin in place of bin/gstack-release-gate; no_claim=a live scheduled run + test('the gate step fails the job when the bin fails without a gated commit', () => { + const gate = release.jobs['release-gate']!.steps.find(s => s.id === 'gate')!; + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'release-gate-step-')); + scratch.push(dir); + const sha = 'a'.repeat(40); + const exit = (written: string, code: number) => { + const stub = path.join(dir, 'stub'); + fs.writeFileSync(stub, `#!/usr/bin/env bash\nprintf '%s' '${written}' >> "$GITHUB_OUTPUT"\nexit ${code}\n`, { mode: 0o755 }); + const out = path.join(dir, 'out'); + fs.writeFileSync(out, ''); + const r = spawnSync('bash', ['-e', '-c', gate.run!.replace('./bin/gstack-release-gate', stub)], { + cwd: dir, encoding: 'utf8', timeout: 30_000, + env: { ...process.env, GITHUB_OUTPUT: out, GITHUB_STEP_SUMMARY: path.join(dir, 'summary'), GITHUB_REPOSITORY: 'o/r', DRY_RUN: '', IGNORE_CADENCE: '' }, + }); + return r.status; + }; + expect(exit(`action=gated\nsha=${sha}\ncheck=main-backstop\n`, 0)).toBe(0); + expect(exit(`action=held\nsha=${sha}\ncheck=main-backstop\n`, 1)).toBe(0); + expect(exit(`action=failed\nsha=${sha}\ncheck=main-backstop\n`, 1)).toBe(0); + expect(exit('action=failed\nsha=\ncheck=\n', 1)).toBe(1); + expect(exit('', 1)).toBe(1); + }); + // Value: protects=the gated commit's check run (what release-cut reads) comes from a job with checks: write and no checkout or action, only for a // gate that ran (success for gated, failure for held, neutral for failed so a later green on the same sha stays releasable), on a validated sha // and check name; fails_when=repository code runs with checks: write, a skipped or inert run publishes a check, a red gate publishes success, or diff --git a/test/release-gate.test.ts b/test/release-gate.test.ts index 957adb238..07e79ab39 100644 --- a/test/release-gate.test.ts +++ b/test/release-gate.test.ts @@ -174,6 +174,18 @@ describe('release gate: run', () => { expect(activeHold(records(f), () => true)).toBeNull(); }); + // Value: protects=a successful gate run whose report-verdict artifact is missing records INCOMPLETE, prints no green line and publishes no gated + // action, so a lost artifact never reads as a green main; fails_when=run success alone records GREEN (GST-1263); why_new=reader note on #3917; + // seam=fake gh; no_claim=a real artifact upload failure + test('a successful run with no report artifact is incomplete, never gated', () => { + const f = fixture(); + const fake = fakeGh(() => ({ conclusion: 'success', artifacts: {} })); + const out = runGate(envFor(f, fake.gh), { gate: GATE, label: 'test', dryRun: false, ignoreCadence: true }); + expect(out.action, out.lines.join('\n')).toBe('failed'); + expect(out.lines.some(l => l.startsWith('RELEASE: green'))).toBe(false); + expect(records(f)[0]).toMatchObject({ sha: f.c[4], verdict: 'INCOMPLETE' }); + }); + // Value: protects=a gate run still going at the budget stays PENDING with its run, and the next run finishes that verdict on the old commit before // gating main's new head, so a red verdict still records RED and holds release-cut after main moved; the new head is gated on the run after; // fails_when=main moving orphans a PENDING record and its red verdict never becomes a hold; why_new=review finding (red-team, l1-p1); seam=fake gh; @@ -346,6 +358,13 @@ describe('release gate: verdict, range and policy units', () => { expect(classifyRun({ status: 'in_progress', conclusion: null }, null).verdict).toBe('PENDING'); }); + // Value: protects=green needs the run's success and a report that says GREEN; a missing or verdict-less report on a successful run is INCOMPLETE; + // fails_when=a lost or truncated report-verdict artifact records GREEN (GST-1263); why_new=reader note on #3917; seam=none; no_claim=report-verdict's writer + test('classifyRun: a successful run without a GREEN report is incomplete', () => { + expect(classifyRun({ status: 'completed', conclusion: 'success' }, null)).toMatchObject({ verdict: 'INCOMPLETE', detail: 'the run concluded success but its report artifact is missing' }); + expect(classifyRun({ status: 'completed', conclusion: 'success' }, { verdict: 'NONE', red: [], other: [], cost: null }).verdict).toBe('INCOMPLETE'); + }); + test('planPrefixes spreads at most MAX_PREFIXES over the interior; narrow moves good and bad and refuses a non-monotone or missing result', () => { const shas = Array.from({ length: 20 }, (_, i) => String(i).padStart(40, '0')); expect(planPrefixes(shas.slice(0, 3))).toEqual(shas.slice(0, 3));