fix(iterate-pr): stop content-classifying review summaries, fix nit punctuation by thecodedrift · Pull Request #311 · taskless/cli

thecodedrift · 2026-09-08T06:13:36Z

Summary

categorizeComment in .agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs had two measured defects in its content fallback (used when no explicit h:/m:/l: marker is present):

Negation blindness. HIGH patterns (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 filed high. Measured on the real review summary of test(cli): cover the tested/failed/refused split on a mixed verify/test run #307.
Punctuation-sensitive tokens. nit[:\s] required a colon or whitespace immediately after the word, so the common Nit, spelling missed and landed in medium, 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

Review summaries are now bucketed structurally, not by content. A summary narrates a review's findings, so it will always contain finding vocabulary, frequently negated, and no content-pattern list survives that. CHANGES_REQUESTED from a non-author reviewer still stays high (unchanged); an explicit h:/m:/l: marker still wins; otherwise the summary is surfaced but filed low so it doesn't inflate needs_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.
The inline-comment fallback (still used for review threads and issue comments) now gives the bracketed-class LOW tokens (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_progress and its precedence in action_required are 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 from medium to low)
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 307 against the real, merged PR — summary.high: 0, summary.needs_attention: 0, confirming the clean review no longer reads as high-priority

Fixes #309

…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.

thecodedrift · 2026-09-08T06:16:45Z

@claude /review

claude · 2026-09-08T06:17:04Z

Claude finished @thecodedrift's task in 5m 22s —— View job

Review complete

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json contained no reviewThreads, reviews, or standalone comments beyond the trigger comment itself, so this is the first review of this PR; nothing was treated as already-addressed.

Read .prior-review.json
Fetch PR diff (gh pr diff) and metadata (gh pr view)
Read changed files in full (fetch_pr_feedback.cjs, SKILL.md)
Run finder angles (correctness, removed-behavior, cross-file, cleanup, conventions)
Verify candidates against the code
Post inline comments

Findings

Posted as inline comments (all [New], first pass on this PR):

.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs:258 — NEGATORS (not|no|non|without|isn't|is not) omits common contractions like won't, doesn't, wasn't, can't, never, nothing. A body like "Nothing critical here, just a heads up" or "This won't be a blocker" still gets filed high — the exact class of bug this PR sets out to fix, just phrased differently.
fetch_pr_feedback.cjs:275 — matchesUnnegated uses a non-global pattern.exec(body), which always returns the first match. If the first occurrence of a HIGH/LOW word is negated but a later, genuine occurrence of the same word exists in the same body, the pattern is treated as fully negated and the real finding is silently dropped (e.g. "not a blocker overall, but there's a real blocker in the retry logic").
fetch_pr_feedback.cjs:41 (and the "Categories" list around line 15-24) — the module's top-of-file doc comment is stale: it says a review summary "defaults to low" absent a marker, but the second commit changed the actual default to the new review_summary bucket (categorizeReviewSummary returns detectLogaf(body) ?? "review_summary"). SKILL.md and the categorizeReviewSummary JSDoc were updated correctly; this block wasn't.
.agents/skills/iterate-pr/SKILL.md, step 3 "Handle Feedback by LOGAF Priority" (~line 165-204; couldn't anchor an inline comment there since it's outside this diff's hunk context) — the Auto-fix / Prompt / Skip-silently / Wait categorization never mentions the new review_summary bucket, even though "Replying to Comments" further down assumes review_summary items exist and need an acknowledgement reply. An agent following step 3 literally would have no cue to ever process them.

Nothing else surfaced past verification — the negation-window sizing (24 chars), the word-boundary fix for nit/suggestion/etc., and the review_summary vs high/marker precedence all look correct and are exercised by the new tests.

thecodedrift · 2026-09-08T16:39:03Z

Re: @claude — "Claude finished @thecodedrift's task in 5m 22s —— View job"
#311 (comment)

All four points addressed:

NEGATORS widened to cover contractions and bare negative words (won't, doesn't, wasn't, wouldn't, can't, cannot, couldn't, shouldn't, aren't, didn't, hasn't, haven't, hadn't, never, nothing, none), each with its own test.
matchesUnnegated now scans every occurrence of a pattern instead of stopping at the first, so a negated first mention no longer masks a later genuine finding. Covered by a test using your exact "not a blocker overall, but there's a real blocker..." repro.
The module docblock (Categories list, self-review section, review-summaries section) now describes the review_summary bucket instead of the stale low default.
SKILL.md step 3 now lists review_summary explicitly under a new "surface, never a fix and never a prompt" heading, and the "Replying to Comments" section now names it under "when to reply."

All three code fixes (1-3) were mutation-checked: broke each, confirmed the corresponding new test failed, restored the fix, confirmed it passed again. Full suite (369 tests) and pnpm lint both pass.

— AI Coding Agent

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.

thecodedrift added 2 commits September 7, 2026 23:13