From 87bd4d62d85049de6354e51012aea83e04c98aaf Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 09:41:59 +0000 Subject: [PATCH] fix(ship-release): fix_check.files narrows the accepted scope; gate-edited and over-cap paths stay behavior (GST-965) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gst-965-finish-review-loop-exception.md | 4 ++-- docs/review-policy.md | 11 ++++++----- lib/ship-release.ts | 10 ++++++---- test/ship-rereview-triggers.test.ts | 13 +++++++++++++ 4 files changed, 27 insertions(+), 11 deletions(-) diff --git a/changelog.d/gst-965-finish-review-loop-exception.md b/changelog.d/gst-965-finish-review-loop-exception.md index 6052d17a2..876075398 100644 --- a/changelog.d/gst-965-finish-review-loop-exception.md +++ b/changelog.d/gst-965-finish-review-loop-exception.md @@ -1,7 +1,7 @@ **`gstack-ship-release finish` now accepts the owner's edit after a fix-check instead of asking for another review round (GST-965).** On gbrain #8425, the fix-check found a blocker, the owner's pre-agreed fallback was applied and "then ship" was granted. Finish still classed the edit as behavior and stopped with `insert 5–11.5 → 12–14 → 16`, a third round the R51 cap forbids, so /ship could not get through Step 16 without running its stages by hand. ### Added -- **`finish --review-loop-exception `** (`lib/ship-release.ts`) takes the same record `gstack-ship-receipt write --review-loop-exception` takes. Paths changed since `fix_check.head` that the branch already touched at that commit (or that the record's `fix_check.files` lists) route as `accepted=` on the `ROUTE:` line, a `REVIEW_LOOP_EXCEPTION:` line names them with the owner, date and channel, and the docs stage carries them (`accepted=` in the DOCS line). A path outside that scope still routes as behavior. A malformed record, or a `fix_check.head` that is not HEAD or an ancestor of it, stops before the release commit. Without the flag nothing changes. +- **`finish --review-loop-exception `** (`lib/ship-release.ts`) takes the same record `gstack-ship-receipt write --review-loop-exception` takes. Paths changed since `fix_check.head` that the branch already touched at that commit (narrowed to the record's `fix_check.files` when it lists them) route as `accepted=` on the `ROUTE:` line, a `REVIEW_LOOP_EXCEPTION:` line names them with the owner, date and channel, and the docs stage carries them (`accepted=` in the DOCS line). A path outside that scope, a gate-edited test and a file too large for the scoped review still route as behavior. A malformed record, or a `fix_check.head` that is not HEAD or an ancestor of it, stops before the release commit. Without the flag nothing changes. ### Tests -- `test/ship-rereview-triggers.test.ts`: an owner fallback edit to a source file and a doc after the fix-check stops without the record and passes Step 16 with it; a new file outside the scope, a fix-check head off the branch, and a malformed record stop. On main the record is ignored and finish stops on behavior. +- `test/ship-rereview-triggers.test.ts`: an owner fallback edit to a source file and a doc after the fix-check stops without the record and passes Step 16 with it; a new file outside the scope (even when `fix_check.files` lists it), a fix-check head off the branch, a malformed record and an unreadable record file stop; a `fix_check.files` list narrows what is accepted. diff --git a/docs/review-policy.md b/docs/review-policy.md index e9888fd03..c25d74622 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -419,11 +419,12 @@ the queue refuses a receipt below the depth its head needs accepted after a fix-check edit goes in `review_loop_exception`, never in prose under `REVIEW_LOOP:`. Step 16 takes the same record: `gstack-ship-release finish --review-loop-exception ` routes the -paths the branch touched at `fix_check.head` (or the record's -`fix_check.files`) and changed since as `accepted=` in `ROUTE:`, prints them -on a `REVIEW_LOOP_EXCEPTION:` line, and carries them through the docs stage. A -path outside that scope still routes as behavior, and without the record -nothing changes. +paths the branch touched at `fix_check.head` (narrowed to the record's +`fix_check.files` when it lists them) and changed since as `accepted=` in +`ROUTE:`, prints them on a `REVIEW_LOOP_EXCEPTION:` line, and carries them +through the docs stage. A path outside that scope, a gate-edited test and a +file too large for the scoped review still route as behavior, and without the +record nothing changes. The receipt's summary line prints `passes= blocking_found= deferred=`, which is what a scorecard tracks (target: a median of 1 pass per PR). A diff --git a/lib/ship-release.ts b/lib/ship-release.ts index e9e16db3c..10648ac64 100644 --- a/lib/ship-release.ts +++ b/lib/ship-release.ts @@ -32,7 +32,7 @@ export const RELEASE_HELP = `gstack-ship-release — /ship Steps 12 and 15-17 (s Step 15's release commit, Step 16 stages 1-4 (your builds, the reviewed-tree route, docs freshness, each lane's receipt run once if stale) and the pregate on the stamped tree; --reviewed-base (the merge base saved with the reviewed tree) routes paths only the base moved as base=, and an adjacent base edit as context=; - --review-loop-exception (the record write takes) routes paths the branch touched at fix_check.head and changed since as accepted= + --review-loop-exception (the record write takes) routes paths the branch touched at fix_check.head (narrowed to fix_check.files when listed) and changed since as accepted= push --branch [--gate-open ] [--gate-open-approved] Step 17 after its guard block: the saved-branch check, the open-gate check, the sealed guard, the idempotency check, git push -u @@ -201,9 +201,11 @@ function route(cwd: string, base: string, reviewed: string, reviewedBase: string const head = loop.fix_check.head; const since = touchedPaths(cwd, head, now); const fork = git(cwd, ['merge-base', baseRef, head]).stdout.trim(); - const scope = loop.fix_check.files ? new Set(loop.fix_check.files) : fork ? touchedPaths(cwd, fork, head) : null; - if (!since || !scope) { r.stop(`git could not list the paths changed since fix-check ${head.slice(0, 12)} or the paths the branch touched at it against ${baseRef}: stage 2 is blocked`); return null; } - by.accepted = [...by.behavior, ...by.docs].filter(p => since.has(p) && scope.has(p)).sort(); + const atHead = fork ? touchedPaths(cwd, fork, head) : null; + if (!since || !atHead) { r.stop(`git could not list the paths changed since fix-check ${head.slice(0, 12)} or the paths the branch touched at it against ${baseRef}: stage 2 is blocked`); return null; } + const listed = loop.fix_check.files; + const forced = new Set([...gated, ...(packetBody?.over ?? [])]); + by.accepted = [...by.behavior, ...by.docs].filter(p => since.has(p) && atHead.has(p) && (!listed || listed.includes(p)) && !forced.has(p)).sort(); by.behavior = by.behavior.filter(p => !by.accepted.includes(p)); by.docs = by.docs.filter(p => !by.accepted.includes(p)); } diff --git a/test/ship-rereview-triggers.test.ts b/test/ship-rereview-triggers.test.ts index 13350e621..4640b4be1 100644 --- a/test/ship-rereview-triggers.test.ts +++ b/test/ship-rereview-triggers.test.ts @@ -705,6 +705,19 @@ describe('finish with an owner-accepted edit after a fix-check (GST-965, GBRA-27 expect([off.status, off.stdout]).toEqual([3, expect.stringContaining(`STOP: --review-loop-exception: fix_check.head eeeeeeeeeeee is not HEAD or an ancestor of it`)]); const bad = finish(f, reviewed, post, ['--review-loop-exception', `@${exceptionFile(f, fixHead, { reason: '' })}`]); expect([bad.status, bad.stdout]).toEqual([3, expect.stringContaining('STOP: --review-loop-exception: review_loop_exception.reason must be a non-empty one-line string')]); + const missing = finish(f, reviewed, post, ['--review-loop-exception', `@${path.join(f.root, 'no-such-record.json')}`]); + expect([missing.status, missing.stdout]).toEqual([3, expect.stringContaining('STOP: --review-loop-exception: ')]); + }); + + // Value: protects=fix_check.files only narrows the frozen scope; fails_when=a listed path the branch never touched at the + // fix-check routes as accepted, or a touched path the list leaves out is accepted anyway; seam=finish's route + test('fix_check.files narrows the accepted scope and cannot add a path the branch had not touched at the fix-check', () => { + const { f, fixHead, reviewed, post } = afterFixCheck(); + f.write({ 'src/new.ts': 'export const n = 1;\n' }); f.commit('a new file after the fix-check'); + const r = finish(f, reviewed, post, ['--review-loop-exception', `@${exceptionFile(f, fixHead, { fix_check: { head: fixHead, findings: 1, files: ['src/x.ts', 'src/new.ts'] } })}`]); + expect(lineOf(r.stdout, 'ROUTE')).toMatch(/ behavior=1 docs=1 metadata=0 tests=0 base=unknown context=0 accepted=1$/); + expect(lineOf(r.stdout, 'REVIEW_LOOP_EXCEPTION')).toStartWith('REVIEW_LOOP_EXCEPTION: accepted src/x.ts ('); + expect([r.status, r.stdout.includes('STOP: behavior, tests or build inputs changed since review (src/new.ts)')]).toEqual([3, true]); }); });