diff --git a/.agents/skills/iterate-pr/SKILL.md b/.agents/skills/iterate-pr/SKILL.md index 097465b0..aa11aaf9 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,18 @@ 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, 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: - **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..7913b7fe 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,62 @@ const isReviewBot = (username) => const isInfoBot = (username) => INFO_BOT_PATTERNS.some((pattern) => pattern.test(username ?? "")); +/** + * The placeholders a review bot posts before it has anything to say. + * + * 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 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. + * + * 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*[*_]*\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. */ +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. * @@ -180,17 +251,29 @@ 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)) { + // 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; feedback[categorizeComment(comment, body)].push(item); } else if (isInfoBot(author)) { @@ -378,7 +461,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 +596,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 +676,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..6bcf4489 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,105 @@ 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 +680,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( @@ -559,3 +706,136 @@ 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); + } +}); + +/** + * 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/); +}); + +// 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 + ); +});