fix(iterate-pr): surface an unfinished review instead of bucketing it as feedback

.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.
 

.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… <img …>        (created)
+ *   22:24:34  ### Review in progress <img …>          (+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,
 };