Stop /ship asking for a money review of files the review policy leaves out (GST-1900) (#4159)

* Stop /ship asking for a money review of files the review policy leaves out (GST-1900)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

* chore: changelog and TODOS

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>

bin/gstack-review-plan

@@ -240,7 +240,7 @@ function main(): number {
         : { base: rangeBase, runtime_lines: range.runtimeLines, ...(range.class === 'gate' ? { touched_gate: true as const } : {}) };
     }
   }
-  const targeted = targetedClasses({ ...collected, ...(active ? { paths: paths.filter(p => !active.excluded.has(p)), added: kept(added), removed: kept(collected.removed) } : {}), scope, gateTagged });
+  const targeted = targetedClasses({ ...collected, scope, gateTagged, ...(active ? { excluded: active.excluded } : {}) });
   const patch = collected.mergeBase ? targetedPatchId(process.cwd(), collected.mergeBase) : 'none';
   const brief = targetedBriefSha();
   const ran = !['uncommitted', 'none'].includes(patch) && flag('--targeted-ran') === patch;

bin/gstack-ship-receipt

@@ -298,7 +298,7 @@ function cmdWrite(rest: string[], json: boolean): number {
   const product = reviewedProduct(repoRoot, base, head, pregateFile);
   let targeted: ReturnType<typeof parseReviewTargeted> | undefined;
   if (flag(rest, '--review-targeted') !== undefined) {
-    try { targeted = parseReviewTargeted(jsonArg(flag(rest, '--review-targeted')), product.product_patch_id ?? 'none', read => reviewedProduct(repoRoot, base, read, pregateFile).product_patch_id, read => targetedClassesSince(repoRoot, read, headSha ?? head, base), read => plainDocsSince(repoRoot, read, headSha ?? head, base), read => spawnSync('git', ['merge-base', '--is-ancestor', read, headSha ?? head], { cwd: repoRoot, timeout: 30_000 }).status === 0); } catch (e: any) { targeted = { ok: false, message: `--review-targeted: ${e.message}` }; }
+    try { targeted = parseReviewTargeted(jsonArg(flag(rest, '--review-targeted')), product.product_patch_id ?? 'none', read => reviewedProduct(repoRoot, base, read, pregateFile).product_patch_id, read => targetedClassesSince(repoRoot, read, headSha ?? head, base, review.exclude.state === 'active' ? review.exclude.excluded : undefined), read => plainDocsSince(repoRoot, read, headSha ?? head, base), read => spawnSync('git', ['merge-base', '--is-ancestor', read, headSha ?? head], { cwd: repoRoot, timeout: 30_000 }).status === 0); } catch (e: any) { targeted = { ok: false, message: `--review-targeted: ${e.message}` }; }
   }
   if (targeted && !targeted.ok) { process.stderr.write(resultLine('gstack-ship-receipt write', 'RECEIPT_INVALID', `${targeted.message}; nothing written`) + '\n'); return EXIT.fail; }
   const approval = approvalForReceipt({ repoRoot, base, head, citationText: [bodyText, ...flags(rest, '--approval')].join('\n'), runDirs: flags(rest, '--approval-run') });

changelog.d/ship-skips-money-review-of-excluded-files.md

@@ -0,0 +1 @@
+- **`/ship` no longer asks for a money review of files your review policy leaves out.** When a repo's `review_exclude_paths` skipped a file such as a committed SQLite ledger, `gstack-review-plan` correctly said no targeted review was needed, but `gstack-ship-receipt write` at the same commit still refused with `REVIEW_TARGETED: required=money`, so the run could not finish (GST-1900). The receipt write, the receipt carry, the merge queue's check and the plan now all leave excluded files out of that check the same way. A money-looking file the patterns don't cover still needs the money review, and an exclusion that isn't active (no policy key, a pattern only the branch adds, or a gate scan the receipt doesn't bind) still excludes nothing.

lib/review-targeted.ts

@@ -9,7 +9,8 @@
  *
  * Matching reads added and deleted lines, so removing a cap, a check or a redaction
  * counts. Path and identifier signals are token-bounded: `costume` never names cost.
- * Docs and tests paths never trigger. Any input the classifier cannot read fails
+ * Docs and tests paths never trigger, nor do paths an active review_exclude_paths
+ * selection excludes (`excluded`), which no reader reads. Any input the classifier cannot read fails
  * closed to all four classes, as a depth error fails the depth closed to full.
  *
  * The gate class is the gate scan's tagged hunks (a relaxed threshold, a skipped test, a
@@ -50,11 +51,16 @@ export interface TargetedInput {
   /** The collector's consumer scan and drawings (ClassInput): a data file no code reads names no money. */
   consumed?: ClassInput['consumed'];
   drawings?: ClassInput['drawings'];
+  /** Paths an active review_exclude_paths selection leaves out (lib/review-exclude.ts): no reader reads them, so they name no class (GST-1900). */
+  excluded?: ReadonlySet<string>;
 }
 export interface TargetedResult { classes: TargetedClass[]; reasons: string[]; errors: string[]; rules: string }
 
 /** The classes a diff touches, in TARGETED_CLASSES order, with one reason per class; any input error returns all four. */
-export function targetedClasses(i: TargetedInput): TargetedResult {
+export function targetedClasses(input: TargetedInput): TargetedResult {
+  const x = input.excluded;
+  const kept = <T,>(rec?: Record<string, T>) => (x && rec ? Object.fromEntries(Object.entries(rec).filter(([p]) => !x.has(p))) : rec);
+  const i = x ? { ...input, paths: input.paths.filter(p => !x.has(p)), added: kept(input.added), removed: kept(input.removed) } : input;
   const errors = (i.errors ?? []).map(e => `${e.code} (${e.input})`);
   const s = i.scope;
   if (!s) errors.push('scope unavailable (gstack-diff-scope did not run)');

lib/ship-receipt-depth.ts

@@ -283,6 +283,9 @@ function headInputs(root: string, baseRef: string, head: string, r?: ShipReceipt
   return { baseSha, c, scope: scopeOfRange(root, BIN_DIR, baseRev, head, merged), x: exclusionAt(root, baseRef, baseSha.ok ? baseSha.out : null, head, c, tagged, 'gstack-ship-queue') };
 }
 
+/** The targeted classes of the head's range under the exclusion its review read: excluded paths name no class, as gstack-review-plan reads them (GST-1900). */
+const targetedAt = (at: HeadInputs, gateTagged: number | null) => targetedClasses({ ...at.c, scope: at.scope, gateTagged, ...(at.x.state === 'active' ? { excluded: at.x.excluded } : {}) });
+
 /** The selection's list hash over the PR's files; the empty list's when the base declares no pattern or none matches (GST-1408). */
 const listOf = (x: Exclusion) => (x.state === 'active' ? x.record.list_sha256 : EMPTY_LIST_SHA256);
 type RecordedExclude = Partial<Pick<ExcludeRecord, 'patterns_sha256' | 'list_sha256' | 'digest'>>;
@@ -510,9 +513,7 @@ export function bindShippedReceipt(root: string, baseRef: string, body: string,
   if (mismatch) return { ok: false, code: 'RECEIPT_INVALID', message: mismatch, line: b.line };
   const d = receiptDepthVerdict(root, baseRef, b.receipt, b.head, at);
   const gateTagged = boundScan(b.receipt, b.head) ?? (gateScanNeeded(at.c.paths, at.c.root) ? null : 0);
-  const x = at.x.state === 'active' ? at.x.excluded : null;
-  const without = <T,>(rec: Record<string, T>) => (x ? Object.fromEntries(Object.entries(rec).filter(([p]) => !x.has(p))) : rec);
-  const t = receiptTargetedVerdict(targetedClasses({ ...at.c, ...(x ? { paths: at.c.paths.filter(p => !x.has(p)), added: without(at.c.added), removed: without(at.c.removed) } : {}), scope: at.scope, gateTagged }).classes, b.receipt);
+  const t = receiptTargetedVerdict(targetedAt(at, gateTagged).classes, b.receipt);
   if (!d.ok) return { ok: false, code: 'QUEUE_RECEIPT_DEPTH_RAISED', message: d.message!, line: b.line, depthLine: d.line, targetedLine: t.line };
   if (!t.ok) return { ok: false, code: 'QUEUE_RECEIPT_TARGETED_MISSING', message: `${t.message}; the receipt is unchanged; run /ship on ${b.head.slice(0, 12)}`, line: b.line, depthLine: d.line, targetedLine: t.line };
   return { ...b, depthLine: d.line, targetedLine: t.line };
@@ -594,8 +595,9 @@ function ownChangeSince(root: string, base: string, from: string, head: string,
  * The targeted classes of what the PR's own change did from `from` to `head` against origin/<base> (ownChangeSince), with
  * gstack-gate-diff run over that change whenever a non-docs path moved (a test edit that skips a case is gate class); null when
  * `from` is not an ancestor of `head`, the change cannot be built, the gate scan did not run, or a class input is unreadable.
+ * `excluded` is the head's active review_exclude_paths selection (receiptReviewAtHead): those paths name no class (GST-1900).
  */
-export function targetedClassesSince(root: string, from: string, head: string, base: string): readonly TargetedClass[] | null {
+export function targetedClassesSince(root: string, from: string, head: string, base: string, excluded?: ReadonlySet<string>): readonly TargetedClass[] | null {
   if (!git(root, ['merge-base', '--is-ancestor', from, head]).ok) return null;
   const since = productChangeSince(root, base, from, head)?.since;
   if (!since) return null;
@@ -607,7 +609,7 @@ export function targetedClassesSince(root: string, from: string, head: string, b
     if (g.status !== 0 || !m) return null;
     gateTagged = m[1]!.split(',').reduce((n, kv) => n + (Number(kv.split(':')[1]) || 0), 0);
   }
-  const t = targetedClasses({ ...at.c, scope: at.scope, gateTagged });
+  const t = targetedClasses({ ...at.c, scope: at.scope, gateTagged, ...(excluded ? { excluded } : {}) });
   return t.errors.length ? null : t.classes;
 }
 
@@ -708,9 +710,9 @@ const DOCS_CLAIM_WORDS = new Set([...MONEY_PATH_TOKENS, ...AUTH_PATH_TOKENS, 'ca
  * never refused on here: the queue reads its own inputs at ready and refuses there if the head needs the reader.
  */
 export function receiptTargetedAtHead(root: string, baseRef: string, r: ShipReceipt, head: string): DepthVerdict {
-  const at = headInputs(root, baseRef, head);
+  const at = headInputs(root, baseRef, head, r);
   const gateTagged = boundScan(r, head) ?? (gateScanNeeded(at.c.paths, at.c.root) ? null : 0);
-  const t = targetedClasses({ ...at.c, scope: at.scope, gateTagged });
+  const t = targetedAt(at, gateTagged);
   const v = receiptTargetedVerdict(t.classes, r);
   return v.ok || !t.errors.length ? v : { ok: true, line: `${v.line} (unchecked: ${t.errors.join('; ')}; the queue decides at ready)` };
 }

test/ship-receipt-targeted-exclude.test.ts

@@ -0,0 +1,132 @@
+/**
+ * The receipt's targeted check under review_exclude_paths (GST-1900; HARN-1, harness-evals#322): a file the base policy
+ * excludes names no targeted class at receipt write, carry or the queue, as gstack-review-plan reads it at the same head.
+ * An exclusion that is not active (no key, a pattern only the branch adds, a gate scan the receipt does not bind) excludes
+ * nothing, so the check stays closed.
+ */
+import { afterEach, describe, expect, test } from 'bun:test';
+import * as fs from 'node:fs';
+import * as path from 'node:path';
+import { EXIT } from '../lib/headless-artifacts';
+import { extractReceipt } from '../lib/ship-receipt';
+import { bindShippedReceipt, receiptReviewAtHead, receiptTargetedAtHead, targetedClassesSince } from '../lib/ship-receipt-depth';
+import { REVIEW_EXCLUDE_KEY_VERSION } from '../lib/ship-policy';
+import { cleanup, commit, makeFixtureRepo, runBin, type FixtureRepo } from './helpers/pregate-fixture';
+
+const repos: FixtureRepo[] = [];
+afterEach(() => { while (repos.length) cleanup(repos.pop()!); });
+const SQLITE = Buffer.concat([Buffer.from('SQLite format 3\0'), Buffer.alloc(64, 1)]);
+const LEDGER = 'probe/probe-cc.ledger.sqlite';
+const policyFile = (patterns: string[]) => ({ '.gstack/ship-policy.json': JSON.stringify({ review_exclude_paths: patterns, min_gstack_version: REVIEW_EXCLUDE_KEY_VERSION }) + '\n' });
+
+/** A branch that commits SQLite ledgers (HARN-1's probe ledger shape) and a docs note; the policy sits on the base, or on the branch only. */
+function branch(patterns: string[] | null, ledgers: string[], on: 'base' | 'branch' = 'base'): FixtureRepo {
+  const repo = makeFixtureRepo({ prefix: 'gstack-targeted-exclude-', baseFiles: patterns && on === 'base' ? policyFile(patterns) : {} });
+  repos.push(repo);
+  for (const rel of ledgers) {
+    fs.mkdirSync(path.dirname(path.join(repo.work, rel)), { recursive: true });
+    fs.writeFileSync(path.join(repo.work, rel), SQLITE);
+    repo.git('add', rel);
+  }
+  commit(repo, { 'docs/probe.md': '# probe\n', ...(patterns && on === 'branch' ? policyFile(patterns) : {}) });
+  return repo;
+}
+const planOf = (repo: FixtureRepo) => JSON.parse(runBin(repo, 'gstack-review-plan', ['--base', 'main', '--json']).out);
+const listOf = (classes: string[]) => (classes.length ? classes.join(',') : 'none');
+const requiredIn = (text: string) => /REVIEW_TARGETED: required=(\S+)/.exec(text)?.[1];
+/** Receipt write with no targeted record and the digest the plan printed, as /ship writes it when the plan names no class. */
+function writeReceipt(repo: FixtureRepo, digest: string) {
+  const body = path.join(repo.root, 'body.md');
+  const r = runBin(repo, 'gstack-ship-receipt', ['write', '--pr', '7', '--base', 'main', '--version', '1.0.0', '--session-kind', 'unattended', '--body', body, '--review-exclude', digest]);
+  return { ...r, body };
+}
+
+describe('the receipt reads the targeted classes the review plan reads (GST-1900)', () => {
+  // Value: protects=a ledger under review_exclude_paths needs no money reader at receipt write, carry or ready, as the plan says
+  //   fails_when=receiptTargetedAtHead classifies excluded paths (HARN-1: REVIEW_TARGETED required=money vs plan classes=none)
+  //   why_new=the exclusion tests check the plan's classes and the receipt digest, never the receipt's targeted verdict
+  //   seam=gstack-review-plan --json, gstack-ship-receipt write, receiptTargetedAtHead, bindShippedReceipt on one head
+  test('an excluded money path gives no class at write, at the queue, or in the plan', () => {
+    const repo = branch(['probe/**'], [LEDGER]);
+    const plan = planOf(repo);
+    expect(plan.review_exclude).toMatchObject({ state: 'active', excluded: 1, kept: 0 });
+    expect(plan.targeted.classes).toEqual([]);
+    const w = writeReceipt(repo, plan.review_exclude.digest);
+    expect({ status: w.status, targeted: requiredIn(w.err) ?? null }).toEqual({ status: EXIT.ok, targeted: null });
+    const text = fs.readFileSync(w.body, 'utf8');
+    const receipt = extractReceipt(text).receipt!;
+    const head = repo.git('rev-parse', 'HEAD');
+    const atHead = receiptTargetedAtHead(repo.work, 'origin/main', receipt, head);
+    expect({ ok: atHead.ok, required: requiredIn(atHead.line) }).toEqual({ ok: true, required: listOf(plan.targeted.classes) });
+    const queue = bindShippedReceipt(repo.work, 'origin/main', text, undefined, [head]);
+    expect(requiredIn(queue.ok ? queue.targetedLine : queue.targetedLine ?? queue.message)).toBe(listOf(plan.targeted.classes));
+  }, 180_000);
+
+  // Value: protects=a money path outside the patterns still needs the money reader beside an excluded one
+  //   fails_when=the exclusion fix drops every path, or classifies only the excluded file set
+  //   why_new=the narrowing must keep the fail-closed check for the paths reviewers read
+  //   seam=gstack-review-plan --json and gstack-ship-receipt write on one head
+  test('a money path the patterns do not match still requires money at write, as the plan says', () => {
+    const repo = branch(['probe/**'], [LEDGER, 'results/spend.ledger.sqlite']);
+    const plan = planOf(repo);
+    expect(plan.review_exclude).toMatchObject({ state: 'active', excluded: 1 });
+    expect(plan.targeted.classes).toEqual(['money']);
+    const w = writeReceipt(repo, plan.review_exclude.digest);
+    expect(w.status).toBe(EXIT.fail);
+    expect(w.err).toContain('REVIEW_TARGETED: required=money receipt=legacy verdict=missing');
+    expect(requiredIn(w.err)).toBe(listOf(plan.targeted.classes));
+    expect(fs.existsSync(w.body)).toBe(false);
+  }, 180_000);
+
+  // Value: protects=no key, or a pattern only the branch adds, excludes nothing, so the ledger still needs the money reader
+  //   fails_when=the receipt reads a head-only or absent policy as an exclusion
+  //   why_new=the fix narrows a fail-closed check; these are the states it must not narrow
+  //   seam=gstack-review-plan --json and gstack-ship-receipt write on one head
+  for (const [name, patterns, on] of [['no review_exclude_paths', null, 'base'], ['a pattern only the branch adds', ['probe/**'], 'branch']] as const) {
+    test(`${name} still requires money at write, as the plan says`, () => {
+      const repo = branch(patterns ? [...patterns] : null, [LEDGER], on);
+      const plan = planOf(repo);
+      expect(plan.review_exclude?.state ?? 'none').not.toBe('active');
+      expect(plan.targeted.classes).toEqual(['money']);
+      const w = writeReceipt(repo, 'none');
+      expect(w.status).toBe(EXIT.fail);
+      expect(requiredIn(w.err)).toBe('money');
+    }, 180_000);
+  }
+
+  // Value: protects=a receipt whose gate scan names another head narrows nothing: the check fails closed and defers to the queue
+  //   fails_when=the targeted check takes the exclusion without the receipt's bound gate scan
+  //   why_new=the gate scan decides which candidates the exclusion keeps; an unbound scan keeps them all
+  //   seam=receiptTargetedAtHead on a written receipt with its gate_scan head moved
+  test('a receipt that does not bind the gate scan to its head fails closed to every class', () => {
+    const repo = branch(['probe/**'], [LEDGER]);
+    const w = writeReceipt(repo, planOf(repo).review_exclude.digest);
+    expect(w.status).toBe(EXIT.ok);
+    const receipt = extractReceipt(fs.readFileSync(w.body, 'utf8')).receipt!;
+    const unbound = { ...receipt, review_depth: { ...receipt.review_depth!, gate_scan: { ...receipt.review_depth!.gate_scan, head: '0'.repeat(40) } } };
+    const v = receiptTargetedAtHead(repo.work, 'origin/main', unbound, repo.git('rev-parse', 'HEAD'));
+    expect(requiredIn(v.line)).toBe('money,gate,auth,secret');
+    expect(v.line).toContain('(unchecked: gate scan unavailable; the queue decides at ready)');
+  }, 180_000);
+
+  // Value: protects=a targeted record carries past a later commit that only changes an excluded ledger
+  //   fails_when=targetedClassesSince classifies paths the head's exclusion leaves out
+  //   why_new=write's since read is the other targeted read in lib/ship-receipt-depth.ts with no exclusion
+  //   seam=targetedClassesSince with the exclusion receiptReviewAtHead gives write; no_claim=the CLI's product-patch check
+  test('the change since a targeted read names no class for a ledger the head excludes, and money without the exclusion', () => {
+    const repo = branch(['probe/**'], []);
+    const read = repo.git('rev-parse', 'HEAD');
+    fs.mkdirSync(path.join(repo.work, 'probe'), { recursive: true });
+    fs.writeFileSync(path.join(repo.work, LEDGER), SQLITE);
+    repo.git('add', LEDGER);
+    const head = commit(repo, { 'docs/probe.md': '# probe\n\nThe ledger is committed.\n' }, 'probe ledger');
+    const prior = process.env.GSTACK_STATE_ROOT;
+    process.env.GSTACK_STATE_ROOT = path.join(repo.root, 'receipt-state');
+    try {
+      const { exclude } = receiptReviewAtHead(repo.work, 'main', path.join(import.meta.dir, '..', 'bin'), head);
+      expect(exclude.state === 'active' ? [...exclude.excluded] : exclude.state).toEqual([LEDGER]);
+      expect(targetedClassesSince(repo.work, read, head, 'main', exclude.state === 'active' ? exclude.excluded : undefined)).toEqual([]);
+      expect(targetedClassesSince(repo.work, read, head, 'main')).toEqual(['money']);
+    } finally { if (prior === undefined) delete process.env.GSTACK_STATE_ROOT; else process.env.GSTACK_STATE_ROOT = prior; }
+  }, 180_000);
+});