diff --git a/bin/gstack-approval-classes b/bin/gstack-approval-classes index 0ebf2b34e..7d5ccee95 100755 --- a/bin/gstack-approval-classes +++ b/bin/gstack-approval-classes @@ -73,6 +73,7 @@ function main(argv: string[]): number { const outcome = runApprovalScan({ repoRoot, base, head: flag(rest, '--head'), diffBase: flag(rest, '--diff-base') }); if ('unavailable' in outcome) { process.stderr.write(resultLine('gstack-approval-classes', 'APPROVAL_SOURCE_UNAVAILABLE', `${outcome.unavailable}; classes are read only from the base, so nothing was checked`) + '\n'); return EXIT.refused; } if ('errors' in outcome) { for (const e of outcome.errors) process.stderr.write(resultLine('gstack-approval-classes', 'APPROVAL_CLASSES_INVALID', `${e.message} at ${e.path || ''} (${outcome.source})`) + '\n'); return EXIT.refused; } + if ('error' in outcome && outcome.unreadable) { process.stderr.write(resultLine('gstack-approval-classes', 'APPROVAL_SOURCE_UNAVAILABLE', `${outcome.error}, so nothing was checked`) + '\n'); return EXIT.refused; } if ('error' in outcome) { process.stderr.write(`gstack-approval-classes: ${outcome.error}\n`); return EXIT.usage; } const { scan, absent } = outcome; process.stdout.write(json ? JSON.stringify({ ...scan, absent }) + '\n' : renderApprovalScan(scan, absent)); diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index e74a64a48..bd3baa6aa 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -109,6 +109,7 @@ function cmdWrite(rest: string[], json: boolean): number { for (const line of approval.lines) process.stderr.write(resultLine('gstack-ship-receipt write', 'APPROVAL_CLASSES_INVALID', line) + '\n'); return EXIT.fail; } + if (approval.kind === 'error' && approval.code) { process.stderr.write(resultLine('gstack-ship-receipt write', approval.code, `${approval.message}; nothing written`) + '\n'); return EXIT.fail; } if (approval.kind === 'error') { process.stderr.write(`gstack-ship-receipt write: approval classes: ${approval.message}; nothing written\n`); return EXIT.fail; } if (approval.kind === 'missing') { for (const m of approval.missing) process.stderr.write(resultLine('gstack-ship-receipt write', 'APPROVAL_CITATION_MISSING', `${m.class} (${m.token}; effects in ${m.files.join(', ')}): ${m.reasons.join('; ')}`) + '\n'); diff --git a/docs/ship-policy.md b/docs/ship-policy.md index ea16ad6e2..d036446b7 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -527,7 +527,11 @@ detectors only report: the ship policy's migration files, when no class sets `migrations: true`, print as `(built-in, report-only)` and need no citation. The digest covers the class's effects: each matched file with its blob at the -scanned head, and each matched added line. Adding, removing or editing an +scanned head, and each matched added line. The scan reads git's native diff +(`--no-ext-diff --no-textconv`), so a configured external diff driver or text +conversion cannot hide a line; when the content diff cannot be read, or its +added lines disagree with git's own count, `check` and the receipt refuse with +`APPROVAL_SOURCE_UNAVAILABLE` instead of passing. Adding, removing or editing an effect changes the token, so an approval given earlier no longer binds. **Enforcement.** `gstack-ship-receipt write` refuses with @@ -544,9 +548,16 @@ that resolves and binds: passed with `--approval-run ` whose gate is APPROVED, and the plan must name the scope token, or the class and every effect path. - A GitHub comment or review URL must be written by a `maintainers` login from - the base's ship policy that equals the cited owner (a review requesting - changes is not an approval), and its body must name the token, or the class - and every effect path. + the base's ship policy that equals the cited owner. A review must be in + state `APPROVED`; a comment-only, dismissed or changes-requested review is + not an approval. Only approval lines count as evidence: a line that starts + (after any list or quote markers) with `Approve` or `Approved`, any case, + and names the token, or the class and every effect path, such as + `Approved data-repair@0123456789ab: preview-first, never at upgrade.` A + comment saying `Do not approve data-repair@0123456789ab` approves nothing. +- A token is authoritative. Evidence that names the class with a different + digest approved earlier effects: it does not bind, and it never falls back + to matching the class and paths. - Anything else (a thread code, a chat message, a URL gstack cannot read) is recorded `citation: unverified` for the coordinator, and only when the line attests which scope it covers with `for @` matching the diff --git a/lib/approval-citations.ts b/lib/approval-citations.ts index 63cf474fc..25ba0b2b7 100644 --- a/lib/approval-citations.ts +++ b/lib/approval-citations.ts @@ -9,15 +9,18 @@ * decision answered and not reject/interrogate), with plan.md matching its * manifest hash; * - a GitHub comment or review URL must be written by a ship-policy - * maintainer whose login is the cited owner (a review requesting changes is - * not an approval); + * maintainer whose login is the cited owner, and only its approval lines + * (APPROVAL_LINE: a line starting with "Approve" or "Approved") count; a + * review must also be in state APPROVED; * - anything else, or a ref that cannot be read here, is `unverified`. * * Resolution alone is not enough: the evidence (the approved plan text or - * the comment body) must bind to the class's current effects, by carrying the - * scope token `@` or by naming the class and every effect - * path. An authentic approval of other work, or one given before an effect - * was added, does not bind. An unverified citation counts only as an explicit + * the comment's approval lines) must bind to the class's current effects, by + * carrying the scope token `@` or by naming the class and + * every effect path. A token is authoritative: evidence naming the class with + * another digest approved earlier effects and never falls back to paths. An + * authentic approval of other work, or one given before an effect was added, + * does not bind. An unverified citation counts only as an explicit * applicability attestation: `for @` on the line, matching * the current scope, so a later effect change invalidates it. * Caller: bin/gstack-ship-receipt write (APPROVAL_CITATION_MISSING). @@ -31,7 +34,7 @@ import { isUnavailable, loadPolicyFromBase } from './ship-policy'; export interface Citation { raw: string; owner: string; date: string; ref: string; for: Array<{ class: string; digest?: string }> } export type Resolution = - | { state: 'resolved'; evidence: string; text: string } + | { state: 'resolved'; evidence: string; text: string; approvalLines?: true } | { state: 'rejected'; why: string } | { state: 'unresolved'; why: string }; export type GhRunner = (args: string[]) => { ok: boolean; out: string; err: string }; @@ -93,9 +96,12 @@ export function resolvePlan(ref: string, runDirs: string[]): Resolution { return rejected ?? { state: 'unresolved', why: runDirs.length ? `no --approval-run directory holds a plan with sha ${ref}` : `plan sha ${ref}: pass the autoplan run with --approval-run ` }; } +/** An affirmative approval in a comment or review: the line starts (after list or quote markers) with "Approve" or "Approved". */ +export const APPROVAL_LINE = /^[\s>*_-]*approved?\b/i; + const GH_URL = /^https:\/\/github\.com\/([\w.-]+)\/([\w.-]+)\/(?:pull|issues)\/(\d+)#(?:issuecomment-(\d+)|pullrequestreview-(\d+)|discussion_r(\d+))$/; -/** A comment or review URL: authored by a ship-policy maintainer whose login is the cited owner. */ +/** A comment or review URL: authored by a ship-policy maintainer whose login is the cited owner; a review must be APPROVED; only approval lines are evidence. */ export function resolveGithub(c: Citation, ctx: CitationContext): Resolution { const m = GH_URL.exec(c.ref)!; const [, owner, repo, num, issueComment, review, discussion] = m; @@ -111,7 +117,9 @@ export function resolveGithub(c: Citation, ctx: CitationContext): Resolution { if (!ctx.maintainers.some(x => x.toLowerCase() === login.toLowerCase())) return { state: 'rejected', why: `${c.ref} is by ${login || 'an unknown author'}, not a ship-policy maintainer` }; if (login.toLowerCase() !== c.owner.toLowerCase()) return { state: 'rejected', why: `the citation names ${c.owner} but ${c.ref} is by ${login}` }; if (review && body.state === 'CHANGES_REQUESTED') return { state: 'rejected', why: `${c.ref} requests changes; it is not an approval` }; - return { state: 'resolved', evidence: `${review ? 'review' : 'comment'} by ${login}`, text: String(body.body ?? '') }; + if (review && body.state !== 'APPROVED') return { state: 'rejected', why: `${c.ref} is a ${String(body.state ?? 'stateless').toLowerCase()} review; only an APPROVED review counts` }; + const approvals = String(body.body ?? '').split(/\r?\n/).filter(l => APPROVAL_LINE.test(l)); + return { state: 'resolved', evidence: `${review ? 'review' : 'comment'} by ${login}`, text: approvals.join('\n'), approvalLines: true }; } export function resolveCitation(c: Citation, ctx: CitationContext): Resolution { @@ -120,10 +128,16 @@ export function resolveCitation(c: Citation, ctx: CitationContext): Resolution { return { state: 'unresolved', why: `${c.ref} is neither a plan sha nor a GitHub comment or review URL` }; } -/** How the evidence names this exact scope: the token, or the class plus every effect path; null when it does not. */ +/** The digests `text` names for this class (`@`), lowercased. */ +function namedDigests(text: string, cls: string): string[] { + return [...text.toLowerCase().matchAll(new RegExp(`(?:^|[^a-z0-9-])${cls}@([0-9a-f]{12,64})(?![0-9a-f])`, 'g'))].map(m => m[1]!); +} + +/** How the evidence names this exact scope: the token, or (when it names no token for the class) the class plus every effect path; null otherwise. */ export function binding(text: string, scope: ApprovalScope): 'token' | 'paths' | null { + const digests = namedDigests(text, scope.class); + if (digests.length) return digests.some(d => scope.digest.startsWith(d)) ? 'token' : null; const lower = text.toLowerCase(); - if (lower.includes(scope.token.toLowerCase())) return 'token'; const named = new RegExp(`(^|[^a-z0-9-])${scope.class}([^a-z0-9-]|$)`).test(lower); return named && scope.files.every(f => text.includes(f)) ? 'paths' : null; } @@ -140,7 +154,10 @@ function judge(c: Citation, scope: ApprovalScope, res: Resolution): ApprovalReco if (res.state === 'resolved') { const how = binding(res.text, scope); if (how) return { ...base, citation: 'verified', binding: how, evidence: res.evidence }; - return `${c.ref}: ${res.evidence} names neither ${scope.token} nor ${scope.class} with every effect path (${scope.files.join(', ')}); an approval of other work, or one given before these effects, does not cover them`; + const stale = namedDigests(res.text, scope.class)[0]; + if (stale) return `${c.ref}: ${res.evidence} names ${scope.class}@${stale.slice(0, 12)}, not the current ${scope.token}; a token is authoritative, so approval of earlier effects never falls back to paths`; + const where = res.approvalLines ? ' on an approval line (one starting with "Approved" or "Approve")' : ''; + return `${c.ref}: ${res.evidence} names neither ${scope.token} nor ${scope.class} with every effect path (${scope.files.join(', ')})${where}; an approval of other work, or one given before these effects, does not cover them`; } const attested = c.for.find(f => f.class === scope.class); if (attested?.digest && scope.digest.startsWith(attested.digest)) return { ...base, citation: 'unverified', binding: 'attested', evidence: `unresolved: ${res.why}` }; @@ -174,7 +191,7 @@ export type ReceiptApproval = | { kind: 'skip' } | { kind: 'ok'; approval: Record; note?: string } | { kind: 'invalid'; lines: string[] } - | { kind: 'error'; message: string } + | { kind: 'error'; message: string; code?: 'APPROVAL_SOURCE_UNAVAILABLE' } | { kind: 'missing'; missing: MissingApproval[] }; const reportOnly = (hits: ApprovalHit[]) => hits.map(h => `${h.class} ${h.file}:${h.line} (${h.text})`); @@ -184,7 +201,7 @@ export function approvalForReceipt(req: ReceiptApprovalRequest): ReceiptApproval const outcome = runApprovalScan({ repoRoot: req.repoRoot, base: req.base, head: req.head }); if ('unavailable' in outcome) return { kind: 'ok', approval: { source: outcome.source, status: 'unavailable', detail: outcome.unavailable, classes: [] }, note: `APPROVAL_CLASSES: unavailable (${outcome.unavailable}); approval classes were not checked` }; if ('errors' in outcome) return { kind: 'invalid', lines: outcome.errors.map(e => `${e.message} at ${e.path || ''} (${outcome.source})`) }; - if ('error' in outcome) return { kind: 'error', message: outcome.error }; + if ('error' in outcome) return { kind: 'error', message: outcome.error, ...(outcome.unreadable ? { code: 'APPROVAL_SOURCE_UNAVAILABLE' as const } : {}) }; const { scan, absent } = outcome; if (absent && !scan.report_only.length) return { kind: 'skip' }; const policy = loadPolicyFromBase(req.repoRoot, req.base); diff --git a/lib/approval-classes.ts b/lib/approval-classes.ts index 4b7847039..1a223fc2d 100644 --- a/lib/approval-classes.ts +++ b/lib/approval-classes.ts @@ -149,13 +149,26 @@ export function baseMigrationRule(repoRoot: string, base: string): MigrationRule export interface ChangedFile { file: string; status: string; blob: string; added: Array<{ line: number; text: string }> } -/** Files changed between `diffBase` and `head` (the working tree when `head` is omitted), with added lines and blob ids. */ -export function changedFiles(repoRoot: string, diffBase: string, head?: string): ChangedFile[] | { error: string } { +/** Native, unquoted git diff output: no external diff driver and no textconv, whatever the repository's config says. */ +const NATIVE_DIFF = ['-c', 'core.quotePath=false', 'diff', '--no-ext-diff', '--no-textconv', '--no-color', '--no-renames']; + +/** + * Files changed between `diffBase` and `head` (the working tree when `head` is omitted), with added lines and blob ids. + * Fails closed (`unreadable`) when the content diff cannot be read or its added lines disagree with git's numstat count. + */ +export function changedFiles(repoRoot: string, diffBase: string, head?: string): ChangedFile[] | { error: string; unreadable: true } { const range = head ? [diffBase, head] : [diffBase]; - const names = git(repoRoot, ['diff', '--name-status', '-z', '--no-renames', ...range], 120_000); - const diff = git(repoRoot, ['diff', '--unified=0', '--no-color', '--no-renames', ...range], 120_000); - if (!names.ok || !diff.ok) return { error: `git diff ${range.join(' ')} failed` }; + const names = git(repoRoot, [...NATIVE_DIFF, '--name-status', '-z', ...range], 120_000); + const counts = git(repoRoot, [...NATIVE_DIFF, '--numstat', '-z', ...range], 120_000); + const diff = git(repoRoot, [...NATIVE_DIFF, '--unified=0', ...range], 120_000); + if (!names.ok || !counts.ok || !diff.ok) return { error: `git diff ${range.join(' ')} failed; the content diff could not be read`, unreadable: true }; const added = addedLines(diff.out); + for (const entry of counts.out.split('\0').filter(Boolean)) { + const [n, , file] = entry.split('\t'); + if (!file || n === '-') continue; + const got = added.get(file)?.length ?? 0; + if (got !== Number(n)) return { error: `the content diff for ${file} shows ${got} added line(s) but git counts ${n}; the content diff could not be read`, unreadable: true }; + } const parts = names.out.split('\0').filter(Boolean); const out: ChangedFile[] = []; for (let i = 0; i + 1 < parts.length; i += 2) { @@ -217,7 +230,7 @@ export type ScanOutcome = | { scan: ApprovalScan; absent: boolean } | { errors: ApprovalClassesError[]; source: string } | { unavailable: string; source: string } - | { error: string }; + | { error: string; unreadable?: true }; /** Load the base's classes and migration rule, diff since the merge base, scan. Skips the diff when nothing could match. */ export function runApprovalScan(req: ScanRequest): ScanOutcome { diff --git a/test/approval-classes.test.ts b/test/approval-classes.test.ts index a3fee440f..8f782a886 100644 --- a/test/approval-classes.test.ts +++ b/test/approval-classes.test.ts @@ -9,11 +9,13 @@ * Real fixture repositories, real autoplan runs and a `gh` shim on PATH. */ import { afterEach, describe, expect, test } from 'bun:test'; +import { spawnSync } from 'node:child_process'; 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 { parseCitations } from '../lib/approval-citations'; +import { evaluateApprovals, parseCitations } from '../lib/approval-citations'; +import { scopeOf, type ApprovalHit } from '../lib/approval-classes'; import { autoplan, runPhase } from './helpers/autoplan-run-fixture'; import { BASE_POLICY, cleanup, makeFixtureRepo, runBin, type FixtureRepo } from './helpers/restamp-fixture'; @@ -212,7 +214,7 @@ describe('gstack-ship-receipt write enforces declared classes (APPROVAL_CITATION commit(repo, { 'src/commands/repair/clear-never-installed.ts': 'export const wipe = true;\n' }, 'post-approval addition'); const after = write(repo, `Approved by garrytan 2026-10-16 via ${COMMENT}\n`, [], gh); expect(after.status).toBe(EXIT.fail); - expect(after.err).toMatch(/effects in src\/commands\/repair\/accepted-takes\.ts, src\/commands\/repair\/clear-never-installed\.ts\).*names neither.*\(APPROVAL_CITATION_MISSING\)$/m); + expect(after.err).toMatch(/effects in src\/commands\/repair\/accepted-takes\.ts, src\/commands\/repair\/clear-never-installed\.ts\).*names data-repair@[0-9a-f]{12}, not the current data-repair@[0-9a-f]{12}; a token is authoritative.*\(APPROVAL_CITATION_MISSING\)$/m); }, 30_000); test('a plan sha resolves through an APPROVED autoplan run whose plan names the class and every effect path; a pending gate, an unrelated plan and a later file do not', () => { @@ -287,3 +289,81 @@ describe('gstack-ship-receipt write enforces declared classes (APPROVAL_CITATION expect(JSON.parse(gone.out).approval).toMatchObject({ status: 'unavailable', classes: [] }); }, 30_000); }); + +describe('review #3235 regressions: what counts as approval, stale tokens, and native diffs', () => { + const hit = (blob: string): ApprovalHit => ({ class: 'data-repair', file: 'src/repair.ts', line: 1, via: 'path', text: 'modified', declared: true, effect: `path\0src/repair.ts\0${blob}` }); + const before = scopeOf('data-repair', [hit('preview-true')]); + const after = scopeOf('data-repair', [hit('preview-false-wipe-all')]); + const judge = (scope: typeof before, ref: string, response: Record) => evaluateApprovals([scope], parseCitations(`Approved by garrytan 2026-10-16 via ${ref}`), { runDirs: [], maintainers: ['garrytan'], gh: () => ({ ok: true, out: JSON.stringify({ user: { login: 'garrytan' }, ...response }), err: '' }) }); + const REVIEW = 'https://github.com/acme/brain/pull/7#pullrequestreview-55'; + + test('P1-2: a maintainer comment naming the token without an approval line approves nothing; an approval line does', () => { + const rejection = judge(after, COMMENT, { body: `Do NOT approve ${after.token}. This deletes customer data.` }); + expect(rejection.records).toEqual([]); + expect(rejection.missing[0]!.reasons[0]).toMatch(/names neither data-repair@[0-9a-f]{12} nor data-repair with every effect path \(src\/repair\.ts\) on an approval line/); + expect(judge(after, COMMENT, { body: `Looks fine.\nNot approving ${after.token} yet.` }).records).toEqual([]); + expect(judge(after, COMMENT, { body: `Context first.\n> Approved ${after.token}: preview-first.` }).records.map(r => `${r.citation}/${r.binding}`)).toEqual(['verified/token']); + expect(judge(after, COMMENT, { body: `- approve ${after.token}` }).records.map(r => r.citation)).toEqual(['verified']); + }); + + test('P1-2: only an APPROVED review counts, and it still needs an approval line naming the scope', () => { + for (const state of ['COMMENTED', 'DISMISSED', 'PENDING']) { + const r = judge(after, REVIEW, { state, body: `Approved ${after.token}` }); + expect(r.records).toEqual([]); + expect(r.missing[0]!.reasons[0]).toContain(`is a ${state.toLowerCase()} review; only an APPROVED review counts`); + } + expect(judge(after, REVIEW, { state: 'APPROVED', body: `Approved ${after.token}` }).records.map(r => `${r.citation}/${r.binding}/${r.evidence}`)).toEqual(['verified/token/review by garrytan']); + expect(judge(after, REVIEW, { state: 'APPROVED', body: `LGTM ${after.token}` }).records).toEqual([]); + }); + + test('P1-3: a stale explicit token is not approval and never falls back to the class and paths; untokened class and paths still bind', () => { + const approval = `Approved ${before.token} (src/repair.ts).`; + expect(judge(before, COMMENT, { body: approval }).records.map(r => r.binding)).toEqual(['token']); + const stale = judge(after, COMMENT, { body: approval }); + expect(stale.records).toEqual([]); + expect(stale.missing[0]!.reasons[0]).toContain(`names ${before.token}, not the current ${after.token}; a token is authoritative`); + expect(judge(after, COMMENT, { body: `Approved data-repair@${before.digest} and ${after.token}` }).records.map(r => r.binding)).toEqual(['token']); + expect(judge(after, COMMENT, { body: 'Approved data-repair for src/repair.ts: preview-first.' }).records.map(r => r.binding)).toEqual(['paths']); + }); + + function defaultRepo(): FixtureRepo { + const repo = classRepo(); + commit(repo, { 'src/config.ts': 'export const DEFAULT_TOKENMAX = true;\n' }); + return repo; + } + const refusedWithoutCitation = (repo: FixtureRepo, env: Record = {}) => { + const c = runBin(repo, 'gstack-approval-classes', ['check', '--base', 'main'], env); + expect(c.status, c.err).toBe(EXIT.fail); + expect(c.out).toMatch(/^APPROVAL_CLASS: default-change src\/config\.ts:1 \(pattern\)/m); + const w = write(repo, '', [], env); + expect(w.status).toBe(EXIT.fail); + expect(w.err).toMatch(/default-change \(default-change@[0-9a-f]{12}; effects in src\/config\.ts\).*\(APPROVAL_CITATION_MISSING\)$/m); + }; + + test('P1-4: diff.external and a textconv driver cannot hide a declared pattern from check or the receipt', () => { + const repo = defaultRepo(); + repo.git('config', 'diff.external', '/bin/true'); + refusedWithoutCitation(repo); + repo.git('config', '--unset', 'diff.external'); + fs.writeFileSync(path.join(repo.work, '.git', 'info', 'attributes'), '*.ts diff=blank\n'); + repo.git('config', 'diff.blank.textconv', 'true'); + refusedWithoutCitation(repo); + }, 30_000); + + test('P1-4: a content diff that is missing or disagrees with git\'s count fails closed with APPROVAL_SOURCE_UNAVAILABLE', () => { + const repo = defaultRepo(); + const real = spawnSync('sh', ['-c', 'command -v git'], { encoding: 'utf8', timeout: 10_000 }).stdout.trim(); + const dir = path.join(repo.root, 'git-shim'); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, 'git'), `#!/bin/sh\ncase " $* " in *" --unified=0 "*) exit 0 ;; esac\nexec "${real}" "$@"\n`, { mode: 0o755 }); + const env = { PATH: `${dir}:${process.env.PATH ?? ''}` }; + const c = runBin(repo, 'gstack-approval-classes', ['check', '--base', 'main'], env); + expect(c.status).toBe(EXIT.refused); + expect(c.err).toMatch(/the content diff for src\/config\.ts shows 0 added line\(s\) but git counts 1; the content diff could not be read, so nothing was checked.*\(APPROVAL_SOURCE_UNAVAILABLE\)$/m); + const w = write(repo, '', [], env); + expect(w.status).toBe(EXIT.fail); + expect(w.err).toMatch(/\(APPROVAL_SOURCE_UNAVAILABLE\)$/m); + expect(w.out).toBe(''); + }, 30_000); +}); +