fix(iterate-pr): stop content-classifying review summaries, fix nit punctuation

.agents/skills/iterate-pr/SKILL.md

@@ -65,12 +65,17 @@ Returns JSON with feedback categorized as:
 - `bot` - Informational automated comments (Codecov, Dependabot, etc.)
 - `resolved` - Already resolved threads, and top-level comments carrying our own 🎉 acknowledgement
 - `review_in_progress` - A review bot's placeholder comment, posted the instant it was triggered and not yet edited to its finished form. Not feedback yet — see below.
+- `review_summary` - A review's top-level narration, bucketed structurally rather than by content. Surfaced and counted (`summary.review_summaries`), never prompted on — see below.
 
 Review bot feedback (from Sentry, Warden, Copilot, Cursor, Bugbot, CodeQL, etc.) appears in `high`/`medium`/`low` with `review_bot: true` — it is NOT placed in the `bot` bucket.
 
 **A review bot's comment existing is not a completion signal, and neither is its `created_at` or a green check run.** The Claude review bot posts a comment the instant it is triggered and edits that same comment in place as it works — a run has been observed to report `success` while the body still read "Review in progress" with unchecked boxes. `fetch_pr_feedback.cjs` detects this by checking whether the body still **opens** with the in-progress marker (a review that happens to discuss this behaviour mid-body is not mistaken for an unfinished one) and reports the count as `summary.review_in_progress` instead of bucketing the placeholder as ordinary feedback. **Before treating `needs_attention: 0` as "reviewed, nothing found," check `summary.review_in_progress` — a nonzero count means the review hasn't finished, not that it found nothing.** When one or more are present, `action_required` says so and outranks every other bucket, since a caller must not stop just because the buckets it can already see look clean.
 
-**Self-review feedback** (from the PR author) appears in `high`/`medium`/`low` with `self_review: true`. Because you can't "Request changes" on your own PR, a self-review lands as `COMMENTED` review summaries and ordinary review threads rather than changes-requested items — these are surfaced (not dropped) and bucketed by content, defaulting to `medium` when there's no `h:/m:/l:` prefix. Treat `self_review` items the same as any other human feedback in step 3.
+**Self-review feedback** (from the PR author) appears in `high`/`medium`/`low` with `self_review: true`. Because you can't "Request changes" on your own PR, a self-review lands as `COMMENTED` review summaries and ordinary review threads rather than changes-requested items — these are surfaced (not dropped). A self-review **thread** (an inline comment) is bucketed by content like any other, defaulting to `medium` absent a `h:/m:/l:` prefix. A self-review **summary** is bucketed structurally, not by content (see below), landing in `review_summary` absent a marker. Treat `self_review` items the same as any other human feedback in step 3.
+
+**A review's top-level summary is bucketed structurally, not by its content.** A summary narrates a review's findings, so it always contains finding vocabulary — often negated ("not a blocker", "no security issue found") — and no content-pattern list survives that. A `CHANGES_REQUESTED` summary from a reviewer who isn't the PR author is always `high`; an explicit `h:/m:/l:` marker still wins over everything; absent both, it goes to its own `review_summary` bucket, counted as `summary.review_summaries`.
+
+**That bucket is deliberately not `low`.** `low` means "an optional suggestion, ask the user which to address", and step 3 presents low items as a numbered list for exactly that. A review that found nothing proposes no work, so filing it there trades a false `high` for a false prompt and asks someone to triage a summary with nothing in it. `review_summary` is surfaced and counted without being actionable, the same shape `review_in_progress` uses. Do not prompt on these, and do not treat one as a finding. The findings a review raises still arrive separately as inline review-thread comments, which _are_ classified by content as usual.
 
 Each feedback item may also include:
 
