diff --git a/docs/ship-policy.md b/docs/ship-policy.md index a6c2ee00f..e4354dbc8 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -420,15 +420,14 @@ that resolves and binds: - 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 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 the - maintainer wrote at top level that starts (after an optional list marker) - with `Approve` or `Approved`, any case, and names the token, or the class - and every effect path, such as + not an approval. The evidence is the body's first non-blank line only. It + must start at column 0 with `Approved` or `Approve` (any case, optionally + in `**` bold; no list marker, quote or indent) and name the token, or the + class and every effect path. Nothing later in the body counts, so a + rejection that quotes or nests an example approval approves nothing. For + example, a comment whose first line is `Approved data-repair@0123456789ab: preview-first, never at upgrade.` - Quoted and code text never counts: a blockquote (`>`), a fenced or indented - code block and an HTML comment are skipped, so a rejection that quotes an - example approval approves nothing, and neither does - `Do not approve data-repair@0123456789ab`. + verifies; one that opens with any other line does not. - 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. diff --git a/lib/approval-citations.ts b/lib/approval-citations.ts index 00fb639e7..831f40bda 100644 --- a/lib/approval-citations.ts +++ b/lib/approval-citations.ts @@ -9,14 +9,14 @@ * 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, and only the approval lines - * the maintainer wrote at top level count (`approvalLines`: a line starting - * with "Approve" or "Approved", a list marker allowed; never quoted, fenced, - * indented-code or HTML-comment text); a review must also be APPROVED; + * maintainer whose login is the cited owner, and the evidence is the body's + * first non-blank line only (`approvalLine`: "Approve" or "Approved" at + * column 0, optionally in `**`, no list marker, quote or indent; nothing + * later in the body counts); a review must also be 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's approval lines) must bind to the class's current effects, by + * the comment's approval line) 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 @@ -35,7 +35,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; approvalLines?: true } + | { state: 'resolved'; evidence: string; text: string; approvalLine?: true } | { state: 'rejected'; why: string } | { state: 'unresolved'; why: string }; export type GhRunner = (args: string[]) => { ok: boolean; out: string; err: string }; @@ -97,26 +97,19 @@ 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 line: up to three spaces, an optional list marker and emphasis, then "Approve" or "Approved". */ -const APPROVAL_LINE = /^ {0,3}(?:(?:[-*+]|\d{1,9}[.)])[ \t]+)?(?:\*\*|__|\*|_)?approved?\b/i; -const FENCE = /^ {0,3}(`{3,}|~{3,})/; - -/** The approval lines a maintainer wrote at top level: text in fenced or indented code, blockquotes and HTML comments never counts. */ -export function approvalLines(body: string): string[] { - const out: string[] = []; - let fence: string | null = null; - for (const line of body.replace(/|$)/g, '').split(/\r?\n/)) { - const marker = FENCE.exec(line)?.[1]; - if (fence) { if (marker && marker[0] === fence[0] && marker.length >= fence.length && !line.slice(line.indexOf(marker) + marker.length).trim()) fence = null; continue; } - if (marker) { fence = marker; continue; } - if (APPROVAL_LINE.test(line)) out.push(line); - } - return out; +/** + * The approval evidence in a comment or review body: its first non-blank line, and only when that line starts at + * column 0 with "Approve" or "Approved" (optionally in `**`), with no list marker, quote or indent; '' otherwise. + * Nothing later in the body counts, so no quoting or nesting can turn a rejection's example into consent. + */ +export function approvalLine(body: string): string { + const first = body.split(/\r?\n/).find(l => l.trim()) ?? ''; + return /^(?:\*\*)?approved?\b/i.test(first) ? first : ''; } 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 review must be APPROVED; only approval lines are evidence. */ +/** A comment or review URL: authored by a ship-policy maintainer whose login is the cited owner; a review must be APPROVED; only the body's first-line approval is evidence. */ export function resolveGithub(c: Citation, ctx: CitationContext): Resolution { const m = GH_URL.exec(c.ref)!; const [, owner, repo, num, issueComment, review, discussion] = m; @@ -133,7 +126,7 @@ export function resolveGithub(c: Citation, ctx: CitationContext): Resolution { 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` }; if (review && body.state !== 'APPROVED') return { state: 'rejected', why: `${c.ref} is a ${String(body.state ?? 'stateless').toLowerCase()} review; only an APPROVED review counts` }; - return { state: 'resolved', evidence: `${review ? 'review' : 'comment'} by ${login}`, text: approvalLines(String(body.body ?? '')).join('\n'), approvalLines: true }; + return { state: 'resolved', evidence: `${review ? 'review' : 'comment'} by ${login}`, text: approvalLine(String(body.body ?? '')), approvalLine: true }; } export function resolveCitation(c: Citation, ctx: CitationContext): Resolution { @@ -170,7 +163,7 @@ function judge(c: Citation, scope: ApprovalScope, res: Resolution): ApprovalReco if (how) return { ...base, citation: 'verified', binding: how, evidence: res.evidence }; 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")' : ''; + const where = res.approvalLine ? ' on its first non-blank line, which must start at column 0 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); diff --git a/test/approval-classes.test.ts b/test/approval-classes.test.ts index 84420b4d0..a1a903564 100644 --- a/test/approval-classes.test.ts +++ b/test/approval-classes.test.ts @@ -300,13 +300,13 @@ describe('review #3235 regressions: what counts as approval, stale tokens, and n 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(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 its first non-blank line, which must start at column 0 with "Approved" or "Approve"/); 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']); + expect(judge(after, COMMENT, { body: `\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: quoted, fenced, indented-code and commented approvals approve nothing; a top-level line, list marker allowed, still does', () => { + test('P1-2: only the first non-blank line counts: quoted, fenced, nested, listed, indented, commented and later-line approvals approve nothing', () => { const FENCE = '```'; const none = (body: string) => expect(judge(after, COMMENT, { body }).records, body).toEqual([]); none(`> Approved ${after.token}`); @@ -317,10 +317,18 @@ describe('review #3235 regressions: what counts as approval, stale tokens, and n none(`\nNot yet.`); none(``); none(`- > Approved ${after.token}`); + none(`Do not approve this change. This list contains an example only:\n\n- ${FENCE}text\n Approved ${after.token}\n ${FENCE}`); + none(`> Example syntax, not approval:\nApproved ${after.token}\n\nI do not approve this change.`); + none(`${FENCE}text\nexample\n${FENCE}\n\nApproved ${after.token}: preview-first.`); + none(`Context first.\n**Approved** ${after.token}: preview-first.`); + none(`Looks right to me.\n\nApproved ${after.token}`); + none(`- approve ${after.token}`); + none(`1. Approved ${after.token}`); + none(` Approved ${after.token}`); const ok = (body: string) => expect(judge(after, COMMENT, { body }).records.map(r => `${r.citation}/${r.binding}`), body).toEqual(['verified/token']); ok(`Approved ${after.token}`); - ok(`${FENCE}text\nexample\n${FENCE}\n\nApproved ${after.token}: preview-first.`); - ok(`1. Approved ${after.token}`); + ok(`**Approved** ${after.token}`); + ok(`\n\nApproved ${after.token}: preview-first.`); }); test('P1-2: only an APPROVED review counts, and it still needs an approval line naming the scope', () => {