diff --git a/bin/gstack-review-plan b/bin/gstack-review-plan index 868009292..71a37b1b1 100755 --- a/bin/gstack-review-plan +++ b/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; diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index 897684d58..c4ef7ae8d 100755 --- a/bin/gstack-ship-receipt +++ b/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 | 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') }); diff --git a/changelog.d/ship-skips-money-review-of-excluded-files.md b/changelog.d/ship-skips-money-review-of-excluded-files.md new file mode 100644 index 000000000..4a6ad1f08 --- /dev/null +++ b/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. diff --git a/lib/review-targeted.ts b/lib/review-targeted.ts index ce8e040ef..79677a147 100644 --- a/lib/review-targeted.ts +++ b/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; } 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 = (rec?: Record) => (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)'); diff --git a/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index 7fc3ceedc..9a54f3e03 100644 --- a/lib/ship-receipt-depth.ts +++ b/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>; @@ -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 = (rec: Record) => (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/ (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): 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)` }; } diff --git a/test/ship-receipt-targeted-exclude.test.ts b/test/ship-receipt-targeted-exclude.test.ts new file mode 100644 index 000000000..69deb4938 --- /dev/null +++ b/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); +});