diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 0f3152ff0..a6c2ee00f 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -420,11 +420,15 @@ 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 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. + 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 + `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`. - 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 25ba0b2b7..00fb639e7 100644 --- a/lib/approval-citations.ts +++ b/lib/approval-citations.ts @@ -9,9 +9,10 @@ * 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 its approval lines - * (APPROVAL_LINE: a line starting with "Approve" or "Approved") count; a - * review must also be in state APPROVED; + * 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; * - anything else, or a ref that cannot be read here, is `unverified`. * * Resolution alone is not enough: the evidence (the approved plan text or @@ -96,8 +97,22 @@ 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; +/** 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; +} const GH_URL = /^https:\/\/github\.com\/([\w.-]+)\/([\w.-]+)\/(?:pull|issues)\/(\d+)#(?:issuecomment-(\d+)|pullrequestreview-(\d+)|discussion_r(\d+))$/; @@ -118,8 +133,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` }; - 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 }; + return { state: 'resolved', evidence: `${review ? 'review' : 'comment'} by ${login}`, text: approvalLines(String(body.body ?? '')).join('\n'), approvalLines: true }; } export function resolveCitation(c: Citation, ctx: CitationContext): Resolution { diff --git a/test/approval-classes.test.ts b/test/approval-classes.test.ts index 8f782a886..84420b4d0 100644 --- a/test/approval-classes.test.ts +++ b/test/approval-classes.test.ts @@ -302,10 +302,27 @@ describe('review #3235 regressions: what counts as approval, stale tokens, and n 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: `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: quoted, fenced, indented-code and commented approvals approve nothing; a top-level line, list marker allowed, still does', () => { + const FENCE = '```'; + const none = (body: string) => expect(judge(after, COMMENT, { body }).records, body).toEqual([]); + none(`> Approved ${after.token}`); + none(`> Approved ${after.token}\n\nI do not approve this change. That quoted approval is only the text you asked me to post.`); + none(`Do not approve this change. This is only an example of the requested syntax:\n\n${FENCE}text\nApproved ${after.token}\n${FENCE}`); + none(`~~~~\nApproved ${after.token}\n~~~\nstill inside the fence\n~~~~`); + none(`Example:\n\n Approved ${after.token}`); + none(`\nNot yet.`); + none(``); + 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}`); + }); + 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}` });