Resolve the queued-vs-unavailable ambiguity on both surfaces (#19 + #23)

.claude/rules/safety-critical-changes.md

@@ -1,5 +1,14 @@
 ---
-paths: ["scripts/dev_session.sh", "scripts/devkit/dev_session.sh"]
+paths:
+  - "scripts/dev_session.sh"
+  - "scripts/devkit/dev_session.sh"
+  # pr_watch.py COMPUTES the merge gate (`mergeable` / the legacy `done` alias)
+  # that dev_session.sh merely re-checks. It was left out while `done` looked
+  # like a watch-loop verdict — but a change here can authorize a merge on an
+  # unreviewed PR just as directly, and the rule has to match the file where the
+  # decision is made, not only the one that acts on it.
+  - "scripts/pr_watch.py"
+  - "scripts/devkit/pr_watch.py"
 # Add your own send-path / gate / kill-path files or globs here. This rule is useful
 # only after its paths match the files that gate sends or destructive/recovery work.
 ---

config/dev-model.yaml

@@ -84,6 +84,14 @@ tracker:
 review:
   # Independent review present on this repo (Principle #5). A down bot is not a
   # review waiver; select the current runtime's substitute pass instead.
+  # Also read by pr_watch.py to identify which status checks and comment authors
+  # belong to a reviewer, so its outage/pending state can reach the merge gate.
+  # Matched case-insensitively: as a SUBSTRING of a check name (your own CI and
+  # bot config, so `Review / CodeRabbit` still matches), and as a PREFIX of a
+  # comment author (anyone may comment on a public PR, and `xcoderabbit` must
+  # not be able to speak for the reviewer). `coderabbit` covers the check
+  # `CodeRabbit` and the author `coderabbitai` either way. Keep entries specific
+  # enough not to collide with a CI job name.
   bots: [coderabbit]
   fallback_commands:
     claude: "/code-review"
@@ -102,13 +110,22 @@ review:
     - "<!-- walkthrough_start -->"
     - "actionable comments posted: 0"
     - "<!-- linear-linkback -->"
-  # Bodies signalling the reviewer could not run. Deliberately NOT noise: these
-  # surface and block `converged`, because a blocked bot is an action signal (run
-  # the configured fallback), never a silent review waiver.
+  # Text signalling the reviewer could not run — a blocked bot is an action
+  # signal (run the configured fallback), never a silent review waiver. Matched
+  # against two surfaces, which behave differently on purpose:
+  #   - a COMMENT body: deliberately not noise, so it surfaces as a new comment
+  #     and blocks `converged` until acknowledged.
+  #   - a configured bot's STATUS-CHECK description: surfaced and reported, but
+  #     blocks nothing — there is no comment to acknowledge, so blocking on it
+  #     would wedge the loop. It is the surface that carried the outage on #22
+  #     and made a rate-limited bot read as a clean review (issue #23).
+  # The same outage is worded differently on each, which is why matching only
+  # comments made detection depend on which one the bot happened to use.
   unavailable_markers:
     - "bugbot needs on-demand usage enabled"
     - "review limit reached"
     - "rate limited by coderabbit"
+    - "review rate limited"       # the status-check wording of "review limit reached"
     - "couldn't start this review"
     - "review skipped"
     - "no review credits"
@@ -117,7 +134,22 @@ review:
   # a trivial follow-up commit, and the watch loop must still be able to finish.
   # Its real findings arrive as comments (which DO block). Matched
   # case-insensitively against the check name.
+  #
+  # Note the deliberate overlap with `bots` above: SAME value, DIFFERENT purpose.
+  # This list is a blocking policy ("this check never blocks the watch loop");
+  # `bots` is an identity ("this check belongs to a reviewer whose state the
+  # merge gate cares about"). Splitting them is what lets a bot's check stay
+  # non-blocking for `converged` while its pending/unavailable state still
+  # reaches `mergeable`.
   informational_checks: [coderabbit]
+  # How long a configured review bot's own check may sit pending before the merge
+  # gate stops waiting for it. BELOW this, a pending bot blocks `mergeable`: a
+  # review is genuinely coming, and a receipt recorded now would bind to a review
+  # that has not happened (issue #19 — a receipt taken against a merely queued
+  # bot let four valid findings land post-merge). ABOVE it, the bot is treated as
+  # never going to report and stops blocking, which is what keeps a dead bot from
+  # wedging the merge gate. Never affects `converged`.
+  bot_pending_grace_minutes: 15
   # Whether a PR must have at least one real (non-informational) CI check before
   # pr_watch will report green. True is the safe default: it stops an autonomous
   # merge on a PR whose CI never ran. Set false for a repo with no CI at all —

docs/agentic-dev-kit/workflows/pr-watch.md

@@ -143,10 +143,65 @@ Self-pace on a bounded cadence — don't busy-wait:
   acknowledging one clears `converged` but still leaves the current-head
   review-evidence blocker on `mergeable` until the configured fallback runs and
   records its receipt.
+- **A bot's outage is detected on both surfaces, and a queued bot is not a finished
+  one.** `review.unavailable_markers` are matched against comment bodies *and*
+  against the status-check description of any check belonging to a configured
+  `review.bots` entry — the same rate limit is worded differently on the two
+  surfaces, and matching only comments made detection depend on which one the bot
+  happened to use. The report's `review_bots` block resolves each bot to:
+  - **unavailable** — an outage announced on either surface. Rendered as
+    `⚠ review unavailable …`, and it never blocks anything: it's the action signal
+    to run `review.fallback_commands`. It stays visible after you `--mark-seen` the
+    notice comment, so the gap is still readable at merge time. Only a
+    **check**-surface outage cancels the pending block below: a check describes
+    the bot's state now, while comments are the whole PR history unscoped by
+    head — and since rate limits are transient, an old outage comment would
+    otherwise wave through every later queued review.
+  - **pending** — the bot's own check is non-terminal and no outage was announced.
+    A verdict is genuinely coming, so this **blocks `mergeable`** (and makes
+    `--record-review` refuse) until the check ages past
+    `review.bot_pending_grace_minutes` (default 15), after which the bot is treated
+    as never going to report and stops blocking. Use
+    `--allow-pending-bot-review` only with evidence the queued verdict will never
+    arrive. CodeRabbit's pending check reports no usable timestamp
+    (`0001-01-01T00:00:00Z`), so the grace clock falls back to when *this engine*
+    first saw the bot pending — persisted per PR under `bot_pending_since`, scoped
+    to the head, and reset by a push. A stored value the engine cannot read — or
+    one dated in the future — is replaced rather than trusted, so the window stays
+    bounded whatever wrote the state file.
+
+  None of this reaches `converged`. That is deliberate and load-bearing: the watch
+  loop must be able to finish while a bot that never reports sits pending forever.
+  Every signal here feeds the merge gate only.
+
+  **Known gaps, so you don't mistake them for coverage:**
+  - The pending block only exists once the bot has *registered* a check. In the
+    window between `gh pr ready` and the bot creating its check row, "the bot
+    hasn't started yet" is indistinguishable from "this repo has no bot", so a
+    receipt recorded in that window is still premature. Give a freshly-readied PR
+    a poll or two before recording one.
+  - `review_bots.signal` is `ok` / `skipped` (no bots configured) / `unavailable`
+    (the check read failed — e.g. a `gh` too old for the requested `--json`
+    fields, or a PR with no checks at all). On `unavailable` **both guards are
+    off**; it's reported rather than blocking, because an environment problem
+    shouldn't become a wedge — and a receipt recorded while the read was
+    `unavailable` carries a `bot_signal` key so the audit trail shows the guard
+    didn't run. (`skipped` is not flagged: nothing was unreadable.)
+  - An *older* `pr_watch.py` polling the same PR drops the persisted grace clock,
+    restarting the window. Merges wait longer; nothing fails open.
 - **Tune this for your own bot mix in `config/dev-model.yaml`, never in the engine.**
-  `review.noise_markers`, `review.unavailable_markers` and
-  `review.informational_checks` are read from config; the engine only carries them
-  as fallbacks for a missing config. Editing the literals inside
+  `review.bots`, `review.noise_markers`, `review.unavailable_markers`,
+  `review.informational_checks` and `review.bot_pending_grace_minutes` are read from
+  config; the engine only carries them as fallbacks for a missing config.
+  `review.bots` and `review.informational_checks` ship with the *same* value and
+  different jobs: the latter is a blocking policy ("this check never blocks the
+  watch loop"), the former an identity ("this check belongs to a reviewer whose
+  state the merge gate cares about"). Bot names match case-insensitively: as a
+  **substring** of a check name (your own CI and bot config) and as a **prefix**
+  of a comment author (anyone may comment on a public PR, so `xcoderabbit` must
+  not be able to speak for the reviewer). `coderabbit` covers the check
+  `CodeRabbit` and the author `coderabbitai` either way — keep entries specific
+  enough not to collide with a CI job name. Editing the literals inside
   `<engine-dir>/pr_watch.py` forks the engine and turns every later kit update into
   a merge conflict. A key you omit keeps the kit default; an explicit empty list
   (`noise_markers: []`) means "filter nothing".