From 94860ee4d14db992d178fabc313cb56b2d6dca51 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 7 Sep 2026 23:13:08 -0700 Subject: [PATCH 1/3] fix(iterate-pr): stop content-classifying review summaries, fix nit punctuation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit categorizeComment's content fallback had two measured defects: HIGH patterns matched a word anywhere with no regard for negation ("not a blocker" filed high), and nit[:\s]/suggestion[:\s]/etc. required a colon or space right after the word, so "Nit," missed and landed in medium. Review summaries are now bucketed structurally instead of by content: a CHANGES_REQUESTED from a non-author reviewer stays high, an explicit h:/m:/l: marker still wins, otherwise the summary is surfaced but filed low so it doesn't inflate needs_attention. Summaries narrate findings and will always contain finding vocabulary, frequently negated, so no pattern list survives being run over them — the findings themselves still arrive separately as inline threads and are classified individually. The inline-comment fallback (still used for review threads and issue comments) gets word boundaries on the bracketed-class LOW tokens (nit/suggestion/optional/minor/style) so punctuation stops mattering, plus a short negation window before HIGH/LOW pattern matches so "not a blocker" and "no security issue" stop counting as findings. detectLogaf (the explicit-marker path) is untouched and still wins over content everywhere, including in review summaries. Fixes #309 --- .agents/skills/iterate-pr/SKILL.md | 4 +- .../iterate-pr/scripts/fetch_pr_feedback.cjs | 120 ++++++++++++++-- .../scripts/fetch_pr_feedback.test.cjs | 128 +++++++++++++++++- 3 files changed, 234 insertions(+), 18 deletions(-) diff --git a/.agents/skills/iterate-pr/SKILL.md b/.agents/skills/iterate-pr/SKILL.md index aa11aaf9..d15e28b6 100644 --- a/.agents/skills/iterate-pr/SKILL.md +++ b/.agents/skills/iterate-pr/SKILL.md @@ -70,7 +70,9 @@ Review bot feedback (from Sentry, Warden, Copilot, Cursor, Bugbot, CodeQL, etc.) **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), defaulting to `low` 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, the summary is surfaced but filed `low` so it doesn't inflate `needs_attention`. 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: diff --git a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs index 7913b7fe..45037823 100755 --- a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs +++ b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs @@ -34,8 +34,24 @@ * - 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 — defaulting + * to `low` absent a marker, so it stays visible without inflating + * `needs_attention`. + * + * 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 is surfaced but filed `low` so it doesn't inflate + * `needs_attention`. 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,16 +228,21 @@ 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, @@ -229,11 +250,44 @@ const LOW_PATTERNS = [ /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. +const NEGATORS = /\b(?:not|no|non|without|isn't|is 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. + */ +const matchesUnnegated = (patterns, body) => + patterns.some((pattern) => { + const match = pattern.exec(body); + return match !== null && !isNegated(body, match.index); + }); + /** * 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 +297,31 @@ 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 is surfaced but filed as `low` rather than `medium`, so it + * doesn't inflate `needs_attention` — the caller-facing number that a + * structurally-known `CHANGES_REQUESTED` (handled by the caller, not here) + * already covers for the case that actually needs gating. + */ +const categorizeReviewSummary = (_comment, body) => detectLogaf(body) ?? "low"; + /** * 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 +335,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 +359,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); } }; @@ -474,7 +558,7 @@ const buildFeedback = (client, { owner, repo, prInfo }) => { // 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 +581,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 + ); } } @@ -671,6 +762,7 @@ module.exports = { bucketByAuthor, buildFeedback, categorizeComment, + categorizeReviewSummary, createClient, detectLogaf, extractFeedbackItem, diff --git a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs index 6bcf4489..b5dafd31 100644 --- a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs +++ b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs @@ -166,6 +166,70 @@ test("categorizeComment falls back to content, then to medium", () => { ); }); +// Reproduced directly from the issue: a HIGH pattern fires on "blocker" and +// "security issue" with no regard for the "not"/"no" right before them. +test("categorizeComment does not read a negated finding as high", () => { + const bot = { user: { login: "claude[bot]" } }; + assert.equal( + categorizeComment( + bot, + "I found no security issue and this is not a blocker." + ), + "medium" + ); + assert.equal( + categorizeComment( + { user: { login: "reviewer" } }, + "This isn't critical, just a thought." + ), + "medium" + ); +}); + +// A negator far enough away from the match must not suppress a real finding — +// the window is short on purpose. +test("categorizeComment still flags a real finding elsewhere in the same body", () => { + assert.equal( + categorizeComment( + { user: { login: "reviewer" } }, + "No comments on the docs. This will break at runtime for empty input." + ), + "high" + ); +}); + +// Reproduced directly from the issue: `nit[:\s]` required a colon or +// whitespace right after the word, so `Nit,` — a real inline comment from +// #304 — missed the pattern and landed in the auto-fixed `medium` bucket. +test("Nit, and nit: reach the same bucket regardless of punctuation", () => { + const human = { user: { login: "reviewer" } }; + assert.equal(categorizeComment(human, "Nit, could be simpler."), "low"); + assert.equal(categorizeComment(human, "nit: could be simpler."), "low"); + assert.equal(categorizeComment(human, "(nit) could be simpler."), "low"); +}); + +// The other bracketed-class tokens had the same `[:\s]` punctuation gap. +test("the other bracketed-class LOW tokens are also punctuation-insensitive", () => { + const human = { user: { login: "reviewer" } }; + assert.equal(categorizeComment(human, "Suggestion, rename this."), "low"); + assert.equal(categorizeComment(human, "Optional, but nice to have."), "low"); + assert.equal(categorizeComment(human, "Minor, just a typo."), "low"); + assert.equal(categorizeComment(human, "Style, not a big deal."), "low"); +}); + +// The explicit-marker path (detectLogaf) must keep winning over content in the +// inline-comment fallback too — this is the one thing the issue says must not +// regress, even on a body containing negated HIGH vocabulary. +test("an explicit h: marker still wins over negated and non-negated content", () => { + assert.equal( + categorizeComment( + { user: { login: "reviewer" } }, + "h: not a blocker, but do this anyway" + ), + "high" + ); +}); + test("extractFeedbackItem truncates the summary but keeps the full body", () => { const body = `${"x".repeat(250)}\nsecond line`; const item = extractFeedbackItem({ body, author: "a" }); @@ -239,7 +303,12 @@ test("a reviewer's CHANGES_REQUESTED summary is always high", () => { // You can't "Request changes" on your own PR, so a self-review arrives as // COMMENTED summaries and ordinary threads. Dropping them would silently lose // the author's own notes to themselves, which is most of what a self-review is. -test("a self-review summary is surfaced, flagged, and bucketed by content", () => { +// +// A review SUMMARY is bucketed structurally, not by content (see +// categorizeReviewSummary): absent a marker it defaults to `low` so it stays +// visible without inflating `needs_attention`, which is a deliberate change +// from the old content-based default of `medium`. +test("a self-review summary is surfaced, flagged, and bucketed structurally", () => { const output = build(fakeClient(), { reviews: [ { @@ -249,9 +318,9 @@ test("a self-review summary is surfaced, flagged, and bucketed by content", () = }, ], }); - assert.equal(output.summary.medium, 1); + assert.equal(output.summary.low, 1); assert.equal(output.summary.self_review_feedback, 1); - assert.equal(output.feedback.medium[0].self_review, true); + assert.equal(output.feedback.low[0].self_review, true); }); test("a self-review marked CHANGES_REQUESTED is not force-promoted to high", () => { @@ -268,6 +337,59 @@ test("a self-review marked CHANGES_REQUESTED is not force-promoted to high", () assert.equal(output.summary.low, 1); }); +// An explicit marker in a review summary still wins, same as everywhere else +// — it is not "content" in the sense the fallback patterns are, it's an +// unambiguous author-supplied signal. +test("an explicit marker in a review summary still wins over the structural default", () => { + const output = build(fakeClient(), { + reviews: [ + { + author: { login: "reviewer" }, + state: "COMMENTED", + body: "h: this needs another look before merge", + }, + ], + }); + assert.equal(output.summary.high, 1); + assert.equal(output.summary.needs_attention, 1); +}); + +// The bug this fix exists for: a clean review's own closing sentence narrates +// what it did NOT find, and that prose must not read as a finding. Measured on +// the real review summary of #307. +test("a clean review summary saying 'not a blocker' is not high", () => { + const output = build(fakeClient(), { + reviews: [ + { + author: { login: "reviewer" }, + state: "COMMENTED", + body: 'I found no security issue and this is not a blocker — worth a look, but not a blocker.', + }, + ], + }); + assert.equal(output.summary.high, 0); + assert.equal(output.summary.needs_attention, 0); + assert.equal(output.summary.low, 1); +}); + +// A review-bot's own summary (e.g. Claude's finished review) is subject to +// the same structural bucketing as a human's — the negation-blindness bug was +// found on exactly this path. +test("a review bot's summary is bucketed structurally, not by content", () => { + const output = build(fakeClient(), { + reviews: [ + { + author: { login: "claude[bot]" }, + state: "COMMENTED", + body: "This is a clean pass — nothing here will break, not a blocker.", + }, + ], + }); + assert.equal(output.summary.high, 0); + assert.equal(output.summary.review_bot_feedback, 1); + assert.equal(output.feedback.low[0].review_bot, true); +}); + test("empty and near-empty review summaries are skipped", () => { const output = build(fakeClient(), { reviews: [ From 701ed67d08b34581bde9bbd734d54394fc5f5f13 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 7 Sep 2026 23:16:33 -0700 Subject: [PATCH 2/3] fix(iterate-pr): give review summaries their own bucket, not `low` Filing a marker-less summary as `low` fixed the false `high` and introduced a false prompt. `low` means "an optional suggestion, ask the user which to address": step 3 presents low items as a numbered list and `action_required` says so. Measured before this change, a review whose own conclusion was "I did not find any correctness bugs" produced: action_required: "Review low-priority suggestions - ask user which to address" which asks someone to triage a summary that proposes no work. Summaries now go to a `review_summary` bucket, counted as `summary.review_summaries` and reported in `action_required` as nothing at all. Surfaced and countable without being actionable, the same shape `review_in_progress` already uses, and the same reason: some things a caller needs to see are not things a caller needs to act on. `review_summary` is included in the `review_bot_feedback` and `self_review_feedback` tallies even though it is not a priority bucket. Those answer "where did this come from", not "how urgent is it", and leaving it out silently zeroed `self_review_feedback` for an author whose only note is a summary, which is the ordinary shape of a self-review. An explicit `h:`/`m:`/`l:` marker still wins, in a summary as anywhere else, and a reviewer's CHANGES_REQUESTED is still `high` on the review state. --- .agents/skills/iterate-pr/SKILL.md | 7 +- .../iterate-pr/scripts/fetch_pr_feedback.cjs | 32 +++++++-- .../scripts/fetch_pr_feedback.test.cjs | 65 +++++++++++++++++-- 3 files changed, 89 insertions(+), 15 deletions(-) diff --git a/.agents/skills/iterate-pr/SKILL.md b/.agents/skills/iterate-pr/SKILL.md index d15e28b6..a20ca702 100644 --- a/.agents/skills/iterate-pr/SKILL.md +++ b/.agents/skills/iterate-pr/SKILL.md @@ -65,14 +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). 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), defaulting to `low` absent a marker. 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, the summary is surfaced but filed `low` so it doesn't inflate `needs_attention`. The findings a review raises still arrive separately as inline review-thread comments, which _are_ classified by content as usual. +**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: diff --git a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs index 45037823..2fe7c8e7 100755 --- a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs +++ b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs @@ -314,13 +314,23 @@ const categorizeComment = (comment, body) => { * 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 is surfaced but filed as `low` rather than `medium`, so it - * doesn't inflate `needs_attention` — the caller-facing number that a - * structurally-known `CHANGES_REQUESTED` (handled by the caller, not here) - * already covers for the case that actually needs gating. + * 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) ?? "low"; +const categorizeReviewSummary = (_comment, body) => + detectLogaf(body) ?? "review_summary"; /** * File one item by its author: an unfinished review is filed on its own ahead @@ -552,6 +562,7 @@ const buildFeedback = (client, { owner, repo, prInfo }) => { bot: [], resolved: [], review_in_progress: [], + review_summary: [], }; // Review summary bodies. Every non-empty summary is surfaced, regardless of @@ -664,7 +675,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, @@ -688,6 +705,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, }, diff --git a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs index b5dafd31..f6eb0d2a 100644 --- a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs +++ b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs @@ -318,9 +318,9 @@ test("a self-review summary is surfaced, flagged, and bucketed structurally", () }, ], }); - assert.equal(output.summary.low, 1); + assert.equal(output.summary.review_summaries, 1); assert.equal(output.summary.self_review_feedback, 1); - assert.equal(output.feedback.low[0].self_review, true); + assert.equal(output.feedback.review_summary[0].self_review, true); }); test("a self-review marked CHANGES_REQUESTED is not force-promoted to high", () => { @@ -334,7 +334,7 @@ test("a self-review marked CHANGES_REQUESTED is not force-promoted to high", () ], }); assert.equal(output.summary.high, 0); - assert.equal(output.summary.low, 1); + assert.equal(output.summary.review_summaries, 1); }); // An explicit marker in a review summary still wins, same as everywhere else @@ -363,13 +363,13 @@ test("a clean review summary saying 'not a blocker' is not high", () => { { author: { login: "reviewer" }, state: "COMMENTED", - body: 'I found no security issue and this is not a blocker — worth a look, but not a blocker.', + body: "I found no security issue and this is not a blocker — worth a look, but not a blocker.", }, ], }); assert.equal(output.summary.high, 0); assert.equal(output.summary.needs_attention, 0); - assert.equal(output.summary.low, 1); + assert.equal(output.summary.review_summaries, 1); }); // A review-bot's own summary (e.g. Claude's finished review) is subject to @@ -387,7 +387,7 @@ test("a review bot's summary is bucketed structurally, not by content", () => { }); assert.equal(output.summary.high, 0); assert.equal(output.summary.review_bot_feedback, 1); - assert.equal(output.feedback.low[0].review_bot, true); + assert.equal(output.feedback.review_summary[0].review_bot, true); }); test("empty and near-empty review summaries are skipped", () => { @@ -961,3 +961,56 @@ test("bold-italic emphasis is matched, and a bare underscore is not emphasis", ( false ); }); + +// A REVIEW THAT FOUND NOTHING MUST NOT ASK THE USER TO TRIAGE IT. `low` means +// "an optional suggestion, ask which to address" — the skill presents low items +// as a numbered list and `action_required` says so. Filing a clean summary +// there trades the false `high` this work removed for a false prompt. +test("a clean review summary is surfaced without becoming an action", () => { + const output = build(fakeClient(), { + reviews: [ + { + author: { login: "claude[bot]" }, + state: "COMMENTED", + body: "I did not find any correctness bugs. Not a blocker.", + }, + ], + }); + assert.equal(output.summary.review_summaries, 1, "it is still surfaced"); + assert.equal(output.summary.high, 0); + assert.equal(output.summary.low, 0, "not filed as a suggestion"); + assert.equal(output.summary.needs_attention, 0); + assert.equal(output.action_required, null, "nothing to ask the user about"); +}); + +// The tallies answer "where did this come from", not "how urgent is it", so an +// author whose only note is a summary must still register as self-review. +test("a self-review summary still counts as self-review feedback", () => { + const output = build(fakeClient(), { + reviews: [ + { + author: { login: "me" }, + state: "COMMENTED", + body: "Notes to self on the approach.", + }, + ], + }); + assert.equal(output.summary.self_review_feedback, 1); + assert.equal(output.summary.review_summaries, 1); +}); + +// An explicit marker is a deliberate signal from a person and still outranks +// the structural default, in a summary as anywhere else. +test("an explicit marker in a summary still reaches its priority bucket", () => { + const output = build(fakeClient(), { + reviews: [ + { + author: { login: "reviewer" }, + state: "COMMENTED", + body: "h: the lease SHA is stale", + }, + ], + }); + assert.equal(output.summary.high, 1); + assert.equal(output.summary.review_summaries, 0); +}); From 2ffb3d1a3bd5f8553b77fed3accce4334f01c4ad Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 8 Sep 2026 09:39:39 -0700 Subject: [PATCH 3/3] fix(iterate-pr): widen NEGATORS, scan every match, fix stale docblock Addresses PR #311 review feedback (3 distinct findings, each posted by two review runs, 6 threads total): - `NEGATORS` only covered `not|no|non|without|isn't|is not`, so contractions and bare negative words ("won't", "doesn't", "can't", "cannot", "never", "nothing", etc.) still read as unnegated and reached `high`. Widened the alternation to cover them explicitly, since `\b` doesn't split on an apostrophe the way it splits on a space. - `matchesUnnegated` used a non-global `pattern.exec`, which always returns the left-most match. A negated first occurrence of a HIGH/LOW word was read as covering the whole body, silently dropping a later, genuine occurrence of the same word ("not a blocker overall, but there's a real blocker in the retry logic"). Now clones each pattern with a global flag and checks every occurrence. - The module's top-of-file docblock still said a review summary "defaults to low absent a marker" after a prior commit moved that default to its own `review_summary` bucket. Updated the Categories list and the self-review/review-summary sections to match SKILL.md and the categorizeReviewSummary JSDoc, which were already correct. Also caught in the same review round: SKILL.md's step-3 LOGAF-priority list never mentioned the review_summary bucket at all, so an agent following it literally had no cue to ever process one. Added a "surface, never a fix and never a prompt" entry, and listed review_summary under "when to reply." Each code fix is covered by a new test and was mutation-checked: broke it, confirmed the new test failed, restored it, confirmed the suite passed again. --- .agents/skills/iterate-pr/SKILL.md | 5 ++ .../iterate-pr/scripts/fetch_pr_feedback.cjs | 56 ++++++++++++++--- .../scripts/fetch_pr_feedback.test.cjs | 62 +++++++++++++++++++ 3 files changed, 113 insertions(+), 10 deletions(-) diff --git a/.agents/skills/iterate-pr/SKILL.md b/.agents/skills/iterate-pr/SKILL.md index a20ca702..9ca381f3 100644 --- a/.agents/skills/iterate-pr/SKILL.md +++ b/.agents/skills/iterate-pr/SKILL.md @@ -194,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 @@ -212,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`): diff --git a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs index 2fe7c8e7..321b8855 100755 --- a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs +++ b/.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 @@ -37,9 +41,9 @@ * 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 — defaulting - * to `low` absent a marker, so it stays visible without inflating - * `needs_attention`. + * 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 @@ -48,10 +52,14 @@ * 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 is surfaced but filed `low` so it doesn't inflate - * `needs_attention`. 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. + * 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 @@ -255,7 +263,16 @@ const LOW_PATTERNS = [ // 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. -const NEGATORS = /\b(?:not|no|non|without|isn't|is not)\b/i; +// +// 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 @@ -271,11 +288,30 @@ const isNegated = (body, 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) => { - const match = pattern.exec(body); - return match !== null && !isNegated(body, match.index); + // 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; }); /** diff --git a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs index f6eb0d2a..91a2a946 100644 --- a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs +++ b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs @@ -198,6 +198,68 @@ test("categorizeComment still flags a real finding elsewhere in the same body", ); }); +// Found in review on #311: the first cut of NEGATORS only covered +// `not`/`no`/`non`/`without`/`isn't`/`is not`, so every contraction below +// still read as unnegated and reached `high`. +test("NEGATORS recognizes contractions and bare negative words, not just 'not'/'no'", () => { + const human = { user: { login: "reviewer" } }; + assert.equal( + categorizeComment(human, "Nothing critical here, just a heads up."), + "medium", + "nothing" + ); + assert.equal( + categorizeComment(human, "This won't be a blocker."), + "medium", + "won't" + ); + assert.equal( + categorizeComment(human, "This doesn't block anything."), + "medium", + "doesn't" + ); + assert.equal( + categorizeComment(human, "This can't fail."), + "medium", + "can't" + ); + assert.equal( + categorizeComment(human, "This cannot break the build."), + "medium", + "cannot" + ); + assert.equal( + categorizeComment(human, "There was never a blocker here."), + "medium", + "never" + ); + assert.equal( + categorizeComment(human, "This wasn't critical to begin with."), + "medium", + "wasn't" + ); + assert.equal( + categorizeComment(human, "It didn't break anything in testing."), + "medium", + "didn't" + ); +}); + +// Found in review on #311: `matchesUnnegated` used a non-global `.exec`, +// which always returns the left-most match. A negated first mention of a +// word was read as covering the whole body, silently dropping a real, +// later occurrence of the same finding — the opposite direction from the +// bug this file exists to fix. +test("a later, genuine occurrence of a HIGH word is still caught after an earlier negated one", () => { + assert.equal( + categorizeComment( + { user: { login: "reviewer" } }, + "It's not a blocker overall, but there's a real blocker in the retry logic that needs fixing." + ), + "high" + ); +}); + // Reproduced directly from the issue: `nit[:\s]` required a colon or // whitespace right after the word, so `Nit,` — a real inline comment from // #304 — missed the pattern and landed in the auto-fixed `medium` bucket.