From e70da9bf4aba2dddbcb2093e75978fb79f8084e4 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 31 Oct 2026 09:48:34 +0000 Subject: [PATCH] fix: no test edit skips review; guard packet writes; exclusive attempt state A test-only edit with no gate tag gets the scoped fix-check of its test hunks instead of a lanes-only rerun. Packet write failures print a STOP or RECEIPT_CARRY_REFUSED. attempt's allowance uses an exclusive create. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-docs-candidate | 7 +++- bin/gstack-ship-receipt | 8 +++- changelog.d/gst-407-ship-rereview-triggers.md | 2 +- docs/troubleshooting.md | 5 ++- lib/ship-patch.ts | 7 ++-- lib/ship-release.ts | 19 +++++++--- test/ship-rereview-triggers.test.ts | 38 ++++++++++++++++++- 7 files changed, 70 insertions(+), 16 deletions(-) diff --git a/bin/gstack-docs-candidate b/bin/gstack-docs-candidate index 15d49b487..73deda3e6 100755 --- a/bin/gstack-docs-candidate +++ b/bin/gstack-docs-candidate @@ -186,7 +186,12 @@ function main(argv: string[]): void { process.exit(3); } fs.mkdirSync(path.dirname(path.resolve(state)), { recursive: true }); - fs.writeFileSync(state, JSON.stringify({ not_counted: seen + 1 }) + '\n', { mode: 0o600 }); + try { fs.writeFileSync(state, JSON.stringify({ not_counted: 1 }) + '\n', { mode: 0o600, flag: 'wx' }); } catch (e) { + if ((e as NodeJS.ErrnoException).code !== 'EEXIST') throw e; + console.log('ATTEMPT: counted (another launch in this invocation already used the allowance)'); + console.log(`STOP: run the install fix (${install}) and confirm its gstack: ok trailer before another launch`); + process.exit(3); + } console.log(`ATTEMPT: not-counted (install refused: ${install})`); return; } diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index d71ac15dd..a1ff24b0d 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -610,8 +610,12 @@ function cmdCarry(rest: string[], json: boolean): number { if (!record) { const gitPath = git(['rev-parse', '--git-path', `gstack/scoped-review-${head.slice(0, 12)}.md`], repoRoot); const packet = path.resolve(repoRoot, gitPath ?? `.git/gstack/scoped-review-${head.slice(0, 12)}.md`); - fs.mkdirSync(path.dirname(packet), { recursive: true }); - fs.writeFileSync(packet, contextPacket(repoRoot, { reviewedBase: fromBase, reviewed: from, currentBase: headBase, current: head }, contextPaths)); + try { + fs.mkdirSync(path.dirname(packet), { recursive: true }); + fs.writeFileSync(packet, contextPacket(repoRoot, { reviewedBase: fromBase, reviewed: from, currentBase: headBase, current: head }, contextPaths)); + } catch (e) { + return refused('RECEIPT_CARRY_REFUSED', `base context moved next to the reviewed hunks in ${contextPaths.join(', ')}, but the scoped-review packet ${packet} cannot be written (${(e as Error).message.split('\n')[0]}); make its directory writable, then rerun carry`); + } return refused('RECEIPT_CARRY_SCOPED_REVIEW', `base context moved next to the reviewed hunks in ${contextPaths.join(', ')}: one fix-check reviewer reads ${packet}, then rerun carry with --scoped-review ({"head":"${head}","paths":[…],"reviewer":"","verdict":"clean"}); a blocking finding means /ship on ${head.slice(0, 12)}`); } const review = readScopedReview(record, { head }, contextPaths); diff --git a/changelog.d/gst-407-ship-rereview-triggers.md b/changelog.d/gst-407-ship-rereview-triggers.md index 499fee61c..1bf2bbec9 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=`. 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`. +- **`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=`: one fix-check reviewer reads its test hunks in the scoped-review packet, and the lanes rerun it, instead of the full loop. No test edit skips review. 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`. - **A version stamp after review counts as release metadata.** `finish` used to read a slot stamp in `stamp_files` paths (a plugin.json `"version"` line, a stamp comment in a doc) as a behavior and docs change, so it stopped for the loop (`ROUTE: … behavior=8 docs=5` on gbrain #8095). A `stamp_files` path whose only edit swaps the old version for the new one, under a built-in form or a `stamp_patterns` template, is now metadata, the same rule the tree receipt and carry use. Any other edit to that path still stops. - **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. diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 74fafbf78..5cc60fd59 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -5071,7 +5071,10 @@ carry reason records the reviewer and the paths. A blocking finding means /ship on the new head. `gstack-ship-release finish` stops the same way with `context=N` and takes `--context-reviewed ` bound to the tree; write that record beside the packet, outside the worktree, so it does not change -the tree it is bound to. +the tree it is bound to. Finish also stops this way for test code changed since +review with no gate edit (`tests=N`): the packet holds those test hunks, and the +reviewer checks that each test still tests what it did. A packet that cannot be +written (an unwritable `.git`) is a STOP or `RECEIPT_CARRY_REFUSED` naming the path. ### `RECEIPT_PROSE_MISMATCH` diff --git a/lib/ship-patch.ts b/lib/ship-patch.ts index fab972fef..3310af01d 100644 --- a/lib/ship-patch.ts +++ b/lib/ship-patch.ts @@ -125,13 +125,14 @@ function hunkTexts(diff: string): string[] { } /** The scoped-review packet: per context path, the hunks that differ between the two branch patches and the base's own change. */ -export function contextPacket(repoRoot: string, s: PatchSides, paths: readonly string[]): string { +export function contextPacket(repoRoot: string, s: PatchSides, paths: readonly string[], tests: readonly string[] = []): string { const fence = (label: string, body: string) => { const all = body.split('\n'); return `${label}${all.length > 200 ? ` (first 200 of ${all.length} lines; read the rest with git diff)` : ''}\n\`\`\`diff\n${all.slice(0, 200).join('\n').trimEnd()}\n\`\`\`\n`; }; const parts = [`# Scoped review: base context moved next to the reviewed hunks\n\nreviewed ${s.reviewed.slice(0, 12)} against ${s.reviewedBase.slice(0, 12)}; current ${s.current.slice(0, 12)} against ${s.currentBase.slice(0, 12)}. The branch's +/- lines are unchanged; read whether the merge kept their meaning next to the base's edit. Report clean, or findings at the blocking floor.\n`]; - for (const rel of paths) { + if (tests.length) parts.push(`Test code changed since review in ${tests.join(', ')}: read whether each test still checks what it did (no weakened or bypassed assertion, early return, new exemption, skip or mock). Report clean, or findings at the blocking floor.\n`); + for (const rel of [...paths, ...tests]) { const r = hunkTexts(pathDiff(repoRoot, s.reviewedBase, s.reviewed, rel, 3) ?? ''); const c = hunkTexts(pathDiff(repoRoot, s.currentBase, s.current, rel, 3) ?? ''); parts.push(`## ${rel}\n`, fence('Reviewed hunks', r.filter(h => !c.includes(h)).join('')), fence('Current hunks', c.filter(h => !r.includes(h)).join('')), @@ -148,7 +149,7 @@ export function isTestCode(rel: string): boolean { /** * 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. + * value), or a diff the scan cannot read. The rest still get the scoped fix-check of their test hunks (lib/ship-release.ts route). */ export function gateEditedPaths(repoRoot: string, from: string, to: string, paths: readonly string[]): Set | null { if (!paths.length) return new Set(); diff --git a/lib/ship-release.ts b/lib/ship-release.ts index 2449c085b..cd3031308 100644 --- a/lib/ship-release.ts +++ b/lib/ship-release.ts @@ -186,17 +186,24 @@ function route(cwd: string, base: string, reviewed: string, reviewedBase: string by.tests = by.tests.filter(p => !gated.has(p)); r.add(`ROUTE: reviewed=${reviewed.slice(0, 12)} current=${now.slice(0, 12)} behavior=${by.behavior.length} docs=${by.docs.length} metadata=${by.metadata.length} tests=${by.tests.length} base=${sides ? by.base.length : 'unknown'} context=${by.context.length}`); 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) { + const scoped = [...by.context, ...by.tests].sort(); + if (scoped.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() || `.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)`); + const why = [by.context.length ? `base context moved next to the reviewed hunks in ${short(by.context)}` : '', by.tests.length ? `test code changed since review in ${short(by.tests)}` : ''].filter(Boolean).join('; '); + try { + fs.mkdirSync(path.dirname(packet), { recursive: true }); + fs.writeFileSync(packet, contextPacket(cwd, sides, by.context, by.tests)); + } catch (e) { + r.stop(`${why}, but the scoped-review packet ${packet} cannot be written (${firstLine((e as Error).message)}): make its directory writable, then rerun finish (stage 2.1)`); + return null; + } + r.stop(`${why}: 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)`); return null; } - const review = readScopedReview(opts.contextReviewed, { tree: now }, by.context); + const review = readScopedReview(opts.contextReviewed, { tree: now }, scoped); if (!review.ok) { r.stop(`--context-reviewed: ${review.message}`); return null; } - r.add(`SCOPED_REVIEW: clean by ${review.review.reviewer} (${short(by.context)})`); + r.add(`SCOPED_REVIEW: clean by ${review.review.reviewer} (${short(scoped)})`); } if (by.docs.length && !opts.docsRechecked) { r.stop(`authored docs changed since review (${short(by.docs)}): recheck the plan items they touch (stage 2.2), then rerun with --docs-rechecked`); return null; } return by; diff --git a/test/ship-rereview-triggers.test.ts b/test/ship-rereview-triggers.test.ts index 1c255f9ef..35d2cab8f 100644 --- a/test/ship-rereview-triggers.test.ts +++ b/test/ship-rereview-triggers.test.ts @@ -263,6 +263,10 @@ describe('adjacent base edits (GST-384, #3631, founderstar#441)', () => { spawnMerge(repo, { 'lib/rows.js': src({ 5: 'exports.v5 = "five";', 6: 'exports.v6 = 600;' }) }); const head = repo.git('rev-parse', 'HEAD'); const carry = (extra: string[] = []) => runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--base', 'main', ...extra]); + fs.writeFileSync(path.join(repo.work, '.git', 'gstack'), 'not a directory'); + const blocked = carry(); + expect([blocked.status, blocked.out.includes('RECEIPT_CARRY_REFUSED'), blocked.err.includes('cannot be written')]).toEqual([EXIT.refused, true, true]); + fs.rmSync(path.join(repo.work, '.git', 'gstack')); const ask = carry(); expect(ask.status).toBe(EXIT.refused); expect(ask.out.trim()).toBe('CARRY: refused (RECEIPT_CARRY_SCOPED_REVIEW)\nGSTACK_RESULT: skill=ship status=refused run=-'); @@ -305,10 +309,36 @@ describe('test-only and audit-requested edits after the docs audit (GST-391, gbr const reviewed = wtree(f); const post = snapshot(f); 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 /); + const ask = finish(f, reviewed, post, ['--reviewed-base', mergeBase(f)]); + expect(lineOf(ask.stdout, 'ROUTE')).toMatch(/ behavior=0 docs=0 metadata=0 tests=1 /); + expect([ask.status, lineOf(ask.stdout, 'STOP')]).toEqual([3, expect.stringContaining('STOP: test code changed since review in test/b.test.ts: one fix-check reviewer reads ')]); + const packet = /reads (\S+\.md), then/.exec(ask.stdout)![1]!; + expect(fs.readFileSync(packet, 'utf8')).toContain("test('b is even'"); + const record = path.join(f.root, 'tests-reviewed.json'); + fs.writeFileSync(record, JSON.stringify({ tree: wtree(f), paths: ['test/b.test.ts'], reviewer: 'anthropic/claude-opus-5-5', verdict: 'clean' })); + const r = finish(f, reviewed, post, ['--reviewed-base', mergeBase(f), '--context-reviewed', record]); + expect(lineOf(r.stdout, 'SCOPED_REVIEW')).toBe('SCOPED_REVIEW: clean by anthropic/claude-opus-5-5 (test/b.test.ts)'); expect(lineOf(r.stdout, 'DOCS')).toContain('fresh'); expect(r.status).toBe(0); + + const table = (extra: string) => `import { expect, test } from 'bun:test';\nconst EXEMPT = [\n 'bin/a',\n${extra}];\ntest('exempt', () => {\n expect(EXEMPT.length).toBeLessThan(5);\n});\n`; + const early = (ret: string) => `import { expect, test } from 'bun:test';\ntest('rows', () => {\n${ret} expect(JSON.parse('[1]')).toEqual([1]);\n});\n`; + for (const [name, before, after] of [['exemption-table entry', table(''), table(" 'bin/b',\n")], ['return before an expect', early(''), early(' return;\n')]]) { + const w = fx({}, { 'src/b.ts': 'export const b = 2;\n', 'test/gate.test.ts': before }); + const rw = wtree(w); + const pw = snapshot(w); + w.write({ 'test/gate.test.ts': after }); w.commit(`test: ${name}`); + const out = finish(w, rw, pw, ['--reviewed-base', mergeBase(w)]); + expect([name, out.status, lineOf(out.stdout, 'STOP')]).toEqual([name, 3, expect.stringContaining('STOP: test code changed since review in test/gate.test.ts: one fix-check reviewer reads ')]); + } + + const locked = fx({}, { 'src/b.ts': 'export const b = 2;\n', 'test/b.test.ts': spec('b') }); + const rl = wtree(locked); + const pl = snapshot(locked); + locked.write({ 'test/b.test.ts': `${spec('b')}test('b again', () => expect(1).toBe(1));\n` }); locked.commit('test: add'); + fs.writeFileSync(path.join(locked.work, '.git', 'gstack'), 'not a directory'); + const blocked = finish(locked, rl, pl, ['--reviewed-base', mergeBase(locked)]); + expect([blocked.status, lineOf(blocked.stdout, 'STOP')]).toEqual([3, expect.stringMatching(/^STOP: test code changed since review in test\/b\.test\.ts, but the scoped-review packet \S+ cannot be written \(/)]); 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]); @@ -387,6 +417,10 @@ describe('test-only and audit-requested edits after the docs audit (GST-391, gbr expect(second.status).toBe(3); expect(second.stdout).toContain('ATTEMPT: counted (a second install refusal in this invocation)'); expect(second.stdout).toContain('STOP: run the install fix'); + const raced = path.join(f.root, 'raced.json'); + fs.writeFileSync(raced, JSON.stringify({ not_counted: 0 })); + const race = f.run(path.join(BIN, 'gstack-docs-candidate'), ['attempt', refused, '--state', raced]); + expect([race.status, race.stdout.includes('ATTEMPT: counted (another launch in this invocation already used the allowance)')]).toEqual([3, true]); const corrupt = path.join(f.root, 'corrupt.json'); fs.writeFileSync(corrupt, '{not json'); const reset = f.run(path.join(BIN, 'gstack-docs-candidate'), ['attempt', refused, '--state', corrupt]);