From 673a336bc447061c445a53ff44f50486556053a0 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 6 Nov 2026 05:06:50 -0800 Subject: [PATCH] ship finish: FIX_CHECK names the grant that covers the fix-check (GST-1610) (#4070) * test: failing-first FIX_CHECK names the covering grant with two grants at different heads (GST-1610) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(ship): finish names the grant whose stop head is newest in the fix-check range (GST-1610) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs: FIX_CHECK grant attribution (GST-1610) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs: changelog fragment prose lint (GST-1610) 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> --- changelog.d/capy-gst-1610-fix-check-grant.md | 1 + docs/review-policy.md | 4 ++- lib/ship-release.ts | 5 ++-- test/ship-finish-ruled-notes.test.ts | 31 ++++++++++++++++++++ 4 files changed, 38 insertions(+), 3 deletions(-) create mode 100644 changelog.d/capy-gst-1610-fix-check-grant.md diff --git a/changelog.d/capy-gst-1610-fix-check-grant.md b/changelog.d/capy-gst-1610-fix-check-grant.md new file mode 100644 index 000000000..6a90cb5ad --- /dev/null +++ b/changelog.d/capy-gst-1610-fix-check-grant.md @@ -0,0 +1 @@ +- **`/ship` finish's `FIX_CHECK:` line names the grant that covered the fix-check.** When a branch held two approved `review_loop_grant` rows whose stop heads both fell inside the fix-check's `range_base..commit`, finish named the first one recorded. On gstack-evals#244 the line credited fix-check fc4 to the earlier fix-check's grant, so the trail misattributed the owner's approval. finish now names the grant whose stop head is newest in the range, which is the stop the fix-check answered. Whether the fix-check counts is unchanged. (GST-1610) diff --git a/docs/review-policy.md b/docs/review-policy.md index 20ec7f7b0..91af7d291 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -694,7 +694,9 @@ holding the reviewed tree, and for which an approved `review_loop_grant` in that is unchanged since, or whose change against the base is still the one the fix-check read after a base merge also edited it (GST-1195), routes as `fix_checked=` in `ROUTE:` and is listed on a -`FIX_CHECK: reviewed` line, a gate-edited test included. Authored docs keep the +`FIX_CHECK: reviewed` line, a gate-edited test included. The line names the +grant whose stop head is newest in that range, the fix-check's own when several +grants fall inside it (GST-1610). Authored docs keep the stage 2.2 recheck, and a path the branch 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` diff --git a/lib/ship-release.ts b/lib/ship-release.ts index ee9b7edd0..56f33af8d 100644 --- a/lib/ship-release.ts +++ b/lib/ship-release.ts @@ -295,8 +295,9 @@ function route(cwd: string, base: string, reviewed: string, reviewedBase: Review for (const n of rulings) r.add(`RULED_NOTE: ${ruledNoteText(n)}${fixRow && !n.applied ? ' (outside this fix-check\'s range; not applied)' : ''}`); const fresh = fixRow && reviewFreshness(fixRow, fixRow.wtree, new Set(rulings.filter(n => n.applied).map(n => n.finding))); const grants = fixRow ? (loc => 'file' in loc ? readRecord(loc).rows : [])(locate({ cwd, env: process.env })) : []; - const grant = grants.find(g => g.outcome === 'approved' && `${g.ask} ${g.scope}`.includes('review_loop_grant') - && (`${g.ask} ${g.scope}`.match(/\b[0-9a-f]{7,40}\b/g) ?? []).some(s => { const c = commitOf(s); return !!c && inFixRange(c); })); + const covering = grants.filter(g => g.outcome === 'approved' && `${g.ask} ${g.scope}`.includes('review_loop_grant')) + .flatMap(g => (`${g.ask} ${g.scope}`.match(/\b[0-9a-f]{7,40}\b/g) ?? []).map(commitOf).filter(c => !!c && inFixRange(c)).map(stop => ({ g, stop }))); + const grant = covering.reduce((best, c) => (!best || ancestor(best.stop, c.stop) ? c : best), undefined)?.g; 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' diff --git a/test/ship-finish-ruled-notes.test.ts b/test/ship-finish-ruled-notes.test.ts index 253fd6e5a..85e62a6d4 100644 --- a/test/ship-finish-ruled-notes.test.ts +++ b/test/ship-finish-ruled-notes.test.ts @@ -176,6 +176,37 @@ describe('finish with owner-ruled notes (GST-1537)', () => { }); }); +// Value: protects=the trail names the owner approval that actually covered the fix-check (gstack-evals#244: finish named the +// earlier fix-check's grant a-ab110530e792 for fc4, whose grant was a-eaa651ef9ed3); fails_when=finish takes the first approved +// review_loop_grant naming any commit in range_base..commit; seam=finish's route picking the grant for the FIX_CHECK line +describe('finish names the grant that covers the counted fix-check (GST-1610)', () => { + test('two grants at different stop heads: the line names the later fix-check\'s own grant', () => { + const CLEAN = { completed: true, converged: true, status: 'clean', issues_found: 0, critical: 0, informational: 0, findings: [] }; + const f = shipFixture({ 'src/x.ts': 'export const x = 1;\n' }, { 'src/x.ts': 'export const x = 2;\n', 'test/x.test.ts': TEST_V1 }); + ships.push(f); + const passHead = f.git('rev-parse', 'HEAD').stdout.trim(); + const reviewed = f.run(path.join(BIN, 'gstack-wtree'), []).stdout.trim(); + const post = path.join(f.root, 'post.json'); + expect(f.run(path.join(BIN, 'gstack-docs-candidate'), ['snapshot', '--out', post, '--audit-id', 'a1', '--mode', 'edit', '--base', 'main', '--docs', 'README.md']).status).toBe(0); + const grantAt = (head: string, n: string) => { + const g = run(f, 'gstack-approvals', ['add', '--ask', `Owner grant: scoped fix-check ${n} (review_loop_grant ${head.slice(0, 12)})`, '--words', `fix-check ${n}`, '--by', 'evals minder (GEVA-2)', '--scope', 'this /ship invocation', '--json']); + expect(g.status, g.stderr).toBe(0); + return JSON.parse(g.stdout).added[0] as string; + }; + const first = grantAt(passHead, 'fc3'); + f.write({ 'src/x.ts': 'export const x = 3;\n' }); f.commit('fix the fc3 blocker'); + const fc3Head = f.git('rev-parse', 'HEAD').stdout.trim(); + logFixCheck(f, { range_base: passHead, ...CLEAN }); + const second = grantAt(fc3Head, 'fc4'); + f.write({ 'test/x.test.ts': TEST_V2 }); f.commit('fix the fc4 blocker'); + const fc4Head = f.git('rev-parse', 'HEAD').stdout.trim(); + logFixCheck(f, { range_base: passHead, ...CLEAN }); + expect(first).not.toBe(second); + const r = finish(f, reviewed, post); + expect(lineOf(r.stdout, 'FIX_CHECK'), r.stdout).toBe(`FIX_CHECK: reviewed src/x.ts,test/x.test.ts (fix-check at ${fc4Head.slice(0, 12)}, range from ${passHead.slice(0, 12)}, grant ${second})`); + }); +}); + describe('ruledNotes reads only approved owner rulings (GST-1537)', () => { const HEAD = '0123456789abcdef0123456789abcdef01234567'; const row = (finding: string, o: Partial<{ ask: string; scope: string; by: string; outcome: string }> = {}) => {