LOGAF fallback classifier reads negated prose and punctuation wrong, inflating needs_attention #309

categorizeComment in .agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs falls back to content patterns when a comment carries no explicit h:/m:/l: marker. Two defects in that fallback were measured while running /iterate-pr over the #284 stack. Neither is dangerous, and together they make needs_attention mean less than it should.

1. Negation blindness promotes a clean review to high

The HIGH patterns match a word anywhere in the body with no regard for what precedes it. Measured on the real review summary of #307, an entirely positive review that found nothing:

matched HIGH pattern: blocker
  -> "…so it's a \"worth a look,\" not a blocker."

The phrase is "not a blocker". A review saying it found no blocker is filed as high-priority feedback. Reproduced directly:

categorizeComment({user:{login:"claude[bot]"}},
  "I found no security issue and this is not a blocker.")
// => "high"

Two HIGH patterns fire there, both negated. A reviewer writing the ordinary closing sentence of a clean review ("no security issues", "not a blocker", "nothing here will break") gets the strongest bucket the scale has.

2. A punctuation-sensitive token misses self-labelled nits

nit[:\s] requires a colon or whitespace immediately after the word, so the common Nit, spelling does not match:

/nit[:\s]/i.test("Nit, could be simpler.")   // false

Measured on a real inline comment from #304, which opens Nit, not a defect: and was filed medium:

categorizeComment(bot, "[New] Nit, not a defect: this REFUSALS case overlaps…")
// => "medium"

A comment that labels itself a nit in its first word lands in the bucket the skill auto-fixes without prompting. The sibling comment on the same PR reached low only by accident, through minor[:\s] matching "Minor reuse" rather than through the nit pattern at all.

Why it is worth fixing

Neither misfires toward silence, so nothing gets dropped. The cost is noise in the one number a caller is told to trust: needs_attention counted 1 on #307 for a review whose own conclusion was "I did not find any correctness bugs", and 2 on #304 where both items were explicitly labelled non-defects. A number that is routinely wrong in the same direction gets discounted, which is the "signal expected to be red is not a signal" failure this repository already documents for workflows.

Recommended fix

Stop content-classifying review summaries. This is the larger half and it removes defect 1 at the root rather than patching its symptoms. A summary's job is to narrate findings, so it will always contain finding vocabulary, frequently negated, and no pattern list survives that. The findings themselves arrive separately as inline threads and are classified individually, so nothing is lost by not re-classifying the prose around them.

Concretely, bucket a review_summary by what is known structurally rather than by its prose:

• state === "CHANGES_REQUESTED" from a reviewer who is not the PR author: high, as today.
• otherwise: surface it, but not in a priority bucket that feeds needs_attention.

Keep self-review summaries surfaced (they are the PR author's own notes and are deliberately not dropped), and keep the review_bot flag. This only changes which counter they inflate.

Then repair the remaining fallback, which still runs on inline comments:

• Give the tokens word boundaries so punctuation stops mattering: nit[:\s] becomes \bnit\b, covering Nit,, nit:, and (nit) alike. Check the other bracketed-class tokens (suggestion[:\s], optional[:\s], minor[:\s], style[:\s]) for the same issue.
• Refuse a match preceded by a negator within a short window, so not a blocker, no security issue, and isn't broken stop counting. A lookbehind over roughly 24 characters for not|no|non|without|isn't|is not is enough for the observed cases and is cheap to test.

Tests

The bodies above are real and make good fixtures. Worth asserting at least:

• a clean review summary that says "not a blocker" is not high
• Nit, and nit: reach the same bucket
• an explicit h: marker still wins over everything (the marker path is correct today and must not regress)

The suite already drives buildFeedback from fixtures with no network (.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs), so this needs no new harness.

Not in scope

The explicit-marker path (detectLogaf) is correct and anchored, and the bot review/info split is correct. This is only about the content fallback that runs when no marker is present.

Found while running /iterate-pr over #303 through #307.