fix(approval-classes): only affirmative APPROVED evidence counts, explicit tokens are authoritative, native diffs fail closed (review #3235 P1-2, P1-3, P1-4)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
garrytanandcapy-ai[bot] committed 2026-10-17 01:22:58 +00:00
1 parent 66aad71726
commit ea49e49d74
6 files changed
+149 -26

No files matched your search

+1
View File
@@ -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 || '<root>'} (${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));
+1
View File
@@ -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');
+15 -4
View File
@@ -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 <run dir>` 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 <class>@<digest12>` matching the
+31 -14
View File
@@ -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 `<class>@<digest12>` 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 `<class>@<digest12>` 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 <class>@<digest12>` 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 <dir>` };
}
/** 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 (`<class>@<hex>`), 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<string, unknown>; 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 || '<root>'} (${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);
+19 -6
View File
@@ -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 {
+82 -2
View File
@@ -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<string, unknown>) => 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<string, string> = {}) => {
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);
});