@@ -189,6 +194,10 @@ Found 3 low-priority suggestions:
 Which would you like to address? (e.g., "1,3" or "all" or "none")
 ```
 
+**Surface, never a fix and never a prompt:**
+
+- `review_summary` - a review's own narration, bucketed structurally rather than by content (see step 2's category list). It is not a finding to fix and not a suggestion to triage — reply to it per "Replying to Comments" below like any other surfaced item, but do not present it in the low-priority numbered list and do not count it toward `needs_attention`.
+
 **Skip silently:**
 
 - `resolved` threads
@@ -207,6 +216,7 @@ After processing a feedback item, acknowledge it on the PR so the trail shows wh
 - `high` and `medium` items — whether fixed or determined to be false positives
 - `low` items — whether fixed or declined by the user
 - `self_review` items — the same as any other human feedback
+- `review_summary` items — a brief acknowledgment; there is nothing to fix, but the trail should still show it was read
 
 **Inline review-thread comments** (items with a `thread_id`):
 

.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs

@@ -22,6 +22,10 @@
  * - review_in_progress: A review bot's placeholder comment, posted the instant
  *   it was triggered and not yet edited to its finished form — not feedback
  *   yet, see "Unfinished reviews" below
+ * - review_summary: A review's top-level narration, bucketed structurally
+ *   rather than by content — surfaced and counted
+ *   (`summary.review_summaries`), but never a priority bucket and never
+ *   prompted on, see "Review summaries" below
  *
  * Bot classification:
  * - Review bots (Sentry, Warden, Cursor, Bugbot, etc.) provide actionable code
@@ -34,8 +38,28 @@
  * - You can't formally "Request changes" on your own PR, so a PR author's own
  *   feedback arrives as `COMMENTED` review summaries and ordinary review
  *   **threads**, not as changes-requested items. These are surfaced (not
- *   dropped) and flagged `self_review: true`, bucketed by content — defaulting
- *   to `medium` when no `h:/m:/l:` prefix is present.
+ *   dropped) and flagged `self_review: true`. A self-review **thread** (an
+ *   inline comment) is bucketed by content like any other, defaulting to
+ *   `medium` absent a `h:/m:/l:` prefix. A self-review **summary** is bucketed
+ *   structurally, not by content — see "Review summaries" below — landing in
+ *   the `review_summary` bucket absent a marker, so it stays visible without
+ *   ever being read as a priority-bucket finding.
+ *
+ * Review summaries:
+ * - A review's top-level summary narrates its findings, so classifying it by
+ *   content is unreliable: the prose is full of finding vocabulary, often
+ *   negated ("not a blocker", "no security issue"), and no pattern list
+ *   survives that. A summary is instead bucketed by what is structurally
+ *   known: `CHANGES_REQUESTED` from a reviewer who is not the PR author is
+ *   `high`; an explicit `h:/m:/l:` marker still wins over everything; absent
+ *   both, the summary goes to its own `review_summary` bucket (counted as
+ *   `summary.review_summaries`), NOT `low` — `low` means "an optional
+ *   suggestion, ask the user which to address," and a review that found
+ *   nothing proposes no work, so filing it there trades the false `high` this
+ *   fix removes for a false prompt. The findings a review raises arrive
+ *   separately as inline review-thread comments, which ARE classified by
+ *   content — nothing is lost by not re-classifying the narration around
+ *   them.
  *
  * Unfinished reviews:
  * - The Claude review bot posts its comment immediately when triggered and
@@ -212,28 +236,94 @@ const HIGH_PATTERNS = [
   /blocker/i,
 ];
 
+// Word-bounded so a leading word is matched regardless of what punctuation (or
+// none at all) follows it. These five used to require a colon or whitespace
+// right after the word (`nit[:\s]`), which matched `nit:` and `nit ` but not
+// the common `Nit,` spelling — measured on a real inline comment from #304
+// that opened `Nit, not a defect:` and was filed medium instead of low.
 const LOW_PATTERNS = [
-  /nit[:\s]/i,
+  /\bnit\b/i,
   /nitpick/i,
-  /suggestion[:\s]/i,
+  /\bsuggestion\b/i,
   /consider\s+/i,
   /could\s+(also\s+)?/i,
   /might\s+(want\s+to|be\s+better)/i,
-  /optional[:\s]/i,
-  /minor[:\s]/i,
-  /style[:\s]/i,
+  /\boptional\b/i,
+  /\bminor\b/i,
+  /\bstyle\b/i,
   /prefer\s+/i,
   /what\s+do\s+you\s+think/i,
   /up\s+to\s+you/i,
   /take\s+it\s+or\s+leave/i,
   /fwiw/i,
 ];
 
+// A HIGH/LOW pattern match is discarded when one of these words appears
+// shortly before it, so "not a blocker" and "no security issue" stop reading
+// as findings. Measured on the real review summary of #307: "…so it's a
+// 'worth a look,' not a blocker" matched `blocker` with nothing to say the
+// word was negated.
+//
+// Contractions are listed explicitly rather than derived from their expanded
+// form (`won't` alongside `will not`) because `\b` does not split on an
+// apostrophe the way it splits on a space — `won't` has to appear as its own
+// alternative or it is invisible to this pattern. Caught in review on #311:
+// the first cut only had `isn't`/`is not`, so "doesn't block", "won't break",
+// "can't fail", "never a blocker", and a bare "nothing" all still read as
+// unnegated and reached `high`.
+const NEGATORS =
+  /\b(?:not|no|non|none|nothing|never|without|isn't|is not|aren't|are not|wasn't|was not|weren't|were not|won't|will not|wouldn't|would not|can't|cannot|can not|couldn't|could not|shouldn't|should not|doesn't|does not|didn't|did not|hasn't|has not|haven't|have not|hadn't|had not)\b/i;
+
+// How far back from a match to look for a negator. Wide enough to cover "is
+// not a blocker" and "found no security issue", narrow enough that an
+// unrelated negation earlier in a long sentence doesn't suppress a real
+// finding.
+const NEGATION_WINDOW = 24;
+
+/** Whether a negator appears in the window immediately before `index`. */
+const isNegated = (body, index) => {
+  const start = Math.max(0, index - NEGATION_WINDOW);
+  return NEGATORS.test(body.slice(start, index));
+};
+
+/**
+ * Whether any pattern matches `body` at a position not preceded by a negator.
+ *
+ * Scans every occurrence of a pattern, not just the first: `pattern.exec`
+ * always returns the left-most match, so a naive single check would read a
+ * negated first mention as covering the whole body and miss a later, genuine
+ * one — e.g. "not a blocker overall, but there's a real blocker in the retry
+ * logic" has two matches of `/blocker/i`, only the first of which is negated.
+ * Caught in review on #311.
+ */
+const matchesUnnegated = (patterns, body) =>
+  patterns.some((pattern) => {
+    // Clone with a `g` flag so `.exec` advances instead of always returning
+    // the left-most match; the source patterns stay non-global everywhere
+    // else they're used (a global regex carries mutable `lastIndex` state,
+    // which is exactly the kind of shared mutable state worth not spreading).
+    const global = new RegExp(pattern.source, `${pattern.flags}g`);
+    let match;
+    while ((match = global.exec(body)) !== null) {
+      if (!isNegated(body, match.index)) return true;
+      // A zero-length match would otherwise leave `lastIndex` unchanged and
+      // loop forever; none of the patterns here can match empty, but this
+      // keeps the loop safe if one ever does.
+      if (match[0].length === 0) global.lastIndex += 1;
+    }
+    return false;
+  });
+
 /**
  * Categorize a comment by content and author, on the LOGAF scale.
  *
  * Info bots are skipped silently; review bots fall through to content
  * categorization so their actionable feedback is not lost.
+ *
+ * This is for INLINE feedback (review threads, issue comments) only — a
+ * single comment that either raises one finding or doesn't. A review
+ * SUMMARY narrates a whole review and is handled separately by
+ * `categorizeReviewSummary`, below, for the reason explained there.
  */
 const categorizeComment = (comment, body) => {
   const author = comment?.author?.login || comment?.user?.login || "";
@@ -243,13 +333,41 @@ const categorizeComment = (comment, body) => {
   const logaf = detectLogaf(body);
   if (logaf) return logaf;
 
-  if (HIGH_PATTERNS.some((pattern) => pattern.test(body))) return "high";
-  if (LOW_PATTERNS.some((pattern) => pattern.test(body))) return "low";
+  if (matchesUnnegated(HIGH_PATTERNS, body)) return "high";
+  if (matchesUnnegated(LOW_PATTERNS, body)) return "low";
 
   // Default to medium for non-bot comments without clear indicators.
   return "medium";
 };
 
+/**
+ * Categorize a review SUMMARY structurally, never by content.
+ *
+ * A summary's job is to narrate a review's findings, so it necessarily
+ * contains finding vocabulary — often negated, as in "not a blocker" or "no
+ * security issue found" — and no content pattern list survives that. The
+ * findings themselves arrive separately as inline review-thread comments and
+ * are classified individually by `categorizeComment`; nothing is lost by not
+ * re-classifying the prose that narrates them.
+ *
+ * An explicit LOGAF marker still wins, same as everywhere else. Absent one the
+ * summary goes to its own `review_summary` bucket, which is surfaced and
+ * counted but is not a priority bucket.
+ *
+ * NOT `low`, which was the obvious choice and is wrong here. `low` means "an
+ * optional suggestion the user should be asked about": the skill presents low
+ * items as a numbered list and asks which to address, and `action_required`
+ * says so. Filing a review that found NOTHING there trades a false `high` for
+ * a false prompt, asking someone to triage a summary that proposes no work.
+ * Its own bucket is surfaced without being actionable, the same shape
+ * `review_in_progress` already uses.
+ *
+ * A `CHANGES_REQUESTED` summary never reaches here; the caller files it `high`
+ * on the review state, which is the structurally-known case that needs gating.
+ */
+const categorizeReviewSummary = (_comment, body) =>
+  detectLogaf(body) ?? "review_summary";
+
 /**
  * File one item by its author: an unfinished review is filed on its own ahead
  * of every other rule, a review bot is flagged and bucketed by content, an
@@ -263,8 +381,20 @@ const categorizeComment = (comment, body) => {
  * unfinished placeholder must never be read as a review bot's finding (`high`),
  * an info bot's noise (`bot`), or ordinary human feedback, from ANY of the
  * three sources.
+ *
+ * `categorize` defaults to the inline-comment classifier; the review-summary
+ * call site passes `categorizeReviewSummary` instead, so the bot/info-bot
+ * split and the in-progress check stay one rule in one place while what
+ * happens to the *content* differs by source.
  */
-const bucketByAuthor = (feedback, item, comment, body, author) => {
+const bucketByAuthor = (
+  feedback,
+  item,
+  comment,
+  body,
+  author,
+  categorize = categorizeComment
+) => {
   // The author gate is load-bearing, not belt and braces. A human writing
   // "Review in progress on my end, back by EOD" would otherwise be filed as an
   // unfinished review, vanish from `needs_attention`, and hang the wait loop
@@ -275,11 +405,11 @@ const bucketByAuthor = (feedback, item, comment, body, author) => {
     feedback.review_in_progress.push(item);
   } else if (isReviewBot(author)) {
     item.review_bot = true;
-    feedback[categorizeComment(comment, body)].push(item);
+    feedback[categorize(comment, body)].push(item);
   } else if (isInfoBot(author)) {
     feedback.bot.push(item);
   } else {
-    feedback[categorizeComment(comment, body)].push(item);
+    feedback[categorize(comment, body)].push(item);
   }
 };
 
@@ -468,13 +598,14 @@ const buildFeedback = (client, { owner, repo, prInfo }) => {
     bot: [],
     resolved: [],
     review_in_progress: [],
+    review_summary: [],
   };
 
   // Review summary bodies. Every non-empty summary is surfaced, regardless of
   // author: a self-review can't be "Request changes", so the PR author's own
   // feedback arrives as COMMENTED summaries and would otherwise be dropped. A
   // real reviewer's CHANGES_REQUESTED is always high; everything else is
-  // bucketed by content (default medium).
+  // bucketed structurally, not by content — see `categorizeReviewSummary`.
   for (const review of prInfo.reviews ?? []) {
     const author = review.author?.login ?? "";
     const body = review.body ?? "";
@@ -497,7 +628,14 @@ const buildFeedback = (client, { owner, repo, prInfo }) => {
     ) {
       feedback.high.push(item);
     } else {
-      bucketByAuthor(feedback, item, review, body, author);
+      bucketByAuthor(
+        feedback,
+        item,
+        review,
+        body,
+        author,
+        categorizeReviewSummary
+      );
     }
   }
 
@@ -573,7 +711,13 @@ const buildFeedback = (client, { owner, repo, prInfo }) => {
   }
 
   const requestedReviewers = client.requestedReviewers(owner, repo, prNumber);
-  const priorities = ["high", "medium", "low"];
+  // `review_summary` counts here even though it is not a priority bucket:
+  // these tallies answer "how much of this came from a bot / from the author",
+  // which is independent of urgency. Leaving it out would silently zero
+  // `self_review_feedback` for an author whose only note is a review summary,
+  // which is the common shape of a self-review. `review_in_progress` stays out:
+  // it is not feedback yet.
+  const priorities = ["high", "medium", "low", "review_summary"];
   const countFlagged = (flag) =>
     priorities.reduce(
       (total, bucket) => total + feedback[bucket].filter((i) => i[flag]).length,
@@ -597,6 +741,7 @@ const buildFeedback = (client, { owner, repo, prInfo }) => {
       review_bot_feedback: countFlagged("review_bot"),
       self_review_feedback: countFlagged("self_review"),
       review_in_progress: feedback.review_in_progress.length,
+      review_summaries: feedback.review_summary.length,
       needs_attention: feedback.high.length + feedback.medium.length,
       pending_reviewers: requestedReviewers.length,
     },
@@ -671,6 +816,7 @@ module.exports = {
   bucketByAuthor,
   buildFeedback,
   categorizeComment,
+  categorizeReviewSummary,
   createClient,
   detectLogaf,
   extractFeedbackItem,