fix(iterate-pr): surface an unfinished review instead of bucketing it as feedback - #303
Conversation
|
@claude /review |
|
Claude finished @thecodedrift's task in 3m 15s —— View job Review
Review mode: incremental — read 0 prior review thread(s) before reviewing. Note on scope: the task briefing said this PR had 2 commits, but Note on tool output: one background research pass flagged that a subagent's reading of AssessmentThe core mechanism (an anchored, markdown-emphasis-tolerant marker checked at the single [New] [New] Two regex edge cases that mirror the exact bug being fixed ( [New] The [New] The Everything else — the docstring/SKILL.md documentation, the |
|
Heads up for the reviewer: this PR changed materially after the review started, and the running review is reading The new commit exists because I measured the placeholder live instead of trusting the issue, and the issue was incomplete. While a review ran on #302 I read the bot comment twice, 29 seconds apart: Issue #292 captured only the middle state. The marker built from the issue text therefore matched only that one, and missed the first half-minute after a trigger, which is exactly when a caller polls too early and the check exists to guard. It would have reported a clean PR during its own blind spot. Both openings are now matched, and the tests use the captured bodies rather than a transcription, with the finished state as a negative case. Worth noting for anyone reviewing this: the comment sitting above this one currently opens with — AI Coding Agent |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@claude /review |
|
Claude finished @thecodedrift's task in 2m 8s —— View job Review
Review mode: incremental — read 3 prior review thread(s) before reviewing. All three are What changed since the last review: the third commit (
I found nothing NEW introduced by the third commit itself — the marker restructuring and the added |
All three inline findings were real and are fixed in The author-gate one was the serious catch. A human writing "Review in progress on my end" was diverted into Good spot on reconstructing the true head, too. The briefing was stale because I pushed — AI Coding Agent |
e5b0eae to
6428081
Compare
… as feedback The Claude review bot posts a placeholder comment the instant it is triggered and edits it in place as it works, so the comment existing, its created_at, and even a green check run are all not completion signals — a run has been observed to report success while the body still read "Review in progress" with unchecked boxes. fetch_pr_feedback.cjs had no way to tell that apart from an ordinary claude[bot] comment, so a caller that fetched feedback promptly got needs_attention: 0 indistinguishable from a clean review. Detect it by checking whether the body still OPENS with the in-progress marker (anchored to the start, not a substring search anywhere in the body, so a finished review that discusses this behaviour is never read as unfinished). Unfinished items go into their own summary.review_in_progress bucket rather than high/medium/low/resolved, and when nonzero, action_required outranks every other bucket, since a caller deciding whether to stop must not read existing high/medium items as the whole story while the review could still add more. Updated SKILL.md so the field is documented alongside the other summary fields, step 4's "wait for review bots" note points at the field instead of at prose, and the exit conditions check it before pending_reviewers. Fixes #292
…ked up The marker only tolerated heading hashes, but this bot opens its FINISHED comment with bold (`**Claude finished …**`), so a bold placeholder is one format change away. Missing it fails silently and lands straight back on the bug the check exists to prevent, where an unstarted review reads as a clean one. Allowing emphasis costs nothing: a body that OPENS with the phrase is not feedback whichever way it is marked up. The trailing boundary is `(?![A-Za-z0-9])` rather than `\b` because underscore is a word character, so `\b` never fired before the closing `__` of underscore emphasis and that spelling was missed. Anchoring is unchanged, and is covered both ways: a finished review that quotes the phrase mid-body is still not in progress.
Measured live on PR #302 while a review ran. The bot's comment passes through three states, not two: 22:24:05 Claude Code is working… <img …> (created) 22:24:34 ### Review in progress <img …> (+29s, checkboxes appear) on finish **Claude finished …** Issue #292 captured the middle one, so a marker derived from the issue text matched only that. The first state went undetected, which is the half-minute immediately after a trigger and precisely when a caller polls too early. The check would have reported a clean PR during the exact window it exists to guard. Both openings are now matched, and the tests use the real captured bodies rather than a transcription, including the finished state as a negative. Records the rejected alternative so it is not re-proposed: inverting this into an allowlist (anything not opening with a completion marker is unfinished) fails safe for this bot, but Sentry, Cursor, Copilot and CodeQL have no completion marker at all, so every comment they post would read as unfinished and the wait loop would never exit. A blocklist of measured placeholders cannot stall a caller. Its cost is that a new wording is missed, so when this bot changes its output, add the new opening.
…its wait Three findings from review, all real. **A human is not a review bot, and this was the dangerous direction.** The in-progress check ran on every comment regardless of author, so a person writing "Review in progress on my end, back by EOD" was filed as an unfinished review, vanished from `needs_attention`, and hung the wait loop permanently: a person's comment never gets edited into a finished form the way the bot's placeholder does, so the count never drops. Gated on `isReviewBot(author)`, with tests covering the same body from both a human and the bot. **Two regex edges could each reintroduce the bug.** Leading emphasis was capped at two characters, so `***Review in progress***` did not match, which is one format drift from the bold case already anticipated. And the trailing `(?![A-Za-z0-9])` admitted a bare underscore, so `Review in progress_notes: …` matched as unfinished. Emphasis is now unbounded, and the trailing side consumes closing emphasis before refusing a word character including `_`, which accepts `__…__` and rejects `progress_notes`. **The wait loop had no escape hatch.** A cancelled run or a crashed job leaves the placeholder in place with nobody to edit it, and the documented procedure said only "repeat until it drops to 0". It is now bounded at roughly ten minutes, and reaching the bound is an "Ask for help" report naming the PR and the bot, rather than more polling. Waiting forever is the same failure as exiting early, reached from the other side.
6428081 to
9ae9ca3
Compare
1 participant
The review bot posts its comment the instant it is triggered and edits it in place as it works, so three things that look like completion signals are not: the comment existing, its
created_at, and the check run concludingsuccess. A run has been observed reporting success while the body still read "Review in progress" with unchecked boxes.buildFeedbackhad no concept of that state, so a placeholder was bucketed as an ordinaryclaude[bot]comment. A caller fetching feedback promptly after triggering a review gotneeds_attention: 0and could not tell "reviewed, nothing found" from "not reviewed yet" — the exact silent-success shape the skill exists to prevent.The fix
A
review_in_progressbucket, with asummary.review_in_progresscount, so a caller distinguishes the two states without pattern-matching prose itself.The check lives inside
bucketByAuthor, the single choke point all three feedback sources already share. An unfinished placeholder must never be read as a review bot's finding, an info bot's noise, or ordinary human feedback, and putting the rule anywhere else would mean applying it to two of the three by hand.action_requiredoutranks everything else when a review is running. That field is what a caller uses to decide whether to stop, and stopping because the visible high and medium items were addressed, while a review that could still surface more is in flight, is the precise failure being fixed. The other items are untouched infeedbackand still processed normally; only the "nothing left to do" reading is blocked.Anchoring is the whole subtlety
Detection tests whether the body opens with the marker, never whether it contains the phrase. A review that legitimately discusses this behaviour, as any review of this PR might, would otherwise read as unfinished forever.
The second commit widens what counts as the opening, for a reason found while reviewing the first. The observed placeholder is a heading, but this same bot opens its finished comment with bold (
**Claude finished …**), so a bold placeholder is one format change from silently reverting the bug. Emphasis is now tolerated alongside heading hashes.That change surfaced a second-order trap worth recording: the trailing boundary cannot be
\b, because underscore is a word character, so\bnever fires before the closing__of underscore emphasis and that spelling was missed. It is(?![A-Za-z0-9])instead.Both directions are tested:
### Review in progress <img/>**Review in progress**__Review in progress__A review in progress is not clean**Claude finished** … quoting "Review in progress"Review in progresses nicelyDocumentation
SKILL.mddocuments the new field where the othersummaryfields are, and step 4's existing "wait for review bots" instruction now points atsummary.review_in_progressrather than at prose, with a note that a green check run is not sufficient. The exit conditions check it beforepending_reviewers. The issue's closing point was that this guidance belongs in the tool rather than in every caller's brief.Verified
350 script tests pass, up from 340.
pnpm lintclean. The script was also run for real against merged PR #299, which reportsreview_in_progress: 0as expected for a completed, addressed review.No changeset: this skill is repository tooling, not published package surface, so it ships no release note.
Fixes #292