From d94af7483ccfb1237f165c5d5666bc32a96d7fde Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 7 Sep 2026 15:16:27 -0700 Subject: [PATCH 1/4] fix(iterate-pr): surface an unfinished review instead of bucketing it as feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .agents/skills/iterate-pr/SKILL.md | 15 +- .../iterate-pr/scripts/fetch_pr_feedback.cjs | 68 +++++++- .../scripts/fetch_pr_feedback.test.cjs | 146 +++++++++++++++++- 3 files changed, 220 insertions(+), 9 deletions(-) diff --git a/.agents/skills/iterate-pr/SKILL.md b/.agents/skills/iterate-pr/SKILL.md index 097465b0..b32fac4d 100644 --- a/.agents/skills/iterate-pr/SKILL.md +++ b/.agents/skills/iterate-pr/SKILL.md @@ -64,9 +64,12 @@ Returns JSON with feedback categorized as: - `low` - Optional (`l:`, nit, style, suggestion) - `bot` - Informational automated comments (Codecov, Dependabot, etc.) - `resolved` - Already resolved threads, and top-level comments carrying our own πŸŽ‰ acknowledgement +- `review_in_progress` - A review bot's placeholder comment, posted the instant it was triggered and not yet edited to its finished form. Not feedback yet β€” see below. Review bot feedback (from Sentry, Warden, Copilot, Cursor, Bugbot, CodeQL, etc.) appears in `high`/`medium`/`low` with `review_bot: true` β€” it is NOT placed in the `bot` bucket. +**A review bot's comment existing is not a completion signal, and neither is its `created_at` or a green check run.** The Claude review bot posts a comment the instant it is triggered and edits that same comment in place as it works β€” a run has been observed to report `success` while the body still read "Review in progress" with unchecked boxes. `fetch_pr_feedback.cjs` detects this by checking whether the body still **opens** with the in-progress marker (a review that happens to discuss this behaviour mid-body is not mistaken for an unfinished one) and reports the count as `summary.review_in_progress` instead of bucketing the placeholder as ordinary feedback. **Before treating `needs_attention: 0` as "reviewed, nothing found," check `summary.review_in_progress` β€” a nonzero count means the review hasn't finished, not that it found nothing.** When one or more are present, `action_required` says so and outranks every other bucket, since a caller must not stop just because the buckets it can already see look clean. + **Self-review feedback** (from the PR author) appears in `high`/`medium`/`low` with `self_review: true`. Because you can't "Request changes" on your own PR, a self-review lands as `COMMENTED` review summaries and ordinary review threads rather than changes-requested items β€” these are surfaced (not dropped) and bucketed by content, defaulting to `medium` when there's no `h:/m:/l:` prefix. Treat `self_review` items the same as any other human feedback in step 3. Each feedback item may also include: @@ -191,6 +194,10 @@ Which would you like to address? (e.g., "1,3" or "all" or "none") - `resolved` threads - `bot` comments (informational only β€” Codecov, Dependabot, etc.) +**Neither address nor skip β€” wait:** + +- `review_in_progress` items. There is nothing to fix yet; the bot hasn't finished writing its findings. Do not treat these as `bot` noise or as a clean review. See the "Wait if pending" note in step 4. + #### Replying to Comments After processing a feedback item, acknowledge it on the PR so the trail shows what was addressed. How you reply depends on whether the item is an **inline thread** or a **top-level comment**. @@ -262,6 +269,8 @@ Run `${CLAUDE_SKILL_ROOT}/scripts/fetch_pr_checks.cjs` to get structured failure **Wait if pending:** If review bot checks (sentry, warden, cursor, bugbot, seer, codeql) are still running, wait before proceedingβ€”they post actionable feedback that must be evaluated. Informational bots (codecov) are not worth waiting for. +This applies even when the check itself has already concluded. A review bot can report its check `success` while its comment is still the placeholder it posted on trigger β€” check `summary.review_in_progress` from `fetch_pr_feedback.cjs` (step 2), not the body text or the check's conclusion, and wait for it to drop to 0 before treating that bot's feedback as final. + #### No PR check asks whether the OpenSpec change is archived A change is archived exactly once, at the END of the work, so an unarchived @@ -383,14 +392,16 @@ If step 7 required code changes (from new feedback after CI passed), return to s ## Exit Conditions -Before exiting, 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: +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). + +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: - **Yes:** sleep 30 seconds, re-check feedback. If new high/medium feedback appeared, address it (return to step 3). If `pending_reviewers` dropped to 0, proceed to exit. Repeat until reviewers complete. - **No:** proceed to the exit conditions below. If waiting produced code changes, return to step 2 for a fresh cycle. -**Success:** All checks pass, post-CI feedback re-check is clean (no new unaddressed high/medium feedback including review bot findings), user has decided on low-priority items, and pending reviewers resolved or user opted to skip. +**Success:** All checks pass, post-CI feedback re-check is clean (no new unaddressed high/medium feedback including review bot findings, and no review still in progress), user has decided on low-priority items, and pending reviewers resolved or user opted to skip. **Ask for help:** Same failure after 2 attempts, feedback needs clarification, infrastructure issues. diff --git a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs index 321ae4fd..8f241bc8 100755 --- a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs +++ b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs @@ -19,6 +19,9 @@ * - low: Optional suggestions (l:, nit, style) * - bot: Informational automated comments (Codecov, Dependabot, etc.) * - resolved: Already resolved threads + * - review_in_progress: A review bot's placeholder comment, posted the instant + * it was triggered and not yet edited to its finished form β€” not feedback + * yet, see "Unfinished reviews" below * * Bot classification: * - Review bots (Sentry, Warden, Cursor, Bugbot, etc.) provide actionable code @@ -33,6 +36,18 @@ * **threads**, not as changes-requested items. These are surfaced (not * dropped) and flagged `self_review: true`, bucketed by content β€” defaulting * to `medium` when no `h:/m:/l:` prefix is present. + * + * Unfinished reviews: + * - The Claude review bot posts its comment immediately when triggered and + * edits it in place as it works, so the comment existing, its `created_at`, + * and the check run concluding `success` are all NOT completion signals β€” a + * run has been observed to report success while the body still read "Review + * in progress" with unchecked boxes. The only reliable signal is that the + * body no longer OPENS with the in-progress marker. An item whose body still + * opens with it is filed in its own `review_in_progress` bucket rather than + * `high`/`medium`/`low`/`resolved` β€” it is not feedback yet, and bucketing it + * as an ordinary comment produces `needs_attention: 0` indistinguishable from + * "reviewed, nothing found". */ const { parseArgs } = require("node:util"); @@ -111,6 +126,23 @@ const isReviewBot = (username) => const isInfoBot = (username) => INFO_BOT_PATTERNS.some((pattern) => pattern.test(username ?? "")); +/** + * The placeholder the Claude review bot posts the instant it is triggered, + * before it has read a single file β€” e.g. `### Review in progress `. + * It edits the same comment in place as it works, so this marker is the only + * way to tell "still running" from "finished with nothing to say". + * + * Anchored to the START of the (trimmed) body on purpose: a review that + * legitimately discusses this behaviour β€” quoting the phrase mid-body, the way + * this very fix does β€” must not be read as unfinished forever. Only an + * in-progress placeholder OPENS with it; a finished review that happens to + * mention the phrase does not. + */ +const IN_PROGRESS_MARKER = /^#{0,6}\s*review in progress\b/i; + +/** Whether a body still opens with the review-bot in-progress placeholder. */ +const isReviewInProgress = (body) => IN_PROGRESS_MARKER.test(body.trimStart()); + /** * Detect a LOGAF marker at the start of a comment body. * @@ -180,17 +212,23 @@ const categorizeComment = (comment, body) => { }; /** - * File one item by its author: a review bot is flagged and bucketed by content, - * an info bot is filed as `bot`, everyone else is bucketed by content. + * File one item by its author: an unfinished review is filed on its own ahead + * of every other rule, a review bot is flagged and bucketed by content, an + * info bot is filed as `bot`, everyone else is bucketed by content. * * The three sources (review summaries, review threads, issue comments) each * wrap this with their own precondition β€” changes-requested, resolved, * acknowledged β€” but the bot classification itself is one rule in one place, so * a new pattern list or a change to the split cannot be applied to two of the - * three by accident. + * three by accident. The in-progress check lives here for the same reason: an + * unfinished placeholder must never be read as a review bot's finding (`high`), + * an info bot's noise (`bot`), or ordinary human feedback, from ANY of the + * three sources. */ const bucketByAuthor = (feedback, item, comment, body, author) => { - if (isReviewBot(author)) { + if (isReviewInProgress(body)) { + feedback.review_in_progress.push(item); + } else if (isReviewBot(author)) { item.review_bot = true; feedback[categorizeComment(comment, body)].push(item); } else if (isInfoBot(author)) { @@ -378,7 +416,14 @@ const buildFeedback = (client, { owner, repo, prInfo }) => { const prNumber = prInfo.number; const prAuthor = prInfo.author?.login ?? ""; - const feedback = { high: [], medium: [], low: [], bot: [], resolved: [] }; + const feedback = { + high: [], + medium: [], + low: [], + bot: [], + resolved: [], + review_in_progress: [], + }; // Review summary bodies. Every non-empty summary is surfaced, regardless of // author: a self-review can't be "Request changes", so the PR author's own @@ -506,13 +551,23 @@ const buildFeedback = (client, { owner, repo, prInfo }) => { resolved: feedback.resolved.length, review_bot_feedback: countFlagged("review_bot"), self_review_feedback: countFlagged("self_review"), + review_in_progress: feedback.review_in_progress.length, needs_attention: feedback.high.length + feedback.medium.length, pending_reviewers: requestedReviewers.length, }, feedback, }; - if (feedback.high.length > 0) { + // `review_in_progress` outranks everything else: it is the one condition a + // caller must not resolve by "stopping", the exact silent-success shape this + // field exists to prevent. A caller that sees needs_attention: 0 and quits + // would otherwise conclude a review that hasn't started yet is a clean one. + // Existing high/medium/low items are still all present in `feedback` and are + // not blocked on this β€” only the "nothing left to do" reading is. + if (feedback.review_in_progress.length > 0) { + output.action_required = + "A review is still in progress - wait for it to finish before treating feedback as final"; + } else if (feedback.high.length > 0) { output.action_required = "Address high-priority feedback before merge"; } else if (feedback.medium.length > 0) { output.action_required = "Address medium-priority feedback"; @@ -576,5 +631,6 @@ module.exports = { extractFeedbackItem, isInfoBot, isReviewBot, + isReviewInProgress, main, }; diff --git a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs index 1edb87e2..aab9389d 100644 --- a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs +++ b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs @@ -21,6 +21,7 @@ const { extractFeedbackItem, isInfoBot, isReviewBot, + isReviewInProgress, main, } = require("./fetch_pr_feedback.cjs"); @@ -100,6 +101,46 @@ test("a review bot whose name also ends in [bot] is still a review bot", () => { ); }); +test("isReviewInProgress recognizes the placeholder Claude posts on trigger", () => { + const body = [ + '### Review in progress ', + "", + "Review mode: incremental β€” read 0 prior review thread(s) before reviewing.", + "", + "- [x] Read `.prior-review.json`", + "- [ ] Manual pass over core logic", + ].join("\n"); + assert.ok(isReviewInProgress(body)); +}); + +test("isReviewInProgress tolerates leading whitespace and a bare heading-less form", () => { + assert.ok(isReviewInProgress(" Review in progress\n\nworking...")); + assert.ok(isReviewInProgress("## review in progress")); +}); + +// The whole point of anchoring to the start: a FINISHED review that discusses +// this very behaviour β€” quoting the phrase mid-body, the way this fix's own PR +// description might β€” must not be read as unfinished forever. Only a body that +// literally OPENS with the marker is in progress. +test("isReviewInProgress ignores the phrase when it is not at the start", () => { + const finishedReviewDiscussingTheIssue = [ + "### Review complete", + "", + "This PR fixes the bug where a caller could not tell an in-progress review", + 'from a finished one. The placeholder always reads "Review in progress" and', + "is edited in place once the review finishes.", + "", + "No other issues found.", + ].join("\n"); + assert.ok(!isReviewInProgress(finishedReviewDiscussingTheIssue)); +}); + +test("isReviewInProgress is false for an ordinary finished review", () => { + assert.ok( + !isReviewInProgress("### Review complete\n\nLooks good, no issues found.") + ); +}); + test("categorizeComment prefers an explicit LOGAF marker over content", () => { const human = { user: { login: "reviewer" } }; // "critical" alone would be high; the marker overrides it. @@ -354,6 +395,102 @@ test("an unacknowledged top-level comment stays in its priority bucket", () => { assert.equal(output.summary.resolved, 0); }); +// This is the exact scenario from the bug report: a caller fetches feedback +// promptly after triggering a review and must be able to tell "reviewed, +// nothing found" from "not reviewed yet" without reading prose itself. +test("an in-progress top-level comment is filed as review_in_progress, not bucketed as feedback", () => { + const output = build( + fakeClient({ + comments: [ + { + id: 1, + body: '### Review in progress \n\n- [x] Read the diff\n- [ ] Manual pass', + user: { login: "claude[bot]" }, + }, + ], + }), + {} + ); + assert.equal(output.summary.review_in_progress, 1); + assert.equal(output.summary.needs_attention, 0); + assert.equal(output.summary.high, 0); + assert.equal(output.summary.medium, 0); + assert.equal(output.summary.low, 0); + assert.equal(output.summary.bot_comments, 0); + assert.equal(output.summary.resolved, 0); + assert.equal(output.feedback.review_in_progress.length, 1); + assert.match(output.action_required, /still in progress/); +}); + +// The counterpart to the case above: once the same bot has finished and edited +// its comment to no longer open with the marker, it is ordinary review-bot +// feedback again and is bucketed by content as usual. +test("a completed review from the same bot is bucketed normally", () => { + const output = build( + fakeClient({ + comments: [ + { + id: 1, + body: "### Review complete\n\nThis will break on empty input.", + user: { login: "claude[bot]" }, + }, + ], + }), + {} + ); + assert.equal(output.summary.review_in_progress, 0); + assert.equal(output.summary.high, 1); + assert.equal(output.summary.needs_attention, 1); + assert.equal(output.action_required, "Address high-priority feedback before merge"); +}); + +// A review that merely mentions the phrase mid-body (e.g. discussing this very +// fix) must not be quarantined forever as "still running". +test("a completed review that mentions the phrase mid-body is not treated as in progress", () => { + const output = build( + fakeClient({ + comments: [ + { + id: 1, + body: 'This adds detection for the "Review in progress" placeholder. No other issues found.', + user: { login: "claude[bot]" }, + }, + ], + }), + {} + ); + assert.equal(output.summary.review_in_progress, 0); + assert.ok(output.feedback.high.length + output.feedback.medium.length > 0); +}); + +// review_in_progress must outrank high/medium/low in action_required: a caller +// deciding whether to stop must not read existing high-priority findings as +// the whole story while a review that could still surface more is running. +test("review_in_progress in action_required outranks an already-present high item", () => { + const output = build( + fakeClient({ + comments: [ + { + id: 1, + body: "### Review in progress\n\n- [ ] still working", + user: { login: "claude[bot]" }, + }, + ], + threads: [ + thread({ + comments: { + nodes: [{ body: "h: fix this now", author: { login: "reviewer" } }], + }, + }), + ], + }), + {} + ); + assert.equal(output.summary.high, 1); + assert.equal(output.summary.review_in_progress, 1); + assert.match(output.action_required, /still in progress/); +}); + test("pending reviewers are counted, and action_required tracks the top bucket", () => { const withNothing = build(fakeClient(), {}); assert.equal(withNothing.action_required, null); @@ -540,7 +677,14 @@ test("a malformed --pr is rejected", () => { // identically whichever source the item arrived from. test("bucketByAuthor applies the same rule to every source", () => { const bucket = (author, body) => { - const feedback = { high: [], medium: [], low: [], bot: [], resolved: [] }; + const feedback = { + high: [], + medium: [], + low: [], + bot: [], + resolved: [], + review_in_progress: [], + }; const item = { author }; bucketByAuthor(feedback, item, { user: { login: author } }, body, author); const [name] = Object.entries(feedback).find( From 115f9ce4fc9473a6d6c65d972b997777b740ba83 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 7 Sep 2026 15:19:05 -0700 Subject: [PATCH 2/4] fix(iterate-pr): match the in-progress marker whichever way it is marked up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../iterate-pr/scripts/fetch_pr_feedback.cjs | 13 +++++++- .../scripts/fetch_pr_feedback.test.cjs | 32 ++++++++++++++++++- 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs index 8f241bc8..4d140f0b 100755 --- a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs +++ b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs @@ -137,8 +137,19 @@ const isInfoBot = (username) => * this very fix does β€” must not be read as unfinished forever. Only an * in-progress placeholder OPENS with it; a finished review that happens to * mention the phrase does not. + * + * Leading markdown emphasis is tolerated as well as heading hashes. The + * observed placeholder is `### Review in progress`, but the same bot opens its + * FINISHED comment with bold (`**Claude finished …**`), so a bold placeholder + * is a format change away. Missing it would fail silently, straight back to + * the bug this exists to prevent, and allowing it costs nothing: a body + * opening with the phrase is not feedback whichever way it is marked up. */ -const IN_PROGRESS_MARKER = /^#{0,6}\s*review in progress\b/i; +// `(?![A-Za-z0-9])` rather than `\b`: underscore is a word character, so a +// `\b` here would not fire before the closing `__` of underscore emphasis and +// the marker would be missed. +const IN_PROGRESS_MARKER = + /^#{0,6}\s*[*_]{0,2}\s*review in progress(?![A-Za-z0-9])/i; /** Whether a body still opens with the review-bot in-progress placeholder. */ const isReviewInProgress = (body) => IN_PROGRESS_MARKER.test(body.trimStart()); diff --git a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs index aab9389d..bdf3f6d7 100644 --- a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs +++ b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs @@ -441,7 +441,10 @@ test("a completed review from the same bot is bucketed normally", () => { assert.equal(output.summary.review_in_progress, 0); assert.equal(output.summary.high, 1); assert.equal(output.summary.needs_attention, 1); - assert.equal(output.action_required, "Address high-priority feedback before merge"); + assert.equal( + output.action_required, + "Address high-priority feedback before merge" + ); }); // A review that merely mentions the phrase mid-body (e.g. discussing this very @@ -703,3 +706,30 @@ test("bucketByAuthor applies the same rule to every source", () => { assert.equal(bucket("a-human", "Why is this here?").name, "medium"); }); + +// The observed placeholder is a heading, but the same bot opens its FINISHED +// comment with bold, so a bold or underscored placeholder is one format change +// away. Missing it would fail silently, straight back to the bug this exists to +// prevent. The trailing boundary is `(?![A-Za-z0-9])` rather than `\\b` because +// underscore is a word character, so `\\b` would not fire before a closing `__`. +test("the in-progress marker survives markdown emphasis, not just headings", () => { + for (const body of [ + '### Review in progress ', + "**Review in progress**", + "__Review in progress__", + "*Review in progress*", + "Review in progress", + ]) { + assert.equal(isReviewInProgress(body), true, body); + } +}); + +test("emphasis tolerance does not loosen the anchor", () => { + for (const body of [ + "A review in progress is not a clean review", + '**Claude finished** β€” a body reading "Review in progress" was mis-bucketed', + "Review in progresses nicely", + ]) { + assert.equal(isReviewInProgress(body), false, body); + } +}); From 3c7a9dee69f2f2b9e87080ebfde3bca33e15c153 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 7 Sep 2026 15:25:59 -0700 Subject: [PATCH 3/4] fix(iterate-pr): detect the placeholder state the issue never captured MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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… (created) 22:24:34 ### Review in progress (+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. --- .../iterate-pr/scripts/fetch_pr_feedback.cjs | 64 ++++++++++++------- .../scripts/fetch_pr_feedback.test.cjs | 51 +++++++++++++++ 2 files changed, 93 insertions(+), 22 deletions(-) diff --git a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs index 4d140f0b..e3373708 100755 --- a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs +++ b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs @@ -127,32 +127,52 @@ const isInfoBot = (username) => INFO_BOT_PATTERNS.some((pattern) => pattern.test(username ?? "")); /** - * The placeholder the Claude review bot posts the instant it is triggered, - * before it has read a single file β€” e.g. `### Review in progress `. - * It edits the same comment in place as it works, so this marker is the only - * way to tell "still running" from "finished with nothing to say". + * The placeholders a review bot posts before it has anything to say. * - * Anchored to the START of the (trimmed) body on purpose: a review that - * legitimately discusses this behaviour β€” quoting the phrase mid-body, the way - * this very fix does β€” must not be read as unfinished forever. Only an - * in-progress placeholder OPENS with it; a finished review that happens to - * mention the phrase does not. + * THERE IS MORE THAN ONE, WHICH IS THE WHOLE TRAP. Measured live on PR #302, + * the comment is created in one state and edited into another 29 seconds later, + * then edited again on completion: + * + * 22:24:05 Claude Code is working… (created) + * 22:24:34 ### Review in progress (+29s, checkboxes appear) + * on finish **Claude finished …** + * + * Matching only the second one leaves the first half-minute after a trigger + * undetected, which is precisely when a caller polls too early. Issue #292 + * captured the middle state, so a marker derived from the issue text alone + * misses the opening one. + * + * Anchored to the START of the (trimmed) body: a review that legitimately + * discusses this behaviour, quoting a phrase mid-body the way this very fix + * does, must not read as unfinished forever. * * Leading markdown emphasis is tolerated as well as heading hashes. The - * observed placeholder is `### Review in progress`, but the same bot opens its - * FINISHED comment with bold (`**Claude finished …**`), so a bold placeholder - * is a format change away. Missing it would fail silently, straight back to - * the bug this exists to prevent, and allowing it costs nothing: a body - * opening with the phrase is not feedback whichever way it is marked up. + * observed placeholder is a heading, but the same bot opens its FINISHED + * comment with bold, so a bold placeholder is a format change away, and missing + * it would fail silently. + * + * REJECTED ALTERNATIVE, recorded so it is not re-proposed: invert this into an + * allowlist, treating any review-bot comment that does not open with a + * completion marker as unfinished. It fails safe for THIS bot, but the other + * review bots (Sentry, Cursor, Copilot, CodeQL) have no completion marker at + * all, so every comment they ever post would read as unfinished and the wait + * loop would never exit. A blocklist of measured placeholders is narrower and + * cannot stall a caller. The cost is that a NEW placeholder wording is missed, + * so when this bot changes its output, add the new opening here. + * + * `(?![A-Za-z0-9])` rather than `\b`: underscore is a word character, so a + * `\b` would not fire before the closing `__` of underscore emphasis. */ -// `(?![A-Za-z0-9])` rather than `\b`: underscore is a word character, so a -// `\b` here would not fire before the closing `__` of underscore emphasis and -// the marker would be missed. -const IN_PROGRESS_MARKER = - /^#{0,6}\s*[*_]{0,2}\s*review in progress(?![A-Za-z0-9])/i; - -/** Whether a body still opens with the review-bot in-progress placeholder. */ -const isReviewInProgress = (body) => IN_PROGRESS_MARKER.test(body.trimStart()); +const IN_PROGRESS_MARKERS = [ + /^#{0,6}\s*[*_]{0,2}\s*review in progress(?![A-Za-z0-9])/i, + /^#{0,6}\s*[*_]{0,2}\s*claude code is working(?![A-Za-z0-9])/i, +]; + +/** Whether a body still opens with one of the in-progress placeholders. */ +const isReviewInProgress = (body) => { + const opening = body.trimStart(); + return IN_PROGRESS_MARKERS.some((marker) => marker.test(opening)); +}; /** * Detect a LOGAF marker at the start of a comment body. diff --git a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs index bdf3f6d7..784aa0de 100644 --- a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs +++ b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs @@ -733,3 +733,54 @@ test("emphasis tolerance does not loosen the anchor", () => { assert.equal(isReviewInProgress(body), false, body); } }); + +/** + * CAPTURED LIVE, not transcribed from the issue. Both bodies below are the real + * comment on PR #302, read 29 seconds apart: the bot creates it in the + * "working" state and edits it into the "review in progress" state. Issue #292 + * recorded only the second, so a marker built from the issue text alone missed + * the first half-minute after a trigger, which is exactly when a caller polls + * too early. + */ +const LIVE_CREATED = + 'Claude Code is working… \n\nI\'ll analyze this and get back to you.\n\n[View job run](https://github.com/taskless/cli/actions/runs/34166525707)'; + +const LIVE_IN_PROGRESS = + '### Review in progress \n\nReview mode: incremental β€” read 0 prior review thread(s) before reviewing.\n\n- [x] Read `.prior-review.json`\n- [ ] Gather PR diff and changed files'; + +const LIVE_FINISHED = + "**Claude finished @thecodedrift's task in 7m 42s** β€”β€” [View job](https://github.com/taskless/cli/actions/runs/34149206216)\n\n---\n### Review complete"; + +test("both live placeholder states are detected, and the finished one is not", () => { + assert.equal(isReviewInProgress(LIVE_CREATED), true, "created state"); + assert.equal(isReviewInProgress(LIVE_IN_PROGRESS), true, "in-progress state"); + assert.equal(isReviewInProgress(LIVE_FINISHED), false, "finished state"); +}); + +test("a finished review quoting either placeholder is not in progress", () => { + for (const quoted of ["Review in progress", "Claude Code is working"]) { + assert.equal( + isReviewInProgress( + `${LIVE_FINISHED}\n\nThe bug was that a body reading "${quoted}" was mis-bucketed.` + ), + false, + quoted + ); + } +}); + +test("an in-progress placeholder keeps every other bucket empty", () => { + const output = build( + fakeClient({ + comments: [{ id: 1, body: LIVE_CREATED, user: { login: "claude[bot]" } }], + }), + {} + ); + assert.equal(output.summary.review_in_progress, 1); + assert.equal(output.summary.needs_attention, 0); + assert.equal( + output.summary.high + output.summary.medium + output.summary.low, + 0 + ); + assert.match(output.action_required, /still in progress/); +}); From 9ae9ca3b4b8835e72fea198babb8fe405a164974 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Mon, 7 Sep 2026 15:40:44 -0700 Subject: [PATCH 4/4] fix(iterate-pr): gate the in-progress check on the author, and bound its wait MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .agents/skills/iterate-pr/SKILL.md | 4 +- .../iterate-pr/scripts/fetch_pr_feedback.cjs | 24 ++++++-- .../scripts/fetch_pr_feedback.test.cjs | 55 +++++++++++++++++++ 3 files changed, 77 insertions(+), 6 deletions(-) diff --git a/.agents/skills/iterate-pr/SKILL.md b/.agents/skills/iterate-pr/SKILL.md index b32fac4d..aa11aaf9 100644 --- a/.agents/skills/iterate-pr/SKILL.md +++ b/.agents/skills/iterate-pr/SKILL.md @@ -392,7 +392,9 @@ If step 7 required code changes (from new feedback after CI passed), return to s ## Exit Conditions -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). +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). + +**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. 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: diff --git a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs index e3373708..7913b7fe 100755 --- a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs +++ b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.cjs @@ -160,12 +160,20 @@ const isInfoBot = (username) => * cannot stall a caller. The cost is that a NEW placeholder wording is missed, * so when this bot changes its output, add the new opening here. * - * `(?![A-Za-z0-9])` rather than `\b`: underscore is a word character, so a - * `\b` would not fire before the closing `__` of underscore emphasis. + * Emphasis is unbounded (`[*_]*`) rather than capped at two, so bold-italic + * (`***…***`) matches; a cap of two failed it, since two of the three leading + * `*` were consumed and the phrase could not then start. + * + * The trailing side consumes closing emphasis and THEN refuses a word + * character, underscore included. `\b` alone would not fire before a closing + * `__`, but a bare `(?![A-Za-z0-9])` went too far the other way and matched + * `Review in progress_notes: …`, a finished comment. Consuming `[*_]*` first + * and excluding `_` from the lookahead accepts `__…__` and rejects + * `progress_notes`. */ const IN_PROGRESS_MARKERS = [ - /^#{0,6}\s*[*_]{0,2}\s*review in progress(?![A-Za-z0-9])/i, - /^#{0,6}\s*[*_]{0,2}\s*claude code is working(?![A-Za-z0-9])/i, + /^#{0,6}\s*[*_]*\s*review in progress[*_]*(?![A-Za-z0-9_])/i, + /^#{0,6}\s*[*_]*\s*claude code is working[*_]*(?![A-Za-z0-9_])/i, ]; /** Whether a body still opens with one of the in-progress placeholders. */ @@ -257,7 +265,13 @@ const categorizeComment = (comment, body) => { * three sources. */ const bucketByAuthor = (feedback, item, comment, body, author) => { - if (isReviewInProgress(body)) { + // The author gate is load-bearing, not belt and braces. A human writing + // "Review in progress on my end, back by EOD" would otherwise be filed as an + // unfinished review, vanish from `needs_attention`, and hang the wait loop + // forever: a person's comment never gets edited into a finished form the way + // the bot's placeholder does, so the count never drops. Only a review bot has + // the lifecycle this bucket describes. + if (isReviewBot(author) && isReviewInProgress(body)) { feedback.review_in_progress.push(item); } else if (isReviewBot(author)) { item.review_bot = true; diff --git a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs index 784aa0de..6bcf4489 100644 --- a/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs +++ b/.agents/skills/iterate-pr/scripts/fetch_pr_feedback.test.cjs @@ -784,3 +784,58 @@ test("an in-progress placeholder keeps every other bucket empty", () => { ); assert.match(output.action_required, /still in progress/); }); + +// A HUMAN IS NOT A REVIEW BOT, AND THIS IS THE DANGEROUS DIRECTION. A person +// writing "Review in progress on my end" would otherwise be filed as an +// unfinished review, vanish from needs_attention, and hang the wait loop +// forever: a person's comment never gets edited into a finished form the way +// the bot's placeholder does, so the count never drops to zero. +test("a human comment opening with the phrase is feedback, not an unfinished review", () => { + const output = build( + fakeClient({ + comments: [ + { + id: 1, + // The LOGAF marker is deliberately absent: it only counts at the + // start, and the start is occupied by the phrase under test. So this + // lands in medium by content, which is the correct default. + body: "Review in progress on my end, will finish by EOD.", + user: { login: "a-human-reviewer" }, + }, + ], + }), + {} + ); + assert.equal(output.summary.review_in_progress, 0); + assert.equal(output.summary.medium, 1, "it stays actionable feedback"); + assert.equal(output.summary.needs_attention, 1); +}); + +test("the same body from the review bot IS an unfinished review", () => { + const output = build( + fakeClient({ + comments: [ + { + id: 1, + body: "Review in progress on my end, will finish by EOD.", + user: { login: "claude[bot]" }, + }, + ], + }), + {} + ); + assert.equal(output.summary.review_in_progress, 1); + assert.equal(output.summary.needs_attention, 0); +}); + +// Two regex edges, both able to reintroduce the bug. Bold-italic is one format +// drift away from the bold case already anticipated; `progress_notes` is a +// finished comment that a too-permissive boundary would freeze the loop on. +test("bold-italic emphasis is matched, and a bare underscore is not emphasis", () => { + assert.equal(isReviewInProgress("***Review in progress***"), true); + assert.equal(isReviewInProgress("___Claude Code is working___"), true); + assert.equal( + isReviewInProgress("Review in progress_notes: nothing else found"), + false + ); +});