Skip to content

Commit 2a45fa2

Browse files
committed
fix(iterate-pr): gate the in-progress check on the author, and bound 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.
1 parent f67adb0 commit 2a45fa2

3 files changed

Lines changed: 77 additions & 6 deletions

File tree

‎.agents/skills/iterate-pr/SKILL.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -392,7 +392,9 @@ If step 7 required code changes (from new feedback after CI passed), return to s
392392

393393
## Exit Conditions
394394

395-
Before exiting, check `summary.review_in_progress`. If it is > 0, do not exit — a review bot's placeholder is not a finished review, and `needs_attention: 0` alongside it means "hasn't started," not "clean." Sleep 30 seconds and re-check feedback; repeat until it drops to 0, addressing any new high/medium feedback that lands as it finishes (return to step 3).
395+
Before exiting, check `summary.review_in_progress`. If it is > 0, do not exit — a review bot's placeholder is not a finished review, and `needs_attention: 0` alongside it means "hasn't started," not "clean." Sleep 30 seconds and re-check feedback, addressing any new high/medium feedback that lands as it finishes (return to step 3).
396+
397+
**This loop has a bound, and reaching it is a report rather than a retry.** Give up after roughly ten minutes of a count that never drops, and tell the user the review appears stuck, naming the PR and the bot. A placeholder can be left behind permanently: the run can be cancelled, or its job can crash, and the comment then sits in its posted state with nobody to edit it. Waiting forever on that is the same failure as exiting early, arrived at from the other side, and it is an infrastructure problem under **Ask for help** rather than something more polling will fix. Re-triggering the review is usually the fix, but that is the user's call, not yours.
396398

397399
Then check `summary.pending_reviewers`. If it is > 0, reviewers have been requested but haven't submitted yet — their review may produce new feedback. Ask the user whether to wait:
398400

‎.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs‎

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -160,12 +160,20 @@ const isInfoBot = (username) =>
160160
* cannot stall a caller. The cost is that a NEW placeholder wording is missed,
161161
* so when this bot changes its output, add the new opening here.
162162
*
163-
* `(?![A-Za-z0-9])` rather than `\b`: underscore is a word character, so a
164-
* `\b` would not fire before the closing `__` of underscore emphasis.
163+
* Emphasis is unbounded (`[*_]*`) rather than capped at two, so bold-italic
164+
* (`***…***`) matches; a cap of two failed it, since two of the three leading
165+
* `*` were consumed and the phrase could not then start.
166+
*
167+
* The trailing side consumes closing emphasis and THEN refuses a word
168+
* character, underscore included. `\b` alone would not fire before a closing
169+
* `__`, but a bare `(?![A-Za-z0-9])` went too far the other way and matched
170+
* `Review in progress_notes: …`, a finished comment. Consuming `[*_]*` first
171+
* and excluding `_` from the lookahead accepts `__…__` and rejects
172+
* `progress_notes`.
165173
*/
166174
const IN_PROGRESS_MARKERS = [
167-
/^#{0,6}\s*[*_]{0,2}\s*review in progress(?![A-Za-z0-9])/i,
168-
/^#{0,6}\s*[*_]{0,2}\s*claude code is working(?![A-Za-z0-9])/i,
175+
/^#{0,6}\s*[*_]*\s*review in progress[*_]*(?![A-Za-z0-9_])/i,
176+
/^#{0,6}\s*[*_]*\s*claude code is working[*_]*(?![A-Za-z0-9_])/i,
169177
];
170178

171179
/** Whether a body still opens with one of the in-progress placeholders. */
@@ -257,7 +265,13 @@ const categorizeComment = (comment, body) => {
257265
* three sources.
258266
*/
259267
const bucketByAuthor = (feedback, item, comment, body, author) => {
260-
if (isReviewInProgress(body)) {
268+
// The author gate is load-bearing, not belt and braces. A human writing
269+
// "Review in progress on my end, back by EOD" would otherwise be filed as an
270+
// unfinished review, vanish from `needs_attention`, and hang the wait loop
271+
// forever: a person's comment never gets edited into a finished form the way
272+
// the bot's placeholder does, so the count never drops. Only a review bot has
273+
// the lifecycle this bucket describes.
274+
if (isReviewBot(author) && isReviewInProgress(body)) {
261275
feedback.review_in_progress.push(item);
262276
} else if (isReviewBot(author)) {
263277
item.review_bot = true;

‎.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -784,3 +784,58 @@ test("an in-progress placeholder keeps every other bucket empty", () => {
784784
);
785785
assert.match(output.action_required, /still in progress/);
786786
});
787+
788+
// A HUMAN IS NOT A REVIEW BOT, AND THIS IS THE DANGEROUS DIRECTION. A person
789+
// writing "Review in progress on my end" would otherwise be filed as an
790+
// unfinished review, vanish from needs_attention, and hang the wait loop
791+
// forever: a person's comment never gets edited into a finished form the way
792+
// the bot's placeholder does, so the count never drops to zero.
793+
test("a human comment opening with the phrase is feedback, not an unfinished review", () => {
794+
const output = build(
795+
fakeClient({
796+
comments: [
797+
{
798+
id: 1,
799+
// The LOGAF marker is deliberately absent: it only counts at the
800+
// start, and the start is occupied by the phrase under test. So this
801+
// lands in medium by content, which is the correct default.
802+
body: "Review in progress on my end, will finish by EOD.",
803+
user: { login: "a-human-reviewer" },
804+
},
805+
],
806+
}),
807+
{}
808+
);
809+
assert.equal(output.summary.review_in_progress, 0);
810+
assert.equal(output.summary.medium, 1, "it stays actionable feedback");
811+
assert.equal(output.summary.needs_attention, 1);
812+
});
813+
814+
test("the same body from the review bot IS an unfinished review", () => {
815+
const output = build(
816+
fakeClient({
817+
comments: [
818+
{
819+
id: 1,
820+
body: "Review in progress on my end, will finish by EOD.",
821+
user: { login: "claude[bot]" },
822+
},
823+
],
824+
}),
825+
{}
826+
);
827+
assert.equal(output.summary.review_in_progress, 1);
828+
assert.equal(output.summary.needs_attention, 0);
829+
});
830+
831+
// Two regex edges, both able to reintroduce the bug. Bold-italic is one format
832+
// drift away from the bold case already anticipated; `progress_notes` is a
833+
// finished comment that a too-permissive boundary would freeze the loop on.
834+
test("bold-italic emphasis is matched, and a bare underscore is not emphasis", () => {
835+
assert.equal(isReviewInProgress("***Review in progress***"), true);
836+
assert.equal(isReviewInProgress("___Claude Code is working___"), true);
837+
assert.equal(
838+
isReviewInProgress("Review in progress_notes: nothing else found"),
839+
false
840+
);
841+
});

0 commit comments

Comments
 (0)