From 37cfb481be033ef0326bd877eaab180fdafe7e3b Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 31 Oct 2026 09:15:22 +0000 Subject: [PATCH] fix: pass-2 review fixes A test edit routes as tests= only when it adds lines with no gate tag; a removed or changed line or an unreadable test diff goes through the full loop. Diffs skip textconv. A hunkless patch (binary, empty, mode-only) is same when its patch-id matches. Without --reviewed-base test files route as before. finish's packet path and attempt's state directory have fallbacks. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-docs-candidate | 1 + changelog.d/gst-407-ship-rereview-triggers.md | 2 +- lib/ship-patch.ts | 22 +++++++--- lib/ship-release.ts | 4 +- test/ship-rereview-triggers.test.ts | 43 +++++++++++++++++-- 5 files changed, 59 insertions(+), 13 deletions(-) diff --git a/bin/gstack-docs-candidate b/bin/gstack-docs-candidate index 511372c17..9d8476e51 100755 --- a/bin/gstack-docs-candidate +++ b/bin/gstack-docs-candidate @@ -185,6 +185,7 @@ function main(argv: string[]): void { console.log(`STOP: run the install fix (${install}) and confirm its gstack: ok trailer before another launch`); process.exit(3); } + fs.mkdirSync(path.dirname(path.resolve(state)), { recursive: true }); fs.writeFileSync(state, JSON.stringify({ not_counted: seen + 1 }) + '\n', { mode: 0o600 }); console.log(`ATTEMPT: not-counted (install refused: ${install})`); return; diff --git a/changelog.d/gst-407-ship-rereview-triggers.md b/changelog.d/gst-407-ship-rereview-triggers.md index 54b69d459..c10753afb 100644 --- a/changelog.d/gst-407-ship-rereview-triggers.md +++ b/changelog.d/gst-407-ship-rereview-triggers.md @@ -1,7 +1,7 @@ **/ship no longer pays for a second review loop when only main moved, a test file changed, or the docs audit's own requested fix landed.** Each of these used to count as a product change: a base merge after review (`ROUTE: … behavior=86` on gbrain #7739), main rewording the line next to a reviewed hunk (#3631, founderstar#441), a test-only fix (gbrain #8084), and a one-line index link the audit asked for (gbrain #8087). Each one cost another ~$19 loop or an owner's documentation exception. A real product change still sends the PR back through the full loop (GST-407, GST-384, GST-391). ### Changed -- **`gstack-ship-release finish --reviewed-base `** (the merge base /ship now saves with the reviewed tree) compares the branch's own patch per path. Paths only the base moved count as `base=`. Test code files with no gate edit (no skipped or removed test, snapshot or relaxed gate value) count as `tests=`, and the lanes rerun them. Without the flag the route is unchanged and prints `base=unknown`. +- **`gstack-ship-release finish --reviewed-base `** (the merge base /ship now saves with the reviewed tree) compares the branch's own patch per path. Paths only the base moved count as `base=`. A test code edit that only adds lines, with no gate edit (a skipped test, a snapshot or a relaxed gate value), counts as `tests=`, and the lanes rerun it. A removed or changed line, such as a weakened assertion, or a test file the scan cannot read still goes through the full loop. Without the flag the route is unchanged and prints `base=unknown`. - **Adjacent base edits get a scoped review, not the full loop.** When the branch's +/- lines are the reviewed ones in the same places and only context lines the base edited differ, `finish` stops with `context=N` and `gstack-ship-receipt carry` with `RECEIPT_CARRY_SCOPED_REVIEW`. Both write a packet with just those hunks. After one fix-check reviewer comes back clean, `--context-reviewed ` or `carry --scoped-review ` continues, and the carry reason names the reviewer. The same lines moved somewhere else are still a product change. - **Docs audit freshness** ignores test code and paths only the base moved. `--docs-requested --docs-result ` carries a fix the audit's own result asked for when its blockers or decisions name that exact path, the edit is at most 5 lines, and the doc caps still pass. - **`gstack-docs-candidate attempt --state `** says whether an audit launch counts against Step 14.5's budget. A launch the child's install check refused is not counted, once per invocation. A second refusal stops for the install fix. diff --git a/lib/ship-patch.ts b/lib/ship-patch.ts index 1b9ae98e5..d3451e370 100644 --- a/lib/ship-patch.ts +++ b/lib/ship-patch.ts @@ -13,7 +13,9 @@ * The merge may still have spliced the two (a JSON entry where the branch changed one key and * the base the next), so a context path needs a scoped review of its hunks, never a silent pass; * - changed: anything else, including the same lines moved to another place, a file-mode change, and any path whose - * patches cannot be read (an empty patch on both sides of a path the branch touches). + * patches cannot be read (an empty patch on both sides of a path the branch touches). A binary, empty or mode-only + * patch has no hunks to place, so it is same when its patch-id matches and changed otherwise. + * Diffs skip external drivers and textconv, so a configured filter cannot hide a line. * Paths reach git as literal pathspecs from NUL-separated listings, so quoted names and glob * characters cannot widen or empty a comparison. */ @@ -39,7 +41,7 @@ export function touchedPaths(repoRoot: string, from: string, to: string): Set h.body !== c0[i]!.body)) return 'changed'; const map = mapper(b0); @@ -143,13 +145,19 @@ export function isTestCode(rel: string): boolean { return matchesAny(CARRY_TEST_CODE_GLOBS, rel) && !/(^|\/)(helpers|fixtures|__fixtures__|__snapshots__)\//.test(rel); } -/** Test paths whose change from..to carries a gate edit (lib/gate-diff RH tags: a removed or skipped test, a snapshot, a relaxed gate value). */ +/** + * Test paths whose change from..to needs review: any removed or changed line (a weakened assertion or a changed + * expected value carries no gate tag), a gate edit lib/gate-diff tags (a skipped test, a snapshot, a relaxed gate + * value), or a diff the scan cannot read. Only added lines with no tag stay a lanes-only rerun. + */ export function gateEditedPaths(repoRoot: string, from: string, to: string, paths: readonly string[]): Set | null { if (!paths.length) return new Set(); - const diff = git(repoRoot, ['--literal-pathspecs', 'diff', '--no-color', '--no-ext-diff', '--no-renames', from, to, '--', ...paths]); + const diff = git(repoRoot, ['--literal-pathspecs', 'diff', '--no-color', '--no-ext-diff', '--no-textconv', '--no-renames', from, to, '--', ...paths]); if (diff === null) return null; - const scan = scanCandidate({ files: parseUnifiedDiff(diff), commits: [], project: EMPTY_PROJECT_CONFIG, specIsTestPath: true, candidate: 'route', artifact: '' }); - return new Set(scan.hunks.filter(h => h.tags.length > 0).map(h => h.path)); + const files = parseUnifiedDiff(diff); + const scan = scanCandidate({ files, commits: [], project: EMPTY_PROJECT_CONFIG, specIsTestPath: true, candidate: 'route', artifact: '' }); + const removed = paths.filter(p => (pathDiff(repoRoot, from, to, p, 0) ?? '-').split('\n').some(l => (l.startsWith('-') && !l.startsWith('--- ')) || l.startsWith('Binary files ') || l.startsWith('GIT binary patch'))); + return new Set([...scan.hunks.filter(h => h.tags.length > 0).map(h => h.path), ...scan.unsupportedPaths.map(u => u.path), ...removed]); } export interface ScopedReview { paths: string[]; reviewer: string; verdict: 'clean'; head?: string; tree?: string } diff --git a/lib/ship-release.ts b/lib/ship-release.ts index 7b9231a21..124756e5a 100644 --- a/lib/ship-release.ts +++ b/lib/ship-release.ts @@ -179,7 +179,7 @@ function route(cwd: string, base: string, reviewed: string, reviewedBase: string const by: Routed = { behavior: [], docs: [], metadata: [], tests: [], base: [], context: [] }; for (const p of [...changedSet].sort()) { const verdict = sides ? (touched.has(p) ? comparePath(cwd, sides, p) : 'same') : 'changed'; - by[verdict === 'same' ? 'base' : release.includes(p) ? 'metadata' : isTestCode(p) ? 'tests' : classifyPath(p, cwd) === 'docs' ? 'docs' : verdict === 'context' ? 'context' : 'behavior'].push(p); + by[verdict === 'same' ? 'base' : release.includes(p) ? 'metadata' : sides && isTestCode(p) ? 'tests' : classifyPath(p, cwd) === 'docs' ? 'docs' : verdict === 'context' ? 'context' : 'behavior'].push(p); } const gated = gateEditedPaths(cwd, reviewed, now, by.tests) ?? new Set(by.tests); by.behavior.push(...by.tests.filter(p => gated.has(p))); @@ -188,7 +188,7 @@ function route(cwd: string, base: string, reviewed: string, reviewedBase: string if (by.behavior.length) { r.stop(`behavior, tests or build inputs changed since review (${short(by.behavior.sort())}): insert 5–11.5 → 12–14 → 16 before Step 17 (stage 2.1)`); return null; } if (by.context.length && sides) { if (!opts.contextReviewed) { - const packet = path.resolve(cwd, git(cwd, ['rev-parse', '--git-path', `gstack/scoped-review-${now.slice(0, 12)}.md`]).stdout.trim()); + const packet = path.resolve(cwd, git(cwd, ['rev-parse', '--git-path', `gstack/scoped-review-${now.slice(0, 12)}.md`]).stdout.trim() || `.git/gstack/scoped-review-${now.slice(0, 12)}.md`); fs.mkdirSync(path.dirname(packet), { recursive: true }); fs.writeFileSync(packet, contextPacket(cwd, sides, by.context)); r.stop(`base context moved next to the reviewed hunks in ${short(by.context)}: one fix-check reviewer reads ${packet}, then rerun with --context-reviewed written beside the packet, outside the worktree ({"tree":"${now}","paths":[…],"reviewer":"","verdict":"clean"}); a blocking finding is a behavior change (stage 2.1)`); diff --git a/test/ship-rereview-triggers.test.ts b/test/ship-rereview-triggers.test.ts index 6370af11c..f5a773be5 100644 --- a/test/ship-rereview-triggers.test.ts +++ b/test/ship-rereview-triggers.test.ts @@ -8,6 +8,7 @@ import { afterEach, describe, expect, setDefaultTimeout, test } from 'bun:test'; import * as fs from 'node:fs'; import * as path from 'node:path'; import { EXIT } from '../lib/headless-artifacts'; +import { comparePath } from '../lib/ship-patch'; import { extractReceipt, placeReceiptFirst } from '../lib/ship-receipt'; import { cleanup, makeFixtureRepo, runBin, write, type FixtureRepo } from './helpers/pregate-fixture'; import { lineOf, RELEASE, shipFixture, type ShipFx } from './helpers/ship-bin-fixture'; @@ -151,6 +152,32 @@ describe('finish route after a base merge (GST-407 #1, GBRA-108 #7739)', () => { expect([feature.status, feature.stdout.includes('is not on origin/main')]).toEqual([3, true]); const plain = finish(f, reviewed, post); expect(lineOf(plain.stdout, 'ROUTE')).toMatch(/ base=unknown context=0$/); + f.write({ 'test/b.test.ts': "import { test } from 'bun:test';\ntest('b', () => {});\n" }); f.commit('test: add'); + const noFlag = finish(f, reviewed, post); + expect([noFlag.status, lineOf(noFlag.stdout, 'ROUTE')]).toEqual([3, expect.stringMatching(/ behavior=1 docs=0 metadata=0 tests=0 base=unknown /)]); + }); + + // Value: protects=a textconv driver the clone configures cannot hide a product line from the comparison; a binary or + // empty file whose patch did not change is same, and one whose bytes changed is not; fails_when=git diff applies + // textconv (an auth flag flips unseen and routes base=), or a hunkless patch always reads changed; seam=lib/ship-patch.ts + // comparePath; no_claim=none + test('textconv cannot hide a product line; hunkless patches compare by patch-id', () => { + const f = fx({ 'src/auth.ts': 'export const authRequired = true;\nexport const a = 1;\n' }, { 'src/auth.ts': 'export const authRequired = true;\nexport const a = 2;\n', 'pkg/__init__.py': '', 'assets/x.bin': 'a\u0000b\n' }); + f.write({ '.gitattributes': 'src/*.ts diff=hide\n' }); f.commit('attributes'); + f.git('config', 'diff.hide.textconv', 'grep -v authRequired'); + const reviewed = wtree(f); + const reviewedBase = mergeBase(f); + const reviewedHead = f.git('rev-parse', 'HEAD').stdout.trim(); + const post = snapshot(f); + f.advanceBase({ 'lib/z.ts': 'export const z = 1;\n' }); + mergeMain(f); + const sides = () => ({ reviewedBase, reviewed: reviewedHead, currentBase: mergeBase(f), current: f.git('rev-parse', 'HEAD').stdout.trim() }); + expect(['pkg/__init__.py', 'assets/x.bin'].map(p => comparePath(f.work, sides(), p))).toEqual(['same', 'same']); + f.write({ 'assets/x.bin': 'a\u0000c\n' }); f.commit('new bytes'); + expect(comparePath(f.work, sides(), 'assets/x.bin')).toBe('changed'); + f.write({ 'assets/x.bin': 'a\u0000b\n', 'src/auth.ts': 'export const authRequired = false;\nexport const a = 2;\n' }); f.commit('flip'); + const r = finish(f, reviewed, post, ['--reviewed-base', reviewedBase]); + expect([r.status, lineOf(r.stdout, 'STOP')]).toEqual([3, 'STOP: behavior, tests or build inputs changed since review (src/auth.ts): insert 5–11.5 → 12–14 → 16 before Step 17 (stage 2.1)']); }); }); @@ -256,15 +283,25 @@ describe('test-only and audit-requested edits after the docs audit (GST-391, gbr const f = fx({}, { 'src/b.ts': 'export const b = 2;\n', 'test/b.test.ts': spec('b') }); const reviewed = wtree(f); const post = snapshot(f); - f.write({ 'test/b.test.ts': spec('b is two') }); f.commit('test: rename'); + f.write({ 'test/b.test.ts': `${spec('b')}test('b is even', () => expect(2 % 2).toBe(0));\n` }); f.commit('test: add a case'); const r = finish(f, reviewed, post, ['--reviewed-base', mergeBase(f)]); expect(lineOf(r.stdout, 'ROUTE')).toMatch(/ behavior=0 docs=0 metadata=0 tests=1 /); expect(lineOf(r.stdout, 'DOCS')).toContain('fresh'); expect(r.status).toBe(0); - f.write({ 'test/b.test.ts': spec('b is two', 'test.skip') }); f.commit('test: skip'); + f.write({ 'test/b.test.ts': `${spec('b', 'test.skip')}test('b is even', () => expect(2 % 2).toBe(0));\n` }); f.commit('test: skip'); const skip = finish(f, reviewed, post, ['--reviewed-base', mergeBase(f)]); expect([skip.status, skip.stdout.includes('STOP: behavior, tests or build inputs changed since review (test/b.test.ts)')]).toEqual([3, true]); + const weak = (body: string) => `import { expect, test } from 'bun:test';\ntest('rows', () => {\n${body}});\n`; + for (const [name, after] of [['changed expected value', ' expect(2).toBe(3);\n expect(() => JSON.parse("")).toThrow();\n'], ['deleted assertion', ' expect(2).toBe(2);\n'], ['binary', ' expect(2).toBe(2);\n expect(() => JSON.parse("")).toThrow();\n const nul = "\u0000";\n']]) { + const w = fx({}, { 'src/b.ts': 'export const b = 2;\n', 'test/rows.test.ts': weak(' expect(2).toBe(2);\n expect(() => JSON.parse("")).toThrow();\n') }); + const rw = wtree(w); + const pw = snapshot(w); + w.write({ 'test/rows.test.ts': weak(after) }); w.commit(`test: ${name}`); + const r = finish(w, rw, pw, ['--reviewed-base', mergeBase(w)]); + expect([name, r.status, r.stdout.includes('STOP: behavior, tests or build inputs changed since review (test/rows.test.ts)')]).toEqual([name, 3, true]); + } + const helper = fx({}, { 'src/b.ts': 'export const b = 2;\n', 'test/helpers/h.test.ts': spec('h') }); const reviewed2 = wtree(helper); const post2 = snapshot(helper); @@ -310,7 +347,7 @@ describe('test-only and audit-requested edits after the docs audit (GST-391, gbr test('gstack-docs-candidate attempt counts only audits that could run', () => { const f = fx(); const out = (name: string, text: string) => { const p = path.join(f.root, name); fs.writeFileSync(p, text); return p; }; - const state = path.join(f.root, 'attempts.json'); + const state = path.join(f.root, 'state-dir', 'attempts.json'); const attempt = (file: string) => f.run(path.join(BIN, 'gstack-docs-candidate'), ['attempt', file, '--state', state]); const refused = out('refused.txt', 'gstack: ok 1.91.354.0 for=review-voice\nINSTALL: fail verdict for=review-voice does not admit document-release; fix: bash install.sh --for document-release\nGSTACK_RESULT: skill=document-release status=refused run=-\n'); expect(attempt(out('crash.txt', 'Error: child crashed\n')).stdout.trim()).toBe('ATTEMPT: counted');