diff --git a/.agents/skills/iterate-pr/SKILL.md b/.agents/skills/iterate-pr/SKILL.md index aa11aaf9..9ca381f3 100644 --- a/.agents/skills/iterate-pr/SKILL.md +++ b/.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`): diff --git a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs index 7913b7fe..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 @@ -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,16 +236,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 +258,72 @@ 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. +// +// 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, 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..91a2a946 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,132 @@ 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" + ); +}); + +// 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. +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 +365,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 +380,9 @@ test("a self-review summary is surfaced, flagged, and bucketed by content", () = }, ], }); - assert.equal(output.summary.medium, 1); + assert.equal(output.summary.review_summaries, 1); assert.equal(output.summary.self_review_feedback, 1); - assert.equal(output.feedback.medium[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", () => { @@ -265,7 +396,60 @@ 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 +// — 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.review_summaries, 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.review_summary[0].review_bot, true); }); test("empty and near-empty review summaries are skipped", () => { @@ -839,3 +1023,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); +});