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

@@ -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;

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 <sha>`** (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 <sha>`** (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 <record>` or `carry --scoped-review <record>` 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 <path> --docs-result <audit 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 <child output> --state <file>`** 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.

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<st
 }
 
 function pathDiff(repoRoot: string, from: string, to: string, rel: string, context: number): string | null {
-  return git(repoRoot, ['--literal-pathspecs', 'diff', '--no-color', '--no-ext-diff', '--no-renames', '--binary', '--full-index', `-U${context}`, from, to, '--', rel]);
+  return git(repoRoot, ['--literal-pathspecs', 'diff', '--no-color', '--no-ext-diff', '--no-textconv', '--no-renames', '--binary', '--full-index', `-U${context}`, from, to, '--', rel]);
 }
 
 function patchId(repoRoot: string, diff: string): string | null {
@@ -110,7 +112,7 @@ export function comparePath(repoRoot: string, s: PatchSides, rel: string): PathV
   if (ids[0] === null || ids[1] === null) return 'changed';
   const r0 = zeroHunks(pathDiff(repoRoot, s.reviewedBase, s.reviewed, rel, 0) ?? '');
   const c0 = zeroHunks(pathDiff(repoRoot, s.currentBase, s.current, rel, 0) ?? '');
-  if (ids[0] === ids[1] && r0 !== null && !r0.length) return 'same';
+  if (ids[0] === ids[1] && r0 === null) return 'same';
   const b0 = zeroHunks(pathDiff(repoRoot, s.reviewedBase, s.currentBase, rel, 0) ?? '');
   if (!r0?.length || !c0 || !b0 || r0.length !== c0.length || r0.some((h, i) => 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<string> | 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 }

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 <record.json> written beside the packet, outside the worktree ({"tree":"${now}","paths":[…],"reviewer":"<model>","verdict":"clean"}); a blocking finding is a behavior change (stage 2.1)`);

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');