fix(iterate-pr): stop content-classifying review summaries, fix nit punctuation - #311
fix(iterate-pr): stop content-classifying review summaries, fix nit punctuation#311thecodedrift wants to merge 2 commits into
Conversation
…unctuation
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
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.
|
@claude /review |
|
Claude finished @thecodedrift's task in 5m 22s —— View job Review completeReview mode: incremental — read 0 prior review thread(s) before reviewing.
FindingsPosted as inline comments (all
Nothing else surfaced past verification — the negation-window sizing (24 chars), the word-boundary fix for |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // 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; |
There was a problem hiding this comment.
[New] NEGATORS is missing common negation contractions, so this fix still misses the exact class of bug it's meant to close.
const NEGATORS = /\b(?:not|no|non|without|isn't|is not)\b/i;won't, doesn't, wasn't, wouldn't, can't, shouldn't, aren't, never, and nothing are all absent. Concretely, "Nothing critical here, just a heads up." and "This won't be a blocker." both still hit HIGH_PATTERNS (critical, blocker) unnegated, because "nothing" and "won't" aren't recognized as negators, and would still be filed high.
Worth extending the alternation (e.g. adding won't|doesn't|wasn't|wouldn't|can't|shouldn't|aren't|never|nothing) rather than shipping a negator list that only covers the two phrasings used in the #307 repro.
| /** | ||
| * Whether any pattern matches `body` at a position not preceded by a negator. | ||
| */ | ||
| const matchesUnnegated = (patterns, body) => |
There was a problem hiding this comment.
[New] matchesUnnegated only ever inspects the first occurrence of each pattern in the body.
const matchesUnnegated = (patterns, body) =>
patterns.some((pattern) => {
const match = pattern.exec(body);
return match !== null && !isNegated(body, match.index);
});pattern.exec(body) on a non-global regex always returns the left-most match. If that first occurrence happens to be negated but a second, real occurrence of the same word exists later in the same body, the pattern is skipped entirely — the later occurrence is never checked.
Example: "It's not a blocker overall, but there's a real blocker in the retry logic that needs fixing." — the first blocker (negated) causes .some to treat the /blocker/i pattern as fully negated, so the second, genuine blocker a few words later is never classified as high. This drops a real finding silently, which is the opposite direction from the bug this PR fixes. Consider matching with a global flag and checking whether every match is negated, or re-searching from match.index + match[0].length when the first hit is negated.
| * 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 |
There was a problem hiding this comment.
[New] This module-level doc comment is now stale and contradicts the actual implementation.
Line 41 says a self-review summary defaults "to low absent a marker", and line 51 says a review summary "is surfaced but filed low". But categorizeReviewSummary (added in this same PR) returns detectLogaf(body) ?? "review_summary" — the actual default bucket is review_summary, not low. That bucket rename happened in the second commit ("give review summaries their own bucket, not low"), which updated SKILL.md and the categorizeReviewSummary JSDoc correctly but missed this top-of-file block.
Also, the "Categories" list near the top of the file (lines ~15-24) never mentions review_summary at all, even though it's now a real bucket in feedback and summary.review_summaries.
Since this is the first doc a maintainer reads when opening the file, it should match the SKILL.md wording ("its own review_summary bucket... never a priority bucket") rather than the pre-second-commit low default.
| // 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; |
There was a problem hiding this comment.
[New] NEGATORS is missing common negation contractions, so this fix still misses the exact class of bug it's meant to close.
const NEGATORS = /\b(?:not|no|non|without|isn't|is not)\b/i;won't, doesn't, wasn't, wouldn't, can't, shouldn't, aren't, never, and nothing are all absent. Concretely:
"This won't break anything, looks good."→will\s+(break|cause|fail)fires on "will break" (inside "won't"... actually on the literal "break" via a different pattern, or directly if phrased "This will not break" the direct-adjacency patterns don't match, but"nothing critical here"and"this won't be a blocker"both still hitHIGH_PATTERNS(critical,blocker) unnegated, because "nothing" and "won't" aren't recognized as negators.- A review body like "Nothing critical here, just a heads up" would still be filed
high.
Worth extending the alternation (e.g. adding won't|doesn't|wasn't|wouldn't|can't|shouldn't|aren't|never|nothing) rather than shipping a negator list that only covers the two phrasings used in the #307 repro.
| /** | ||
| * Whether any pattern matches `body` at a position not preceded by a negator. | ||
| */ | ||
| const matchesUnnegated = (patterns, body) => |
There was a problem hiding this comment.
[New] matchesUnnegated only ever inspects the first occurrence of each pattern in the body.
const matchesUnnegated = (patterns, body) =>
patterns.some((pattern) => {
const match = pattern.exec(body);
return match !== null && !isNegated(body, match.index);
});pattern.exec(body) on a non-global regex always returns the left-most match. If that first occurrence happens to be negated but a second, real occurrence of the same word exists later in the same body, the pattern is skipped entirely — the later occurrence is never checked.
Example: "It's not a blocker overall, but there's a real blocker in the retry logic that needs fixing." — the first blocker (negated) causes .some to treat the /blocker/i pattern as fully negated, so the second, genuine blocker a few words later is never classified as high. This is a real finding getting silently dropped, which is the opposite direction from the bug this PR fixes. Consider matching with a global flag and checking whether every match is negated, or at least searching from match.index + match[0].length when the first hit is negated.
| * 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 |
There was a problem hiding this comment.
[New] This module-level doc comment is now stale and contradicts the actual implementation.
Line 41 says a self-review summary defaults "to low absent a marker", and line 51 says a review summary "is surfaced but filed low". But categorizeReviewSummary (added in this same PR, see below) returns detectLogaf(body) ?? "review_summary" — the actual default bucket is review_summary, not low. That bucket rename happened in the second commit ("give review summaries their own bucket, not low"), which updated SKILL.md and the categorizeReviewSummary JSDoc correctly but missed this top-of-file block.
Also, the "Categories" list near the top of the file (lines ~15-24) never mentions review_summary at all, even though it's now a real bucket in feedback and summary.review_summaries.
Since this is the first doc a maintainer reads when opening the file, it should match the SKILL.md wording ("its own review_summary bucket... never a priority bucket") rather than the pre-second-commit low default.
1 participant
Summary
categorizeCommentin.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjshad two measured defects in its content fallback (used when no explicith:/m:/l:marker is present):blocker,security issue,critical, …) matched anywhere in the body with no regard for what preceded them, so a clean review's own closing sentence — "not a blocker", "no security issue" — got filedhigh. Measured on the real review summary of test(cli): cover the tested/failed/refused split on a mixed verify/test run #307.nit[:\s]required a colon or whitespace immediately after the word, so the commonNit,spelling missed and landed inmedium, the bucket the skill auto-fixes without prompting. Measured on a real inline comment from test(cli): cover PlatformBinaryResolution.source and asMatchMode's refusal #304.Fix
CHANGES_REQUESTEDfrom a non-author reviewer still stayshigh(unchanged); an explicith:/m:/l:marker still wins; otherwise the summary is surfaced but filedlowso it doesn't inflateneeds_attention. This applies to both human and review-bot summaries — the test(cli): cover the tested/failed/refused split on a mixed verify/test run #307 misfire was a bot's own review summary. The findings themselves still arrive separately as inline review-thread comments and are classified individually, so nothing is lost.nit,suggestion,optional,minor,style) word boundaries (\bnit\b) so punctuation stops mattering, and refuses a HIGH/LOW match preceded by a negator (not/no/non/without/isn't/is not) within a 24-character window, so "not a blocker" and "no security issue" stop counting as findings.detectLogaf(the explicit-marker path) is untouched, and an explicit marker still wins over everything, including in review summaries — covered by a new test.Self-review summaries stay surfaced and flagged
self_review: true(a PR author can't formally request changes on their own PR);review_in_progressand its precedence inaction_requiredare untouched.Test plan
NODE_OPTIONS= pnpm test:scripts— 364 tests pass (was 356; added tests for negation,Nit,punctuation, marker precedence, and structural review-summary bucketing; updated one existing test whose expected bucket intentionally changed frommediumtolow)NODE_OPTIONS= pnpm lint— passes (typecheck + eslint + built-CLI house-style check)NODE_OPTIONS= node .agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs --pr 307against the real, merged PR —summary.high: 0,summary.needs_attention: 0, confirming the clean review no longer reads as high-priorityFixes #309