From dead4874dc1bea90b528f4f4fbeb5ab326ac1a2e Mon Sep 17 00:00:00 2001 From: garrytan Date: Wed, 4 Nov 2026 00:36:54 +0000 Subject: [PATCH] fix: pre-landing review fixes Review pass 2 (below floor): import takes only autoplan-regate records with autoplan:@gate_rev=N evidence; record diffs with --no-renames so a rename is a delete plus an add; a scoped-fix-check also checks the approval record for the branch's K1 grant, so a fresh checkout with an empty ledger still stops; a trailing }, N) counts as a timeout only where it closes a test callback; the re-gate words add paid, eval and model names; Step 19 sets NS on its own line so a path with spaces survives. Also launches gstack-review-read through runScriptSync (free-tests #3197 tripwire, run 41746166882). Review-Finding: specialist-batched#1 reader=specialist-batched Review-Finding: specialist-batched#3 reader=specialist-batched Review-Finding: specialist-batched#4 reader=specialist-batched Review-Finding: red-team#1 reader=red-team Review-Finding: red-team#2 reader=red-team Review-Finding: red-team#3 reader=red-team Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-no-stop | 25 +++++++++---- lib/no-stop.ts | 9 +++-- ship/sections/pr-body.md | 3 +- ship/sections/pr-body.md.tmpl | 3 +- test/no-stop.test.ts | 37 +++++++++++++++++-- test/size-ledger/ship/gsta258-l5-no-stop.json | 4 +- 6 files changed, 63 insertions(+), 18 deletions(-) diff --git a/bin/gstack-no-stop b/bin/gstack-no-stop index 9ec362ba6..27f76261a 100755 --- a/bin/gstack-no-stop +++ b/bin/gstack-no-stop @@ -14,9 +14,10 @@ */ import { spawnSync } from 'node:child_process'; import * as path from 'node:path'; -import { appendRows, buildRow, locate } from '../lib/approval-record'; -import { EXIT, NO_STOP_CLASSES, exitTableHelp } from '../lib/headless-artifacts'; +import { appendRows, buildRow, locate, readRecord } from '../lib/approval-record'; +import { EXIT, exitTableHelp } from '../lib/headless-artifacts'; import { appendLedger, checkNoStop, ledgerPath, readLedger } from '../lib/no-stop'; +import { runScriptSync } from '../lib/script-invocation'; const HELP = `gstack-no-stop — record one of the five no-stop defaults (docs/review-policy.md#no-stop) @@ -44,10 +45,12 @@ ${exitTableHelp()}`; const flag = (args: string[], name: string) => { const i = args.indexOf(name); return i >= 0 ? args[i + 1] : undefined; }; const git = (args: string[]) => spawnSync('git', args, { cwd: process.cwd(), encoding: 'utf8', timeout: 60_000, maxBuffer: 64 * 1024 * 1024 }); const NEEDS_RANGE = ['scoped-fix-check', 'test-timeout-fix', 'stamp-only-carry']; +const K1_BY = 'gstack no-stop default (K1)'; +const AUTOPLAN_EVIDENCE = /^autoplan:\S+@gate_rev=\d+$/; /** The commit the newest full or scoped pass reviewed (its review-log row's commit_full): round 2's PASS_HEAD. */ function lastPassHead(): string | null { - const r = spawnSync(path.join(import.meta.dir, 'gstack-review-read'), [], { cwd: process.cwd(), encoding: 'utf8', timeout: 60_000, maxBuffer: 64 * 1024 * 1024 }); + const r = runScriptSync(path.join(import.meta.dir, 'gstack-review-read'), [], { cwd: process.cwd(), encoding: 'utf8', timeout: 60_000, maxBuffer: 64 * 1024 * 1024 }); const rows = (r.stdout ?? '').split('---CONFIG---')[0]!.split('\n').flatMap(l => { try { return [JSON.parse(l)]; } catch { return []; } }); const pass = rows.filter(x => x.skill === 'review' && typeof x.commit_full === 'string').at(-1); return pass ? pass.commit_full : null; @@ -60,7 +63,7 @@ function grant(head: string, detail: string, ledger: string): string | { error: const row = buildRow({ ask: `No-stop default: one scoped fix-check past R51's cap (review_loop_grant ${head})`, words: `K1 no-stop default, docs/review-policy.md#no-stop: ${detail}`, - by: 'gstack no-stop default (K1)', scope: `review_loop_grant ${head}`, outcome: 'approved', source: `no-stop:${ledger.split('/').pop()}`, + by: K1_BY, scope: `review_loop_grant ${head}`, outcome: 'approved', source: `no-stop:${ledger.split('/').pop()}`, }, branch); if ('code' in row) return { error: row.message }; const added = appendRows(loc, [row]); @@ -81,6 +84,11 @@ function record(args: string[], json: boolean): number { process.stderr.write(`NO_STOP: refused ${cls} (--range-base ${rangeBase} is not the commit the last review pass read (${pass ? pass.slice(0, 12) : 'no review-log row'}), so the checked range would not be the range the fix-check covers); stop for the owner\n`); return EXIT.refused; } + const loc = locate({ cwd: process.cwd(), env: process.env, branch: git(['rev-parse', '--abbrev-ref', 'HEAD']).stdout.trim() }); + if (!readLedger(file).some(r => r.class === cls) && !('code' in loc) && readRecord(loc).rows.some(r => r.by === K1_BY)) { + process.stderr.write(`NO_STOP: refused ${cls} (the approval record already holds this branch's K1 grant, from this or another checkout; a second goes to the owner); stop for the owner\n`); + return EXIT.refused; + } } let diff: string | undefined; if (rangeBase) { @@ -91,7 +99,7 @@ function record(args: string[], json: boolean): number { if (merged.status !== 0) { process.stderr.write(`NO_STOP: refused ${cls} (git merge-tree ${base} ${rangeBase} failed: ${merged.stderr.trim() || 'conflict'}); stop for the owner\n`); return EXIT.refused; } from = merged.stdout.split('\n')[0]!.trim(); } - const d = git(['diff', '--no-color', '--no-ext-diff', from, 'HEAD']); + const d = git(['diff', '--no-color', '--no-ext-diff', '--no-renames', from, 'HEAD']); if (d.status !== 0) { process.stderr.write(`gstack-no-stop record: git diff ${rangeBase} HEAD failed: ${d.stderr.trim()}\n`); return EXIT.fail; } diff = d.stdout; } @@ -126,9 +134,12 @@ function main(argv: string[]): number { const file = ledgerPath(process.cwd()); if (!src || !file) { process.stderr.write(`gstack-no-stop import: ${src ? 'not inside a git checkout' : '--from is required'}\n`); return EXIT.usage; } const have = readLedger(file); - const fresh = readLedger(src).filter(r => (NO_STOP_CLASSES as readonly string[]).includes(r.class) && r.reason && r.evidence && !have.some(h => JSON.stringify(h) === JSON.stringify(r))); + const rows = readLedger(src); + const eligible = rows.filter(r => r.class === 'autoplan-regate' && r.reason && AUTOPLAN_EVIDENCE.test(r.evidence ?? '')); + const fresh = eligible.filter(r => !have.some(h => JSON.stringify(h) === JSON.stringify(r))); for (const r of fresh) appendLedger(file, r); - process.stdout.write(json ? JSON.stringify({ file, imported: fresh.length }) + '\n' : `NO_STOP: imported ${fresh.length} record(s) from ${src}\nNO_STOP_FILE: ${file}\n`); + const skipped = rows.length - eligible.length; + process.stdout.write(json ? JSON.stringify({ file, imported: fresh.length, skipped }) + '\n' : [`NO_STOP: imported ${fresh.length} record(s) from ${src}`, ...(skipped ? [`NO_STOP: skipped ${skipped} record(s): import takes only autoplan-regate records with autoplan:@gate_rev=N evidence; record the other classes with gstack-no-stop record`] : []), `NO_STOP_FILE: ${file}`].join('\n') + '\n'); return EXIT.ok; } if (cmd === 'list') { diff --git a/lib/no-stop.ts b/lib/no-stop.ts index 29fccf797..174102015 100644 --- a/lib/no-stop.ts +++ b/lib/no-stop.ts @@ -34,7 +34,7 @@ const VERSION = /\d+\.\d+\.\d+(?:\.\d+)?/g; const TEST_PATH = /(?:^|\/)(?:test|tests|__tests__|spec)\/|\.(?:test|spec)\.[cm]?[jt]sx?$/; const STAMP_PATHS = ['VERSION', 'package.json', 'CHANGELOG.md', 'agents-digest/gstack-AGENTS.md']; /** Plan-delta words that mean spend, a cap, an eval arm or a verdict changed: those re-gates go to the owner. */ -export const REGATE_WORDS = /\$|\b(?:spend|spending|spent|costs?|budgets?|prices?|caps?|capped|credits?|limits?|max|maximum|arms?|models?|judges?|verdicts?|thresholds?|bars?|pass(?:es)? at|trials?|samples?|stop rules?|kill rules?)\b|\bn\s*=/i; +export const REGATE_WORDS = /\$|\b(?:spend|spending|spent|costs?|budgets?|prices?|caps?|capped|credits?|limits?|max|maximum|arms?|models?|judges?|verdicts?|thresholds?|bars?|pass(?:es)? at|trials?|samples?|stop rules?|kill rules?|paid|evals?|opus|sonnet|haiku|gpt[\w.-]*|gemini|claude)\b|\bn\s*=/i; /** Unified diff (git diff) to files, each with its one-for-one changed line pairs; a run of unequal - and + counts is unpaired. */ export function parseDiff(diff: string): FileDiff[] { @@ -65,8 +65,11 @@ export function parseDiff(diff: string): FileDiff[] { return files; } -const timeoutValues = (s: string) => [...s.matchAll(TIMEOUT_VALUE)].filter(m => !m[1]!.startsWith('}') || /\)\s*;?\s*$/.test(s.slice(m.index! + m[0].length))).map(m => Number(m[2]!.replace(/_/g, ''))); -const maskTimeouts = (s: string) => s.replace(TIMEOUT_VALUE, (_m, key: string, _n: string, at: number) => (!key.startsWith('}') || /\)\s*;?\s*$/.test(s.slice(at + _m.length)) ? `${key}#` : _m)); +const TEST_CALL = /^\s*(?:test|it|describe|bench|beforeAll|beforeEach|afterAll|afterEach)(?:\.\w+)*\(/; +/** A trailing `}, N)` is a timeout only where it closes a test callback: the line opens a test call, or the brace starts the line. */ +const isTimeout = (s: string, key: string, at: number, len: number) => !key.startsWith('}') || ((TEST_CALL.test(s) || !s.slice(0, at).trim()) && /\)\s*;?\s*$/.test(s.slice(at + len))); +const timeoutValues = (s: string) => [...s.matchAll(TIMEOUT_VALUE)].filter(m => isTimeout(s, m[1]!, m.index!, m[0].length)).map(m => Number(m[2]!.replace(/_/g, ''))); +const maskTimeouts = (s: string) => s.replace(TIMEOUT_VALUE, (_m, key: string, _n: string, at: number) => (isTimeout(s, key, at, _m.length) ? `${key}#` : _m)); /** Why the diff is not timeout-only, or null: every changed line pairs with one that differs only in larger timeout values (never an assertion). */ export function notTimeoutOnly(files: FileDiff[]): string | null { diff --git a/ship/sections/pr-body.md b/ship/sections/pr-body.md index 90e409cc5..898fd5576 100644 --- a/ship/sections/pr-body.md +++ b/ship/sections/pr-body.md @@ -220,8 +220,9 @@ per-lane counts (`{"":{"pass":N,"fail":N,"skip":N}}`) to ```bash rm -f .gstack/tmp/review-loop.json +NS=$(~/.claude/skills/gstack/bin/gstack-no-stop list | sed -n 's/^NO_STOP_FILE: //p'); [ -s "$NS" ] || NS= ~/.claude/skills/gstack/bin/gstack-review-scope summary --since "$REVIEW_SINCE" --no-ledger --out .gstack/tmp/review-loop.json && -~/.claude/skills/gstack/bin/gstack-ship-receipt write --pr --base --gate @.gstack/tmp/ship-gate.json --pregate .gstack/tmp/pregate.json --issues @.gstack/tmp/issues.json --review-loop @.gstack/tmp/review-loop.json --docs @.gstack/tmp/docs-audit.json $([ -s .gstack/tmp/targeted.json ] && echo --review-targeted @.gstack/tmp/targeted.json) $(NS=$(~/.claude/skills/gstack/bin/gstack-no-stop list | sed -n 's/^NO_STOP_FILE: //p'); [ -s "$NS" ] && echo --no-stop @"$NS") --body "$PR_BODY_FILE" +~/.claude/skills/gstack/bin/gstack-ship-receipt write --pr --base --gate @.gstack/tmp/ship-gate.json --pregate .gstack/tmp/pregate.json --issues @.gstack/tmp/issues.json --review-loop @.gstack/tmp/review-loop.json --docs @.gstack/tmp/docs-audit.json $([ -s .gstack/tmp/targeted.json ] && echo --review-targeted @.gstack/tmp/targeted.json) ${NS:+--no-stop "@$NS"} --body "$PR_BODY_FILE" ``` A Step 5 red-base pause adds `--base-red ` (a conditional receipt the queue diff --git a/ship/sections/pr-body.md.tmpl b/ship/sections/pr-body.md.tmpl index 64680ed82..9b510a3f9 100644 --- a/ship/sections/pr-body.md.tmpl +++ b/ship/sections/pr-body.md.tmpl @@ -203,8 +203,9 @@ per-lane counts (`{"":{"pass":N,"fail":N,"skip":N}}`) to ```bash rm -f .gstack/tmp/review-loop.json +NS=$(~/.claude/skills/gstack/bin/gstack-no-stop list | sed -n 's/^NO_STOP_FILE: //p'); [ -s "$NS" ] || NS= ~/.claude/skills/gstack/bin/gstack-review-scope summary --since "$REVIEW_SINCE" --no-ledger --out .gstack/tmp/review-loop.json && -~/.claude/skills/gstack/bin/gstack-ship-receipt write --pr --base --gate @.gstack/tmp/ship-gate.json --pregate .gstack/tmp/pregate.json --issues @.gstack/tmp/issues.json --review-loop @.gstack/tmp/review-loop.json --docs @.gstack/tmp/docs-audit.json $([ -s .gstack/tmp/targeted.json ] && echo --review-targeted @.gstack/tmp/targeted.json) $(NS=$(~/.claude/skills/gstack/bin/gstack-no-stop list | sed -n 's/^NO_STOP_FILE: //p'); [ -s "$NS" ] && echo --no-stop @"$NS") --body "$PR_BODY_FILE" +~/.claude/skills/gstack/bin/gstack-ship-receipt write --pr --base --gate @.gstack/tmp/ship-gate.json --pregate .gstack/tmp/pregate.json --issues @.gstack/tmp/issues.json --review-loop @.gstack/tmp/review-loop.json --docs @.gstack/tmp/docs-audit.json $([ -s .gstack/tmp/targeted.json ] && echo --review-targeted @.gstack/tmp/targeted.json) ${NS:+--no-stop "@$NS"} --body "$PR_BODY_FILE" ``` A Step 5 red-base pause adds `--base-red ` (a conditional receipt the queue diff --git a/test/no-stop.test.ts b/test/no-stop.test.ts index 0e00f83fa..855e2150b 100644 --- a/test/no-stop.test.ts +++ b/test/no-stop.test.ts @@ -79,6 +79,13 @@ describe('lib/no-stop: what qualifies', () => { expect(regateTouches('- Run 40 jobs per sweep\n+ Run 400 jobs per sweep').length).toBeGreaterThan(0); expect(regateTouches('- Keep the budgets\n+ Double the budgets').length).toBeGreaterThan(0); }); + + test('review pass 2 holes: a retry count is not a timeout, and dropping a paid eval or swapping a model is a re-gate', () => { + expect(check('test-timeout-fix', diff('test/a.test.ts', [' retry({ fn }, 3);'], [' retry({ fn }, 10);']))).toMatchObject({ ok: false }); + expect(check('test-timeout-fix', diff('test/a.test.ts', [' }, 5_000);'], [' }, 30_000);']))).toMatchObject({ ok: true }); + expect(regateTouches('- Run the paid eval\n+ Skip the paid eval').length).toBeGreaterThan(0); + expect(regateTouches('- Score it on Opus\n+ Score it on Haiku').length).toBeGreaterThan(0); + }); }); describe('gstack-no-stop record: the ledger, the grant and the receipt', () => { @@ -136,6 +143,16 @@ describe('gstack-no-stop record: the ledger, the grant and the receipt', () => { expect(run('list', '--json').stdout).toBe(before.stdout); }); + test('a checkout with an empty ledger still sees the branch\'s K1 grant in the approval record and stops', () => { + const file = JSON.parse(run('list', '--json').stdout).file as string; + const saved = fs.readFileSync(file, 'utf8'); + fs.rmSync(file); + const r = run('record', '--class', 'scoped-fix-check', '--range-base', base, '--reason', 'again', '--evidence', 'commit x'); + fs.writeFileSync(file, saved); + expect(r.status).toBe(3); + expect(r.stderr).toContain('the approval record already holds this branch\'s K1 grant'); + }); + test('a class outside the five stops, and the range-based classes need --range-base', () => { expect(run('record', '--class', 'merge-anyway', '--reason', 'r', '--evidence', 'e').status).toBe(3); expect(run('record', '--class', 'stamp-only-carry', '--reason', 'r', '--evidence', 'e').status).toBe(2); @@ -181,10 +198,22 @@ describe('gstack-no-stop record --class stamp-only-carry --base, and import', () expect(r.stderr).toContain('app.ts is not a stamp file'); }); - test('import adds an autoplan run\'s records once and skips anything outside the five', () => { + test('a rename into changelog.d with an edit is a deletion plus an add, so it still stops', () => { + write('notes.md', 'one\ntwo\nthree\nfour\nfive\nsix\n'); git('add', '-A'); git('commit', '-qm', 'notes'); + const from = git('rev-parse', 'HEAD'); + fs.mkdirSync(path.join(work, 'changelog.d'), { recursive: true }); + git('mv', 'notes.md', 'changelog.d/notes.md'); write('changelog.d/notes.md', 'one\ntwo\nthree\nfour\nfive\nseven\n'); git('add', '-A'); git('commit', '-qm', 'move'); + const r = run('record', '--class', 'stamp-only-carry', '--range-base', from, '--reason', 'r', '--evidence', 'e'); + expect(r.status).toBe(3); + expect(r.stderr).toContain('notes.md is not a stamp file'); + }); + + test('import adds an autoplan run\'s re-gate records once and skips every other class', () => { const src = path.join(tmp, 'run-no-stop.json'); - fs.writeFileSync(src, JSON.stringify([{ class: 'autoplan-regate', reason: 'plan edit, no spend', evidence: 'autoplan:run-1@gate_rev=2' }, { class: 'merge-anyway', reason: 'x', evidence: 'y' }])); - expect(run('import', '--from', src).stdout).toMatch(/^NO_STOP: imported 1 record\(s\)/m); + fs.writeFileSync(src, JSON.stringify([{ class: 'autoplan-regate', reason: 'plan edit, no spend', evidence: 'autoplan:run-1@gate_rev=2' }, { class: 'merge-anyway', reason: 'x', evidence: 'y' }, { class: 'stamp-only-carry', reason: 'x', evidence: 'y' }, { class: 'autoplan-regate', reason: 'x', evidence: 'hand-written' }])); + const first = run('import', '--from', src).stdout; + expect(first).toMatch(/^NO_STOP: imported 1 record\(s\)/m); + expect(first).toMatch(/^NO_STOP: skipped 3 record\(s\): import takes only autoplan-regate records/m); expect(run('import', '--from', src).stdout).toMatch(/^NO_STOP: imported 0 record\(s\)/m); expect(JSON.parse(run('list', '--json').stdout).records.map((r: NoStopRecord) => r.class)).toEqual(['stamp-only-carry', 'autoplan-regate']); }); @@ -201,7 +230,7 @@ describe('the no-stop defaults in the /ship and /autoplan sources', () => { test('a refused case and any other stop still stop; the receipt write carries the ledger', () => { expect(ship).toContain('three other routine stops go through `gstack-no-stop record` (`--help`); `recorded` goes on,\n`refused` stops.'); expect(help).toContain('NO_STOP: refused (exit 3): the stop stands. Anything else still stops.'); - expect(read('ship/sections/pr-body.md.tmpl')).toContain('echo --no-stop @"$NS"'); + expect(read('ship/sections/pr-body.md.tmpl')).toContain('${NS:+--no-stop "@$NS"}'); expect(read('ship/DRIVER.md.tmpl')).toContain('end the run, except `gstack-no-stop`'); expect(read('autoplan/DRIVER.md.tmpl')).toContain("K1's `autoplan-regate` no-stop"); }); diff --git a/test/size-ledger/ship/gsta258-l5-no-stop.json b/test/size-ledger/ship/gsta258-l5-no-stop.json index 0d327227e..a42d2e779 100644 --- a/test/size-ledger/ship/gsta258-l5-no-stop.json +++ b/test/size-ledger/ship/gsta258-l5-no-stop.json @@ -1,4 +1,4 @@ { - "bytes": 346, - "why": "GSTA-258 L5 (K1): the no-stop pointer in Step 5, the --no-stop receipt flag in Step 19's write, and the driver's gstack-no-stop exception" + "bytes": 349, + "why": "GSTA-258 L5 (K1): the no-stop pointer in Step 5, the --no-stop receipt flag in Step 19's write (NS set on its own line so a path with spaces survives), and the driver's gstack-no-stop exception" }