fix(approval-classes): only top-level approval lines count; quoted, fenced, indented-code and commented text never does (review #3235 r2)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

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.

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 <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;
+/** 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(/<!--[\s\S]*?(?:-->|$)/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 {

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(`<!-- Approved ${after.token} -->\nNot yet.`);
+    none(`<!--\nApproved ${after.token}\n-->`);
+    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}` });