From 01593be831ac1656e7bda88545dfd9bfcf480e22 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 02:16:21 +0000 Subject: [PATCH] fix(ship-release): fix-check range base must hold the reviewed tree; docs keep the recheck (review round 1) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gst-1130-fix-check-range.md | 4 ++-- docs/review-policy.md | 11 ++++++----- lib/ship-release.ts | 16 +++++++++------- test/ship-rereview-triggers.test.ts | 15 +++++++++++++++ 4 files changed, 32 insertions(+), 14 deletions(-) diff --git a/changelog.d/gst-1130-fix-check-range.md b/changelog.d/gst-1130-fix-check-range.md index 0bae56e39..2028961bd 100644 --- a/changelog.d/gst-1130-fix-check-range.md +++ b/changelog.d/gst-1130-fix-check-range.md @@ -1,9 +1,9 @@ **A granted fix-check's clean read now counts at Step 16.** On gstack-evals#168 the owner granted one scoped fix-check, the reader read `9b0c56c..7ea1907` and found nothing, and `gstack-ship-release finish` still stopped with `STOP: behavior, tests or build inputs changed since review (tests/panel-kit-v3-1.test.ts)`, sending the test file the fix-check had read back through Steps 5 to 11.5. The review-loop exception could not help, because it is the owner accepting unreviewed edits and it never accepts a gate-edited test. The fix-check row also read `UNVERIFIED`, because it was logged without its `completed` and `converged` fields (GST-1130, GEVA-218). ### Fixed -- **`gstack-ship-release finish`** reads the newest `review-fix-check` row. When the row is a completed clean read on committed content with its `range_base` on the branch, every path inside that range that matched the reviewed tree at the range base and is unchanged since routes as `fix_checked=` on the `ROUTE:` line, with a `FIX_CHECK: reviewed ` line. That includes a gate-edited test. A path edited after the fix-check still stops. A row that does not qualify prints `FIX_CHECK: not counted ()` and changes nothing. +- **`gstack-ship-release finish`** reads the newest `review-fix-check` row. When the row is a completed clean read on committed content and its `range_base` is a branch commit holding the reviewed tree, every code or test path inside that range that is unchanged since routes as `fix_checked=` on the `ROUTE:` line, with a `FIX_CHECK: reviewed ` line. That includes a gate-edited test. Authored docs still get the plan-item recheck, and a path edited after the fix-check still stops. A row that does not qualify prints `FIX_CHECK: not counted ()` and changes nothing. - **`gstack-review-log`** refuses a `review-fix-check` row that does not carry `completed` and `converged` as true or false, and keeps the start token for the retry. The refusal names the missing fields, so the row is fixed when it is written instead of reading `UNVERIFIED` at Step 11.5. ### Tests -- `test/ship-rereview-triggers.test.ts`: a fixture shaped like gstack-evals#168 (a round-2 pass, then a fix commit that edits the source and a gate-edited test) stops on main and passes finish once a clean fix-check row with its range base is logged through the real `gstack-review-log`. Negative controls: an edit after the fix-check still stops, and an incomplete row or a row with no range base is not counted. +- `test/ship-rereview-triggers.test.ts`: a fixture shaped like gstack-evals#168 (a round-2 pass, then a fix commit that edits the source and a gate-edited test) stops on main and passes finish once a clean fix-check row with its range base is logged through the real `gstack-review-log`. Negative controls: an edit after the fix-check still stops, and an incomplete row, a row with no range base, a range base that does not hold the reviewed tree, or a row logged on uncommitted content is not counted. - `test/review-log-findings.test.ts`: a `review-fix-check` row missing either field, or with a non-boolean value, is refused and keeps its start token; the complete row binds as verified. diff --git a/docs/review-policy.md b/docs/review-policy.md index 24057a892..a432fdf91 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -496,11 +496,12 @@ too large for the scoped review, and without the record nothing changes. A completed fix-check is a review, not an owner acceptance (GST-1130). Finish reads the newest `review-fix-check` row from `gstack-review-read`. When it is a completed clean read on committed content (freshness `CURRENT` against its own -tree) whose `range_base` and commit are on the branch, each path inside -`range_base..commit` that matched the reviewed tree at `range_base` and is -unchanged since routes as `fix_checked=` in `ROUTE:` and is listed on a -`FIX_CHECK: reviewed` line, a gate-edited test included. A path edited after the -fix-check still routes by the rules above. Any other row prints +tree) whose commit is on the branch and whose `range_base` is a branch commit +holding the reviewed tree, each code or test path inside `range_base..commit` +that is unchanged since routes as `fix_checked=` in `ROUTE:` and is listed on a +`FIX_CHECK: reviewed` line, a gate-edited test included. Authored docs keep the +stage 2.2 recheck, and a path edited after the fix-check still routes by the +rules above. Any other row prints `FIX_CHECK: not counted ()` and changes nothing. `gstack-review-log` refuses a `review-fix-check` row without boolean `completed` and `converged`, before it consumes the start token, because without them the row reads diff --git a/lib/ship-release.ts b/lib/ship-release.ts index 088d40e6f..fcf13475f 100644 --- a/lib/ship-release.ts +++ b/lib/ship-release.ts @@ -163,8 +163,9 @@ function commitRelease(cwd: string, release: string[], trailer: string | undefin /** * Step 16 stage 2's route. With --reviewed-base, a path whose branch patch is the reviewed one counts as base (lib/ship-patch.ts). - * A path the newest completed clean review-fix-check row read (inside its range_base..commit range, the same at range_base as - * in the reviewed tree, and unchanged since) counts as fix_checked: a granted fix-check after Step 11 is a review (GST-1130). + * A code or test path the newest completed clean review-fix-check row read (inside its range_base..commit range, where + * range_base holds the reviewed tree, and unchanged since) counts as fix_checked: a granted fix-check after Step 11 is a review + * (GST-1130). Authored docs keep the stage 2.2 recheck. */ type Routed = Record<'behavior' | 'docs' | 'metadata' | 'tests' | 'base' | 'context' | 'accepted' | 'fix_checked', string[]>; function route(cwd: string, base: string, reviewed: string, reviewedBase: string | undefined, release: string[], opts: { docsRechecked: boolean; contextReviewed?: string; loop?: ReviewLoopException }, r: Report): Routed | null { @@ -209,14 +210,15 @@ function route(cwd: string, base: string, reviewed: string, reviewedBase: string const fresh = fixRow && reviewFreshness(fixRow, fixRow.wtree); const notCounted = !fixRow ? '' : fresh?.status !== 'CURRENT' ? fresh?.reason ?? 'not a review row' : fixRow.tree !== fixRow.wtree || !onBranch(fixCommit) ? 'the row was logged on content no branch commit holds' - : !onBranch(fixFrom) ? 'the row names no range_base commit on this branch' : ''; + : !onBranch(fixFrom) ? 'the row names no range_base commit on this branch' + : git(cwd, ['rev-parse', `${fixFrom}^{tree}`]).stdout.trim() !== reviewed ? `range_base ${fixFrom.slice(0, 12)} does not hold the reviewed tree` : ''; if (notCounted) r.add(`FIX_CHECK: not counted (${notCounted})`); const counted = !!fixRow && !notCounted; if (counted) { - const range = touchedPaths(cwd, fixFrom, fixCommit), after = touchedPaths(cwd, fixCommit, now), before = touchedPaths(cwd, reviewed, fixFrom); - if (!range || !after || !before) { r.stop(`git could not diff the fix-check range ${fixFrom.slice(0, 12)}..${fixCommit.slice(0, 12)} against the reviewed and current trees: stage 2 is blocked`); return null; } - by.fix_checked = [...by.behavior, ...by.tests, ...by.context, ...by.docs].filter(p => range.has(p) && !after.has(p) && !before.has(p)).sort(); - for (const k of ['behavior', 'tests', 'context', 'docs'] as const) by[k] = by[k].filter(p => !by.fix_checked.includes(p)); + const range = touchedPaths(cwd, fixFrom, fixCommit), after = touchedPaths(cwd, fixCommit, now); + if (!range || !after) { r.stop(`git could not diff the fix-check range ${fixFrom.slice(0, 12)}..${fixCommit.slice(0, 12)} against the reviewed and current trees: stage 2 is blocked`); return null; } + by.fix_checked = [...by.behavior, ...by.tests, ...by.context].filter(p => range.has(p) && !after.has(p)).sort(); + for (const k of ['behavior', 'tests', 'context'] as const) by[k] = by[k].filter(p => !by.fix_checked.includes(p)); r.add(`FIX_CHECK: reviewed ${by.fix_checked.length ? short(by.fix_checked) : 'none'} (fix-check at ${fixCommit.slice(0, 12)}, range from ${fixFrom.slice(0, 12)})`); } const loop = opts.loop; diff --git a/test/ship-rereview-triggers.test.ts b/test/ship-rereview-triggers.test.ts index 54abd32d7..6507a4461 100644 --- a/test/ship-rereview-triggers.test.ts +++ b/test/ship-rereview-triggers.test.ts @@ -785,5 +785,20 @@ describe('finish after a completed fix-check (GST-1130, GEVA-218)', () => { const unranged = finish(f, reviewed, post); expect(lineOf(unranged.stdout, 'FIX_CHECK')).toBe('FIX_CHECK: not counted (the row names no range_base commit on this branch)'); expect(unranged.status).toBe(3); + const fork = mergeBase(f); + expect(logFixCheck(f, { completed: true, converged: true, range_base: fork }).status).toBe(0); + const early = finish(f, reviewed, post); + expect(lineOf(early.stdout, 'FIX_CHECK')).toBe(`FIX_CHECK: not counted (range_base ${fork.slice(0, 12)} does not hold the reviewed tree)`); + expect(early.status).toBe(3); + }); + + test('a fix-check logged on uncommitted content is not counted', () => { + const { f, passHead, reviewed, post } = afterFixCheck(); + f.write({ 'src/x.ts': 'export const x = 9;\n' }); + expect(logFixCheck(f, { completed: true, converged: true, range_base: passHead }).status).toBe(0); + f.git('checkout', '--', 'src/x.ts'); + const r = finish(f, reviewed, post); + expect(lineOf(r.stdout, 'FIX_CHECK')).toBe('FIX_CHECK: not counted (the row was logged on content no branch commit holds)'); + expect(r.status).toBe(3); }); });