From 233fdd0462991850d520266c47fc57ab1f34cfc5 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 15:23:09 +0300 Subject: [PATCH 1/9] Resolve queued-vs-unavailable on both surfaces (#19 + #23) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two issues, one predicate. A configured review bot's status check is excluded from the blocking tally so a bot that never reports cannot wedge the watch loop — but that exclusion made an *unavailable* bot indistinguishable from one that reviewed and found nothing (#23), and a merely *queued* bot indistinguishable from a finished one (#19). Neither could be fixed by letting that check block, because the exclusion is the anti-wedge property. The way out is that nothing here touches `decide_converged`. The anti-wedge property lives entirely in that predicate and is left alone; every new signal feeds the merge gate, which already requires someone to explicitly record a review receipt. So the gate can wait longer, but the poll/fix/ack loop can always finish. `summarize_review_bots` resolves each `review.bots` entry to: - unavailable — an `unavailable_markers` hit on a comment body OR on the bot's status-check DESCRIPTION (#23; `gh pr view --json statusCheckRollup` carries no description at all, hence the second `gh pr checks` fetch). Never blocks; it is the action signal to run the fallback, and it survives `--mark-seen` so the gap stays readable at merge time. It also cancels the pending block below — a bot that announced it is not reviewing will never leave pending. - pending — the bot's check is non-terminal and no outage was announced. Blocks `mergeable` and makes `--record-review` refuse (#19: on #16 a receipt was taken against a queued CodeRabbit and four valid findings landed post-merge), until it ages past `review.bot_pending_grace_minutes` (default 15) — the bound that keeps a dead bot from wedging the gate. Also here because the fix does not work without them: - `"review rate limited"` added to `unavailable_markers`. The mechanism alone detects nothing: on #22 the check description used that wording while #24's comment said "review limit reached", an hour apart from the same bot. - kitconfig now parses decimal scalars, gated on a literal `.` so `nan`/`inf`/ `1e5` stay strings as PyYAML resolves them. Without it a `2.5` grace silently fell back to the default. - `_load_review_config` returns a NamedTuple; the shape grew, and positional unpacking makes every future growth a breaking change for every reader. - `safety-critical-changes.md` now matches `pr_watch.py`. It computes the gate `dev_session.sh` merely re-checks, and was matching only the latter. Blockers are strictly additive: `done` (the legacy alias an older `dev_session.sh` reads) can only go true→false, so per-file engine skew makes merges wait, never fire early. Tests 204 → 217. Closes #19 Closes #23 Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --- .claude/rules/safety-critical-changes.md | 11 +- config/dev-model.yaml | 28 +- docs/agentic-dev-kit/workflows/pr-watch.md | 34 +- init.sh | 52 +++ kit-manifest.json | 6 +- scripts/lib/kitconfig.py | 12 +- scripts/pr_watch.py | 479 ++++++++++++++++++-- scripts/tests/test_kitconfig.py | 30 ++ scripts/tests/test_pr_watch.py | 486 ++++++++++++++++++++- 9 files changed, 1083 insertions(+), 55 deletions(-) diff --git a/.claude/rules/safety-critical-changes.md b/.claude/rules/safety-critical-changes.md index 7e90a78..fffbf52 100644 --- a/.claude/rules/safety-critical-changes.md +++ b/.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. --- diff --git a/config/dev-model.yaml b/config/dev-model.yaml index cce3f64..8cea474 100644 --- a/config/dev-model.yaml +++ b/config/dev-model.yaml @@ -84,6 +84,11 @@ 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 as a case-insensitive SUBSTRING of a check name or comment author — + # `coderabbit` covers the check `CodeRabbit` and the author `coderabbitai` — + # so keep entries specific enough not to collide with a CI job name. bots: [coderabbit] fallback_commands: claude: "/code-review" @@ -102,13 +107,17 @@ review: - "" - "actionable comments posted: 0" - "" - # Bodies signalling the reviewer could not run. Deliberately NOT noise: these + # Text 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. + # the configured fallback), never a silent review waiver. Matched against both + # comment bodies AND a configured bot's status-check description — the same + # outage is worded differently on the two surfaces, and matching only comments + # made detection depend on which one the bot happened to use (issue #23). 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 +126,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 — diff --git a/docs/agentic-dev-kit/workflows/pr-watch.md b/docs/agentic-dev-kit/workflows/pr-watch.md index 6409a85..b3f6cf9 100644 --- a/docs/agentic-dev-kit/workflows/pr-watch.md +++ b/docs/agentic-dev-kit/workflows/pr-watch.md @@ -143,10 +143,38 @@ 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. + - **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. + + 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. - **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 as a case-insensitive + substring of a check name or comment author, so `coderabbit` covers the check + `CodeRabbit` and the author `coderabbitai` — keep entries specific enough not to + collide with a CI job name. Editing the literals inside `/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". diff --git a/init.sh b/init.sh index 7aee470..58c9fee 100755 --- a/init.sh +++ b/init.sh @@ -292,6 +292,7 @@ migrate_kit_schema() { - "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" @@ -300,6 +301,57 @@ migrate_kit_schema() { require_ci: true' echo "added review marker/CI config to config/dev-model.yaml" fi + + # A config migrated BEFORE this key existed already has noise_markers, so the + # block above is skipped and this needs its own guard. Two separate additions, + # two separate guards — a single guard would silently skip whichever key the + # adopter's config happens not to have. + if ! grep -q '^ bot_pending_grace_minutes:' "$CONFIG_FILE"; then + append_to_section "review:" ' # 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 + # receipt recorded now would bind to a review that has not happened); above it, + # the bot is treated as never going to report, so a dead bot cannot wedge the + # gate. Never affects `converged`. + bot_pending_grace_minutes: 15' + echo "added review.bot_pending_grace_minutes to config/dev-model.yaml" + fi + + # The status-check wording of the same rate-limit outage. Added separately for + # the same reason: an adopter migrated before it existed keeps their list. + # Anchored on the FIRST entry under unavailable_markers rather than on a + # specific marker text, which an adopter is free to have edited away. If the + # anchor is still missed, SAY SO — a migration that silently no-ops is + # indistinguishable from one that ran, and that class of failure has already + # cost this repo three bugs in one session. + if grep -q '^ unavailable_markers:' "$CONFIG_FILE" \ + && ! grep -qi 'review rate limited' "$CONFIG_FILE"; then + awk ' + inserted == 0 && in_list == 1 && $0 !~ /^ - / { + print " - \"review rate limited\" # status-check wording of \"review limit reached\"" + inserted = 1 + in_list = 0 + } + /^ unavailable_markers:/ { in_list = 1 } + { print } + END { + if (inserted == 0 && in_list == 1) + print " - \"review rate limited\" # status-check wording of \"review limit reached\"" + } + ' "$CONFIG_FILE" > "$CONFIG_FILE.tmp" + # Post-condition, not a trusted exit code: verify the marker actually landed + # before overwriting the adopter's config, and keep the original if it did + # not. A migration that silently no-ops is indistinguishable from one that + # ran — that class of failure has already cost this repo three bugs. + if grep -qi 'review rate limited' "$CONFIG_FILE.tmp"; then + mv "$CONFIG_FILE.tmp" "$CONFIG_FILE" + echo "added the status-check rate-limit marker to config/dev-model.yaml" + else + rm -f "$CONFIG_FILE.tmp" + echo "WARNING: could not add the \"review rate limited\" marker to review.unavailable_markers" >&2 + echo " in $CONFIG_FILE — add it by hand, or a rate-limited review bot that reports" >&2 + echo " the outage only as a status-check description will read as a clean review." >&2 + fi + fi } # ── narrative-doc templates ────────────────────────────────────────────── diff --git a/kit-manifest.json b/kit-manifest.json index 97e3592..d01f639 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -23,7 +23,7 @@ }, "docs/agentic-dev-kit/workflows/pr-watch.md": { "role": "workflow", - "sha256": "0b187c8ea7d725872215b4d4a5e56929814011228f7979adf1aa8423652048c8" + "sha256": "e916c72f133b0ca0ff302931878a23ca74d7069d44716aed9a120d3e1a9faae9" }, "docs/agentic-dev-kit/workflows/session-start.md": { "role": "workflow", @@ -75,7 +75,7 @@ }, "scripts/lib/kitconfig.py": { "role": "engine", - "sha256": "41c01a2956185d898f6cb75865d713845630a2d8335976f1460de1ed5f875317" + "sha256": "1ef950fcff6167fd2048e13c02e2d1e2f97cbf4d8e0e837cd41e4d4305b3168c" }, "scripts/lib/repo_root.sh": { "role": "engine", @@ -99,7 +99,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "735428aa7a06141da7b3d385324a350e7a722fd2c2c47d0e5f97047fda41df37" + "sha256": "50e1b53077c765feb990d4cf3968c86454fc9908622d245885a8155ccf8f65c1" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/lib/kitconfig.py b/scripts/lib/kitconfig.py index caf2d9a..28fed14 100644 --- a/scripts/lib/kitconfig.py +++ b/scripts/lib/kitconfig.py @@ -97,7 +97,17 @@ def _coerce(raw: str) -> Any: try: return int(text) except ValueError: - return text + pass + # Floats, gated on an explicit decimal point. `float()` alone would accept + # `nan`, `inf` and `1e5`, none of which PyYAML's resolver treats as numbers — + # widening past the point would trade one divergence from the parity + # invariant for three others. A dot is what YAML 1.1 requires too. + if "." in text: + try: + return float(text) + except ValueError: + return text + return text def _parse_flow_list(text: str) -> list[Any]: diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index 037d245..055edf8 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -23,6 +23,13 @@ lets a caller watch to convergence without being forced to record a review receipt just to terminate the loop (see `decide_converged`). +It also resolves each configured review bot (`review.bots`) to *unavailable* or +*pending* (`summarize_review_bots`). A bot's own status check is excluded from +the blocking tally so a bot that never reports cannot wedge the loop — but that +exclusion used to make an outage indistinguishable from a clean review, and a +merely-queued bot indistinguishable from a finished one. Both signals now feed +the MERGE GATE only, never `converged`, so the anti-wedge property is untouched. + `done` is a LEGACY alias, always equal to `mergeable`. Its meaning is unchanged and must stay that way: engine upgrades are per-file, so a new `pr_watch.py` can run against an older `dev_session.sh` that gates merges on `done` — repurposing @@ -83,6 +90,7 @@ import sys import time from pathlib import Path +from typing import NamedTuple def _find_repo_root(start: Path) -> Path: @@ -210,6 +218,12 @@ def _resolve_state_root(start: Path, repo_root: Path, state_root_env: str | None "couldn't start this review", "review skipped", "no review credits", + # The status-check phrasing of the same outage. Same bot, same rate limit, an + # hour apart on #22 vs #24, but a different wording on a different surface — + # the comment said "review limit reached", the check description said "Review + # rate limited". Matching only the comment wording is what made detection + # depend on which surface the bot happened to use (#23). + "review rate limited", ) # Status contexts that are advisory only — they must NEVER block "done". A @@ -220,6 +234,26 @@ def _resolve_state_root(start: Path, repo_root: Path, state_root_env: str | None # new_comments). Matched case-insensitively against the check name/context. _DEFAULT_INFORMATIONAL_CHECK_NAMES = ("coderabbit",) +# The configured independent review bots (`review.bots`). Distinct in PURPOSE +# from `_DEFAULT_INFORMATIONAL_CHECK_NAMES` even though the shipped values +# coincide: that list is a *blocking policy* ("this check never blocks"), this +# one is an *identity* ("these checks belong to a reviewer whose state we care +# about"). Keeping them separate is what lets a bot's check stay non-blocking +# for `converged` while its state still informs the merge gate — the exact +# split issues #19 and #23 need. Matched as a case-insensitive SUBSTRING of a +# check name or comment author, so `coderabbit` covers the check `CodeRabbit` +# and the author `coderabbitai`; keep entries specific enough not to collide +# with a CI job name. +_DEFAULT_REVIEW_BOTS = ("coderabbit",) + +# How long a configured review bot's own check may sit non-terminal before the +# merge gate stops waiting for it. Below the bound, a pending bot is "a review +# is coming" and blocks `mergeable` (issue #19 — a receipt recorded against a +# merely *slow* bot let four post-merge findings through). Above it, the bot is +# treated as never going to report and stops blocking — which is what preserves +# the anti-wedge property that `_DEFAULT_INFORMATIONAL_CHECK_NAMES` exists for. +_DEFAULT_BOT_PENDING_GRACE_MINUTES = 15.0 + # Whether a PR must carry at least one real (non-informational) check before it # can read as green. True is the safe default — see :func:`summarize_checks`. _DEFAULT_REQUIRE_CI = True @@ -230,18 +264,33 @@ def _resolve_state_root(start: Path, repo_root: Path, state_root_env: str | None sys.path.insert(0, str(Path(__file__).resolve().parent / "lib")) -def _load_review_config( - config_path: str | Path | None = None, -) -> tuple[tuple[str, ...], tuple[str, ...], frozenset[str], bool]: +class ReviewConfig(NamedTuple): + """The resolved ``review.*`` knobs. + + A NamedTuple rather than a bare tuple because this shape GREW — it carried + four fields until #19/#23 needed the reviewer's identity and a pending + bound. Positional unpacking makes every future addition a breaking change + for every reader; named fields make it additive, which is the same property + the report schema is held to. + """ + + noise_markers: tuple[str, ...] + unavailable_markers: tuple[str, ...] + informational_checks: frozenset[str] + require_ci: bool + bots: tuple[str, ...] + bot_pending_grace_minutes: float + + +def _load_review_config(config_path: str | Path | None = None) -> ReviewConfig: """Resolve the ``review.*`` knobs from config, falling back to the defaults. ``config_path`` overrides the default ``config/dev-model.yaml`` lookup (used by the tests to exercise a real adopter config without touching this repo's). - Returns ``(noise_markers, unavailable_markers, informational_checks, - require_ci)``. Marker strings are lower-cased here because every call site - matches them case-insensitively against a lower-cased body/name — so a - config author may write them in any case. + Returns a :class:`ReviewConfig`. Marker strings are lower-cased here because + every call site matches them case-insensitively against a lower-cased + body/name — so a config author may write them in any case. **Never raises.** A missing config file, an absent ``scripts/lib/``, a parse failure, a wrong-typed value: all fall back to the in-module defaults, @@ -259,12 +308,19 @@ def _load_review_config( - ``require_ci`` accepts only a real boolean; anything else (a stray ``yes``, a typo) keeps the default. The unsafe direction here is reading as *False* by accident, which would let a zero-check PR report green. + - ``bot_pending_grace_minutes`` accepts a non-negative number (``bool`` is + rejected despite being an ``int`` subclass). The unsafe direction is a + huge value, which would let a dead bot block the merge gate for hours — + but that direction fails *closed* (merges wait), so a typo is annoying + rather than dangerous, and the default applies to anything non-numeric. """ - defaults = ( - _DEFAULT_NOISE_MARKERS, - _DEFAULT_REVIEW_UNAVAILABLE_MARKERS, - frozenset(_DEFAULT_INFORMATIONAL_CHECK_NAMES), - _DEFAULT_REQUIRE_CI, + defaults = ReviewConfig( + noise_markers=_DEFAULT_NOISE_MARKERS, + unavailable_markers=_DEFAULT_REVIEW_UNAVAILABLE_MARKERS, + informational_checks=frozenset(_DEFAULT_INFORMATIONAL_CHECK_NAMES), + require_ci=_DEFAULT_REQUIRE_CI, + bots=_DEFAULT_REVIEW_BOTS, + bot_pending_grace_minutes=_DEFAULT_BOT_PENDING_GRACE_MINUTES, ) try: from kitconfig import get, get_str_list, load_config @@ -281,9 +337,17 @@ def _load_review_config( "review.informational_checks", list(_DEFAULT_INFORMATIONAL_CHECK_NAMES), ) + bots = get_str_list(config, "review.bots", list(_DEFAULT_REVIEW_BOTS)) require_ci = get(config, "review.require_ci", _DEFAULT_REQUIRE_CI) if not isinstance(require_ci, bool): require_ci = _DEFAULT_REQUIRE_CI + grace = get( + config, + "review.bot_pending_grace_minutes", + _DEFAULT_BOT_PENDING_GRACE_MINUTES, + ) + if isinstance(grace, bool) or not isinstance(grace, (int, float)) or grace < 0: + grace = _DEFAULT_BOT_PENDING_GRACE_MINUTES except FileNotFoundError: # `load_config` raises this for an absent config file — a standalone # engine run. Defaults are exactly right; stay quiet. @@ -299,20 +363,25 @@ def _load_review_config( file=sys.stderr, ) return defaults - return ( - tuple(marker.lower() for marker in noise), - tuple(marker.lower() for marker in unavailable), - frozenset(name.strip().lower() for name in informational if name.strip()), - require_ci, + return ReviewConfig( + noise_markers=tuple(marker.lower() for marker in noise), + unavailable_markers=tuple(marker.lower() for marker in unavailable), + informational_checks=frozenset( + name.strip().lower() for name in informational if name.strip() + ), + require_ci=require_ci, + bots=tuple(bot.strip().lower() for bot in bots if bot.strip()), + bot_pending_grace_minutes=float(grace), ) -( - _NOISE_MARKERS, - _REVIEW_UNAVAILABLE_MARKERS, - _INFORMATIONAL_CHECK_NAMES, - _REQUIRE_CI, -) = _load_review_config() +_REVIEW_CONFIG = _load_review_config() +_NOISE_MARKERS = _REVIEW_CONFIG.noise_markers +_REVIEW_UNAVAILABLE_MARKERS = _REVIEW_CONFIG.unavailable_markers +_INFORMATIONAL_CHECK_NAMES = _REVIEW_CONFIG.informational_checks +_REQUIRE_CI = _REVIEW_CONFIG.require_ci +_REVIEW_BOTS = _REVIEW_CONFIG.bots +_BOT_PENDING_GRACE_MINUTES = _REVIEW_CONFIG.bot_pending_grace_minutes # --------------------------------------------------------------------------- gh @@ -344,6 +413,60 @@ def _gh_json(args: list[str]): return json.loads(_gh(args)) +def fetch_check_details(pr: int, *, bots: tuple[str, ...] | None = None) -> list[dict]: + """Per-check ``{name, state, bucket, description, startedAt}`` for one PR. + + A SECOND ``gh`` call, and deliberately so. ``gh pr view --json + statusCheckRollup`` — the source :func:`summarize_checks` reads — returns a + fixed sub-shape with **no description field** on either a ``CheckRun`` or a + ``StatusContext``. That omission is the whole of issue #23: CodeRabbit + reported its rate limit *only* as a check description ("Review rate + limited") on an otherwise-``SUCCESS`` context, so nothing pr_watch could see + carried the outage. ``gh pr checks --json`` normalizes both check kinds and + does expose ``description``. + + The two sources are kept in their own lanes on purpose: this one feeds ONLY + :func:`summarize_review_bots` (reviewer identity + state), never the + blocking tally. Letting a second fetch influence ``all_green`` would mean two + views of CI that can disagree between calls. + + **Never raises, and never blocks the loop.** ``gh pr checks`` exits non-zero + for perfectly normal states (8 = some check pending, 1 = some check failing) + and errors outright on a PR with no checks at all, so the exit code is + ignored and only parseable JSON on stdout is used. Any failure degrades to + ``[]`` — which reads as "no bot signal", the same fail-open direction the + informational-check exclusion already takes. + + Skips the call entirely when no review bots are configured: with nothing to + match, the result could only ever be discarded. + """ + if bots is None: + bots = _REVIEW_BOTS + if not bots: + return [] + cmd = [ + "gh", + "pr", + "checks", + str(pr), + "--json", + "name,state,bucket,description,startedAt", + ] + try: + result = subprocess.run( # noqa: S603 + cmd, + capture_output=True, + text=True, + check=False, + cwd=str(REPO_ROOT), + timeout=60, + ) + parsed = json.loads(result.stdout) + except (OSError, subprocess.SubprocessError, json.JSONDecodeError): + return [] + return [item for item in parsed if isinstance(item, dict)] if isinstance(parsed, list) else [] + + def resolve_pr(explicit: int | None) -> int: """Return the PR number — explicit, or the current branch's open PR.""" if explicit is not None: @@ -602,6 +725,201 @@ def new_actionable(comments: list[dict], seen: set[str]) -> list[dict]: ] +def _match_bot(text: str, bots: tuple[str, ...]) -> str | None: + """The configured review bot whose name appears in ``text``, if any. + + Substring, case-insensitive: one bot key (``coderabbit``) has to cover both + the check name GitHub shows (``CodeRabbit``) and the comment author + (``coderabbitai``), which no exact match spans. + """ + low = (text or "").strip().lower() + if not low: + return None + return next((bot for bot in bots if bot in low), None) + + +# Check states that mean the reviewer has finished — anything else (PENDING, +# QUEUED, IN_PROGRESS, EXPECTED, "") means a verdict may still be coming. +_TERMINAL_CHECK_STATES = { + "SUCCESS", + "NEUTRAL", + "SKIPPED", + "FAILURE", + "ERROR", + "CANCELLED", + "TIMED_OUT", + "ACTION_REQUIRED", + "STARTUP_FAILURE", +} + + +def _check_is_pending(detail: dict) -> bool: + """Whether one ``gh pr checks`` row is still awaiting a verdict. + + Prefers gh's own ``bucket`` (its normalization across ``CheckRun`` and + ``StatusContext``); falls back to the raw state when a gh version omits it. + """ + bucket = (detail.get("bucket") or "").strip().lower() + if bucket: + return bucket == "pending" + return (detail.get("state") or "").strip().upper() not in _TERMINAL_CHECK_STATES + + +def _age_minutes(timestamp: str | None, now: datetime) -> float | None: + """Minutes between ``timestamp`` (ISO 8601) and ``now``; ``None`` if unusable. + + GitHub stamps an unstarted check with the zero time (``0001-01-01T…``), which + parses fine but means "no time recorded" — treated as unknown, not as an age + of two millennia. + """ + if not timestamp: + return None + try: + parsed = datetime.fromisoformat(str(timestamp).replace("Z", "+00:00")) + except ValueError: + return None + if parsed.tzinfo is None: + parsed = parsed.replace(tzinfo=timezone.utc) + if parsed.year < 2000: + return None + return max(0.0, (now - parsed).total_seconds() / 60.0) + + +def summarize_review_bots( + check_details: list[dict], + comments: list[dict], + *, + now: datetime, + bots: tuple[str, ...] | None = None, + grace_minutes: float | None = None, +) -> dict: + """Resolve each configured review bot to *unavailable*, *pending*, or neither. + + This is the single predicate issues #19 and #23 both needed. They are two + branches of one question — "has the independent reviewer had its say?" — and + the reason neither could be fixed alone is that the obvious local fix for + each ("let the bot's check block") is the one move the design forbids: that + check is excluded from the blocking tally precisely so a bot which never + reports cannot wedge the watch loop forever. + + The way out is that **nothing here touches** :func:`decide_converged`. The + anti-wedge property lives entirely in that predicate, and it is left alone. + Everything below feeds only the merge gate, which already requires someone to + explicitly record a review receipt — so tightening it can delay a merge but + can never stall the poll/fix/ack loop. + + The three outcomes: + + - **unavailable** — an ``unavailable_markers`` hit on either surface: a + comment body (already detected today) *or* a bot check's description (#23, + the surface that was invisible). Never blocks anything. It is an action + signal: run the configured fallback review. It also SUPPRESSES the pending + block below — a bot that announced it is not reviewing is not "about to + report", so waiting on it would be waiting forever. + - **pending** — the bot's own check is non-terminal and no unavailability was + announced. A verdict is genuinely coming, so a receipt recorded now would + be premature (#19: exactly this, on #16, let four valid findings land after + the merge). Blocks the merge gate — but only while the check is younger + than ``grace_minutes``, so a permanently-stuck check ages out instead of + wedging. + - **neither** — terminal and unmarked: the bot reviewed. Nothing to add. + + An unknown check age (no usable timestamp) fails **open**: it does not block, + and it is reported as ``age_unknown`` so the render can say so out loud. The + alternative — blocking on an age we cannot measure — is indistinguishable + from a wedge, which is the one failure this whole design is built to avoid. + + Returns ``{grace_minutes, unavailable, pending, blockers}``. ``blockers`` are + ready-made ``merge_blockers`` strings. + """ + if bots is None: + bots = _REVIEW_BOTS + if grace_minutes is None: + grace_minutes = _BOT_PENDING_GRACE_MINUTES + + unavailable: list[dict] = [] + unavailable_bots: set[str] = set() + + # Surface 1 — comment bodies. Already detected today, but only ever per + # comment: once acked it vanished from `new_comments` and with it the fact + # that the primary reviewer never ran. Aggregating it here keeps the gap + # visible at merge time. + for comment in comments or []: + reason = comment.get("review_unavailable_reason") + if not reason: + continue + author = comment.get("author") or "?" + bot = _match_bot(author, bots) + unavailable.append( + { + "bot": bot, + "surface": "comment", + "where": f"@{author}", + "reason": reason, + } + ) + if bot: + unavailable_bots.add(bot) + + # Surface 2 — the bot's own check description. Issue #23: on PR #22 this was + # the ONLY place the rate limit appeared, and it read as a clean review. + pending: list[dict] = [] + for detail in check_details or []: + name = detail.get("name") or detail.get("context") or "" + bot = _match_bot(name, bots) + if not bot: + continue + reason = review_unavailable_reason(detail.get("description") or "") + if reason: + unavailable.append( + { + "bot": bot, + "surface": "check", + "where": name, + "reason": reason, + } + ) + unavailable_bots.add(bot) + continue + if not _check_is_pending(detail): + continue + age = _age_minutes(detail.get("startedAt"), now) + pending.append( + { + "bot": bot, + "check": name, + "state": (detail.get("state") or "").upper(), + "since": detail.get("startedAt"), + "age_minutes": None if age is None else round(age, 1), + "age_unknown": age is None, + # Filled in below, once every surface has been read: an + # unavailability announced in a *comment* must be able to cancel + # a pending *check*, and the comment loop already ran. + "blocking": False, + } + ) + + blockers: list[str] = [] + for entry in pending: + if entry["bot"] in unavailable_bots: + continue # announced as not reviewing — waiting on it is a wedge + if entry["age_unknown"] or entry["age_minutes"] >= grace_minutes: + continue # unmeasurable or aged out — fail open, never wedge + entry["blocking"] = True + blockers.append( + f"review bot {entry['bot']} has not reported yet " + f"(check {entry['check']} pending {entry['age_minutes']}m " + f"< {grace_minutes:g}m grace)" + ) + + return { + "grace_minutes": grace_minutes, + "unavailable": unavailable, + "pending": pending, + "blockers": blockers, + } + + def decide_converged( checks: dict, new_items: list[dict], @@ -764,13 +1082,35 @@ def mark_seen(pr: int) -> dict: return {"pr": pr, "marked_seen": True, "marked_seen_keys": sorted(pending)} -def record_review(pr: int, source: str, expected_head: str) -> dict: +def record_review( + pr: int, + source: str, + expected_head: str, + *, + allow_pending_bot: bool = False, + now: datetime | None = None, +) -> dict: """Persist independent-review evidence bound to the PR's current head SHA. The caller runs this only after an independent reviewer (the configured bot, or the configured fallback when that bot is unavailable) has completed and every finding has been handled. A later push changes ``headRefOid`` and automatically invalidates the receipt. + + **Refuses while a configured review bot's own verdict is still coming.** This + is the moment issue #19's failure happened: on #16 a fallback receipt was + recorded while CodeRabbit's check read ``PENDING — Review queued``, the merge + fired, and its four valid findings landed minutes later. Every input was + correct at that instant; the missing one was the reviewer's own state. The + judgment the doctrine asks for — *is this bot unavailable, or merely slow?* — + is made right here, so this is where it gets mechanized. + + It refuses only for a bot that is genuinely mid-review: an announced outage + (either surface) means "not coming" and is allowed straight through to the + fallback, and a check pending past the grace window ages out of blocking. See + :func:`summarize_review_bots`. ``allow_pending_bot`` is the operator's + documented override for the remaining case — evidence the queued review will + never arrive that pr_watch cannot see. """ source = source.strip() if not source: @@ -778,7 +1118,9 @@ def record_review(pr: int, source: str, expected_head: str) -> dict: expected_head = expected_head.strip() if not expected_head: raise ValueError("expected reviewed head must not be empty") - snapshot = _gh_json(["pr", "view", str(pr), "--json", "number,headRefOid"]) + snapshot = _gh_json( + ["pr", "view", str(pr), "--json", "number,headRefOid,comments,reviews"] + ) current_head = snapshot.get("headRefOid") if not current_head: raise ValueError("PR has no headRefOid; cannot bind review evidence") @@ -787,10 +1129,28 @@ def record_review(pr: int, source: str, expected_head: str) -> dict: f"PR head changed during review (expected {expected_head}, current {current_head}); " "review the new head before recording evidence" ) + now = now or datetime.now(timezone.utc) + if not allow_pending_bot: + # Comments are fetched alongside the head so an outage announced in a + # COMMENT can still cancel a pending CHECK. Without them, the documented + # fallback path would be refused in exactly the case it exists for: a + # bot that says "rate limited" while its check never leaves pending. + bot_status = summarize_review_bots( + fetch_check_details(pr), + collect_comments(snapshot, []), + now=now, + ) + if bot_status["blockers"]: + raise ValueError( + "; ".join(bot_status["blockers"]) + + ". A receipt recorded now would bind to a review that has not happened " + "— wait for the bot, or pass --allow-pending-bot-review if you have " + "evidence its verdict will never arrive" + ) receipt = { "head": expected_head, "source": source, - "recorded_at": datetime.now(timezone.utc).isoformat(), + "recorded_at": now.isoformat(), } state = load_state(pr) state["review_receipt"] = receipt @@ -814,6 +1174,8 @@ def build_report( prior_head: str | None = None, prior_max_total: int = 0, review_receipt: dict | None = None, + check_details: list[dict] | None = None, + now: datetime | None = None, ) -> dict: """Assemble the JSON-serializable watch report for one PR snapshot. @@ -839,9 +1201,15 @@ def build_report( under a new id stays handled. - ``review_evidence`` — whether a persisted independent-review receipt is bound to this exact head SHA. + - ``review_bots`` — :func:`summarize_review_bots`: each configured review + bot resolved to *unavailable* (an outage announced on either the comment + or the check-description surface — an action signal, never a blocker) or + *pending* (a verdict still coming, which blocks the merge gate until it + ages past the grace window). Advisory to ``converged`` by construction. - ``merge_blockers`` — deterministic reasons the PR is not currently safe to merge (draft, blocked/unknown merge state, requested changes, non-open PR, - or missing current-head review evidence). + missing current-head review evidence, or a configured review bot whose own + verdict has not landed yet). - ``converged`` — :func:`decide_converged`: all checks green, no fresh comments, and not ``settling``. The **watch-loop** predicate: "is there more to fix?" A converged PR is NOT necessarily safe to merge. @@ -855,6 +1223,11 @@ def build_report( checks = summarize_checks(view.get("statusCheckRollup") or []) comments = collect_comments(view, inline) fresh = new_actionable(comments, seen) + review_bots = summarize_review_bots( + check_details or [], + comments, + now=now or datetime.now(timezone.utc), + ) # False-settle guard: right after a push, `gh` can still report the OLD # commit's all-green rollup before the new commit's checks register — so a @@ -903,6 +1276,10 @@ def build_report( merge_blockers.append("review decision is CHANGES_REQUESTED") if not review_evidence["valid"]: merge_blockers.append("independent review evidence is missing for current head") + # Additive to the merge gate only. `done` is an alias of `mergeable`, so + # these tighten `done` too — the safe skew direction: an older + # `dev_session.sh` reading `done` merges LESS, never more. + merge_blockers.extend(review_bots["blockers"]) report = { "pr": view.get("number"), @@ -913,6 +1290,7 @@ def build_report( "merge_state": merge_state, "review_decision": review_decision, "review_evidence": review_evidence, + "review_bots": review_bots, "merge_blockers": merge_blockers, "head": head, "head_changed": head_changed, @@ -976,6 +1354,31 @@ def render(report: dict) -> str: ) for f in ck["failing"]: lines.append(f" ✗ {f['name']} ({f['status']})") + # The reviewer-outage action signal, hoisted out of the per-comment loop + # below. It has to survive `--mark-seen`: acking the notice comment used to + # be the last time anyone saw that the primary reviewer never ran, and the + # check-description surface never produced a comment to ack in the first + # place (#23). Printed even when the PR is otherwise clean — a clean report + # that hides an outage is the exact failure being fixed. + bots = report.get("review_bots") or {} + for entry in bots.get("unavailable") or []: + lines.append( + f" ⚠ review unavailable [{entry['surface']}] {entry['where']}: " + f"{entry['reason']} — run the configured fallback review" + ) + for entry in bots.get("pending") or []: + if entry.get("age_unknown"): + lines.append( + f" ⚠ review bot {entry['bot']} check {entry['check']} is pending with no " + "usable timestamp — age unmeasurable, so it is NOT blocking the merge gate; " + "confirm the review landed before recording a receipt" + ) + elif not entry.get("blocking"): + lines.append( + f" ⚠ review bot {entry['bot']} check {entry['check']} still pending after " + f"{entry['age_minutes']}m (past the {bots.get('grace_minutes')}m grace) — " + "treated as not coming; run the configured fallback review" + ) for blocker in report.get("merge_blockers") or []: lines.append(f" ✗ merge blocker: {blocker}") if report["new_comments"]: @@ -1062,6 +1465,15 @@ def main(argv: list[str] | None = None) -> int: metavar="EXPECTED_SHA", help="exact head SHA reviewed; required with --record-review", ) + parser.add_argument( + "--allow-pending-bot-review", + action="store_true", + help=( + "with --record-review: record the receipt even though a configured review " + "bot's own check is still pending. Only when you have evidence its verdict " + "will never arrive — a queued bot is SLOW, not unavailable (issue #19)" + ), + ) mode_group = parser.add_mutually_exclusive_group() mode_group.add_argument( "--mark-seen", @@ -1100,6 +1512,8 @@ def main(argv: list[str] | None = None) -> int: parser.error("--record-review requires --head ") if args.head and args.record_review is None: parser.error("--head is only valid with --record-review") + if args.allow_pending_bot_review and args.record_review is None: + parser.error("--allow-pending-bot-review is only valid with --record-review") try: pr = resolve_pr(args.pr) @@ -1120,7 +1534,12 @@ def main(argv: list[str] | None = None) -> int: if args.record_review is not None: try: - review_report = record_review(pr, args.record_review, args.head) + review_report = record_review( + pr, + args.record_review, + args.head, + allow_pending_bot=args.allow_pending_bot_review, + ) except (RuntimeError, KeyError, ValueError) as exc: print(f"error: {exc}", file=sys.stderr) return 2 @@ -1158,6 +1577,9 @@ def main(argv: list[str] | None = None) -> int: except (RuntimeError, KeyError, ValueError) as exc: print(f"error: {exc}", file=sys.stderr) return 2 + # Deliberately outside the try: this call never raises and never blocks the + # loop — see :func:`fetch_check_details`. + check_details = fetch_check_details(pr) state = load_state(pr) seen = set(state.get("seen", [])) @@ -1168,6 +1590,7 @@ def main(argv: list[str] | None = None) -> int: prior_head=state.get("head"), prior_max_total=int(state.get("max_total") or 0), review_receipt=state.get("review_receipt"), + check_details=check_details, ) persist_poll(pr, report, seen) diff --git a/scripts/tests/test_kitconfig.py b/scripts/tests/test_kitconfig.py index fe0b9c6..3607b9b 100644 --- a/scripts/tests/test_kitconfig.py +++ b/scripts/tests/test_kitconfig.py @@ -64,6 +64,36 @@ def test_nested_mappings_and_scalar_types(): assert parsed["top"]["nested"]["deeper"]["leaf"] == "found" +def test_decimal_scalars_parse_as_floats_without_widening_past_pyyaml(): + """A dotted number is a float; the things `float()` accepts and YAML doesn't + stay strings. + + The gate matters more than the feature: `float()` alone would swallow `nan`, + `inf` and `1e5`, which PyYAML resolves as strings — so an unguarded version + would fix one divergence from the parity invariant and open three. + """ + parsed = kitconfig.loads( + """ +review: + grace: 2.5 + whole: 15 + negative: -0.5 + exponent: 1.0e+3 + not_a_number: nan + bare_exponent: 1e5 + version_ish: 1.2.3 +""" + ) + assert parsed["review"]["grace"] == 2.5 + assert parsed["review"]["whole"] == 15 + assert isinstance(parsed["review"]["whole"], int) + assert parsed["review"]["negative"] == -0.5 + assert parsed["review"]["exponent"] == 1000.0 + assert parsed["review"]["not_a_number"] == "nan" + assert parsed["review"]["bare_exponent"] == "1e5" + assert parsed["review"]["version_ish"] == "1.2.3" + + def test_inline_and_block_lists(): parsed = kitconfig.loads( """ diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index 534104d..9bee9f2 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -1,6 +1,7 @@ from __future__ import annotations import importlib.util +from datetime import datetime, timedelta, timezone from pathlib import Path from types import ModuleType @@ -640,12 +641,428 @@ def test_zero_check_pr_never_settles_done_while_require_ci_holds( assert report["done"] is False +# --------------------------------------------------------------------------- # +# review-bot state: queued vs. unavailable (issues #19 + #23) +# --------------------------------------------------------------------------- # + +NOW = datetime(2026, 7, 25, 12, 0, tzinfo=timezone.utc) + + +def _bot_check(**overrides): + """One `gh pr checks --json` row for the configured review bot.""" + detail = { + "name": "CodeRabbit", + "state": "SUCCESS", + "bucket": "pass", + "description": "", + "startedAt": "2026-07-25T11:50:00Z", + } + detail.update(overrides) + return detail + + +def _minutes_ago(minutes: float) -> str: + return (NOW - timedelta(minutes=minutes)).isoformat().replace("+00:00", "Z") + + +def test_rate_limit_in_a_check_description_is_detected(monkeypatch) -> None: + """Issue #23, the exact repro: PR #22 head 32f3e4f. + + CodeRabbit was rate-limited and said so ONLY in its status-check + description, on a check whose conclusion was `SUCCESS` and whose name is + classified informational. Both mechanisms missed it — `unavailable_markers` + read comment bodies and there was no comment; the check that carried the + reason was excluded from the blocking tally. A rate-limited bot rendered + identically to one that reviewed and found nothing. + """ + pr_watch = _load_pr_watch() + + status = pr_watch.summarize_review_bots( + [ + _bot_check(description="Review rate limited"), + {"name": "toolkit", "state": "SUCCESS", "bucket": "pass", "description": ""}, + ], + [], + now=NOW, + ) + + assert [(e["bot"], e["surface"]) for e in status["unavailable"]] == [ + ("coderabbit", "check") + ] + assert status["unavailable"][0]["reason"] == "review rate limited" + # An outage is an ACTION signal, never a gate: making it block would need the + # informational-check exclusion inverted, and that exclusion is what stops a + # bot which never reports from wedging the loop. + assert status["blockers"] == [] + + +def test_the_unavailable_marker_matches_both_surface_wordings() -> None: + """Same bot, same rate limit, ~1h apart on #22 vs #24 — different wording per + surface. Matching only the comment phrasing is what made detection depend on + which surface the bot happened to use that time. + """ + pr_watch = _load_pr_watch() + + assert pr_watch.review_unavailable_reason("Review rate limited") is not None + assert pr_watch.review_unavailable_reason("review limit reached") is not None + + +def test_a_queued_bot_blocks_the_merge_gate_but_never_convergence() -> None: + """Issue #19: on #16 a receipt was recorded while CodeRabbit's check read + `PENDING — Review queued`; the merge fired and its four valid findings landed + minutes later. + + The fix has to hold BOTH halves at once, which is why #19 and #23 could not be + solved separately: the gate must wait for a queued bot, and the watch loop + must still be able to terminate while it waits. + """ + pr_watch = _load_pr_watch() + + report = pr_watch.build_report( + _green_view(), + [], + set(), + review_receipt={"head": "abc123", "source": "fallback:codex"}, + check_details=[ + _bot_check( + state="PENDING", + bucket="pending", + description="Review queued", + startedAt=_minutes_ago(3), + ) + ], + now=NOW, + ) + + assert report["converged"] is True # the loop can finish — no wedge + assert report["mergeable"] is False # but the merge waits for the reviewer + assert report["done"] is report["mergeable"] # legacy alias stays in lockstep + assert any("has not reported yet" in b for b in report["merge_blockers"]) + + +def test_a_pending_bot_past_the_grace_window_stops_blocking() -> None: + """The anti-wedge bound. A review bot's check can sit pending forever after a + trivial follow-up commit — the documented reason its check is informational + in the first place. Without a bound, the #19 fix would reintroduce exactly + the wedge that exclusion exists to prevent, at the merge gate instead of the + watch loop. + """ + pr_watch = _load_pr_watch() + + status = pr_watch.summarize_review_bots( + [_bot_check(state="PENDING", bucket="pending", startedAt=_minutes_ago(120))], + [], + now=NOW, + grace_minutes=15, + ) + + assert status["blockers"] == [] + assert status["pending"][0]["blocking"] is False + + +def test_an_announced_outage_cancels_the_pending_block_on_the_same_bot() -> None: + """"Unavailable" and "pending" are mutually exclusive answers to one question. + + A bot that has announced it is not reviewing will never move its check off + pending, so waiting for it is a wedge with extra steps. The outage wins, on + either surface — including a COMMENT cancelling a stuck CHECK, which is the + combination the fallback path actually needs. + """ + pr_watch = _load_pr_watch() + stuck = [_bot_check(state="PENDING", bucket="pending", startedAt=_minutes_ago(1))] + + assert pr_watch.summarize_review_bots(stuck, [], now=NOW)["blockers"] != [] + + via_comment = pr_watch.summarize_review_bots( + stuck, + [{"author": "coderabbitai", "review_unavailable_reason": "review limit reached"}], + now=NOW, + ) + via_check = pr_watch.summarize_review_bots( + [ + _bot_check( + state="PENDING", + bucket="pending", + description="Review rate limited", + startedAt=_minutes_ago(1), + ) + ], + [], + now=NOW, + ) + + assert via_comment["blockers"] == [] + assert via_check["blockers"] == [] + + +def test_an_unmeasurable_pending_age_fails_open_and_says_so() -> None: + """No usable timestamp means we cannot tell "queued 10 seconds ago" from + "queued last Tuesday". + + Blocking on an age we cannot measure is indistinguishable from a wedge, so it + fails open — and a guard that fails open must be loud (three silent-no-op + bugs in one session is what taught this repo that). + """ + pr_watch = _load_pr_watch() + + status = pr_watch.summarize_review_bots( + [ + _bot_check(state="PENDING", bucket="pending", startedAt=None), + _bot_check( + name="CodeRabbit-2", state="PENDING", bucket="pending", + startedAt="0001-01-01T00:00:00Z", + ), + ], + [], + now=NOW, + ) + + assert status["blockers"] == [] + assert [e["age_unknown"] for e in status["pending"]] == [True, True] + + rendered = pr_watch.render( + { + "pr": 1, "url": "u", "converged": True, "mergeable": False, + "checks": {"success": 1, "total": 1, "pending": 0, "failing": []}, + "new_comments": [], "merge_blockers": [], "review_bots": status, + } + ) + assert "age unmeasurable" in rendered + assert "NOT blocking" in rendered + + +def test_an_outage_stays_visible_after_the_notice_comment_is_acked() -> None: + """The comment surface used to be the ONLY record that the primary reviewer + never ran — and `--mark-seen` erases it from `new_comments`. Aggregating the + outage separately is what keeps the gap readable at merge time rather than + reconstructible only from the PR thread. + """ + pr_watch = _load_pr_watch() + view = _green_view( + comments=[ + { + "id": "c1", + "author": {"login": "coderabbitai"}, + "body": "Review limit reached — try again later.", + } + ] + ) + + report = pr_watch.build_report( + view, + [], + # Every key of that comment already acked: it is gone from new_comments. + set(pr_watch.build_report(view, [], set(), now=NOW)["all_seen_keys"]), + review_receipt={"head": "abc123", "source": "fallback:codex"}, + now=NOW, + ) + + assert report["new_comments"] == [] + assert report["review_bots"]["unavailable"][0]["reason"] == "review limit reached" + assert "review unavailable" in pr_watch.render(report) + + +def test_a_bot_that_never_posts_a_check_blocks_nothing() -> None: + """The repo with no review bot, and the bot that simply does not run on this + PR. Absent is not pending: an empty rollup must add no blocker at all. + """ + pr_watch = _load_pr_watch() + + assert pr_watch.summarize_review_bots([], [], now=NOW)["blockers"] == [] + assert ( + pr_watch.summarize_review_bots( + [{"name": "toolkit", "state": "PENDING", "bucket": "pending"}], [], now=NOW + )["blockers"] + == [] + ) + assert ( + pr_watch.summarize_review_bots( + [_bot_check(state="PENDING", bucket="pending")], [], now=NOW, bots=() + )["blockers"] + == [] + ) + + +def test_check_detail_fetch_never_raises_and_degrades_to_no_signal( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """`gh pr checks` exits non-zero for ordinary states (8 = something pending, + 1 = something failing) and errors outright on a PR with no checks. Trusting + the exit code would drop the bot signal on the most common polls; raising + would wedge the loop on a PR that has none. + """ + pr_watch = _load_pr_watch() + + class _Result: + def __init__(self, stdout: str, returncode: int) -> None: + self.stdout = stdout + self.returncode = returncode + + monkeypatch.setattr( + pr_watch.subprocess, + "run", + lambda *a, **k: _Result('[{"name":"CodeRabbit","state":"PENDING"}]', 8), + ) + assert pr_watch.fetch_check_details(1) == [ + {"name": "CodeRabbit", "state": "PENDING"} + ] + + monkeypatch.setattr( + pr_watch.subprocess, "run", lambda *a, **k: _Result("no checks reported", 1) + ) + assert pr_watch.fetch_check_details(1) == [] + + def _boom(*a, **k): + raise OSError("gh is not installed") + + monkeypatch.setattr(pr_watch.subprocess, "run", _boom) + assert pr_watch.fetch_check_details(1) == [] + + +def test_record_review_refuses_while_the_bot_is_still_queued( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The #16 sequence, mechanized at the moment the judgment is made. + + Every input `decide_done` could see was correct at that instant — CI green, + no unacked comments, a receipt bound to the exact head. The one thing that + was wrong was the reviewer's own state, which nothing consulted. + """ + pr_watch = _load_pr_watch() + monkeypatch.setattr( + pr_watch, + "_gh_json", + lambda args: {"number": 16, "headRefOid": "abc123", "comments": [], "reviews": []}, + ) + monkeypatch.setattr( + pr_watch, + "fetch_check_details", + lambda pr, **kw: [ + _bot_check( + state="PENDING", + bucket="pending", + description="Review queued", + startedAt=_minutes_ago(2), + ) + ], + ) + recorded: list[dict] = [] + monkeypatch.setattr(pr_watch, "save_state", lambda pr, state: recorded.append(state)) + monkeypatch.setattr(pr_watch, "load_state", lambda pr: {}) + + with pytest.raises(ValueError, match="has not reported yet"): + pr_watch.record_review(16, "fallback:codex", "abc123", now=NOW) + assert recorded == [] # refused BEFORE persisting anything + + # The documented override is the only way past it, and it is explicit. + pr_watch.record_review( + 16, "fallback:codex", "abc123", allow_pending_bot=True, now=NOW + ) + assert recorded[0]["review_receipt"]["source"] == "fallback:codex" + + +def test_record_review_allows_the_fallback_when_the_bot_is_unavailable( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The refusal must not block the path it exists to protect. + + "A blocked bot is an action signal, run the fallback" is the doctrine — so a + bot that announced an outage in a COMMENT while its check sits stuck at + pending has to let the fallback receipt through. Reading only the check + would refuse exactly here. + """ + pr_watch = _load_pr_watch() + monkeypatch.setattr( + pr_watch, + "_gh_json", + lambda args: { + "number": 22, + "headRefOid": "32f3e4f", + "comments": [ + { + "id": "c1", + "author": {"login": "coderabbitai"}, + "body": "Review limit reached.", + } + ], + "reviews": [], + }, + ) + monkeypatch.setattr( + pr_watch, + "fetch_check_details", + lambda pr, **kw: [ + _bot_check(state="PENDING", bucket="pending", startedAt=_minutes_ago(1)) + ], + ) + recorded: list[dict] = [] + monkeypatch.setattr(pr_watch, "save_state", lambda pr, state: recorded.append(state)) + monkeypatch.setattr(pr_watch, "load_state", lambda pr: {}) + + pr_watch.record_review(22, "fallback:panel", "32f3e4f", now=NOW) + + assert recorded[0]["review_receipt"]["source"] == "fallback:panel" + + +def test_bot_state_never_reaches_the_watch_loop_predicate() -> None: + """The load-bearing invariant, pinned directly rather than inferred. + + Everything in this section feeds the merge gate. If any of it ever reached + `decide_converged`, a bot that never reports would wedge the poll/fix/ack + loop forever — the failure the informational-check exclusion exists to + prevent, and the constraint that made #19 and #23 unfixable in isolation. + """ + pr_watch = _load_pr_watch() + view = _green_view() + + baseline = pr_watch.build_report(view, [], set(), now=NOW) + for details in ( + [_bot_check(state="PENDING", bucket="pending", startedAt=_minutes_ago(1))], + [_bot_check(description="Review rate limited")], + [_bot_check(state="PENDING", bucket="pending", startedAt=None)], + ): + report = pr_watch.build_report( + view, [], set(), check_details=details, now=NOW + ) + assert report["converged"] is baseline["converged"] is True, details + assert report["checks"] == baseline["checks"], details + + +def test_bot_blockers_only_ever_tighten_the_merge_gate() -> None: + """The skew direction that makes this safe to ship per-file. + + Engine upgrades are per-file, so a new `pr_watch.py` can run against an older + `dev_session.sh` whose gate reads `done`. Adding a blocker can only make + `done` false where it was true — merges wait, never fire early. The reverse + would be a silent fail-open on an unreviewed PR. + """ + pr_watch = _load_pr_watch() + args = dict(review_receipt={"head": "abc123", "source": "coderabbit"}, now=NOW) + + without = pr_watch.build_report(_green_view(), [], set(), **args) + with_pending = pr_watch.build_report( + _green_view(), + [], + set(), + check_details=[ + _bot_check(state="PENDING", bucket="pending", startedAt=_minutes_ago(1)) + ], + **args, + ) + + assert without["done"] is True + assert with_pending["done"] is False + # Strictly a superset — no pre-existing blocker was dropped to make room. + assert set(without["merge_blockers"]) <= set(with_pending["merge_blockers"]) + + # --------------------------------------------------------------------------- # # review-bot knowledge comes from config, not from engine literals # --------------------------------------------------------------------------- # ADOPTER_CONFIG = """review: + bots: [OtherBot] noise_markers: - "" - "NOTHING TO REPORT" @@ -653,6 +1070,7 @@ def test_zero_check_pr_never_settles_done_while_require_ci_holds( - "OtherBot Is Out Of Credits" informational_checks: [OtherBot, " Advisory "] require_ci: false + bot_pending_grace_minutes: 30 """ @@ -668,18 +1086,21 @@ def test_review_knowledge_is_read_from_config_not_engine_literals( ) -> None: pr_watch = _load_pr_watch() - noise, unavailable, informational, require_ci = pr_watch._load_review_config( - _write_config(tmp_path, ADOPTER_CONFIG) - ) + resolved = pr_watch._load_review_config(_write_config(tmp_path, ADOPTER_CONFIG)) # Lower-cased/stripped at load so every call site can keep matching # case-insensitively against a folded body / check name. - assert noise == ("", "nothing to report") - assert unavailable == ("otherbot is out of credits",) - assert informational == frozenset({"otherbot", "advisory"}) - assert require_ci is False + assert resolved.noise_markers == ( + "", + "nothing to report", + ) + assert resolved.unavailable_markers == ("otherbot is out of credits",) + assert resolved.informational_checks == frozenset({"otherbot", "advisory"}) + assert resolved.require_ci is False + assert resolved.bots == ("otherbot",) + assert resolved.bot_pending_grace_minutes == 30 # None of the kit's own default markers leak in — config replaces, not extends. - assert "" not in noise + assert "" not in resolved.noise_markers def test_missing_config_falls_back_to_defaults_silently( @@ -695,6 +1116,8 @@ def test_missing_config_falls_back_to_defaults_silently( pr_watch._DEFAULT_REVIEW_UNAVAILABLE_MARKERS, frozenset(pr_watch._DEFAULT_INFORMATIONAL_CHECK_NAMES), True, + pr_watch._DEFAULT_REVIEW_BOTS, + pr_watch._DEFAULT_BOT_PENDING_GRACE_MINUTES, ) assert capsys.readouterr().err == "" @@ -717,8 +1140,8 @@ def test_unreadable_config_warns_and_falls_back_without_crashing( resolved = pr_watch._load_review_config(path) - assert resolved[3] is True # NOT the config's require_ci: false - assert resolved[0] == pr_watch._DEFAULT_NOISE_MARKERS + assert resolved.require_ci is True # NOT the config's require_ci: false + assert resolved.noise_markers == pr_watch._DEFAULT_NOISE_MARKERS assert "could not read review config" in capsys.readouterr().err @@ -736,8 +1159,8 @@ def test_explicit_empty_noise_list_is_honored_but_absent_key_is_not( _write_config(tmp_path / "b", "review:\n bots: [coderabbit]\n") ) - assert empty[0] == () - assert absent[0] == pr_watch._DEFAULT_NOISE_MARKERS + assert empty.noise_markers == () + assert absent.noise_markers == pr_watch._DEFAULT_NOISE_MARKERS def test_non_boolean_require_ci_keeps_the_safe_default(tmp_path: Path) -> None: @@ -746,10 +1169,37 @@ def test_non_boolean_require_ci_keeps_the_safe_default(tmp_path: Path) -> None: pr_watch = _load_pr_watch() for value in ("yes", "0", '"false"'): - _, _, _, require_ci = pr_watch._load_review_config( + resolved = pr_watch._load_review_config( _write_config(tmp_path, f"review:\n require_ci: {value}\n") ) - assert require_ci is True, value + assert resolved.require_ci is True, value + + +def test_non_numeric_or_negative_grace_keeps_the_default(tmp_path: Path) -> None: + """The pending-bot bound must survive a typo. + + `true` is the interesting one: `bool` is an `int` subclass, so a naive + isinstance check would read it as a grace of 1 minute — turning the #19 + guard off for every bot that takes longer than a minute to review, which is + all of them. + """ + pr_watch = _load_pr_watch() + + for value in ("soon", "true", "-5", '"15"'): + resolved = pr_watch._load_review_config( + _write_config(tmp_path, f"review:\n bot_pending_grace_minutes: {value}\n") + ) + assert ( + resolved.bot_pending_grace_minutes + == pr_watch._DEFAULT_BOT_PENDING_GRACE_MINUTES + ), value + + # …but a real number, including 0 ("never wait"), is honored. + for value, expected in (("0", 0.0), ("2.5", 2.5), ("60", 60.0)): + resolved = pr_watch._load_review_config( + _write_config(tmp_path, f"review:\n bot_pending_grace_minutes: {value}\n") + ) + assert resolved.bot_pending_grace_minutes == expected, value def test_configured_unavailable_marker_still_beats_configured_noise( @@ -758,15 +1208,17 @@ def test_configured_unavailable_marker_still_beats_configured_noise( """The "a down reviewer is never auto-noise" invariant is a property of the engine, not of which list a marker happens to sit on in config.""" pr_watch = _load_pr_watch() - noise, unavailable, _, _ = pr_watch._load_review_config( + resolved = pr_watch._load_review_config( _write_config( tmp_path, 'review:\n noise_markers: ["OtherBot Is Out Of Credits"]\n' ' unavailable_markers: ["OtherBot Is Out Of Credits"]\n', ) ) - monkeypatch.setattr(pr_watch, "_NOISE_MARKERS", noise) - monkeypatch.setattr(pr_watch, "_REVIEW_UNAVAILABLE_MARKERS", unavailable) + monkeypatch.setattr(pr_watch, "_NOISE_MARKERS", resolved.noise_markers) + monkeypatch.setattr( + pr_watch, "_REVIEW_UNAVAILABLE_MARKERS", resolved.unavailable_markers + ) body = "OtherBot is out of credits for this repo." From d3898e2610681431b489649875946aa2614eea4b Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 15:29:25 +0300 Subject: [PATCH 2/9] Give the grace clock a source that actually exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by running the first cut against this PR: CodeRabbit's pending status context reports `startedAt: 0001-01-01T00:00:00Z` — the zero time, i.e. no timestamp. So the age fallback ("unmeasurable ⇒ fail open, don't block") was not an edge case at all: it was the *only* path CodeRabbit ever took, and the #19 guard printed "age unmeasurable, NOT blocking" against a live review-in-progress. The guard was dead code for the one bot it was written for. The clock now falls back to when this engine first saw the bot pending, persisted per PR. Because it is our clock and only ever advances, every pending bot reaches the grace bound — so the fail-open branch is gone entirely rather than merely narrowed, and there is no unmeasurable case left to wedge on. Stored as `bot_pending_since: {"head": sha, "bots": {bot: iso}}` — self- describing rather than scoped by the sibling `state["head"]`. That field is the false-settle guard's input; making `record_review` maintain it just to scope this clock would put two intents on one key, and that key decides whether a just-pushed commit may settle. `record_review` persists the first sighting BEFORE refusing. Without it a cold `--record-review` restarts the clock at zero on every retry, so the refusal could never expire and the override would be the only way through — a wedge dressed as a guard. Verified live on this PR: the merge gate now reports `review bot coderabbit has not reported yet (check CodeRabbit pending 0.0m < 15m grace)`. Tests 217 → 221. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --- docs/agentic-dev-kit/workflows/pr-watch.md | 6 +- kit-manifest.json | 4 +- scripts/pr_watch.py | 114 ++++++++++--- scripts/tests/test_pr_watch.py | 185 ++++++++++++++++++--- 4 files changed, 257 insertions(+), 52 deletions(-) diff --git a/docs/agentic-dev-kit/workflows/pr-watch.md b/docs/agentic-dev-kit/workflows/pr-watch.md index b3f6cf9..e7f354b 100644 --- a/docs/agentic-dev-kit/workflows/pr-watch.md +++ b/docs/agentic-dev-kit/workflows/pr-watch.md @@ -159,7 +159,11 @@ Self-pace on a bounded cadence — don't busy-wait: `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. + 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. It only ever advances, so the block always + expires on its own. 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. diff --git a/kit-manifest.json b/kit-manifest.json index d01f639..973eb87 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -23,7 +23,7 @@ }, "docs/agentic-dev-kit/workflows/pr-watch.md": { "role": "workflow", - "sha256": "e916c72f133b0ca0ff302931878a23ca74d7069d44716aed9a120d3e1a9faae9" + "sha256": "5bd464ad411b24ff4b959293141366fe648421ccadee8fec62335e406056e309" }, "docs/agentic-dev-kit/workflows/session-start.md": { "role": "workflow", @@ -99,7 +99,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "50e1b53077c765feb990d4cf3968c86454fc9908622d245885a8155ccf8f65c1" + "sha256": "adb1e2fc3f22c4e562189b0588a1404ca47866ae0ac9e3ad15eb4598f4335e44" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index 055edf8..f890d18 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -770,7 +770,9 @@ def _age_minutes(timestamp: str | None, now: datetime) -> float | None: GitHub stamps an unstarted check with the zero time (``0001-01-01T…``), which parses fine but means "no time recorded" — treated as unknown, not as an age - of two millennia. + of two millennia. That is not an edge case: CodeRabbit's pending status + context reports exactly that, so the caller must always have a fallback + clock (see ``pending_since`` in :func:`summarize_review_bots`). """ if not timestamp: return None @@ -792,6 +794,7 @@ def summarize_review_bots( now: datetime, bots: tuple[str, ...] | None = None, grace_minutes: float | None = None, + pending_since: dict | None = None, ) -> dict: """Resolve each configured review bot to *unavailable*, *pending*, or neither. @@ -824,18 +827,31 @@ def summarize_review_bots( wedging. - **neither** — terminal and unmarked: the bot reviewed. Nothing to add. - An unknown check age (no usable timestamp) fails **open**: it does not block, - and it is reported as ``age_unknown`` so the render can say so out loud. The - alternative — blocking on an age we cannot measure — is indistinguishable - from a wedge, which is the one failure this whole design is built to avoid. - - Returns ``{grace_minutes, unavailable, pending, blockers}``. ``blockers`` are - ready-made ``merge_blockers`` strings. + **The grace clock cannot come from the check alone.** CodeRabbit's pending + status context reports ``startedAt: 0001-01-01T00:00:00Z`` — the zero time, + i.e. no timestamp — so an implementation that only reads the check has no + age to compare and quietly stops guarding the one bot it was written for. + (Observed on this very PR: the first cut of this function printed "age + unmeasurable, NOT blocking" against a live CodeRabbit review-in-progress.) + + ``pending_since`` is the fallback clock: a ``{bot: iso8601}`` map of when + THIS engine first saw that bot pending at the current head, threaded through + per-PR state by the caller. An unseen bot is recorded at ``now`` and so + starts at age 0 — blocking, and ageing normally from there. Because the + clock is ours and only ever advances, every pending bot reaches the grace + bound and stops blocking; there is no unmeasurable case left to fail open on. + The map is scoped to a head: a push means a fresh review, so the caller + resets it. + + Returns ``{grace_minutes, unavailable, pending, blockers, pending_since}``. + ``blockers`` are ready-made ``merge_blockers`` strings; ``pending_since`` is + the updated map for the caller to persist. """ if bots is None: bots = _REVIEW_BOTS if grace_minutes is None: grace_minutes = _BOT_PENDING_GRACE_MINUTES + observed: dict[str, str] = dict(pending_since or {}) unavailable: list[dict] = [] unavailable_bots: set[str] = set() @@ -884,14 +900,23 @@ def summarize_review_bots( if not _check_is_pending(detail): continue age = _age_minutes(detail.get("startedAt"), now) + since = detail.get("startedAt") + source = "check" + if age is None: + # No usable stamp on the check — fall back to our own first-sighting + # clock, recording one now if this is the first sighting. + since = observed.get(bot) or now.isoformat() + observed[bot] = since + age = _age_minutes(since, now) or 0.0 + source = "observed" pending.append( { "bot": bot, "check": name, "state": (detail.get("state") or "").upper(), - "since": detail.get("startedAt"), - "age_minutes": None if age is None else round(age, 1), - "age_unknown": age is None, + "since": since, + "age_source": source, + "age_minutes": round(age, 1), # Filled in below, once every surface has been read: an # unavailability announced in a *comment* must be able to cancel # a pending *check*, and the comment loop already ran. @@ -903,8 +928,8 @@ def summarize_review_bots( for entry in pending: if entry["bot"] in unavailable_bots: continue # announced as not reviewing — waiting on it is a wedge - if entry["age_unknown"] or entry["age_minutes"] >= grace_minutes: - continue # unmeasurable or aged out — fail open, never wedge + if entry["age_minutes"] >= grace_minutes: + continue # aged out — treated as never going to report entry["blocking"] = True blockers.append( f"review bot {entry['bot']} has not reported yet " @@ -917,6 +942,11 @@ def summarize_review_bots( "unavailable": unavailable, "pending": pending, "blockers": blockers, + # Only the bots still pending: an entry for a bot that has since + # reported would otherwise keep a stale clock alive across polls. + "pending_since": { + bot: at for bot, at in observed.items() if any(e["bot"] == bot for e in pending) + }, } @@ -1010,11 +1040,40 @@ def _seen_path(pr: int) -> Path: return STATE_DIR / f"{pr}.json" +def read_pending_since(state: dict, head: str | None) -> dict: + """The persisted grace clock, but only if it belongs to ``head``. + + Stored as ``{"head": sha, "bots": {bot: iso}}`` — self-describing rather than + scoped by the sibling ``state["head"]``. That field is the false-settle + guard's input, written by :func:`persist_poll` on every poll; making a second + writer (:func:`record_review`) maintain it just to scope this clock would put + two different intents on one key, and the guard it feeds decides whether a + just-pushed commit can settle. A push means a fresh review, so a clock from + another head is discarded, not aged. + """ + stored = state.get("bot_pending_since") + if not isinstance(stored, dict) or not head or stored.get("head") != head: + return {} + bots = stored.get("bots") + return bots if isinstance(bots, dict) else {} + + +def write_pending_since(state: dict, head: str | None, bots: dict) -> dict: + """Set the head-scoped grace clock on ``state`` (dropping an empty one).""" + if bots and head: + state["bot_pending_since"] = {"head": head, "bots": bots} + else: + state.pop("bot_pending_since", None) + return state + + def load_state(pr: int) -> dict: """Full per-PR watch state (missing/corrupt → {}). Keys: ``seen`` (acked comment keys), ``head`` / ``max_total`` (false-settle - guard, see :func:`build_report`), and ``pending_seen`` — the ``all_seen_keys`` + guard, see :func:`build_report`), ``bot_pending_since`` (the head-scoped + fallback grace clock for a review bot whose check carries no usable + timestamp, see :func:`read_pending_since`), and ``pending_seen`` — the ``all_seen_keys`` of the most recently *reported* plain poll, present only between a poll and the ``--mark-seen`` that consumes it (see :func:`mark_seen`). """ @@ -1130,6 +1189,7 @@ def record_review( "review the new head before recording evidence" ) now = now or datetime.now(timezone.utc) + state = load_state(pr) if not allow_pending_bot: # Comments are fetched alongside the head so an outage announced in a # COMMENT can still cancel a pending CHECK. Without them, the documented @@ -1139,8 +1199,15 @@ def record_review( fetch_check_details(pr), collect_comments(snapshot, []), now=now, + pending_since=read_pending_since(state, current_head), ) if bot_status["blockers"]: + # Persist the first sighting BEFORE refusing. Without this, a cold + # `--record-review` (no poll loop running) restarts the grace clock + # at zero on every retry, so the refusal could never expire and the + # override would be the only way through — a wedge dressed as a guard. + write_pending_since(state, current_head, bot_status["pending_since"]) + save_state(pr, state) raise ValueError( "; ".join(bot_status["blockers"]) + ". A receipt recorded now would bind to a review that has not happened " @@ -1152,7 +1219,6 @@ def record_review( "source": source, "recorded_at": now.isoformat(), } - state = load_state(pr) state["review_receipt"] = receipt save_state(pr, state) return {"pr": pr, "recorded_review": True, "review_receipt": receipt} @@ -1176,6 +1242,7 @@ def build_report( review_receipt: dict | None = None, check_details: list[dict] | None = None, now: datetime | None = None, + prior_pending_since: dict | None = None, # already head-scoped by the caller ) -> dict: """Assemble the JSON-serializable watch report for one PR snapshot. @@ -1227,6 +1294,7 @@ def build_report( check_details or [], comments, now=now or datetime.now(timezone.utc), + pending_since=prior_pending_since or {}, ) # False-settle guard: right after a push, `gh` can still report the OLD @@ -1367,13 +1435,7 @@ def render(report: dict) -> str: f"{entry['reason']} — run the configured fallback review" ) for entry in bots.get("pending") or []: - if entry.get("age_unknown"): - lines.append( - f" ⚠ review bot {entry['bot']} check {entry['check']} is pending with no " - "usable timestamp — age unmeasurable, so it is NOT blocking the merge gate; " - "confirm the review landed before recording a receipt" - ) - elif not entry.get("blocking"): + if not entry.get("blocking"): lines.append( f" ⚠ review bot {entry['bot']} check {entry['check']} still pending after " f"{entry['age_minutes']}m (past the {bots.get('grace_minutes')}m grace) — " @@ -1443,6 +1505,13 @@ def persist_poll(pr: int, report: dict, seen: set[str]) -> dict: "max_total": report["max_total"], "pending_seen": report["all_seen_keys"], } + # The fallback grace clock for a bot whose check carries no usable timestamp. + # Persisted rather than derived per poll, because a clock that restarts on + # every poll never advances — and a guard that never advances is a permanent + # block, the exact wedge this design avoids. + write_pending_since( + new_state, report["head"], report["review_bots"]["pending_since"] + ) previous = load_state(pr) if isinstance(previous.get("review_receipt"), dict): new_state["review_receipt"] = previous["review_receipt"] @@ -1591,6 +1660,7 @@ def main(argv: list[str] | None = None) -> int: prior_max_total=int(state.get("max_total") or 0), review_receipt=state.get("review_receipt"), check_details=check_details, + prior_pending_since=read_pending_since(state, view.get("headRefOid")), ) persist_poll(pr, report, seen) diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index 9bee9f2..679495c 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -795,40 +795,117 @@ def test_an_announced_outage_cancels_the_pending_block_on_the_same_bot() -> None assert via_check["blockers"] == [] -def test_an_unmeasurable_pending_age_fails_open_and_says_so() -> None: - """No usable timestamp means we cannot tell "queued 10 seconds ago" from - "queued last Tuesday". +def test_a_check_with_no_usable_timestamp_falls_back_to_an_observed_clock() -> None: + """The grace clock cannot come from the check alone — found by running this. + + CodeRabbit's pending status context reports `startedAt: 0001-01-01T00:00:00Z` + (the zero time). An implementation that reads only the check therefore has no + age for the one bot the guard exists for, and quietly stops guarding it. The + first cut of this feature did exactly that, and printed "age unmeasurable, + NOT blocking" against a live review-in-progress on PR #25. + """ + pr_watch = _load_pr_watch() + zero_time = [ + _bot_check(state="PENDING", bucket="pending", startedAt="0001-01-01T00:00:00Z") + ] + + first = pr_watch.summarize_review_bots(zero_time, [], now=NOW) + + assert first["blockers"] != [] # unseen ⇒ age 0 ⇒ blocking, not waved through + assert first["pending"][0]["age_source"] == "observed" + assert first["pending_since"] == {"coderabbit": NOW.isoformat()} + + # …and OUR clock advances, so it always reaches the bound. This is what makes + # the fallback safe: the block expires on its own, with no wedge to escape. + later = pr_watch.summarize_review_bots( + zero_time, + [], + now=NOW + timedelta(minutes=20), + pending_since=first["pending_since"], + ) + assert later["blockers"] == [] + assert later["pending"][0]["age_minutes"] == 20.0 + + +def test_the_observed_clock_is_head_scoped_and_self_describing() -> None: + """A clock that restarts every poll never advances, and a guard that never + advances is a permanent block rather than a bounded one. + + It is stored as `{"head": sha, "bots": {...}}` rather than scoped by the + sibling `state["head"]` — that field is the false-settle guard's input, and + putting two intents on one key would mean a second writer maintaining the + field that decides whether a just-pushed commit may settle. + """ + pr_watch = _load_pr_watch() + clock = {"coderabbit": NOW.isoformat()} + state = pr_watch.write_pending_since({}, "abc123", clock) + + assert state["bot_pending_since"] == {"head": "abc123", "bots": clock} + assert pr_watch.read_pending_since(state, "abc123") == clock + # A push means a fresh review: the clock is discarded, not aged. + assert pr_watch.read_pending_since(state, "def456") == {} + # Corrupt / legacy / absent state degrades to "no clock", never to a crash. + for bad in ({}, {"bot_pending_since": "nonsense"}, {"bot_pending_since": {}}): + assert pr_watch.read_pending_since(bad, "abc123") == {} + # An empty clock is dropped rather than persisted as a husk. + assert "bot_pending_since" not in pr_watch.write_pending_since({}, "abc123", {}) + + +def test_persist_poll_carries_the_clock_so_a_later_poll_can_age_it_out( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The round trip through the REAL persistence writer. - Blocking on an age we cannot measure is indistinguishable from a wedge, so it - fails open — and a guard that fails open must be loud (three silent-no-op - bugs in one session is what taught this repo that). + `persist_poll` is the single source of truth for what a poll stores. A test + that rebuilt that shape by hand could pass while the engine silently dropped + the clock — and a dropped clock reads as "first sighting" forever, which is a + permanent block rather than a bounded one. """ pr_watch = _load_pr_watch() + zero_time = [ + _bot_check(state="PENDING", bucket="pending", startedAt="0001-01-01T00:00:00Z") + ] + store: dict = {} + monkeypatch.setattr(pr_watch, "save_state", lambda pr, state: store.update(state)) + monkeypatch.setattr(pr_watch, "load_state", lambda pr: dict(store)) + + first = pr_watch.build_report( + _green_view(), [], set(), check_details=zero_time, now=NOW + ) + pr_watch.persist_poll(7, first, set()) + + assert store["bot_pending_since"] == { + "head": "abc123", + "bots": {"coderabbit": NOW.isoformat()}, + } + assert first["review_bots"]["blockers"] != [] + + aged = pr_watch.build_report( + _green_view(), + [], + set(), + check_details=zero_time, + now=NOW + timedelta(minutes=20), + prior_head=store["head"], + prior_pending_since=pr_watch.read_pending_since(store, "abc123"), + ) + assert aged["review_bots"]["blockers"] == [] + + +def test_a_reported_bot_drops_out_of_the_persisted_clock() -> None: + """Otherwise a stale entry outlives the pending state it timed, and a later + re-review inherits an already-expired clock instead of a fresh window.""" + pr_watch = _load_pr_watch() status = pr_watch.summarize_review_bots( - [ - _bot_check(state="PENDING", bucket="pending", startedAt=None), - _bot_check( - name="CodeRabbit-2", state="PENDING", bucket="pending", - startedAt="0001-01-01T00:00:00Z", - ), - ], + [_bot_check()], # terminal — the bot reported [], now=NOW, + pending_since={"coderabbit": (NOW - timedelta(minutes=5)).isoformat()}, ) - assert status["blockers"] == [] - assert [e["age_unknown"] for e in status["pending"]] == [True, True] - - rendered = pr_watch.render( - { - "pr": 1, "url": "u", "converged": True, "mergeable": False, - "checks": {"success": 1, "total": 1, "pending": 0, "failing": []}, - "new_comments": [], "merge_blockers": [], "review_bots": status, - } - ) - assert "age unmeasurable" in rendered - assert "NOT blocking" in rendered + assert status["pending"] == [] + assert status["pending_since"] == {} def test_an_outage_stays_visible_after_the_notice_comment_is_acked() -> None: @@ -952,13 +1029,66 @@ def test_record_review_refuses_while_the_bot_is_still_queued( with pytest.raises(ValueError, match="has not reported yet"): pr_watch.record_review(16, "fallback:codex", "abc123", now=NOW) - assert recorded == [] # refused BEFORE persisting anything + # No receipt was persisted. The grace clock IS written on the refusal path — + # without it a cold `--record-review` restarts the clock at zero on every + # retry, so the refusal could never expire. + assert all("review_receipt" not in state for state in recorded) # The documented override is the only way past it, and it is explicit. pr_watch.record_review( 16, "fallback:codex", "abc123", allow_pending_bot=True, now=NOW ) - assert recorded[0]["review_receipt"]["source"] == "fallback:codex" + assert recorded[-1]["review_receipt"]["source"] == "fallback:codex" + + +def test_a_cold_record_review_refusal_expires_instead_of_wedging( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """`--record-review` outside a poll loop must still see a clock that advances. + + The bot here reports no usable timestamp (CodeRabbit's real behaviour), so + the clock is the engine's own first sighting. If the refusal path did not + persist it, every retry would restart at zero and the only way past would be + the override — a wedge dressed as a guard. + """ + pr_watch = _load_pr_watch() + monkeypatch.setattr( + pr_watch, + "_gh_json", + lambda args: {"number": 16, "headRefOid": "abc123", "comments": [], "reviews": []}, + ) + monkeypatch.setattr( + pr_watch, + "fetch_check_details", + lambda pr, **kw: [ + _bot_check( + state="PENDING", bucket="pending", startedAt="0001-01-01T00:00:00Z" + ) + ], + ) + store: dict = {} + monkeypatch.setattr(pr_watch, "save_state", lambda pr, state: store.update(state)) + monkeypatch.setattr(pr_watch, "load_state", lambda pr: dict(store)) + + with pytest.raises(ValueError, match="has not reported yet"): + pr_watch.record_review(16, "fallback:codex", "abc123", now=NOW) + assert store["bot_pending_since"] == { + "head": "abc123", + "bots": {"coderabbit": NOW.isoformat()}, + } + + # Retrying still refuses, and — the point — does NOT reset the clock. + with pytest.raises(ValueError, match="has not reported yet"): + pr_watch.record_review( + 16, "fallback:codex", "abc123", now=NOW + timedelta(minutes=1) + ) + assert store["bot_pending_since"]["bots"] == {"coderabbit": NOW.isoformat()} + + # …so past the grace window it goes through on its own, with no override. + pr_watch.record_review( + 16, "fallback:codex", "abc123", now=NOW + timedelta(minutes=20) + ) + assert store["review_receipt"]["source"] == "fallback:codex" def test_record_review_allows_the_fallback_when_the_bot_is_unavailable( @@ -1020,6 +1150,7 @@ def test_bot_state_never_reaches_the_watch_loop_predicate() -> None: [_bot_check(state="PENDING", bucket="pending", startedAt=_minutes_ago(1))], [_bot_check(description="Review rate limited")], [_bot_check(state="PENDING", bucket="pending", startedAt=None)], + [_bot_check(state="PENDING", bucket="pending", startedAt=_minutes_ago(999))], ): report = pr_watch.build_report( view, [], set(), check_details=details, now=NOW From a9737c1f9d2aee78b7387804195af3d34ea2c626 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 15:33:48 +0300 Subject: [PATCH 3/9] Address CodeRabbit's findings on 233fdd0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All four valid, including one real bug in my own migration. **init.sh corrupted an inline `unavailable_markers` list.** The awk insert anchored on `^ unavailable_markers:`, which also matches an inline flow list (`unavailable_markers: ["a", "b"]`) — equally valid YAML and equally supported by kitconfig. It then appended a ` - ` line under a key that already has a scalar value, producing a malformed mapping, and wrote it straight over the adopter's config. Now only a bare block-list key is migrated; an inline list falls through to the existing by-hand warning. Verified against both shapes. **`fetch_check_details` degraded silently.** Both new guards depend on that fetch, so an older `gh` rejecting one of the requested `--json` fields would disable #19's and #23's detection with no trace — "checked and clean" indistinguishable from "never checked", the failure mode that cost this repo three silent-no-op bugs in one session. It now warns on stderr, once per process rather than per poll (a warning repeated every round of a watch loop is skimmed past exactly like silence). **The PyYAML parity claim is now checked, not restated.** The float-gating docstring argued "PyYAML resolves these as strings" while asserting only the expected values — which would pass just as happily if the reference resolver disagreed with every one of them. Compared against `yaml.safe_load` directly. **`.tmp.$$`** for the migration temp file, matching `append_to_section`. Tests 221 → 222. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --- init.sh | 54 ++++++++++++++++++++------------- kit-manifest.json | 2 +- scripts/pr_watch.py | 48 ++++++++++++++++++++++++++--- scripts/tests/test_kitconfig.py | 13 ++++++-- scripts/tests/test_pr_watch.py | 31 +++++++++++++++++++ 5 files changed, 119 insertions(+), 29 deletions(-) diff --git a/init.sh b/init.sh index 58c9fee..02880fd 100755 --- a/init.sh +++ b/init.sh @@ -325,28 +325,40 @@ migrate_kit_schema() { # cost this repo three bugs in one session. if grep -q '^ unavailable_markers:' "$CONFIG_FILE" \ && ! grep -qi 'review rate limited' "$CONFIG_FILE"; then - awk ' - inserted == 0 && in_list == 1 && $0 !~ /^ - / { - print " - \"review rate limited\" # status-check wording of \"review limit reached\"" - inserted = 1 - in_list = 0 - } - /^ unavailable_markers:/ { in_list = 1 } - { print } - END { - if (inserted == 0 && in_list == 1) + # ONLY a block list is safe to append an item to. An inline flow list + # (`unavailable_markers: ["a", "b"]`) is equally valid YAML and equally + # supported by the config reader, but appending a ` - ` line under it + # produces a mapping with both a scalar value and children — malformed, + # and written straight over the adopter's config. Warn instead. + if grep -qE '^ unavailable_markers:[[:space:]]*(#.*)?$' "$CONFIG_FILE"; then + tmp="$CONFIG_FILE.tmp.$$" + awk ' + inserted == 0 && in_list == 1 && $0 !~ /^ - / { print " - \"review rate limited\" # status-check wording of \"review limit reached\"" - } - ' "$CONFIG_FILE" > "$CONFIG_FILE.tmp" - # Post-condition, not a trusted exit code: verify the marker actually landed - # before overwriting the adopter's config, and keep the original if it did - # not. A migration that silently no-ops is indistinguishable from one that - # ran — that class of failure has already cost this repo three bugs. - if grep -qi 'review rate limited' "$CONFIG_FILE.tmp"; then - mv "$CONFIG_FILE.tmp" "$CONFIG_FILE" - echo "added the status-check rate-limit marker to config/dev-model.yaml" - else - rm -f "$CONFIG_FILE.tmp" + inserted = 1 + in_list = 0 + } + /^ unavailable_markers:/ { in_list = 1 } + { print } + END { + if (inserted == 0 && in_list == 1) + print " - \"review rate limited\" # status-check wording of \"review limit reached\"" + } + ' "$CONFIG_FILE" > "$tmp" + # Post-condition, not a trusted exit code: verify the marker actually + # landed before overwriting the adopter's config, and keep the original if + # it did not. A migration that silently no-ops is indistinguishable from + # one that ran — that class of failure has already cost this repo three + # bugs in one session. + if grep -qi 'review rate limited' "$tmp"; then + mv "$tmp" "$CONFIG_FILE" + echo "added the status-check rate-limit marker to config/dev-model.yaml" + rate_limit_marker_added=1 + else + rm -f "$tmp" + fi + fi + if [ -z "${rate_limit_marker_added:-}" ]; then echo "WARNING: could not add the \"review rate limited\" marker to review.unavailable_markers" >&2 echo " in $CONFIG_FILE — add it by hand, or a rate-limited review bot that reports" >&2 echo " the outage only as a status-check description will read as a clean review." >&2 diff --git a/kit-manifest.json b/kit-manifest.json index 973eb87..477ee99 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -99,7 +99,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "adb1e2fc3f22c4e562189b0588a1404ca47866ae0ac9e3ad15eb4598f4335e44" + "sha256": "f3317b99cf1eab666250af6a7ccb050bb7b9bab8dd5897ffa70ae4a8db508225" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index f890d18..72d8f80 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -413,6 +413,27 @@ def _gh_json(args: list[str]): return json.loads(_gh(args)) +_bot_signal_warned = False + + +def _warn_bot_signal_lost(reason: str) -> None: + """Say once, on stderr, that the review-bot guards are running blind. + + Once per process, not per poll: a watch loop calls this every round, and a + warning repeated forty times is skimmed past exactly like silence. + """ + global _bot_signal_warned + if _bot_signal_warned: + return + _bot_signal_warned = True + print( + f"warning: could not read check details ({' '.join(reason.split())[:160]}); " + "review-bot pending/outage state is unavailable, so a queued or " + "rate-limited reviewer will not be detected", + file=sys.stderr, + ) + + def fetch_check_details(pr: int, *, bots: tuple[str, ...] | None = None) -> list[dict]: """Per-check ``{name, state, bucket, description, startedAt}`` for one PR. @@ -437,6 +458,12 @@ def fetch_check_details(pr: int, *, bots: tuple[str, ...] | None = None) -> list ``[]`` — which reads as "no bot signal", the same fail-open direction the informational-check exclusion already takes. + **But it says so, once.** Degrading silently would disable both #19's and + #23's guards without a trace — an older ``gh`` that rejects one of these + ``--json`` fields fails exactly this way, and "checked and clean" would be + indistinguishable from "never checked". Warned once per process rather than + per poll, so a watch loop does not turn one real problem into a wall. + Skips the call entirely when no review bots are configured: with nothing to match, the result could only ever be discarded. """ @@ -461,10 +488,20 @@ def fetch_check_details(pr: int, *, bots: tuple[str, ...] | None = None) -> list cwd=str(REPO_ROOT), timeout=60, ) + except (OSError, subprocess.SubprocessError) as exc: + _warn_bot_signal_lost(str(exc)) + return [] + try: parsed = json.loads(result.stdout) - except (OSError, subprocess.SubprocessError, json.JSONDecodeError): + except json.JSONDecodeError: + # stdout wasn't JSON, so the exit code IS the story here — an older `gh` + # rejecting one of these fields, or a PR with no checks at all. + _warn_bot_signal_lost(result.stderr or f"gh exited {result.returncode}") + return [] + if not isinstance(parsed, list): + _warn_bot_signal_lost("gh pr checks returned an unexpected shape") return [] - return [item for item in parsed if isinstance(item, dict)] if isinstance(parsed, list) else [] + return [item for item in parsed if isinstance(item, dict)] def resolve_pr(explicit: int | None) -> int: @@ -943,9 +980,12 @@ def summarize_review_bots( "pending": pending, "blockers": blockers, # Only the bots still pending: an entry for a bot that has since - # reported would otherwise keep a stale clock alive across polls. + # reported would otherwise keep a stale clock alive across polls, and a + # later re-review would inherit an already-expired window. "pending_since": { - bot: at for bot, at in observed.items() if any(e["bot"] == bot for e in pending) + bot: at + for bot, at in observed.items() + if bot in {entry["bot"] for entry in pending} }, } diff --git a/scripts/tests/test_kitconfig.py b/scripts/tests/test_kitconfig.py index 3607b9b..2bfb2c1 100644 --- a/scripts/tests/test_kitconfig.py +++ b/scripts/tests/test_kitconfig.py @@ -71,9 +71,12 @@ def test_decimal_scalars_parse_as_floats_without_widening_past_pyyaml(): The gate matters more than the feature: `float()` alone would swallow `nan`, `inf` and `1e5`, which PyYAML resolves as strings — so an unguarded version would fix one divergence from the parity invariant and open three. + + That claim is checked against PyYAML rather than restated: asserting only the + expected values would pass just as happily if the reference resolver disagreed + with every one of them. """ - parsed = kitconfig.loads( - """ + text = """ review: grace: 2.5 whole: 15 @@ -83,7 +86,8 @@ def test_decimal_scalars_parse_as_floats_without_widening_past_pyyaml(): bare_exponent: 1e5 version_ish: 1.2.3 """ - ) + parsed = kitconfig.loads(text) + assert parsed["review"]["grace"] == 2.5 assert parsed["review"]["whole"] == 15 assert isinstance(parsed["review"]["whole"], int) @@ -93,6 +97,9 @@ def test_decimal_scalars_parse_as_floats_without_widening_past_pyyaml(): assert parsed["review"]["bare_exponent"] == "1e5" assert parsed["review"]["version_ish"] == "1.2.3" + yaml = pytest.importorskip("yaml", reason="PyYAML absent — parity check skipped") + assert parsed == yaml.safe_load(text) + def test_inline_and_block_lists(): parsed = kitconfig.loads( diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index 679495c..8b0faa9 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -974,6 +974,7 @@ class _Result: def __init__(self, stdout: str, returncode: int) -> None: self.stdout = stdout self.returncode = returncode + self.stderr = "" monkeypatch.setattr( pr_watch.subprocess, @@ -996,6 +997,36 @@ def _boom(*a, **k): assert pr_watch.fetch_check_details(1) == [] +def test_losing_the_bot_signal_warns_once_rather_than_degrading_silently( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + """Both new guards depend on this fetch, so a silent `[]` disables them + without a trace. + + An older `gh` that rejects one of the requested `--json` fields fails exactly + this way, and "checked and clean" would be indistinguishable from "never + checked" — the failure mode that cost this repo three silent-no-op bugs in + one session. Once per process, not per poll: a warning repeated every round + of a watch loop is skimmed past exactly like silence. + """ + pr_watch = _load_pr_watch() + + class _Result: + stdout = "" + stderr = 'unknown JSON field: "startedAt"' + returncode = 1 + + monkeypatch.setattr(pr_watch.subprocess, "run", lambda *a, **k: _Result()) + + assert pr_watch.fetch_check_details(1) == [] + first = capsys.readouterr().err + assert "unknown JSON field" in first + assert "will not be detected" in first + + assert pr_watch.fetch_check_details(1) == [] + assert capsys.readouterr().err == "" # not once per poll + + def test_record_review_refuses_while_the_bot_is_still_queued( monkeypatch: pytest.MonkeyPatch, ) -> None: From 3293604ba03719d5111369d7a454777ff3e2077e Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 15:49:23 +0300 Subject: [PATCH 4/9] Address the dual-lens review: three real defects, all in the new code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fresh-context passes over the diff (adversarial + general-correctness) found almost disjoint sets, which is the doctrine's claim about why a gate needs both. **A stale outage comment permanently cancelled the pending block — #19 walking back in through the door built to close it.** `collect_comments` returns the whole PR history, unscoped by head or age, so one transient rate limit on commit 1 put the bot in `unavailable_bots` for the rest of the PR and waved through every later queued review. Rate limits are transient by construction, which makes "this bot was rate-limited earlier" the ORDINARY state of a later poll, not a corner case. Now only a CHECK-surface outage cancels: a check describes the bot's state now, a comment describes the past. Comment outages are still reported — that is #23's action signal — they just cannot speak for the present. **A corrupt or foreign `bot_pending_since` value wedged the merge gate forever.** `age = parse(stored) or 0.0` pinned an unparseable value at the maximally- blocking age *and* wrote it back, so every later poll re-read the same poison. Reachable from a corrupt state file, an older engine, or a richer future format. Anything unreadable is now replaced rather than coerced, so the window stays bounded whatever wrote the state. **The init.sh migration silently orphaned every existing marker on a 2-space block list.** The guard checked the key line's indent while the awk anchored on 4-space items, so a config formatted with dashes at the key's own indent (what several YAML formatters emit, and what kitconfig explicitly supports) got the new marker inserted at the wrong depth — dropping all the originals, while the post-condition grep still passed and `mv`'d it over the adopter's config. That is fail-OPEN on the very markers this PR exists to extend. Now indent-aware, verified against 4-space, 2-space and inline shapes. Also: `review_bots.signal` makes a failed check-read machine-readable (the merge gate consumes stdout; a stderr warning is not a guard on an autonomous path); the override is recorded on the receipt; comment authors match anchored rather than by substring (that input is attacker-controlled on a public repo, check names are not); the grace bound compares the exact age, not the rounded display value; `render` no longer claims "past the grace" for a check cancelled by an outage; kitconfig's float rule is YAML 1.1's own (signed exponent required) with its three remaining divergences pinned as divergences; and init.sh's idempotency grep is scoped to the marker block so it cannot silently skip both the migration and its warning. Tests 222 → 229. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --- config/dev-model.yaml | 17 +- docs/agentic-dev-kit/workflows/pr-watch.md | 19 +- init.sh | 62 ++-- kit-manifest.json | 6 +- scripts/lib/kitconfig.py | 35 ++- scripts/pr_watch.py | 197 +++++++++---- scripts/tests/test_kitconfig.py | 79 ++++-- scripts/tests/test_pr_watch.py | 313 +++++++++++++++++---- 8 files changed, 559 insertions(+), 169 deletions(-) diff --git a/config/dev-model.yaml b/config/dev-model.yaml index 8cea474..ae38e17 100644 --- a/config/dev-model.yaml +++ b/config/dev-model.yaml @@ -107,12 +107,17 @@ review: - "" - "actionable comments posted: 0" - "" - # Text 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. Matched against both - # comment bodies AND a configured bot's status-check description — the same - # outage is worded differently on the two surfaces, and matching only comments - # made detection depend on which one the bot happened to use (issue #23). + # 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" diff --git a/docs/agentic-dev-kit/workflows/pr-watch.md b/docs/agentic-dev-kit/workflows/pr-watch.md index e7f354b..f6ca5e9 100644 --- a/docs/agentic-dev-kit/workflows/pr-watch.md +++ b/docs/agentic-dev-kit/workflows/pr-watch.md @@ -152,7 +152,11 @@ Self-pace on a bounded cadence — don't busy-wait: - **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. + 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 @@ -168,6 +172,19 @@ Self-pace on a bounded cadence — don't busy-wait: 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). On `unavailable` **both guards are off**; it's reported rather than + blocking, because an environment problem shouldn't become a wedge. + - 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.bots`, `review.noise_markers`, `review.unavailable_markers`, `review.informational_checks` and `review.bot_pending_grace_minutes` are read from diff --git a/init.sh b/init.sh index 02880fd..6f2d01f 100755 --- a/init.sh +++ b/init.sh @@ -323,18 +323,34 @@ migrate_kit_schema() { # anchor is still missed, SAY SO — a migration that silently no-ops is # indistinguishable from one that ran, and that class of failure has already # cost this repo three bugs in one session. + # The idempotency check is scoped to the unavailable_markers BLOCK, not the + # whole file: a whole-file grep also matches the phrase in a noise_markers + # entry or an adopter's own comment, and would then skip both the migration + # AND its warning — a silent no-op, the class this whole block guards against. + existing_markers=$(awk ' + /^ unavailable_markers:/ { in_list = 1; print; next } + in_list == 1 && $0 ~ /^[[:space:]]+- / { print; next } + in_list == 1 { exit } + ' "$CONFIG_FILE") + if grep -q '^ unavailable_markers:' "$CONFIG_FILE" \ - && ! grep -qi 'review rate limited' "$CONFIG_FILE"; then - # ONLY a block list is safe to append an item to. An inline flow list - # (`unavailable_markers: ["a", "b"]`) is equally valid YAML and equally - # supported by the config reader, but appending a ` - ` line under it - # produces a mapping with both a scalar value and children — malformed, - # and written straight over the adopter's config. Warn instead. - if grep -qE '^ unavailable_markers:[[:space:]]*(#.*)?$' "$CONFIG_FILE"; then + && ! printf '%s\n' "$existing_markers" | grep -qi 'review rate limited'; then + # Two shapes are unsafe to append to, and both are valid YAML the config + # reader accepts: + # - an inline flow list (`unavailable_markers: ["a", "b"]`) — appending a + # `- ` line under it yields a key with both a scalar value and children + # - a block list whose dashes sit at the KEY's indent (what several YAML + # formatters emit) — inserting at a deeper indent silently orphans every + # original entry, which the post-condition grep would still pass + # So: require a bare key line, and reuse the indent of the list's own first + # item rather than assuming one. + item_indent=$(printf '%s\n' "$existing_markers" | awk 'NR > 1 { sub(/-.*/, ""); print; exit }') + if grep -qE '^ unavailable_markers:[[:space:]]*(#.*)?$' "$CONFIG_FILE" \ + && [ -n "$item_indent" ]; then tmp="$CONFIG_FILE.tmp.$$" - awk ' - inserted == 0 && in_list == 1 && $0 !~ /^ - / { - print " - \"review rate limited\" # status-check wording of \"review limit reached\"" + awk -v indent="$item_indent" ' + inserted == 0 && in_list == 1 && $0 !~ /^[[:space:]]+- / { + print indent "- \"review rate limited\" # status-check wording of \"review limit reached\"" inserted = 1 in_list = 0 } @@ -342,15 +358,16 @@ migrate_kit_schema() { { print } END { if (inserted == 0 && in_list == 1) - print " - \"review rate limited\" # status-check wording of \"review limit reached\"" + print indent "- \"review rate limited\" # status-check wording of \"review limit reached\"" } ' "$CONFIG_FILE" > "$tmp" - # Post-condition, not a trusted exit code: verify the marker actually - # landed before overwriting the adopter's config, and keep the original if - # it did not. A migration that silently no-ops is indistinguishable from - # one that ran — that class of failure has already cost this repo three - # bugs in one session. - if grep -qi 'review rate limited' "$tmp"; then + # Post-conditions, not a trusted exit code. The marker is inserted EARLY, + # so "the marker is present" alone would also accept a file truncated + # mid-write (disk full, signal) — hence the line count must be exactly one + # more than the original. A migration that silently corrupts is worse than + # one that silently no-ops, and this repo has already paid for both. + if grep -qi 'review rate limited' "$tmp" \ + && [ "$(wc -l < "$tmp")" -eq "$(( $(wc -l < "$CONFIG_FILE") + 1 ))" ]; then mv "$tmp" "$CONFIG_FILE" echo "added the status-check rate-limit marker to config/dev-model.yaml" rate_limit_marker_added=1 @@ -364,6 +381,17 @@ migrate_kit_schema() { echo " the outage only as a status-check description will read as a clean review." >&2 fi fi + + # `review.bots` became load-bearing for the merge gate in the same change: + # pr_watch reads it to decide which checks and comment authors belong to a + # reviewer. The interactive prompt below only REWRITES an existing line, so a + # `review:` section predating the key would silently fall through to the + # engine default. Benign while the default matches, but that is the same + # silent-no-op shape as the bugs above. + if ! grep -q '^ bots:' "$CONFIG_FILE"; then + append_to_section "review:" ' bots: [coderabbit]' + echo "added review.bots to config/dev-model.yaml" + fi } # ── narrative-doc templates ────────────────────────────────────────────── diff --git a/kit-manifest.json b/kit-manifest.json index 477ee99..99ff2b3 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -23,7 +23,7 @@ }, "docs/agentic-dev-kit/workflows/pr-watch.md": { "role": "workflow", - "sha256": "5bd464ad411b24ff4b959293141366fe648421ccadee8fec62335e406056e309" + "sha256": "9c01a5799ecab67c0aeb50de2f61997e6c7e5c2f8a45bde2cd61d21d3809371b" }, "docs/agentic-dev-kit/workflows/session-start.md": { "role": "workflow", @@ -75,7 +75,7 @@ }, "scripts/lib/kitconfig.py": { "role": "engine", - "sha256": "1ef950fcff6167fd2048e13c02e2d1e2f97cbf4d8e0e837cd41e4d4305b3168c" + "sha256": "11079f6327792437b566e8092ed9a3802334a294243319cbe3c6de0607201d86" }, "scripts/lib/repo_root.sh": { "role": "engine", @@ -99,7 +99,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "f3317b99cf1eab666250af6a7ccb050bb7b9bab8dd5897ffa70ae4a8db508225" + "sha256": "f0ed507b8e7d6161f9af52ab69cc49d868abe9f62ebc119d6bdae586827d5408" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/lib/kitconfig.py b/scripts/lib/kitconfig.py index 28fed14..78c2934 100644 --- a/scripts/lib/kitconfig.py +++ b/scripts/lib/kitconfig.py @@ -82,6 +82,20 @@ def _strip_comment(value: str) -> str: return value +# YAML 1.1's float form, minus the `.nan` / `.inf` special cases (see _coerce). +# The exponent's `[-+]` is REQUIRED, matching PyYAML: `1.0e+3` is a float there, +# `1.0e3` is a string. +_YAML_FLOAT_RE = re.compile( + r"""^[-+]? # optional sign + (?: [0-9]+\.[0-9]* # 1. / 1.5 + | \.[0-9]+ # .5 + ) + (?: [eE][-+][0-9]+ )? # optional, SIGNED exponent + $""", + re.VERBOSE, +) + + def _coerce(raw: str) -> Any: """Turn one scalar token into a Python value.""" text = _strip_comment(raw).strip() @@ -98,14 +112,23 @@ def _coerce(raw: str) -> Any: return int(text) except ValueError: pass - # Floats, gated on an explicit decimal point. `float()` alone would accept - # `nan`, `inf` and `1e5`, none of which PyYAML's resolver treats as numbers — - # widening past the point would trade one divergence from the parity - # invariant for three others. A dot is what YAML 1.1 requires too. - if "." in text: + # Floats, matched against YAML 1.1's own resolver rather than handed to + # `float()`. `float()` accepts `nan`, `inf` and `1e5`, and PyYAML resolves + # all three as STRINGS — so the loose version would trade one divergence + # from the parity invariant for three more. Note the exponent's sign is + # mandatory: PyYAML reads `1.0e+3` as 1000.0 but `1.0e3` as the string + # "1.0e3", and matching that exactly is the whole point of using its rule + # instead of an approximation of it. + # + # Three known, deliberate gaps remain, all pinned by the parity test: + # `.nan`, `.inf`/`-.inf`, and YAML 1.1's underscore digit separators + # (`1_0.5` → 10.5 in PyYAML, a string here). None is meaningful as a config + # value in this kit, and supporting them would mean carrying YAML's + # special-form table for no adopter benefit. + if _YAML_FLOAT_RE.match(text): try: return float(text) - except ValueError: + except ValueError: # pragma: no cover — the pattern already guarantees this return text return text diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index 72d8f80..ec02ca0 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -309,10 +309,14 @@ def _load_review_config(config_path: str | Path | None = None) -> ReviewConfig: ``yes``, a typo) keeps the default. The unsafe direction here is reading as *False* by accident, which would let a zero-check PR report green. - ``bot_pending_grace_minutes`` accepts a non-negative number (``bool`` is - rejected despite being an ``int`` subclass). The unsafe direction is a - huge value, which would let a dead bot block the merge gate for hours — - but that direction fails *closed* (merges wait), so a typo is annoying - rather than dangerous, and the default applies to anything non-numeric. + rejected despite being an ``int`` subclass; anything non-numeric keeps the + default). Both directions are reachable and only one is dangerous: a large + value makes a dead bot hold the merge gate for hours — annoying, but + fail-*closed* — while **``0`` disables the guard outright**, since the + bound is ``age >= grace``. That is a legitimate setting for a repo that + wants the #23 outage signal without the #19 wait, but it is opt-out of a + safety check, so set it deliberately rather than as a way to quiet a slow + bot. """ defaults = ReviewConfig( noise_markers=_DEFAULT_NOISE_MARKERS, @@ -434,7 +438,19 @@ def _warn_bot_signal_lost(reason: str) -> None: ) -def fetch_check_details(pr: int, *, bots: tuple[str, ...] | None = None) -> list[dict]: +class CheckDetails(NamedTuple): + """``gh pr checks`` rows plus whether they could be read at all. + + The signal is the point: an empty ``rows`` from a failed fetch is otherwise + byte-identical to an empty one from a genuinely quiet bot, and both #19's and + #23's guards go silently dead in the first case. + """ + + rows: list[dict] + signal: str # "ok" | "skipped" | "unavailable" + + +def fetch_check_details(pr: int, *, bots: tuple[str, ...] | None = None) -> CheckDetails: """Per-check ``{name, state, bucket, description, startedAt}`` for one PR. A SECOND ``gh`` call, and deliberately so. ``gh pr view --json @@ -470,7 +486,7 @@ def fetch_check_details(pr: int, *, bots: tuple[str, ...] | None = None) -> list if bots is None: bots = _REVIEW_BOTS if not bots: - return [] + return CheckDetails([], "skipped") cmd = [ "gh", "pr", @@ -490,18 +506,18 @@ def fetch_check_details(pr: int, *, bots: tuple[str, ...] | None = None) -> list ) except (OSError, subprocess.SubprocessError) as exc: _warn_bot_signal_lost(str(exc)) - return [] + return CheckDetails([], "unavailable") try: parsed = json.loads(result.stdout) except json.JSONDecodeError: # stdout wasn't JSON, so the exit code IS the story here — an older `gh` # rejecting one of these fields, or a PR with no checks at all. _warn_bot_signal_lost(result.stderr or f"gh exited {result.returncode}") - return [] + return CheckDetails([], "unavailable") if not isinstance(parsed, list): _warn_bot_signal_lost("gh pr checks returned an unexpected shape") - return [] - return [item for item in parsed if isinstance(item, dict)] + return CheckDetails([], "unavailable") + return CheckDetails([item for item in parsed if isinstance(item, dict)], "ok") def resolve_pr(explicit: int | None) -> int: @@ -762,16 +778,27 @@ def new_actionable(comments: list[dict], seen: set[str]) -> list[dict]: ] -def _match_bot(text: str, bots: tuple[str, ...]) -> str | None: - """The configured review bot whose name appears in ``text``, if any. +def _match_bot(text: str, bots: tuple[str, ...], *, anchored: bool = False) -> str | None: + """The configured review bot named in ``text``, if any. Case-insensitive. + + Substring by default: one bot key (``coderabbit``) has to cover the check + name GitHub shows (``CodeRabbit``), a namespaced variant (``Review / + CodeRabbit``), and the comment author (``coderabbitai``), which no exact + match spans. - Substring, case-insensitive: one bot key (``coderabbit``) has to cover both - the check name GitHub shows (``CodeRabbit``) and the comment author - (``coderabbitai``), which no exact match spans. + ``anchored`` requires the text to START with the bot key, and is used for + comment authors because that input is not the repo's to control: on a public + repo any account may comment, and an unrelated login merely *containing* + ``coderabbit`` (``xcoderabbit``) should not be read as the reviewer. Check + names come from the repo's own CI and bot configuration, so the looser match + is appropriate there. Different rules because the inputs have different + trust, not by oversight. """ - low = (text or "").strip().lower() + low = str(text or "").strip().lower() if not low: return None + if anchored: + return next((bot for bot in bots if low.startswith(bot)), None) return next((bot for bot in bots if bot in low), None) @@ -795,6 +822,9 @@ def _check_is_pending(detail: dict) -> bool: Prefers gh's own ``bucket`` (its normalization across ``CheckRun`` and ``StatusContext``); falls back to the raw state when a gh version omits it. + A row carrying *neither* reads as pending — fail-closed, so a truncated row + whose name matches a bot holds the merge gate for the grace window rather + than waving it through. """ bucket = (detail.get("bucket") or "").strip().lower() if bucket: @@ -832,6 +862,7 @@ def summarize_review_bots( bots: tuple[str, ...] | None = None, grace_minutes: float | None = None, pending_since: dict | None = None, + signal: str = "ok", ) -> dict: """Resolve each configured review bot to *unavailable*, *pending*, or neither. @@ -853,9 +884,16 @@ def summarize_review_bots( - **unavailable** — an ``unavailable_markers`` hit on either surface: a comment body (already detected today) *or* a bot check's description (#23, the surface that was invisible). Never blocks anything. It is an action - signal: run the configured fallback review. It also SUPPRESSES the pending - block below — a bot that announced it is not reviewing is not "about to - report", so waiting on it would be waiting forever. + signal: run the configured fallback review. + + Only a **check**-surface hit suppresses the pending block below. A check + describes the bot's state now; a comment describes the past, and + ``collect_comments`` returns the entire PR history unscoped by head or + age — so letting a comment cancel would mean one transient rate limit on + commit 1 waves through every queued review for the rest of the PR. Since + rate limits are transient by construction, that is the ordinary case, not + a corner one. A comment-surface outage is reported and nothing more; the + bot's check is what says whether it is still working. - **pending** — the bot's own check is non-terminal and no unavailability was announced. A verdict is genuinely coming, so a receipt recorded now would be premature (#19: exactly this, on #16, let four valid findings land after @@ -880,9 +918,21 @@ def summarize_review_bots( The map is scoped to a head: a push means a fresh review, so the caller resets it. - Returns ``{grace_minutes, unavailable, pending, blockers, pending_since}``. - ``blockers`` are ready-made ``merge_blockers`` strings; ``pending_since`` is - the updated map for the caller to persist. + Returns ``{grace_minutes, signal, unavailable, pending, blockers, + pending_since}``. ``blockers`` are ready-made ``merge_blockers`` strings; + ``pending_since`` is the updated map for the caller to persist. + + ``unavailable[].bot`` is ``None`` when a comment matched a marker but its + author matches no configured bot — a human writing "review skipped" in a PR + comment. Reported (the operator should see it) and attributed to nobody, so + it can never suppress anything. + + ``signal`` distinguishes three states a bare empty result cannot: ``"ok"`` + (checks were read), ``"skipped"`` (no bots configured — nothing to read), + and ``"unavailable"`` (the read failed, so both guards are off). Without it + a failed fetch is byte-identical to a genuinely clean bot, and the merge gate + consumes JSON on stdout — a stderr warning is not readable by + ``dev_session.sh merge``. """ if bots is None: bots = _REVIEW_BOTS @@ -897,28 +947,35 @@ def summarize_review_bots( # comment: once acked it vanished from `new_comments` and with it the fact # that the primary reviewer never ran. Aggregating it here keeps the gap # visible at merge time. + # + # Reported, NOT counted toward `unavailable_bots`. `collect_comments` returns + # the whole PR history, unscoped by head or age, so an outage comment from + # commit 1 would otherwise cancel the pending block on commit 5 — and rate + # limits are transient by construction, which makes "this bot was + # rate-limited earlier on this PR" the *normal* state of a later poll. That + # would be issue #19 walking back in through the door built to close it. + # A check is a statement about the bot's state NOW; a comment is a statement + # about the past. Only the former may cancel (surface 2 below). for comment in comments or []: reason = comment.get("review_unavailable_reason") if not reason: continue author = comment.get("author") or "?" - bot = _match_bot(author, bots) unavailable.append( { - "bot": bot, + "bot": _match_bot(author, bots, anchored=True), "surface": "comment", "where": f"@{author}", "reason": reason, } ) - if bot: - unavailable_bots.add(bot) # Surface 2 — the bot's own check description. Issue #23: on PR #22 this was # the ONLY place the rate limit appeared, and it read as a clean review. pending: list[dict] = [] + exact_ages: list[float] = [] for detail in check_details or []: - name = detail.get("name") or detail.get("context") or "" + name = str(detail.get("name") or detail.get("context") or "") bot = _match_bot(name, bots) if not bot: continue @@ -941,10 +998,20 @@ def summarize_review_bots( source = "check" if age is None: # No usable stamp on the check — fall back to our own first-sighting - # clock, recording one now if this is the first sighting. - since = observed.get(bot) or now.isoformat() + # clock. A stored value that will not parse is REPLACED, not coerced + # to age 0: `age = parse(x) or 0.0` would pin an unparseable value at + # the maximally-blocking age *and* write it back, so every later poll + # re-reads the same poison and the gate blocks forever. Anything we + # cannot read is treated as "no clock yet" and restamped, which keeps + # the window bounded whatever wrote the state (a corrupt file, a + # different engine version, a richer future format). + age = _age_minutes(observed.get(bot), now) + if age is None: + since = now.isoformat() + age = 0.0 + else: + since = observed[bot] observed[bot] = since - age = _age_minutes(since, now) or 0.0 source = "observed" pending.append( { @@ -953,20 +1020,26 @@ def summarize_review_bots( "state": (detail.get("state") or "").upper(), "since": since, "age_source": source, + # Rounded for display only — the grace comparison below uses the + # exact value, so a 14.96m check does not round its way past a + # 15m bound. "age_minutes": round(age, 1), - # Filled in below, once every surface has been read: an - # unavailability announced in a *comment* must be able to cancel - # a pending *check*, and the comment loop already ran. "blocking": False, + "cancelled_by": None, } ) + exact_ages.append(age) blockers: list[str] = [] - for entry in pending: + for entry, exact_age in zip(pending, exact_ages): if entry["bot"] in unavailable_bots: - continue # announced as not reviewing — waiting on it is a wedge - if entry["age_minutes"] >= grace_minutes: - continue # aged out — treated as never going to report + # This bot's own CHECK announced an outage: it is not going to move + # off pending, so waiting on it is a wedge with extra steps. + entry["cancelled_by"] = "outage" + continue + if exact_age >= grace_minutes: + entry["cancelled_by"] = "grace" + continue entry["blocking"] = True blockers.append( f"review bot {entry['bot']} has not reported yet " @@ -976,6 +1049,7 @@ def summarize_review_bots( return { "grace_minutes": grace_minutes, + "signal": "skipped" if not bots else signal, "unavailable": unavailable, "pending": pending, "blockers": blockers, @@ -1217,9 +1291,7 @@ def record_review( expected_head = expected_head.strip() if not expected_head: raise ValueError("expected reviewed head must not be empty") - snapshot = _gh_json( - ["pr", "view", str(pr), "--json", "number,headRefOid,comments,reviews"] - ) + snapshot = _gh_json(["pr", "view", str(pr), "--json", "number,headRefOid"]) current_head = snapshot.get("headRefOid") if not current_head: raise ValueError("PR has no headRefOid; cannot bind review evidence") @@ -1231,15 +1303,17 @@ def record_review( now = now or datetime.now(timezone.utc) state = load_state(pr) if not allow_pending_bot: - # Comments are fetched alongside the head so an outage announced in a - # COMMENT can still cancel a pending CHECK. Without them, the documented - # fallback path would be refused in exactly the case it exists for: a - # bot that says "rate limited" while its check never leaves pending. + # Checks only. Comments cannot cancel a pending block (see + # :func:`summarize_review_bots`), so fetching them here would cost a + # round trip to compute nothing — and would give this path a different + # view of the same predicate than the poll has. + details = fetch_check_details(pr) bot_status = summarize_review_bots( - fetch_check_details(pr), - collect_comments(snapshot, []), + details.rows, + [], now=now, pending_since=read_pending_since(state, current_head), + signal=details.signal, ) if bot_status["blockers"]: # Persist the first sighting BEFORE refusing. Without this, a cold @@ -1259,6 +1333,11 @@ def record_review( "source": source, "recorded_at": now.isoformat(), } + if allow_pending_bot: + # The escape hatch on a safety gate is the one thing that must leave a + # trace. Without it, a receipt taken over an active override is + # indistinguishable from one taken after a clean bot verdict. + receipt["override"] = "pending-bot" state["review_receipt"] = receipt save_state(pr, state) return {"pr": pr, "recorded_review": True, "review_receipt": receipt} @@ -1280,7 +1359,7 @@ def build_report( prior_head: str | None = None, prior_max_total: int = 0, review_receipt: dict | None = None, - check_details: list[dict] | None = None, + check_details: list[dict] | CheckDetails | None = None, now: datetime | None = None, prior_pending_since: dict | None = None, # already head-scoped by the caller ) -> dict: @@ -1330,11 +1409,19 @@ def build_report( checks = summarize_checks(view.get("statusCheckRollup") or []) comments = collect_comments(view, inline) fresh = new_actionable(comments, seen) + # A plain list is accepted so a test (or an embedder) can pass rows directly; + # only a CheckDetails carries the "could we read them at all" signal. + details = ( + check_details + if isinstance(check_details, CheckDetails) + else CheckDetails(list(check_details or []), "ok") + ) review_bots = summarize_review_bots( - check_details or [], + details.rows, comments, now=now or datetime.now(timezone.utc), pending_since=prior_pending_since or {}, + signal=details.signal, ) # False-settle guard: right after a push, `gh` can still report the OLD @@ -1469,16 +1556,26 @@ def render(report: dict) -> str: # place (#23). Printed even when the PR is otherwise clean — a clean report # that hides an outage is the exact failure being fixed. bots = report.get("review_bots") or {} + if bots.get("signal") == "unavailable": + lines.append( + " ⚠ review-bot state could not be read — a queued or rate-limited " + "reviewer will NOT be detected on this poll (see stderr)" + ) for entry in bots.get("unavailable") or []: lines.append( f" ⚠ review unavailable [{entry['surface']}] {entry['where']}: " f"{entry['reason']} — run the configured fallback review" ) + grace = bots.get("grace_minutes") for entry in bots.get("pending") or []: - if not entry.get("blocking"): + # Only the aged-out reason is reported here. An entry cancelled by an + # outage already has its own ⚠ line above, and printing "past the 15m + # grace" for a 1-minute-old check — which the single `not blocking` + # branch used to do — states a reason that is simply false. + if entry.get("cancelled_by") == "grace": lines.append( f" ⚠ review bot {entry['bot']} check {entry['check']} still pending after " - f"{entry['age_minutes']}m (past the {bots.get('grace_minutes')}m grace) — " + f"{entry['age_minutes']}m (past the {grace:g}m grace) — " "treated as not coming; run the configured fallback review" ) for blocker in report.get("merge_blockers") or []: diff --git a/scripts/tests/test_kitconfig.py b/scripts/tests/test_kitconfig.py index 2bfb2c1..623436e 100644 --- a/scripts/tests/test_kitconfig.py +++ b/scripts/tests/test_kitconfig.py @@ -64,41 +64,62 @@ def test_nested_mappings_and_scalar_types(): assert parsed["top"]["nested"]["deeper"]["leaf"] == "found" -def test_decimal_scalars_parse_as_floats_without_widening_past_pyyaml(): - """A dotted number is a float; the things `float()` accepts and YAML doesn't - stay strings. +# Every scalar form where "is this a float?" is a judgement call, plus the three +# known-and-deliberate divergences. Kept as one list so the parity check below +# cannot quietly cover a smaller set than the type assertions do. +_FLOAT_EDGE_CASES = ( + "2.5", "15", "-0.5", "+1.5", ".5", "1.", + "1.0e+3", # signed exponent — a float in YAML 1.1 + "1.0e3", "1.0E3", "1.5e3", # UNSIGNED exponent — a string in YAML 1.1 + "nan", "inf", "1e5", "1.2.3", # things bare float() would swallow +) +_KNOWN_PYYAML_DIVERGENCES = { + ".nan": "float nan in PyYAML; string here", + ".inf": "float inf in PyYAML; string here", + "-.inf": "float -inf in PyYAML; string here", + "1_0.5": "10.5 in PyYAML (YAML 1.1 digit separators); string here", +} + + +def test_decimal_scalars_match_pyyaml_except_three_documented_forms(): + """Floats are resolved by YAML 1.1's rule, not by handing text to `float()`. + + `float()` accepts `nan`, `inf` and `1e5`, all of which PyYAML resolves as + strings — so the loose version would trade one divergence from the parity + invariant for three more. The signed exponent is the subtle half: PyYAML + reads `1.0e+3` as 1000.0 and `1.0e3` as a string, and matching that exactly + is why the rule is copied rather than approximated. + + Parity is asserted over the whole edge-case list rather than a couple of + hand-picked values, and the known divergences are pinned as divergences — so + closing one later fails this test instead of passing silently. + """ + yaml = pytest.importorskip("yaml", reason="PyYAML absent — parity check skipped") - The gate matters more than the feature: `float()` alone would swallow `nan`, - `inf` and `1e5`, which PyYAML resolves as strings — so an unguarded version - would fix one divergence from the parity invariant and open three. + for token in _FLOAT_EDGE_CASES: + doc = f"v: {token}\n" + assert kitconfig.loads(doc) == yaml.safe_load(doc), token - That claim is checked against PyYAML rather than restated: asserting only the - expected values would pass just as happily if the reference resolver disagreed - with every one of them. + for token, why in _KNOWN_PYYAML_DIVERGENCES.items(): + doc = f"v: {token}\n" + assert kitconfig.loads(doc)["v"] == token, f"{token} should stay a string ({why})" + assert kitconfig.loads(doc) != yaml.safe_load(doc), ( + f"{token} now agrees with PyYAML — good, but remove it from " + f"_KNOWN_PYYAML_DIVERGENCES and from _coerce's docstring ({why})" + ) + + +def test_decimal_scalars_keep_their_python_types_without_pyyaml(): + """The same resolution, asserted directly — the engines run with no PyYAML, + so the parity test above skips in exactly the environment that matters most. """ - text = """ -review: - grace: 2.5 - whole: 15 - negative: -0.5 - exponent: 1.0e+3 - not_a_number: nan - bare_exponent: 1e5 - version_ish: 1.2.3 -""" - parsed = kitconfig.loads(text) + parsed = kitconfig.loads( + "review:\n grace: 2.5\n whole: 15\n bare_exponent: 1e5\n" + ) assert parsed["review"]["grace"] == 2.5 - assert parsed["review"]["whole"] == 15 - assert isinstance(parsed["review"]["whole"], int) - assert parsed["review"]["negative"] == -0.5 - assert parsed["review"]["exponent"] == 1000.0 - assert parsed["review"]["not_a_number"] == "nan" + assert isinstance(parsed["review"]["whole"], int) # not silently a float assert parsed["review"]["bare_exponent"] == "1e5" - assert parsed["review"]["version_ish"] == "1.2.3" - - yaml = pytest.importorskip("yaml", reason="PyYAML absent — parity check skipped") - assert parsed == yaml.safe_load(text) def test_inline_and_block_lists(): diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index 8b0faa9..072bc73 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -665,7 +665,7 @@ def _minutes_ago(minutes: float) -> str: return (NOW - timedelta(minutes=minutes)).isoformat().replace("+00:00", "Z") -def test_rate_limit_in_a_check_description_is_detected(monkeypatch) -> None: +def test_rate_limit_in_a_check_description_is_detected() -> None: """Issue #23, the exact repro: PR #22 head 32f3e4f. CodeRabbit was rate-limited and said so ONLY in its status-check @@ -760,24 +760,33 @@ def test_a_pending_bot_past_the_grace_window_stops_blocking() -> None: assert status["pending"][0]["blocking"] is False -def test_an_announced_outage_cancels_the_pending_block_on_the_same_bot() -> None: - """"Unavailable" and "pending" are mutually exclusive answers to one question. +def test_only_a_check_surface_outage_cancels_the_pending_block() -> None: + """A check says what the bot is doing NOW; a comment says what it once did. - A bot that has announced it is not reviewing will never move its check off - pending, so waiting for it is a wedge with extra steps. The outage wins, on - either surface — including a COMMENT cancelling a stuck CHECK, which is the - combination the fallback path actually needs. + `collect_comments` returns the whole PR history, unscoped by head or age. If + a comment could cancel, one transient rate limit on commit 1 would wave + through every queued review for the rest of the PR — and since rate limits + are transient by construction, "this bot was rate-limited earlier" is the + ordinary state of a later poll, not a corner case. That is issue #19 walking + back in through the door built to close it. """ pr_watch = _load_pr_watch() stuck = [_bot_check(state="PENDING", bucket="pending", startedAt=_minutes_ago(1))] + stale_outage = [ + {"author": "coderabbitai", "review_unavailable_reason": "review limit reached"} + ] assert pr_watch.summarize_review_bots(stuck, [], now=NOW)["blockers"] != [] - via_comment = pr_watch.summarize_review_bots( - stuck, - [{"author": "coderabbitai", "review_unavailable_reason": "review limit reached"}], - now=NOW, - ) + # The historical comment is REPORTED (the operator still needs to see that + # the primary reviewer had an outage) but does not cancel the live block. + via_comment = pr_watch.summarize_review_bots(stuck, stale_outage, now=NOW) + assert via_comment["blockers"] != [] + assert via_comment["unavailable"][0]["surface"] == "comment" + + # The bot's own check saying so DOES cancel. A check whose description + # carries the outage is classified unavailable outright and never counted as + # pending at all… via_check = pr_watch.summarize_review_bots( [ _bot_check( @@ -790,9 +799,84 @@ def test_an_announced_outage_cancels_the_pending_block_on_the_same_bot() -> None [], now=NOW, ) - - assert via_comment["blockers"] == [] assert via_check["blockers"] == [] + assert via_check["pending"] == [] + assert via_check["unavailable"][0]["surface"] == "check" + + # …and it also cancels a SECOND, separately-pending check from the same bot, + # which is the only way a pending entry can survive to be cancelled. + two_checks = pr_watch.summarize_review_bots( + [ + _bot_check(description="Review rate limited"), + _bot_check( + name="CodeRabbit / incremental", + state="PENDING", + bucket="pending", + startedAt=_minutes_ago(1), + ), + ], + [], + now=NOW, + ) + assert two_checks["blockers"] == [] + assert two_checks["pending"][0]["cancelled_by"] == "outage" + + +def test_a_lookalike_commenter_cannot_speak_for_the_bot() -> None: + """Comment authors are attacker-controlled on a public repo; check names are + the repo's own. Matching them by the same loose rule is what would let + `xcoderabbit` posting "review skipped" impersonate the reviewer. + """ + pr_watch = _load_pr_watch() + bots = ("coderabbit",) + + assert pr_watch._match_bot("CodeRabbit", bots) == "coderabbit" + assert pr_watch._match_bot("Review / CodeRabbit", bots) == "coderabbit" + # Authors are anchored: the real bot logins match, a lookalike does not. + assert pr_watch._match_bot("coderabbitai", bots, anchored=True) == "coderabbit" + assert pr_watch._match_bot("coderabbitai[bot]", bots, anchored=True) == "coderabbit" + assert pr_watch._match_bot("xcoderabbit", bots, anchored=True) is None + + # …and an unattributed outage comment is reported with bot None, so it can + # never suppress anything even if the wording matches. + status = pr_watch.summarize_review_bots( + [_bot_check(state="PENDING", bucket="pending", startedAt=_minutes_ago(1))], + [{"author": "xcoderabbit", "review_unavailable_reason": "review skipped"}], + now=NOW, + ) + assert status["unavailable"][0]["bot"] is None + assert status["blockers"] != [] + + +def test_a_corrupt_or_foreign_clock_value_is_replaced_not_trusted() -> None: + """`age = parse(stored) or 0.0` would pin an unreadable value at the + maximally-blocking age AND write it straight back, so every later poll + re-reads the same poison and the gate blocks forever — a wedge dressed as a + guard. + + Anything unreadable is treated as "no clock yet" and restamped, whatever + wrote it: a corrupt file, an older engine, a richer future format. + """ + pr_watch = _load_pr_watch() + zero_time = [ + _bot_check(state="PENDING", bucket="pending", startedAt="0001-01-01T00:00:00Z") + ] + + for poison in (12345, {"at": "2026-07-25T11:00:00Z"}, "0001-01-01T00:00:00+00:00", ""): + status = pr_watch.summarize_review_bots( + zero_time, [], now=NOW, pending_since={"coderabbit": poison} + ) + # Blocks now (age 0 is a real first sighting)… + assert status["blockers"] != [], poison + # …but the poison is GONE, replaced by a timestamp that will age out. + assert status["pending_since"]["coderabbit"] == NOW.isoformat(), poison + later = pr_watch.summarize_review_bots( + zero_time, + [], + now=NOW + timedelta(minutes=20), + pending_since=status["pending_since"], + ) + assert later["blockers"] == [], poison def test_a_check_with_no_usable_timestamp_falls_back_to_an_observed_clock() -> None: @@ -981,20 +1065,24 @@ def __init__(self, stdout: str, returncode: int) -> None: "run", lambda *a, **k: _Result('[{"name":"CodeRabbit","state":"PENDING"}]', 8), ) - assert pr_watch.fetch_check_details(1) == [ - {"name": "CodeRabbit", "state": "PENDING"} - ] + assert pr_watch.fetch_check_details(1) == ( + [{"name": "CodeRabbit", "state": "PENDING"}], + "ok", + ) monkeypatch.setattr( pr_watch.subprocess, "run", lambda *a, **k: _Result("no checks reported", 1) ) - assert pr_watch.fetch_check_details(1) == [] + assert pr_watch.fetch_check_details(1) == ([], "unavailable") def _boom(*a, **k): raise OSError("gh is not installed") monkeypatch.setattr(pr_watch.subprocess, "run", _boom) - assert pr_watch.fetch_check_details(1) == [] + assert pr_watch.fetch_check_details(1) == ([], "unavailable") + + # No bots configured is a THIRD state: nothing to read, not a failed read. + assert pr_watch.fetch_check_details(1, bots=()) == ([], "skipped") def test_losing_the_bot_signal_warns_once_rather_than_degrading_silently( @@ -1018,12 +1106,12 @@ class _Result: monkeypatch.setattr(pr_watch.subprocess, "run", lambda *a, **k: _Result()) - assert pr_watch.fetch_check_details(1) == [] + assert pr_watch.fetch_check_details(1).rows == [] first = capsys.readouterr().err assert "unknown JSON field" in first assert "will not be detected" in first - assert pr_watch.fetch_check_details(1) == [] + assert pr_watch.fetch_check_details(1).rows == [] assert capsys.readouterr().err == "" # not once per poll @@ -1040,19 +1128,22 @@ def test_record_review_refuses_while_the_bot_is_still_queued( monkeypatch.setattr( pr_watch, "_gh_json", - lambda args: {"number": 16, "headRefOid": "abc123", "comments": [], "reviews": []}, + lambda args: {"number": 16, "headRefOid": "abc123"}, ) monkeypatch.setattr( pr_watch, "fetch_check_details", - lambda pr, **kw: [ - _bot_check( - state="PENDING", - bucket="pending", - description="Review queued", - startedAt=_minutes_ago(2), - ) - ], + lambda pr, **kw: pr_watch.CheckDetails( + [ + _bot_check( + state="PENDING", + bucket="pending", + description="Review queued", + startedAt=_minutes_ago(2), + ) + ], + "ok", + ), ) recorded: list[dict] = [] monkeypatch.setattr(pr_watch, "save_state", lambda pr, state: recorded.append(state)) @@ -1086,16 +1177,19 @@ def test_a_cold_record_review_refusal_expires_instead_of_wedging( monkeypatch.setattr( pr_watch, "_gh_json", - lambda args: {"number": 16, "headRefOid": "abc123", "comments": [], "reviews": []}, + lambda args: {"number": 16, "headRefOid": "abc123"}, ) monkeypatch.setattr( pr_watch, "fetch_check_details", - lambda pr, **kw: [ - _bot_check( - state="PENDING", bucket="pending", startedAt="0001-01-01T00:00:00Z" - ) - ], + lambda pr, **kw: pr_watch.CheckDetails( + [ + _bot_check( + state="PENDING", bucket="pending", startedAt="0001-01-01T00:00:00Z" + ) + ], + "ok", + ), ) store: dict = {} monkeypatch.setattr(pr_watch, "save_state", lambda pr, state: store.update(state)) @@ -1127,34 +1221,28 @@ def test_record_review_allows_the_fallback_when_the_bot_is_unavailable( ) -> None: """The refusal must not block the path it exists to protect. - "A blocked bot is an action signal, run the fallback" is the doctrine — so a - bot that announced an outage in a COMMENT while its check sits stuck at - pending has to let the fallback receipt through. Reading only the check - would refuse exactly here. + "A blocked bot is an action signal, run the fallback" is the doctrine — so + once the bot's own check reports the outage, the fallback receipt goes + through immediately rather than waiting out the grace window. """ pr_watch = _load_pr_watch() monkeypatch.setattr( - pr_watch, - "_gh_json", - lambda args: { - "number": 22, - "headRefOid": "32f3e4f", - "comments": [ - { - "id": "c1", - "author": {"login": "coderabbitai"}, - "body": "Review limit reached.", - } - ], - "reviews": [], - }, + pr_watch, "_gh_json", lambda args: {"number": 22, "headRefOid": "32f3e4f"} ) monkeypatch.setattr( pr_watch, "fetch_check_details", - lambda pr, **kw: [ - _bot_check(state="PENDING", bucket="pending", startedAt=_minutes_ago(1)) - ], + lambda pr, **kw: pr_watch.CheckDetails( + [ + _bot_check( + state="PENDING", + bucket="pending", + description="Review rate limited", + startedAt=_minutes_ago(1), + ) + ], + "ok", + ), ) recorded: list[dict] = [] monkeypatch.setattr(pr_watch, "save_state", lambda pr, state: recorded.append(state)) @@ -1163,6 +1251,117 @@ def test_record_review_allows_the_fallback_when_the_bot_is_unavailable( pr_watch.record_review(22, "fallback:panel", "32f3e4f", now=NOW) assert recorded[0]["review_receipt"]["source"] == "fallback:panel" + assert "override" not in recorded[0]["review_receipt"] + + +def test_the_override_is_recorded_on_the_receipt( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The escape hatch on a safety gate is the one thing that must leave a + trace — otherwise a receipt taken over an active override reads exactly like + one taken after a clean bot verdict.""" + pr_watch = _load_pr_watch() + monkeypatch.setattr( + pr_watch, "_gh_json", lambda args: {"number": 9, "headRefOid": "abc123"} + ) + recorded: list[dict] = [] + monkeypatch.setattr(pr_watch, "save_state", lambda pr, state: recorded.append(state)) + monkeypatch.setattr(pr_watch, "load_state", lambda pr: {}) + + pr_watch.record_review( + 9, "fallback:panel", "abc123", allow_pending_bot=True, now=NOW + ) + + assert recorded[0]["review_receipt"]["override"] == "pending-bot" + + +def test_an_unreadable_bot_signal_is_machine_readable_not_just_a_warning( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """`dev_session.sh merge` consumes the JSON on stdout; stderr scrolls past on + an autonomous run. + + A failed fetch produces a `review_bots` block otherwise byte-identical to a + genuinely clean bot, so without this field "both guards are off" and "the + bot is fine" are the same report. + """ + pr_watch = _load_pr_watch() + + clean = pr_watch.build_report( + _green_view(), [], set(), check_details=[_bot_check()], now=NOW + ) + blind = pr_watch.build_report( + _green_view(), + [], + set(), + check_details=pr_watch.CheckDetails([], "unavailable"), + now=NOW, + ) + + assert clean["review_bots"]["signal"] == "ok" + assert blind["review_bots"]["signal"] == "unavailable" + assert "could not be read" in pr_watch.render(blind) + assert "could not be read" not in pr_watch.render(clean) + # Still fail-open on the gate — an old `gh` is an environment problem, and + # blocking on it would turn that into a wedge. Visible, not blocking. + assert blind["review_bots"]["blockers"] == [] + + +def test_a_cancelled_pending_check_is_not_reported_as_aged_out() -> None: + """The single `not blocking` branch printed "past the 15m grace" for a + check cancelled by an outage — which is a 1-minute-old check being given a + reason that is simply false, in exactly the #22/#24 scenario this PR is + built on.""" + pr_watch = _load_pr_watch() + + cancelled = pr_watch.summarize_review_bots( + [ + _bot_check(description="Review rate limited"), + _bot_check( + name="CodeRabbit / incremental", + state="PENDING", + bucket="pending", + startedAt=_minutes_ago(1), + ), + ], + [], + now=NOW, + ) + assert cancelled["pending"][0]["cancelled_by"] == "outage" + aged = pr_watch.summarize_review_bots( + [_bot_check(state="PENDING", bucket="pending", startedAt=_minutes_ago(120))], + [], + now=NOW, + ) + + def _render(status): + return pr_watch.render( + { + "pr": 1, "url": "u", "converged": True, "mergeable": False, + "checks": {"success": 1, "total": 1, "pending": 0, "failing": []}, + "new_comments": [], "merge_blockers": [], "review_bots": status, + } + ) + + assert "past the" not in _render(cancelled) + assert "review unavailable [check]" in _render(cancelled) + assert "past the 15m grace" in _render(aged) + + +def test_the_grace_bound_uses_the_exact_age_not_the_rounded_one() -> None: + """`age_minutes` is rounded to one decimal for display. Comparing the + rounded value lets a 14.96m check round its way past a 15m bound.""" + pr_watch = _load_pr_watch() + + status = pr_watch.summarize_review_bots( + [_bot_check(state="PENDING", bucket="pending", startedAt=_minutes_ago(14.96))], + [], + now=NOW, + grace_minutes=15, + ) + + assert status["pending"][0]["age_minutes"] == 15.0 # display rounds up + assert status["blockers"] != [] # …the bound does not def test_bot_state_never_reaches_the_watch_loop_predicate() -> None: From 8a1d5dfdf9edad4d8a1db5ee3f6a7dd5cad53917 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 16:06:57 +0300 Subject: [PATCH 5/9] Fix the regressions the fix round introduced (re-review #2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The doctrine's "re-review after every fix round, not one pass" earning its keep: a third fresh-context pass found that two of the three fixes were incomplete and that the round introduced a new data-loss path. **`record_review` computed the bot signal and threw it away.** When the check read fails there are no blockers to raise, so the receipt was taken with the #19 guard simply switched off — and unlike the explicit override, which the same round had just made auditable, this left no trace at all. The commit message even justified `signal` as machine-readable evidence while no consumer read it. Now recorded on the receipt as `bot_signal`, surfaced by the renderer. Still not a refusal: an old `gh`, or a PR with no checks, is an environment problem, and refusing would turn it into a wedge whose only exit is the override flag. **The poison-clock fix missed the parseable half.** A *future-dated* `bot_pending_since` survives `_age_minutes`, gets clamped to age 0 by `max(0.0, …)`, and is re-persisted verbatim — so the block lasts until real time catches up with the stamp. Measured at 30 days on a copied state file. Anything beyond a small skew tolerance is now treated as unusable and restamped, so the window is bounded whatever wrote it. The previous test only covered unparseable values, which is why this survived. **The new `review.bots` migration lost adopter values.** `grep -q '^ bots:'` misses a 4-space-indented `review:` block and then appends a duplicate at 2 spaces, which the reader resolves last-key-wins — silently dropping a `[bugbot, coderabbit]` down to `[coderabbit]`. It also reported success when it had written nothing, and was satisfied by an unrelated `bots:` under any other section. **The marker migration still no-opped silently, and could write to the wrong section.** Scoping the *idempotency grep* to the block left the outer key anchor whole-file, so: a tab- or 4-space-indented key skipped both migration and warning; a same-named list under an earlier section got the marker while the success message claimed `review.unavailable_markers` had it; a comment or blank line mid-list truncated the idempotency view and re-inserted a marker already present; and a file with no trailing newline failed the `wc -l` post-condition. Both migrations now go through `section_range` / `section_lines`, which bound every read and write to the `review:` section at any indent. Every whole-file `grep '^ key:'` in a migration is a latent bug in two directions at once — it misses the key at another indent and matches a same-named key elsewhere — and this change had shipped one of each. Also: kitconfig's float pattern is now transcribed from PyYAML's resolver rather than approximated. Hoisting the sign out of the alternation diverged on `+.5`, `-.5` and `-.5e+3` — the exact "float() is too loose" class the previous commit claimed to have eliminated. Parity verified over 24 forms; the two remaining gaps (`.inf`/`.nan`, sexagesimal) are pinned as gaps. Plus the blocker string no longer reads `pending 15.0m < 15m grace`, and three docs describing anchored author matching as substring are corrected. Migrations verified against seven real config shapes (4-space, 2-space, inline flow, comment-mid-list, empty list, no-trailing-newline, same-named key under another section), each round-tripped through the real kitconfig. Tests 229 → 231. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --- config/dev-model.yaml | 9 +- docs/agentic-dev-kit/workflows/pr-watch.md | 21 ++-- init.sh | 132 ++++++++++++++------- kit-manifest.json | 6 +- scripts/lib/kitconfig.py | 32 ++--- scripts/pr_watch.py | 69 +++++++++-- scripts/tests/test_kitconfig.py | 27 +++-- scripts/tests/test_pr_watch.py | 65 ++++++++++ 8 files changed, 271 insertions(+), 90 deletions(-) diff --git a/config/dev-model.yaml b/config/dev-model.yaml index ae38e17..5498b55 100644 --- a/config/dev-model.yaml +++ b/config/dev-model.yaml @@ -86,9 +86,12 @@ review: # 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 as a case-insensitive SUBSTRING of a check name or comment author — - # `coderabbit` covers the check `CodeRabbit` and the author `coderabbitai` — - # so keep entries specific enough not to collide with a CI job name. + # 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" diff --git a/docs/agentic-dev-kit/workflows/pr-watch.md b/docs/agentic-dev-kit/workflows/pr-watch.md index f6ca5e9..b82a7ba 100644 --- a/docs/agentic-dev-kit/workflows/pr-watch.md +++ b/docs/agentic-dev-kit/workflows/pr-watch.md @@ -166,8 +166,9 @@ Self-pace on a bounded cadence — don't busy-wait: 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. It only ever advances, so the block always - expires on its own. + 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. @@ -181,8 +182,10 @@ Self-pace on a bounded cadence — don't busy-wait: 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). On `unavailable` **both guards are off**; it's reported rather than - blocking, because an environment problem shouldn't become a wedge. + 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 in that state carries a + `bot_signal` key so the audit trail shows the guard didn't run. - 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.** @@ -192,10 +195,12 @@ Self-pace on a bounded cadence — don't busy-wait: `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 as a case-insensitive - substring of a check name or comment author, so `coderabbit` covers the check - `CodeRabbit` and the author `coderabbitai` — keep entries specific enough not to - collide with a CI job name. Editing the literals inside + 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 `/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". diff --git a/init.sh b/init.sh index 6f2d01f..82753d5 100755 --- a/init.sh +++ b/init.sh @@ -188,6 +188,31 @@ append_to_section() { rm -f "$blockfile" } +# The 1-based line range [start end] of a top-level section's body, or "0 0" if +# the section is absent. A section runs from its header to the next line that +# starts in column 1 with a key. +# +# These two helpers exist because every whole-file `grep '^ key:'` in a +# migration is a latent bug in two directions at once: it misses the key when +# the adopter's file uses a different indent, and it MATCHES a same-named key +# under an unrelated section. Both were shipped and both were caught in review. +section_range() { + awk -v section="$1" ' + index($0, section) == 1 && $0 ~ /^[A-Za-z_]/ { inside = 1; start = NR; next } + inside && /^[A-Za-z_][A-Za-z0-9_]*:/ { print start + 1, NR - 1; found = 1; exit } + END { if (!found) print (inside ? start + 1 : 0), (inside ? NR : 0) } + ' "$2" +} + +# The body lines of a top-level section (empty when absent). +section_lines() { + range=$(section_range "$1" "$2") + start=${range% *} + end=${range#* } + [ "$start" -gt 0 ] || return 0 + awk -v s="$start" -v e="$end" 'NR >= s && NR <= e' "$2" +} + # Migrate the original single-runtime schema in place. Re-running init.sh is # documented as idempotent; silently leaving an old config without these keys # would make the new runtime-aware launchers appear bootstrapped while falling @@ -316,45 +341,75 @@ migrate_kit_schema() { echo "added review.bot_pending_grace_minutes to config/dev-model.yaml" fi + # `review.bots` became load-bearing for the merge gate in the same change: + # pr_watch reads it to decide which checks and comment authors belong to a + # reviewer. The interactive prompt later only REWRITES an existing line, so a + # `review:` section predating the key would silently fall through to the + # engine default — benign while the default matches, dangerous for an adopter + # whose reviewer is not CodeRabbit. + # + # Scoped to the review SECTION, at any indent. A `^ bots:` guard would both + # miss a 4-space-indented `review:` block (appending a duplicate at 2 spaces, + # which the reader resolves last-key-wins — silently dropping the adopter's + # real value) and be satisfied by an unrelated `bots:` under another section. + if [ -z "$(section_lines review: "$CONFIG_FILE" | grep -E '^[[:space:]]+bots:')" ]; then + append_to_section "review:" ' bots: [coderabbit]' + if [ -n "$(section_lines review: "$CONFIG_FILE" | grep -E '^[[:space:]]+bots:')" ]; then + echo "added review.bots to config/dev-model.yaml" + else + echo "WARNING: could not add review.bots to $CONFIG_FILE — add it by hand," >&2 + echo " or pr_watch will assume your review bot is CodeRabbit." >&2 + fi + fi + # The status-check wording of the same rate-limit outage. Added separately for # the same reason: an adopter migrated before it existed keeps their list. - # Anchored on the FIRST entry under unavailable_markers rather than on a - # specific marker text, which an adopter is free to have edited away. If the - # anchor is still missed, SAY SO — a migration that silently no-ops is - # indistinguishable from one that ran, and that class of failure has already - # cost this repo three bugs in one session. - # The idempotency check is scoped to the unavailable_markers BLOCK, not the - # whole file: a whole-file grep also matches the phrase in a noise_markers - # entry or an adopter's own comment, and would then skip both the migration - # AND its warning — a silent no-op, the class this whole block guards against. - existing_markers=$(awk ' - /^ unavailable_markers:/ { in_list = 1; print; next } - in_list == 1 && $0 ~ /^[[:space:]]+- / { print; next } + # + # EVERYTHING here is scoped to the review section and warns on any path it + # cannot complete. Both properties are load-bearing and both were got wrong + # first time: a whole-file idempotency grep matches the phrase in a + # `noise_markers` entry and skips the migration AND its warning; a whole-file + # key anchor happily inserts the marker into a same-named list under a + # different section, then reports success. A migration that silently no-ops is + # indistinguishable from one that ran, and one that silently writes to the + # wrong place is worse. + markers_block=$(section_lines review: "$CONFIG_FILE" | awk ' + /^[[:space:]]+unavailable_markers:/ { in_list = 1; print; next } + # Comments and blank lines do NOT end the list — treating them as the end + # truncates the idempotency view, which re-inserts a marker that is already + # there a few lines further down. + in_list == 1 && ($0 ~ /^[[:space:]]+- / || $0 ~ /^[[:space:]]*(#.*)?$/) { print; next } in_list == 1 { exit } - ' "$CONFIG_FILE") + ') - if grep -q '^ unavailable_markers:' "$CONFIG_FILE" \ - && ! printf '%s\n' "$existing_markers" | grep -qi 'review rate limited'; then - # Two shapes are unsafe to append to, and both are valid YAML the config - # reader accepts: + if [ -n "$markers_block" ] \ + && ! printf '%s\n' "$markers_block" | grep -qi 'review rate limited'; then + # Two list shapes are unsafe to append to, and both are valid YAML the + # config reader accepts: # - an inline flow list (`unavailable_markers: ["a", "b"]`) — appending a # `- ` line under it yields a key with both a scalar value and children - # - a block list whose dashes sit at the KEY's indent (what several YAML - # formatters emit) — inserting at a deeper indent silently orphans every - # original entry, which the post-condition grep would still pass + # - a block list whose dashes sit at the KEY's own indent (what several + # YAML formatters emit) — inserting at a deeper indent silently orphans + # every original entry, and the marker-present post-condition still passes # So: require a bare key line, and reuse the indent of the list's own first # item rather than assuming one. - item_indent=$(printf '%s\n' "$existing_markers" | awk 'NR > 1 { sub(/-.*/, ""); print; exit }') - if grep -qE '^ unavailable_markers:[[:space:]]*(#.*)?$' "$CONFIG_FILE" \ + key_line=$(printf '%s\n' "$markers_block" | head -n 1) + item_indent=$(printf '%s\n' "$markers_block" \ + | awk 'NR > 1 && /^[[:space:]]+- / { sub(/-.*/, ""); print; exit }') + if printf '%s\n' "$key_line" | grep -qE '^[[:space:]]+unavailable_markers:[[:space:]]*(#.*)?$' \ && [ -n "$item_indent" ]; then tmp="$CONFIG_FILE.tmp.$$" - awk -v indent="$item_indent" ' + # `start`/`end` bound the edit to the review section's line range, so a + # same-named key elsewhere in the file cannot be targeted by mistake. + awk -v indent="$item_indent" \ + -v start="$(section_range review: "$CONFIG_FILE" | cut -d' ' -f1)" \ + -v end="$(section_range review: "$CONFIG_FILE" | cut -d' ' -f2)" ' inserted == 0 && in_list == 1 && $0 !~ /^[[:space:]]+- / { print indent "- \"review rate limited\" # status-check wording of \"review limit reached\"" inserted = 1 in_list = 0 } - /^ unavailable_markers:/ { in_list = 1 } + NR >= start && NR <= end && /^[[:space:]]+unavailable_markers:/ { in_list = 1 } { print } END { if (inserted == 0 && in_list == 1) @@ -363,11 +418,13 @@ migrate_kit_schema() { ' "$CONFIG_FILE" > "$tmp" # Post-conditions, not a trusted exit code. The marker is inserted EARLY, # so "the marker is present" alone would also accept a file truncated - # mid-write (disk full, signal) — hence the line count must be exactly one - # more than the original. A migration that silently corrupts is worse than - # one that silently no-ops, and this repo has already paid for both. - if grep -qi 'review rate limited' "$tmp" \ - && [ "$(wc -l < "$tmp")" -eq "$(( $(wc -l < "$CONFIG_FILE") + 1 ))" ]; then + # mid-write (disk full, signal) — hence the record count must be exactly + # one more than the original. `awk END{print NR}` rather than `wc -l` so a + # file with no trailing newline is counted the same way on both sides. + before=$(awk 'END { print NR }' "$CONFIG_FILE") + after=$(awk 'END { print NR }' "$tmp") + if printf '%s\n' "$(section_lines review: "$tmp")" | grep -qi 'review rate limited' \ + && [ "$after" -eq "$(( before + 1 ))" ]; then mv "$tmp" "$CONFIG_FILE" echo "added the status-check rate-limit marker to config/dev-model.yaml" rate_limit_marker_added=1 @@ -380,17 +437,12 @@ migrate_kit_schema() { echo " in $CONFIG_FILE — add it by hand, or a rate-limited review bot that reports" >&2 echo " the outage only as a status-check description will read as a clean review." >&2 fi - fi - - # `review.bots` became load-bearing for the merge gate in the same change: - # pr_watch reads it to decide which checks and comment authors belong to a - # reviewer. The interactive prompt below only REWRITES an existing line, so a - # `review:` section predating the key would silently fall through to the - # engine default. Benign while the default matches, but that is the same - # silent-no-op shape as the bugs above. - if ! grep -q '^ bots:' "$CONFIG_FILE"; then - append_to_section "review:" ' bots: [coderabbit]' - echo "added review.bots to config/dev-model.yaml" + elif [ -z "$markers_block" ]; then + # No unavailable_markers under review: at all. The engine default applies, + # which already contains the marker — but say so rather than staying silent, + # since "absent" and "present and migrated" are indistinguishable otherwise. + echo "note: review.unavailable_markers is absent from $CONFIG_FILE;" >&2 + echo " pr_watch's built-in defaults apply (they include the new marker)." >&2 fi } diff --git a/kit-manifest.json b/kit-manifest.json index 99ff2b3..555a80e 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -23,7 +23,7 @@ }, "docs/agentic-dev-kit/workflows/pr-watch.md": { "role": "workflow", - "sha256": "9c01a5799ecab67c0aeb50de2f61997e6c7e5c2f8a45bde2cd61d21d3809371b" + "sha256": "9b381d3d2bead4130e950e3e4e89df72089fafea969d9c80e55efa227bef9107" }, "docs/agentic-dev-kit/workflows/session-start.md": { "role": "workflow", @@ -75,7 +75,7 @@ }, "scripts/lib/kitconfig.py": { "role": "engine", - "sha256": "11079f6327792437b566e8092ed9a3802334a294243319cbe3c6de0607201d86" + "sha256": "f24b7122ac6e46262427c7eaff7babac84dd24337a8efdd93ade511ae5f7dc8d" }, "scripts/lib/repo_root.sh": { "role": "engine", @@ -99,7 +99,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "f0ed507b8e7d6161f9af52ab69cc49d868abe9f62ebc119d6bdae586827d5408" + "sha256": "975a949e7a346ed32c3fe6325022ae66511d712191df86ffd944366452b964a9" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/lib/kitconfig.py b/scripts/lib/kitconfig.py index 78c2934..cd3d86d 100644 --- a/scripts/lib/kitconfig.py +++ b/scripts/lib/kitconfig.py @@ -82,16 +82,18 @@ def _strip_comment(value: str) -> str: return value -# YAML 1.1's float form, minus the `.nan` / `.inf` special cases (see _coerce). -# The exponent's `[-+]` is REQUIRED, matching PyYAML: `1.0e+3` is a float there, -# `1.0e3` is a string. +# Transcribed from PyYAML's own float resolver, not approximated from it — the +# sign rules are asymmetric in a way no reasonable guess reproduces: +# - the exponent's [-+] is REQUIRED (`1.0e+3` is a float, `1.0e3` a string) +# - a leading sign is allowed on `digits.digits` but NOT on `.digits` +# (`-0.5` is a float, `-.5` a string) +# - `_` is a digit separator anywhere in the mantissa (`1_0.5` -> 10.5) +# Omits PyYAML's `.inf` / `.nan` / sexagesimal (`1:30.0`) branches — see _coerce. _YAML_FLOAT_RE = re.compile( - r"""^[-+]? # optional sign - (?: [0-9]+\.[0-9]* # 1. / 1.5 - | \.[0-9]+ # .5 - ) - (?: [eE][-+][0-9]+ )? # optional, SIGNED exponent - $""", + r"""^(?: + [-+]? [0-9][0-9_]* \. [0-9_]* (?:[eE][-+][0-9]+)? # 1. 1.5 -0.5 1_0.5 + | \. [0-9][0-9_]* (?:[eE][-+][0-9]+)? # .5 (unsigned only) + )$""", re.VERBOSE, ) @@ -120,14 +122,14 @@ def _coerce(raw: str) -> Any: # "1.0e3", and matching that exactly is the whole point of using its rule # instead of an approximation of it. # - # Three known, deliberate gaps remain, all pinned by the parity test: - # `.nan`, `.inf`/`-.inf`, and YAML 1.1's underscore digit separators - # (`1_0.5` → 10.5 in PyYAML, a string here). None is meaningful as a config - # value in this kit, and supporting them would mean carrying YAML's - # special-form table for no adopter benefit. + # Two known, deliberate gaps remain, both pinned by the parity test: YAML + # 1.1's `.inf` / `.nan` special forms, and its sexagesimal floats (`1:30.0` + # → 90.0). Neither is meaningful as a config value in this kit, and + # supporting them would mean carrying YAML's special-form table for no + # adopter benefit. if _YAML_FLOAT_RE.match(text): try: - return float(text) + return float(text.replace("_", "")) # `_` is a YAML digit separator except ValueError: # pragma: no cover — the pattern already guarantees this return text return text diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index ec02ca0..202e34a 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -241,8 +241,9 @@ def _resolve_state_root(start: Path, repo_root: Path, state_root_env: str | None # about"). Keeping them separate is what lets a bot's check stay non-blocking # for `converged` while its state still informs the merge gate — the exact # split issues #19 and #23 need. Matched as a case-insensitive SUBSTRING of a -# check name or comment author, so `coderabbit` covers the check `CodeRabbit` -# and the author `coderabbitai`; keep entries specific enough not to collide +# check name, and as a case-insensitive PREFIX of a comment author (that input +# is not the repo's to control) — so `coderabbit` covers the check `CodeRabbit` +# and the author `coderabbitai`. Keep entries specific enough not to collide # with a CI job name. _DEFAULT_REVIEW_BOTS = ("coderabbit",) @@ -832,6 +833,13 @@ def _check_is_pending(detail: dict) -> bool: return (detail.get("state") or "").strip().upper() not in _TERMINAL_CHECK_STATES +# How far ahead of `now` a timestamp may sit and still be believed. Small skew +# between GitHub's clock and ours is normal on a just-created check; a value +# genuinely in the future is not a clock, it is corruption — and one that +# `max(0.0, …)` would clamp to age 0 forever while re-persisting itself. +_FUTURE_SKEW_TOLERANCE_MINUTES = 2.0 + + def _age_minutes(timestamp: str | None, now: datetime) -> float | None: """Minutes between ``timestamp`` (ISO 8601) and ``now``; ``None`` if unusable. @@ -851,7 +859,15 @@ def _age_minutes(timestamp: str | None, now: datetime) -> float | None: parsed = parsed.replace(tzinfo=timezone.utc) if parsed.year < 2000: return None - return max(0.0, (now - parsed).total_seconds() / 60.0) + age = (now - parsed).total_seconds() / 60.0 + if age < -_FUTURE_SKEW_TOLERANCE_MINUTES: + # Meaningfully in the future: unusable, NOT age 0. Clamping it would pin + # the grace clock at its most-blocking value while the caller re-persists + # the same future stamp every poll — the block would then last until real + # time caught up with it. Reachable from a state file copied between + # machines, or a VM clock that ran ahead and was then NTP-corrected. + return None + return max(0.0, age) def summarize_review_bots( @@ -1043,7 +1059,9 @@ def summarize_review_bots( entry["blocking"] = True blockers.append( f"review bot {entry['bot']} has not reported yet " - f"(check {entry['check']} pending {entry['age_minutes']}m " + # The exact age, not the display-rounded one: `pending 15.0m < 15m + # grace` is a true statement rendered as a self-contradiction. + f"(check {entry['check']} pending {exact_age:.2f}m " f"< {grace_minutes:g}m grace)" ) @@ -1278,12 +1296,15 @@ def record_review( judgment the doctrine asks for — *is this bot unavailable, or merely slow?* — is made right here, so this is where it gets mechanized. - It refuses only for a bot that is genuinely mid-review: an announced outage - (either surface) means "not coming" and is allowed straight through to the - fallback, and a check pending past the grace window ages out of blocking. See - :func:`summarize_review_bots`. ``allow_pending_bot`` is the operator's - documented override for the remaining case — evidence the queued review will - never arrive that pr_watch cannot see. + It refuses only for a bot that is genuinely mid-review. An outage announced + on the bot's own **check** means "not coming" and goes straight through to + the fallback; a check pending past the grace window ages out of blocking. An + outage announced only in a *comment* does NOT clear the refusal — comments + are unscoped PR history, so a stale one would wave through a live review (see + :func:`summarize_review_bots`) — but the grace window bounds that wait to + minutes. ``allow_pending_bot`` is the operator's documented override for the + remaining case: evidence the queued review will never arrive that pr_watch + cannot see. It is recorded on the receipt, as is a failed check read. """ source = source.strip() if not source: @@ -1302,6 +1323,9 @@ def record_review( ) now = now or datetime.now(timezone.utc) state = load_state(pr) + # Stays "ok" under an explicit override: `override` already records that the + # bot state was deliberately not consulted, so a second key would be noise. + bot_signal = "ok" if not allow_pending_bot: # Checks only. Comments cannot cancel a pending block (see # :func:`summarize_review_bots`), so fetching them here would cost a @@ -1315,6 +1339,7 @@ def record_review( pending_since=read_pending_since(state, current_head), signal=details.signal, ) + bot_signal = bot_status["signal"] if bot_status["blockers"]: # Persist the first sighting BEFORE refusing. Without this, a cold # `--record-review` (no poll loop running) restarts the grace clock @@ -1338,6 +1363,15 @@ def record_review( # trace. Without it, a receipt taken over an active override is # indistinguishable from one taken after a clean bot verdict. receipt["override"] = "pending-bot" + if bot_signal != "ok": + # The SILENT bypass, and the worse of the two: when the check read fails + # there are no blockers to raise, so the receipt is taken with the #19 + # guard simply switched off. Recording an explicit override but not this + # would leave the deliberate escape auditable and the accidental one + # invisible. Not a refusal — a `gh` too old for these fields, or a PR + # with no checks at all, is an environment problem, and refusing would + # turn it into a wedge with no way out but the override flag. + receipt["bot_signal"] = bot_signal state["review_receipt"] = receipt save_state(pr, state) return {"pr": pr, "recorded_review": True, "review_receipt": receipt} @@ -1596,10 +1630,21 @@ def render(report: dict) -> str: def render_record_review(report: dict) -> str: receipt = report["review_receipt"] - return ( + lines = [ f"PR #{report['pr']} — recorded independent review from " f"{receipt['source']} for head {receipt['head']}" - ) + ] + if receipt.get("override"): + lines.append( + f" ⚠ recorded over an active override ({receipt['override']}) — a " + "configured review bot had not reported yet" + ) + if receipt.get("bot_signal"): + lines.append( + f" ⚠ review-bot state was unreadable ({receipt['bot_signal']}) when this " + "receipt was taken — the queued-reviewer guard did not run" + ) + return "\n".join(lines) def render_mark_seen(report: dict) -> str: diff --git a/scripts/tests/test_kitconfig.py b/scripts/tests/test_kitconfig.py index 623436e..d5e7989 100644 --- a/scripts/tests/test_kitconfig.py +++ b/scripts/tests/test_kitconfig.py @@ -64,31 +64,40 @@ def test_nested_mappings_and_scalar_types(): assert parsed["top"]["nested"]["deeper"]["leaf"] == "found" -# Every scalar form where "is this a float?" is a judgement call, plus the three +# Every scalar form where "is this a float?" is a judgement call, plus the # known-and-deliberate divergences. Kept as one list so the parity check below # cannot quietly cover a smaller set than the type assertions do. _FLOAT_EDGE_CASES = ( - "2.5", "15", "-0.5", "+1.5", ".5", "1.", + "2.5", "15", "1.", "0.0", "-0.0", + "-0.5", "+1.5", # sign IS allowed on the digits.digits form + ".5", "-.5", "+.5", # …and is NOT allowed on the .digits form "1.0e+3", # signed exponent — a float in YAML 1.1 - "1.0e3", "1.0E3", "1.5e3", # UNSIGNED exponent — a string in YAML 1.1 - "nan", "inf", "1e5", "1.2.3", # things bare float() would swallow + "1.0e3", "1.0E3", "1.5e3", "-.5e+3", # UNSIGNED exponent — a string + "1_0.5", "._5", # `_` is a digit separator, but not in the leading position + "nan", "inf", "1e5", "1.2.3", # things a bare float() would swallow ) _KNOWN_PYYAML_DIVERGENCES = { ".nan": "float nan in PyYAML; string here", ".inf": "float inf in PyYAML; string here", "-.inf": "float -inf in PyYAML; string here", - "1_0.5": "10.5 in PyYAML (YAML 1.1 digit separators); string here", + "1:30.0": "90.0 in PyYAML (YAML 1.1 sexagesimal); string here", } -def test_decimal_scalars_match_pyyaml_except_three_documented_forms(): +def test_decimal_scalars_match_pyyaml_except_the_documented_forms(): """Floats are resolved by YAML 1.1's rule, not by handing text to `float()`. `float()` accepts `nan`, `inf` and `1e5`, all of which PyYAML resolves as strings — so the loose version would trade one divergence from the parity - invariant for three more. The signed exponent is the subtle half: PyYAML - reads `1.0e+3` as 1000.0 and `1.0e3` as a string, and matching that exactly - is why the rule is copied rather than approximated. + invariant for three more. + + The sign rules are the subtle half, and they are asymmetric in a way no + reasonable guess reproduces: the exponent's sign is REQUIRED (`1.0e+3` is a + float, `1.0e3` a string), and a leading sign is allowed on `digits.digits` + but not on `.digits` (`-0.5` is a float, `-.5` a string). That is why the + pattern is transcribed from PyYAML's resolver rather than approximated — + an earlier version hoisted the sign out of the alternation and silently + diverged on all three of `+.5`, `-.5`, `-.5e+3`. Parity is asserted over the whole edge-case list rather than a couple of hand-picked values, and the known divergences are pinned as divergences — so diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index 072bc73..a79394e 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -1254,6 +1254,71 @@ def test_record_review_allows_the_fallback_when_the_bot_is_unavailable( assert "override" not in recorded[0]["review_receipt"] +def test_a_future_dated_clock_is_replaced_rather_than_clamped() -> None: + """The incomplete half of the poison-clock fix. + + An unparseable value is replaced, but a future-dated one is perfectly + *parseable* — so it survived, got clamped to age 0 by `max(0.0, …)`, and was + re-persisted verbatim. The block then lasted until real time caught up with + the stamp: a state file copied between machines, or a VM clock that ran ahead + and was NTP-corrected back, could hold the merge gate for days. + """ + pr_watch = _load_pr_watch() + zero_time = [ + _bot_check(state="PENDING", bucket="pending", startedAt="0001-01-01T00:00:00Z") + ] + far_future = (NOW + timedelta(days=30)).isoformat() + + status = pr_watch.summarize_review_bots( + zero_time, [], now=NOW, pending_since={"coderabbit": far_future} + ) + + assert status["pending_since"]["coderabbit"] == NOW.isoformat() + assert status["blockers"] != [] + # …and it ages out on our clock, in minutes rather than in a month. + later = pr_watch.summarize_review_bots( + zero_time, + [], + now=NOW + timedelta(minutes=20), + pending_since=status["pending_since"], + ) + assert later["blockers"] == [] + + # Small skew stays tolerated — a check GitHub stamped a few seconds ahead of + # our clock is normal and must not be thrown away as corruption. + skewed = (NOW + timedelta(seconds=30)).isoformat() + assert pr_watch._age_minutes(skewed, NOW) == 0.0 + + +def test_a_failed_check_read_is_recorded_on_the_receipt( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The silent bypass, and the worse of the two. + + When the check read fails there are no blockers to raise, so the receipt is + taken with the #19 guard simply switched off. Recording the deliberate + override but not this would leave the intentional escape auditable and the + accidental one invisible. + """ + pr_watch = _load_pr_watch() + monkeypatch.setattr( + pr_watch, "_gh_json", lambda args: {"number": 9, "headRefOid": "abc123"} + ) + monkeypatch.setattr( + pr_watch, + "fetch_check_details", + lambda pr, **kw: pr_watch.CheckDetails([], "unavailable"), + ) + recorded: list[dict] = [] + monkeypatch.setattr(pr_watch, "save_state", lambda pr, state: recorded.append(state)) + monkeypatch.setattr(pr_watch, "load_state", lambda pr: {}) + + report = pr_watch.record_review(9, "fallback:panel", "abc123", now=NOW) + + assert recorded[0]["review_receipt"]["bot_signal"] == "unavailable" + assert "guard did not run" in pr_watch.render_record_review(report) + + def test_the_override_is_recorded_on_the_receipt( monkeypatch: pytest.MonkeyPatch, ) -> None: From 954b93fd2fd995f7bfee94c8d63c527e5db7c8dd Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 16:31:10 +0300 Subject: [PATCH 6/9] Stop doing surgery on adopters' config, and test what's left (re-review #3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third pass. It found that 8a1d5df applied its own section-scoping fix to ONE of three guards in the same function, that the marker insertion had a third corruption mode, and that the highest-blast-radius code in the diff had no tests at all. All three are the same underlying mistake: shell text-munging on a file the adopter owns, defended one special case at a time. **The marker migration is gone.** Three review rounds produced three distinct ways for list surgery to corrupt a config — wrong indent orphaned every existing entry; a whole-file key anchor wrote into a same-named list under another section; and inserting before "the first line that is not an item" splices a multi-line item in half, yielding YAML PyYAML refuses to load *entirely*. Each passed the migration's own post-conditions and printed success. The payoff was one string in a list. `init.sh` now detects the gap and prints exactly what to add — a trade that stops being worth defending after the third corruption. **Guards are section-scoped everywhere now, not in one place.** A whole-file `grep '^ key:'` fails in two directions at once: it misses the key when the adopter's section uses another indent, and it is satisfied by a same-named key elsewhere. `migrate_kit_schema` had one of each; `migrate_runtime_schema` still had three more, which is why a 4-space config was non-idempotent — every run re-appended `fallback_commands`. **Per-key review migration.** One `noise_markers` guard used to gate a block defining five keys, so an adopter with `unavailable_markers` but no `noise_markers` got a second definition of it — and both readers resolve last-key-wins, silently replacing their list with the kit's defaults. **`append_to_section` re-indents to the section it is writing into.** Appending a 2-space block to a 4-space section produced a mapping with two indent levels: PyYAML fails on the WHOLE file, kitconfig tolerates it and applies last-key-wins. Note what did NOT ship: making this function return non-zero on "section not found" seemed like the obvious way to stop callers reporting false success — under `set -eu`, with callers that don't test its status, it aborted init.sh on the first config missing any optional section. Caught by running it. **Engine fixes.** The observed clock now wins over a check's own stamp whenever we have one: preferring the check let the age REGRESS when a future-dated stamp slid inside the skew tolerance, restarting a window that had already expired and making merge_blockers non-monotonic in wall-clock time. And `bot_signal` is recorded only for `unavailable`, not for `skipped` — "you configured no bots" was being reported as "the guard did not run", putting a permanent false warning on every receipt a bot-less adopter takes. **Tests, finally.** Eight config shapes (2-space, 4-space, flush-indent, inline flow, decoy section, header with trailing space, multi-line item, no trailing newline) × corruption, idempotency, exactly-once, and the instruct-when-it-can't path. Every corruption above fails at least one. The kitconfig-vs-PyYAML disagreement on multi-line scalars is asserted as a known reader limitation rather than skipped, so closing it later fails loudly. Verified the shipped config is unchanged relative to main (both reformat the same 5 comment-alignment lines via the pre-existing set_field). Tests 231 → 249. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --- docs/agentic-dev-kit/workflows/pr-watch.md | 5 +- init.sh | 210 +++++++++------------ kit-manifest.json | 4 +- scripts/pr_watch.py | 50 +++-- scripts/tests/test_portability.py | 185 ++++++++++++++++++ 5 files changed, 313 insertions(+), 141 deletions(-) diff --git a/docs/agentic-dev-kit/workflows/pr-watch.md b/docs/agentic-dev-kit/workflows/pr-watch.md index b82a7ba..44a5e63 100644 --- a/docs/agentic-dev-kit/workflows/pr-watch.md +++ b/docs/agentic-dev-kit/workflows/pr-watch.md @@ -184,8 +184,9 @@ Self-pace on a bounded cadence — don't busy-wait: (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 in that state carries a - `bot_signal` key so the audit trail shows the guard didn't run. + 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.** diff --git a/init.sh b/init.sh index 82753d5..7128321 100755 --- a/init.sh +++ b/init.sh @@ -165,19 +165,39 @@ insert_before_section() { } # Append a block to a named top-level section, immediately before the next one. +# +# Always returns 0. It is called under `set -eu` by migrations that do not test +# its status, so returning non-zero on "section not found" aborts init.sh +# entirely — which is how an earlier version of this change broke adoption for +# every config lacking one optional section. Callers that need to know whether +# the write landed must check the file afterwards; `ensure_review_key` does. +# +# The header is matched by PREFIX, not equality, so `review: `, `review:\r` and +# `review: # comment` are still the section. The old `$0 == section` missed all +# three and disagreed with section_range about whether the section existed. append_to_section() { section="$1" block="$2" tmpfile="${CONFIG_FILE}.tmp.$$" blockfile="${tmpfile}.block" - printf '%s\n' "$block" > "$blockfile" + # Blocks are authored at the kit's own 2-space body indent. Writing them + # verbatim into a section indented differently produces a mapping with two + # indent levels — which PyYAML refuses to load at all, taking the WHOLE config + # down, while the stdlib reader tolerates it and silently applies last-key- + # wins. Re-indent to whatever the section actually uses. + body_indent=$(section_lines "$section" "$CONFIG_FILE" \ + | awk 'NF && $0 !~ /^[[:space:]]*#/ { match($0, /^[[:space:]]*/); print RLENGTH; exit }') + printf '%s\n' "$block" \ + | awk -v extra="$(( ${body_indent:-2} - 2 ))" ' + { printf "%*s%s\n", (extra > 0 ? extra : 0), "", $0 } + ' > "$blockfile" awk -v section="$section" -v blockfile="$blockfile" ' function emit( line) { while ((getline line < blockfile) > 0) print line close(blockfile) } - $0 == section { inside = 1 } - inside && $0 != section && $0 ~ /^[A-Za-z_][A-Za-z0-9_]*:/ && !inserted { + index($0, section) == 1 && $0 ~ /^[A-Za-z_]/ { inside = 1; header = NR } + inside && NR != header && $0 ~ /^[A-Za-z_][A-Za-z0-9_]*:/ && !inserted { emit() inserted = 1 inside = 0 @@ -188,6 +208,30 @@ append_to_section() { rm -f "$blockfile" } +# Add one `review:` key if the review SECTION does not already define it. +# Per-key rather than per-block: the old migration appended a block defining +# five keys behind a single `noise_markers` guard, so an adopter who had +# `unavailable_markers` but not `noise_markers` got a SECOND definition of it — +# and both YAML readers resolve last-key-wins, silently replacing their list. +# Section-scoped and indent-agnostic: a whole-file `grep '^ key:'` both misses +# a 4-space-indented `review:` block (then appends a 2-space duplicate into it, +# which is not even valid YAML) and is satisfied by a same-named key under an +# unrelated section (then silently skips). +ensure_review_key() { + key="$1" + block="$2" + if [ -n "$(section_lines review: "$CONFIG_FILE" | grep -E "^[[:space:]]+$key:")" ]; then + return 0 + fi + append_to_section "review:" "$block" + if [ -n "$(section_lines review: "$CONFIG_FILE" | grep -E "^[[:space:]]+$key:")" ]; then + echo "added review.$key to config/dev-model.yaml" + else + echo "WARNING: could not add review.$key to $CONFIG_FILE — add it by hand." >&2 + return 1 + fi +} + # The 1-based line range [start end] of a top-level section's body, or "0 0" if # the section is absent. A section runs from its header to the next line that # starts in column 1 with a key. @@ -198,7 +242,10 @@ append_to_section() { # under an unrelated section. Both were shipped and both were caught in review. section_range() { awk -v section="$1" ' - index($0, section) == 1 && $0 ~ /^[A-Za-z_]/ { inside = 1; start = NR; next } + # FIRST match only. append_to_section writes to the first occurrence, so a + # reader that reported the last one would have the two helpers disagreeing + # about which block they are talking about on a duplicated section. + !start && index($0, section) == 1 && $0 ~ /^[A-Za-z_]/ { inside = 1; start = NR; next } inside && /^[A-Za-z_][A-Za-z0-9_]*:/ { print start + 1, NR - 1; found = 1; exit } END { if (!found) print (inside ? start + 1 : 0), (inside ? NR : 0) } ' "$2" @@ -235,8 +282,13 @@ detect_engines_dir() { printf 'scripts\n' } +# Guards are SECTION-scoped, not whole-file `grep '^ key:'`. That form is a bug +# in two directions at once: it misses the key when the adopter's section uses a +# different indent (so the migration re-runs forever, appending a duplicate each +# time), and it is satisfied by a same-named key under an unrelated section (so +# the migration never runs and says nothing). Both were shipped here. migrate_runtime_schema() { - if ! grep -q '^ engines:' "$CONFIG_FILE"; then + if [ -z "$(section_lines paths: "$CONFIG_FILE" | grep -E '^[[:space:]]+engines:')" ]; then detected_engines="$(detect_engines_dir)" append_to_section "paths:" " # Directory containing the deterministic kit engines. engines: $detected_engines" @@ -253,7 +305,7 @@ migrate_runtime_schema() { echo "added runtime mappings to config/dev-model.yaml" fi - if ! grep -q '^ fallback_commands:' "$CONFIG_FILE"; then + if [ -z "$(section_lines review: "$CONFIG_FILE" | grep -E '^[[:space:]]+fallback_commands:')" ]; then old_fallback=$(get_field "review:" "" "^ fallback_command:") [ -n "$old_fallback" ] || old_fallback="/code-review" append_to_section "review:" " fallback_commands: @@ -262,7 +314,7 @@ migrate_runtime_schema() { echo "added runtime review fallbacks to config/dev-model.yaml" fi - if ! grep -q '^ tiers:' "$CONFIG_FILE"; then + if [ -z "$(section_lines models: "$CONFIG_FILE" | grep -E '^[[:space:]]+tiers:')" ]; then old_cheap=$(get_field "models:" "" "^ cheap:") old_default=$(get_field "models:" "" "^ default:") old_expensive=$(get_field "models:" "" "^ expensive:") @@ -303,8 +355,7 @@ migrate_kit_schema() { echo "stamped kit.version=2 in config/dev-model.yaml" fi - if ! grep -q '^ noise_markers:' "$CONFIG_FILE"; then - append_to_section "review:" ' # Read by pr_watch.py. These used to be literals inside the engine, which meant + ensure_review_key noise_markers ' # Read by pr_watch.py. These used to be literals inside the engine, which meant # adopting required EDITING the engine — and an edited engine can never be # replaced by a kit update (Principle #10). noise_markers: @@ -312,34 +363,28 @@ migrate_kit_schema() { - "" - "actionable comments posted: 0" - - "" - unavailable_markers: + - ""' || true + + ensure_review_key unavailable_markers ' 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" - informational_checks: [coderabbit] - # False only for a repo with NO CI at all — otherwise pr-watch never converges. - require_ci: true' - echo "added review marker/CI config to config/dev-model.yaml" - fi + - "no review credits"' || true + + ensure_review_key informational_checks ' informational_checks: [coderabbit]' || true - # A config migrated BEFORE this key existed already has noise_markers, so the - # block above is skipped and this needs its own guard. Two separate additions, - # two separate guards — a single guard would silently skip whichever key the - # adopter's config happens not to have. - if ! grep -q '^ bot_pending_grace_minutes:' "$CONFIG_FILE"; then - append_to_section "review:" ' # How long a configured review bot'"'"'s own check may sit pending before the merge + ensure_review_key require_ci ' # False only for a repo with NO CI at all — otherwise pr-watch never converges. + require_ci: true' || true + + ensure_review_key bot_pending_grace_minutes ' # 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 # receipt recorded now would bind to a review that has not happened); above it, # the bot is treated as never going to report, so a dead bot cannot wedge the # gate. Never affects `converged`. - bot_pending_grace_minutes: 15' - echo "added review.bot_pending_grace_minutes to config/dev-model.yaml" - fi + bot_pending_grace_minutes: 15' || true # `review.bots` became load-bearing for the merge gate in the same change: # pr_watch reads it to decide which checks and comment authors belong to a @@ -347,103 +392,34 @@ migrate_kit_schema() { # `review:` section predating the key would silently fall through to the # engine default — benign while the default matches, dangerous for an adopter # whose reviewer is not CodeRabbit. - # - # Scoped to the review SECTION, at any indent. A `^ bots:` guard would both - # miss a 4-space-indented `review:` block (appending a duplicate at 2 spaces, - # which the reader resolves last-key-wins — silently dropping the adopter's - # real value) and be satisfied by an unrelated `bots:` under another section. - if [ -z "$(section_lines review: "$CONFIG_FILE" | grep -E '^[[:space:]]+bots:')" ]; then - append_to_section "review:" ' bots: [coderabbit]' - if [ -n "$(section_lines review: "$CONFIG_FILE" | grep -E '^[[:space:]]+bots:')" ]; then - echo "added review.bots to config/dev-model.yaml" - else - echo "WARNING: could not add review.bots to $CONFIG_FILE — add it by hand," >&2 - echo " or pr_watch will assume your review bot is CodeRabbit." >&2 - fi - fi + ensure_review_key bots ' bots: [coderabbit]' || true - # The status-check wording of the same rate-limit outage. Added separately for - # the same reason: an adopter migrated before it existed keeps their list. + # The status-check wording of the same rate-limit outage (issue #23). # - # EVERYTHING here is scoped to the review section and warns on any path it - # cannot complete. Both properties are load-bearing and both were got wrong - # first time: a whole-file idempotency grep matches the phrase in a - # `noise_markers` entry and skips the migration AND its warning; a whole-file - # key anchor happily inserts the marker into a same-named list under a - # different section, then reports success. A migration that silently no-ops is - # indistinguishable from one that ran, and one that silently writes to the - # wrong place is worse. - markers_block=$(section_lines review: "$CONFIG_FILE" | awk ' + # DETECT AND INSTRUCT — deliberately not an in-place edit. Three review rounds + # produced three distinct ways for list surgery to corrupt an adopter's config: + # inserting at the wrong indent orphaned every existing entry; a whole-file key + # anchor wrote into a same-named list under another section; and inserting + # before "the first line that is not an item" splices a multi-line item in + # half, yielding YAML that PyYAML refuses to load — each while the + # post-conditions passed and success was printed. The payoff was ONE string in + # a list an adopter can add in five seconds. That trade is not worth defending + # a fourth time, so this now reads the config and tells them what to add. + markers=$(section_lines review: "$CONFIG_FILE" | awk ' /^[[:space:]]+unavailable_markers:/ { in_list = 1; print; next } - # Comments and blank lines do NOT end the list — treating them as the end - # truncates the idempotency view, which re-inserts a marker that is already - # there a few lines further down. - in_list == 1 && ($0 ~ /^[[:space:]]+- / || $0 ~ /^[[:space:]]*(#.*)?$/) { print; next } + # Comments, blanks and continuation lines all stay inside the list — ending + # it early truncates the view and reports a marker as missing when it is + # simply further down. + in_list == 1 && $0 !~ /^[[:space:]]*[a-zA-Z_]+:/ { print; next } in_list == 1 { exit } ') - - if [ -n "$markers_block" ] \ - && ! printf '%s\n' "$markers_block" | grep -qi 'review rate limited'; then - # Two list shapes are unsafe to append to, and both are valid YAML the - # config reader accepts: - # - an inline flow list (`unavailable_markers: ["a", "b"]`) — appending a - # `- ` line under it yields a key with both a scalar value and children - # - a block list whose dashes sit at the KEY's own indent (what several - # YAML formatters emit) — inserting at a deeper indent silently orphans - # every original entry, and the marker-present post-condition still passes - # So: require a bare key line, and reuse the indent of the list's own first - # item rather than assuming one. - key_line=$(printf '%s\n' "$markers_block" | head -n 1) - item_indent=$(printf '%s\n' "$markers_block" \ - | awk 'NR > 1 && /^[[:space:]]+- / { sub(/-.*/, ""); print; exit }') - if printf '%s\n' "$key_line" | grep -qE '^[[:space:]]+unavailable_markers:[[:space:]]*(#.*)?$' \ - && [ -n "$item_indent" ]; then - tmp="$CONFIG_FILE.tmp.$$" - # `start`/`end` bound the edit to the review section's line range, so a - # same-named key elsewhere in the file cannot be targeted by mistake. - awk -v indent="$item_indent" \ - -v start="$(section_range review: "$CONFIG_FILE" | cut -d' ' -f1)" \ - -v end="$(section_range review: "$CONFIG_FILE" | cut -d' ' -f2)" ' - inserted == 0 && in_list == 1 && $0 !~ /^[[:space:]]+- / { - print indent "- \"review rate limited\" # status-check wording of \"review limit reached\"" - inserted = 1 - in_list = 0 - } - NR >= start && NR <= end && /^[[:space:]]+unavailable_markers:/ { in_list = 1 } - { print } - END { - if (inserted == 0 && in_list == 1) - print indent "- \"review rate limited\" # status-check wording of \"review limit reached\"" - } - ' "$CONFIG_FILE" > "$tmp" - # Post-conditions, not a trusted exit code. The marker is inserted EARLY, - # so "the marker is present" alone would also accept a file truncated - # mid-write (disk full, signal) — hence the record count must be exactly - # one more than the original. `awk END{print NR}` rather than `wc -l` so a - # file with no trailing newline is counted the same way on both sides. - before=$(awk 'END { print NR }' "$CONFIG_FILE") - after=$(awk 'END { print NR }' "$tmp") - if printf '%s\n' "$(section_lines review: "$tmp")" | grep -qi 'review rate limited' \ - && [ "$after" -eq "$(( before + 1 ))" ]; then - mv "$tmp" "$CONFIG_FILE" - echo "added the status-check rate-limit marker to config/dev-model.yaml" - rate_limit_marker_added=1 - else - rm -f "$tmp" - fi - fi - if [ -z "${rate_limit_marker_added:-}" ]; then - echo "WARNING: could not add the \"review rate limited\" marker to review.unavailable_markers" >&2 - echo " in $CONFIG_FILE — add it by hand, or a rate-limited review bot that reports" >&2 - echo " the outage only as a status-check description will read as a clean review." >&2 - fi - elif [ -z "$markers_block" ]; then - # No unavailable_markers under review: at all. The engine default applies, - # which already contains the marker — but say so rather than staying silent, - # since "absent" and "present and migrated" are indistinguishable otherwise. - echo "note: review.unavailable_markers is absent from $CONFIG_FILE;" >&2 - echo " pr_watch's built-in defaults apply (they include the new marker)." >&2 + if [ -n "$markers" ] && ! printf '%s\n' "$markers" | grep -qi 'review rate limited'; then + echo "ACTION NEEDED: add this entry to review.unavailable_markers in $CONFIG_FILE:" >&2 + echo ' - "review rate limited"' >&2 + echo " Without it, a review bot that reports a rate limit ONLY as a status-check" >&2 + echo " description (CodeRabbit does this) reads as a clean review. See issue #23." >&2 fi + } # ── narrative-doc templates ────────────────────────────────────────────── diff --git a/kit-manifest.json b/kit-manifest.json index 555a80e..f64107e 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -23,7 +23,7 @@ }, "docs/agentic-dev-kit/workflows/pr-watch.md": { "role": "workflow", - "sha256": "9b381d3d2bead4130e950e3e4e89df72089fafea969d9c80e55efa227bef9107" + "sha256": "02ec1f95ef6bb0228ec7d5923823fcba5d82a6ec6a355072f39ebf6cfa3b0556" }, "docs/agentic-dev-kit/workflows/session-start.md": { "role": "workflow", @@ -99,7 +99,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "975a949e7a346ed32c3fe6325022ae66511d712191df86ffd944366452b964a9" + "sha256": "cc1bc38b6a648814325e88e53158c77a5662f88c65db7d4fd4760b896c76583d" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index 202e34a..973d655 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -1009,26 +1009,31 @@ def summarize_review_bots( continue if not _check_is_pending(detail): continue - age = _age_minutes(detail.get("startedAt"), now) - since = detail.get("startedAt") - source = "check" + # Our own clock wins whenever we already have one. Preferring the + # check's stamp would let the age REGRESS: a stamp a few minutes ahead of + # our clock reads as unusable at first (so we start observing), then + # slides inside the skew tolerance a few minutes later and reads as age + # 0 — restarting the window after it had already aged out, and making + # `merge_blockers` non-monotonic in wall-clock time. + age = _age_minutes(observed.get(bot), now) + since = observed.get(bot) + source = "observed" if age is None: - # No usable stamp on the check — fall back to our own first-sighting - # clock. A stored value that will not parse is REPLACED, not coerced - # to age 0: `age = parse(x) or 0.0` would pin an unparseable value at - # the maximally-blocking age *and* write it back, so every later poll - # re-reads the same poison and the gate blocks forever. Anything we - # cannot read is treated as "no clock yet" and restamped, which keeps - # the window bounded whatever wrote the state (a corrupt file, a - # different engine version, a richer future format). - age = _age_minutes(observed.get(bot), now) - if age is None: - since = now.isoformat() - age = 0.0 - else: - since = observed[bot] - observed[bot] = since + age = _age_minutes(detail.get("startedAt"), now) + since = detail.get("startedAt") + source = "check" + if age is None: + # Neither usable — start our own clock now. A stored value that will + # not parse is REPLACED, not coerced to age 0: `age = parse(x) or + # 0.0` would pin it at the maximally-blocking age *and* write it + # back, so every later poll re-read the same poison and the gate + # blocked forever. Same for a value dated in the future, which is + # parseable and would otherwise block until real time caught up. + since = now.isoformat() + age = 0.0 source = "observed" + if source == "observed": + observed[bot] = since pending.append( { "bot": bot, @@ -1304,7 +1309,8 @@ def record_review( :func:`summarize_review_bots`) — but the grace window bounds that wait to minutes. ``allow_pending_bot`` is the operator's documented override for the remaining case: evidence the queued review will never arrive that pr_watch - cannot see. It is recorded on the receipt, as is a failed check read. + cannot see. It is recorded on the receipt, as is a failed check read (but + not "no bots configured" — nothing was unreadable in that case). """ source = source.strip() if not source: @@ -1363,7 +1369,7 @@ def record_review( # trace. Without it, a receipt taken over an active override is # indistinguishable from one taken after a clean bot verdict. receipt["override"] = "pending-bot" - if bot_signal != "ok": + if bot_signal == "unavailable": # The SILENT bypass, and the worse of the two: when the check read fails # there are no blockers to raise, so the receipt is taken with the #19 # guard simply switched off. Recording an explicit override but not this @@ -1371,6 +1377,10 @@ def record_review( # invisible. Not a refusal — a `gh` too old for these fields, or a PR # with no checks at all, is an environment problem, and refusing would # turn it into a wedge with no way out but the override flag. + # + # `"skipped"` (no bots configured) is deliberately NOT recorded: nothing + # was unreadable and there was no guard to run, so flagging it would put + # a permanent false warning on every receipt a bot-less adopter takes. receipt["bot_signal"] = bot_signal state["review_receipt"] = receipt save_state(pr, state) diff --git a/scripts/tests/test_portability.py b/scripts/tests/test_portability.py index b395a85..8d0dda1 100644 --- a/scripts/tests/test_portability.py +++ b/scripts/tests/test_portability.py @@ -3,8 +3,10 @@ import importlib.util import json import os +import re import shutil import subprocess +import sys from pathlib import Path from types import ModuleType @@ -960,3 +962,186 @@ def test_shared_lane_contract_has_no_runtime_specific_peer_api() -> None: assert "SendMessage" not in script assert "&& claude" not in script + + +# --------------------------------------------------------------------------- # +# config migrations — the highest-blast-radius code in the kit +# --------------------------------------------------------------------------- # +# +# `init.sh` rewrites a file the ADOPTER owns, so a bug here corrupts their +# config rather than merely misbehaving. Three review rounds on one PR produced +# three distinct corruptions from list surgery alone (wrong indent orphaning +# every entry; a whole-file key anchor writing into a same-named list under +# another section; a multi-line item spliced in half), each of which passed the +# migration's own post-conditions and printed success. None was caught by a test +# because there were none. Table-driven over the shapes real configs actually +# take. + +_MIGRATION_SHAPES = { + # (name): (config text, what must still be true afterwards) + "two_space": """review: + bots: [bugbot] + unavailable_markers: + - "my in-house reviewer is offline" +""", + "four_space": """review: + bots: [bugbot] + unavailable_markers: + - "my in-house reviewer is offline" +""", + "flush_indent": """review: + bots: [bugbot] + unavailable_markers: + - "my in-house reviewer is offline" +""", + "inline_flow": """review: + bots: [bugbot] + unavailable_markers: ["my in-house reviewer is offline"] +""", + "decoy_section_first": """other: + bots: [nope] + unavailable_markers: + - "decoy" +review: + bots: [bugbot] + unavailable_markers: + - "my in-house reviewer is offline" +""", + "header_trailing_space": """review: + bots: [bugbot] + unavailable_markers: + - "my in-house reviewer is offline" +""", + "multiline_item": '''review: + bots: [bugbot] + unavailable_markers: + - "the reviewer could not run because the + account is out of credits" +''', + "no_trailing_newline": """review: + bots: [bugbot] + unavailable_markers: + - "my in-house reviewer is offline\"""", +} + + +def _run_init(tmp_path: Path, name: str, config_text: str): + repo = tmp_path / name + (repo / "config").mkdir(parents=True) + shutil.copy2(REPO_ROOT / "init.sh", repo / "init.sh") + (repo / "config" / "dev-model.yaml").write_text(config_text, encoding="utf-8") + proc = subprocess.run( + ["sh", "init.sh"], cwd=repo, check=True, capture_output=True, text=True + ) + return repo / "config" / "dev-model.yaml", proc + + +@pytest.mark.parametrize("shape", sorted(_MIGRATION_SHAPES)) +def test_migration_never_corrupts_or_silently_drops_adopter_config( + tmp_path: Path, shape: str +) -> None: + """Whatever else it does, the migration must leave a config that still + PARSES and still says what the adopter said. + + `review.bots: [bugbot]` is the canary: an adopter whose reviewer is not + CodeRabbit. Every corruption found in review showed up here as either a + parse failure or that value silently becoming `[coderabbit]`. + """ + path, _ = _run_init(tmp_path, shape, _MIGRATION_SHAPES[shape]) + text = path.read_text(encoding="utf-8") + + parsed = yaml.safe_load(text) # raises on the corruptions found in review + assert parsed["review"]["bots"] == ["bugbot"], "adopter's reviewer was overwritten" + markers = parsed["review"]["unavailable_markers"] + assert markers, "adopter's marker list was emptied" + assert any("in-house" in m or "could not run" in m for m in markers), ( + f"adopter's own marker was dropped: {markers}" + ) + # kitconfig (what the engines actually use) must agree with PyYAML — the + # reader exists so engines can drop the dependency, so a disagreement means + # the migration produced a file the two halves of the kit read differently. + sys.path.insert(0, str(REPO_ROOT / "scripts" / "lib")) + import kitconfig # noqa: PLC0415 + + if shape == "multiline_item": + # kitconfig has no multi-line-scalar support, so it reads a wrapped list + # item as a truncated one. PRE-EXISTING and unrelated to migration: it + # reads the input the same way. Asserted as a known divergence rather + # than skipped, so closing it later fails here instead of passing quietly. + assert kitconfig.loads(text) != parsed + assert kitconfig.loads(_MIGRATION_SHAPES[shape]) != yaml.safe_load( + _MIGRATION_SHAPES[shape] + ), "the divergence is in the reader, not in what the migration wrote" + else: + assert kitconfig.loads(text) == parsed + + # A decoy section with the same key names must be left exactly as it was. + if shape == "decoy_section_first": + assert parsed["other"]["unavailable_markers"] == ["decoy"] + assert parsed["other"]["bots"] == ["nope"] + + +@pytest.mark.parametrize("shape", sorted(_MIGRATION_SHAPES)) +def test_migration_is_idempotent(tmp_path: Path, shape: str) -> None: + """Re-running `./init.sh` is the documented upgrade path, so a second run + must be a no-op — not a second copy of every key it added.""" + path, _ = _run_init(tmp_path, shape, _MIGRATION_SHAPES[shape]) + once = path.read_text(encoding="utf-8") + + subprocess.run( + ["sh", "init.sh"], cwd=path.parent.parent, check=True, capture_output=True, text=True + ) + + assert path.read_text(encoding="utf-8") == once + + +def test_migration_adds_every_review_key_exactly_once(tmp_path: Path) -> None: + """Per-key guards, not one guard over a block of five. + + A single `noise_markers` guard over a block defining five keys meant an + adopter who had `unavailable_markers` but not `noise_markers` got a SECOND + definition of it — and both readers resolve last-key-wins, so their list was + silently replaced by the kit's defaults. + """ + path, _ = _run_init( + tmp_path, + "partial", + 'review:\n bots: [bugbot]\n unavailable_markers:\n - "mine"\n', + ) + text = path.read_text(encoding="utf-8") + + for key in ( + "bots", + "noise_markers", + "unavailable_markers", + "informational_checks", + "require_ci", + "bot_pending_grace_minutes", + ): + assert len(re.findall(rf"^\s+{key}:", text, re.MULTILINE)) == 1, key + assert yaml.safe_load(text)["review"]["unavailable_markers"] == ["mine"] + + +def test_migration_says_so_when_it_cannot_act(tmp_path: Path) -> None: + """A migration that silently no-ops is indistinguishable from one that ran. + + The marker can't be inserted safely into every list shape, so it isn't + inserted at all — the adopter is told exactly what to add instead. + """ + _, proc = _run_init( + tmp_path, + "instructs", + 'review:\n bots: [bugbot]\n unavailable_markers:\n - "mine"\n', + ) + + assert "ACTION NEEDED" in proc.stderr + assert '- "review rate limited"' in proc.stderr + + # …and stays quiet once the adopter has added it. + _, quiet = _run_init( + tmp_path, + "quiet", + 'review:\n bots: [bugbot]\n unavailable_markers:\n' + ' - "mine"\n - "review rate limited"\n', + ) + assert "ACTION NEEDED" not in quiet.stderr From 30f241ee76dc391dc2fc12fbc5b96b350e287de6 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 16:41:55 +0300 Subject: [PATCH 7/9] Address CodeRabbit's review of the current head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Its first pass covered `233fdd0`; this is its first look at the actual design, after the grace clock and three rounds of fallback-panel fixes. All three findings valid. **The "ACTION NEEDED" message walked inline-list adopters into the corruption it replaced.** Dropping the list surgery left an instruction that always prints `- "review rate limited"` — a block item. An adopter whose `unavailable_markers` is a flow list (`["a", "b"]`) who follows that literally hangs a block item off a flow scalar and breaks their config. The step is read-only, so it could not corrupt anything itself; it would just have talked someone else into doing it. Now detects the style and says "add the string inside the brackets" instead. **`zip(pending, exact_ages, strict=True)`** — the lists are appended in lockstep, so a later edit adding a `continue` between them would pair each entry with the wrong age, silently, and only for a bot with more than one pending check. **The no-PyYAML type test covered 3 of 21 tokens**, which inverts the invariant its own neighbour states ("kept as one list so the parity check cannot quietly cover a smaller set"). The parity test skips precisely where the engines run — a bare env with no PyYAML — so those three were the only coverage that environment had. Now parametrized over the full set with a declared expected type, plus a guard that the two sets stay equal, so adding a token cannot leave it untested where it matters most. Tests 249 → 270. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --- init.sh | 13 +++++++++-- kit-manifest.json | 2 +- scripts/pr_watch.py | 6 ++++- scripts/tests/test_kitconfig.py | 39 +++++++++++++++++++++++++-------- 4 files changed, 47 insertions(+), 13 deletions(-) diff --git a/init.sh b/init.sh index 7128321..d0ff6b3 100755 --- a/init.sh +++ b/init.sh @@ -414,8 +414,17 @@ migrate_kit_schema() { in_list == 1 { exit } ') if [ -n "$markers" ] && ! printf '%s\n' "$markers" | grep -qi 'review rate limited'; then - echo "ACTION NEEDED: add this entry to review.unavailable_markers in $CONFIG_FILE:" >&2 - echo ' - "review rate limited"' >&2 + echo "ACTION NEEDED: add \"review rate limited\" to review.unavailable_markers" >&2 + echo " in $CONFIG_FILE." >&2 + # The instruction has to match the list style the adopter actually uses. + # Telling someone with an inline list to add a `- ` item would have them + # hang a block item off a flow scalar — this step is read-only, but it would + # still be walking them into the same corruption the surgery used to cause. + if printf '%s\n' "$markers" | head -n 1 | grep -qE ':[[:space:]]*\['; then + echo ' Yours is written as an inline list — add the string inside the brackets.' >&2 + else + echo ' - "review rate limited"' >&2 + fi echo " Without it, a review bot that reports a rate limit ONLY as a status-check" >&2 echo " description (CodeRabbit does this) reads as a clean review. See issue #23." >&2 fi diff --git a/kit-manifest.json b/kit-manifest.json index f64107e..ff0facc 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -99,7 +99,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "cc1bc38b6a648814325e88e53158c77a5662f88c65db7d4fd4760b896c76583d" + "sha256": "d354acdfb0374013cbb64aa5fe7d8961b3f72356a31055c92c5edfa40d9ce3be" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index 973d655..888edd1 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -1052,7 +1052,11 @@ def summarize_review_bots( exact_ages.append(age) blockers: list[str] = [] - for entry, exact_age in zip(pending, exact_ages): + # strict=True: the two lists are appended in lockstep in the loop above, and + # a future edit that adds a `continue` between them would otherwise pair + # each entry with the WRONG age — silently, and only for a bot with more + # than one pending check. + for entry, exact_age in zip(pending, exact_ages, strict=True): if entry["bot"] in unavailable_bots: # This bot's own CHECK announced an outage: it is not going to move # off pending, so waiting on it is a wedge with extra steps. diff --git a/scripts/tests/test_kitconfig.py b/scripts/tests/test_kitconfig.py index d5e7989..3fa5ec2 100644 --- a/scripts/tests/test_kitconfig.py +++ b/scripts/tests/test_kitconfig.py @@ -118,17 +118,38 @@ def test_decimal_scalars_match_pyyaml_except_the_documented_forms(): ) -def test_decimal_scalars_keep_their_python_types_without_pyyaml(): - """The same resolution, asserted directly — the engines run with no PyYAML, - so the parity test above skips in exactly the environment that matters most. +# The resolved type of every edge case above. Duplicated deliberately: the +# parity test derives its expectation from PyYAML, this one states it outright, +# so a regression has to fool two independent descriptions of the same rule. +_EXPECTED_TYPES = { + "2.5": float, "15": int, "1.": float, "0.0": float, "-0.0": float, + "-0.5": float, "+1.5": float, ".5": float, + "-.5": str, "+.5": str, # sign not allowed on the .digits form + "1.0e+3": float, # signed exponent + "1.0e3": str, "1.0E3": str, "1.5e3": str, "-.5e+3": str, # unsigned + "1_0.5": float, "._5": str, # `_` separates digits, cannot lead + "nan": str, "inf": str, "1e5": str, "1.2.3": str, +} + + +def test_every_float_edge_case_has_a_declared_type(): + """The no-PyYAML assertions must cover the SAME set as the parity check. + + Without this, adding a token to `_FLOAT_EDGE_CASES` silently leaves it + untested in the one environment the reader exists for — a bare env with no + PyYAML, where the parity test skips. """ - parsed = kitconfig.loads( - "review:\n grace: 2.5\n whole: 15\n bare_exponent: 1e5\n" - ) + assert set(_EXPECTED_TYPES) == set(_FLOAT_EDGE_CASES) + + +@pytest.mark.parametrize("token", _FLOAT_EDGE_CASES) +def test_decimal_scalars_keep_their_python_types_without_pyyaml(token: str): + """The engines run with no PyYAML, so the parity test above skips in exactly + the environment that matters most. This one never skips.""" + value = kitconfig.loads(f"v: {token}\n")["v"] - assert parsed["review"]["grace"] == 2.5 - assert isinstance(parsed["review"]["whole"], int) # not silently a float - assert parsed["review"]["bare_exponent"] == "1e5" + assert isinstance(value, _EXPECTED_TYPES[token]), (token, value) + assert not isinstance(value, bool) # `15` must not arrive as True def test_inline_and_block_lists(): From 07ff258e8a75066c940324d9edaeedf1a9919ef9 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 16:55:43 +0300 Subject: [PATCH 8/9] Cover the branch a mutation proved was dead-codeable (re-review #4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **The style-detection branch had no test, proven by mutation.** Hardwiring the condition to a constant — so inline-list adopters get the corrupting `- ` advice — still passed the whole suite. The only assertion checked the block direction. Sharpest part: `_MIGRATION_SHAPES` already contained an `inline_flow` fixture and the commit message advertised the path as covered; the fixture was there, the assertion was never written. Now parametrized per style, and the same mutation fails. **Style detection missed a real flow spelling.** `head -n 1 | grep '\['` only sees the key line, so a flow list whose value sits on the FOLLOWING line (`unavailable_markers:\n ["a", "b"]`) read as block style and got the corrupting advice. Presence of `- ` items is the discriminator now, which also gives the empty-key case its own answer: no value means the engine defaults apply, and those already contain the marker, so asking for it is noise. **A marker named only in a trailing comment counted as present.** The grep ran over raw lines, and the kit's own shipped config carries a `# the status-check wording of …` comment on that exact line — so an adopter with the phrase in a comment and not in the list would be told nothing. Matched against comment-stripped values now. **The no-PyYAML type assertions covered 21 of 25 tokens** while the guard's docstring claimed they covered the same set as the parity check. The parity test walks two collections; the guard compared against one. The four missing tokens are the divergences — exactly where this reader departs from PyYAML on purpose, so an accidental change there looks like a fix. Both collections are covered now, and the divergent tokens' string-identity assertion moved out from behind the `importorskip` it never needed. Tests 270 → 281. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --- init.sh | 36 ++++++++++--- scripts/tests/test_kitconfig.py | 26 +++++++--- scripts/tests/test_portability.py | 85 +++++++++++++++++++++++++------ 3 files changed, 116 insertions(+), 31 deletions(-) diff --git a/init.sh b/init.sh index d0ff6b3..3ad1eae 100755 --- a/init.sh +++ b/init.sh @@ -413,20 +413,40 @@ migrate_kit_schema() { in_list == 1 && $0 !~ /^[[:space:]]*[a-zA-Z_]+:/ { print; next } in_list == 1 { exit } ') - if [ -n "$markers" ] && ! printf '%s\n' "$markers" | grep -qi 'review rate limited'; then - echo "ACTION NEEDED: add \"review rate limited\" to review.unavailable_markers" >&2 - echo " in $CONFIG_FILE." >&2 + # Match the marker in the VALUES only. The kit's own shipped config carries a + # trailing `# the status-check wording of …` comment on that very line, so a + # raw-line grep would also be satisfied by an adopter who has the phrase in a + # comment and not in the list. + marker_values=$(printf '%s\n' "$markers" | sed 's/[[:space:]]*#.*$//') + if [ -n "$markers" ] && ! printf '%s\n' "$marker_values" | grep -qi 'review rate limited'; then # The instruction has to match the list style the adopter actually uses. # Telling someone with an inline list to add a `- ` item would have them # hang a block item off a flow scalar — this step is read-only, but it would # still be walking them into the same corruption the surgery used to cause. - if printf '%s\n' "$markers" | head -n 1 | grep -qE ':[[:space:]]*\['; then - echo ' Yours is written as an inline list — add the string inside the brackets.' >&2 + # + # Presence of `- ` items is the discriminator, not a `[` on the key line: a + # flow list may put its value on the FOLLOWING line, where a key-line test + # sees no bracket and hands out the corrupting advice. + if printf '%s\n' "$markers" | grep -qE '^[[:space:]]*- '; then + style=block + elif printf '%s\n' "$markers" | grep -q '\['; then + style=flow else - echo ' - "review rate limited"' >&2 + # A key with no value at all: the reader falls back to the engine + # defaults, which already contain the marker. Nothing to ask for. + style=none + fi + if [ "$style" != none ]; then + echo "ACTION NEEDED: add \"review rate limited\" to review.unavailable_markers" >&2 + echo " in $CONFIG_FILE." >&2 + if [ "$style" = flow ]; then + echo ' Yours is written as an inline list — add the string inside the brackets.' >&2 + else + echo ' - "review rate limited"' >&2 + fi + echo " Without it, a review bot that reports a rate limit ONLY as a status-check" >&2 + echo " description (CodeRabbit does this) reads as a clean review. See issue #23." >&2 fi - echo " Without it, a review bot that reports a rate limit ONLY as a status-check" >&2 - echo " description (CodeRabbit does this) reads as a clean review. See issue #23." >&2 fi } diff --git a/scripts/tests/test_kitconfig.py b/scripts/tests/test_kitconfig.py index 3fa5ec2..109fef8 100644 --- a/scripts/tests/test_kitconfig.py +++ b/scripts/tests/test_kitconfig.py @@ -111,7 +111,6 @@ def test_decimal_scalars_match_pyyaml_except_the_documented_forms(): for token, why in _KNOWN_PYYAML_DIVERGENCES.items(): doc = f"v: {token}\n" - assert kitconfig.loads(doc)["v"] == token, f"{token} should stay a string ({why})" assert kitconfig.loads(doc) != yaml.safe_load(doc), ( f"{token} now agrees with PyYAML — good, but remove it from " f"_KNOWN_PYYAML_DIVERGENCES and from _coerce's docstring ({why})" @@ -129,20 +128,29 @@ def test_decimal_scalars_match_pyyaml_except_the_documented_forms(): "1.0e3": str, "1.0E3": str, "1.5e3": str, "-.5e+3": str, # unsigned "1_0.5": float, "._5": str, # `_` separates digits, cannot lead "nan": str, "inf": str, "1e5": str, "1.2.3": str, + # The four forms where this reader deliberately departs from PyYAML — see + # _KNOWN_PYYAML_DIVERGENCES. All stay strings here. + ".nan": str, ".inf": str, "-.inf": str, "1:30.0": str, } def test_every_float_edge_case_has_a_declared_type(): - """The no-PyYAML assertions must cover the SAME set as the parity check. + """The no-PyYAML assertions must cover every token the parity check does. - Without this, adding a token to `_FLOAT_EDGE_CASES` silently leaves it - untested in the one environment the reader exists for — a bare env with no - PyYAML, where the parity test skips. + The parity test walks TWO collections — the edge cases and the known + divergences — so checking set-equality against only the first leaves the + four divergent tokens with no coverage at all in a bare env. Those are the + highest-risk ones: they are precisely where the implementation deliberately + departs from PyYAML, so an accidental change there looks like a fix. """ - assert set(_EXPECTED_TYPES) == set(_FLOAT_EDGE_CASES) + assert set(_EXPECTED_TYPES) == set(_FLOAT_EDGE_CASES) | set( + _KNOWN_PYYAML_DIVERGENCES + ) -@pytest.mark.parametrize("token", _FLOAT_EDGE_CASES) +@pytest.mark.parametrize( + "token", (*_FLOAT_EDGE_CASES, *_KNOWN_PYYAML_DIVERGENCES) +) def test_decimal_scalars_keep_their_python_types_without_pyyaml(token: str): """The engines run with no PyYAML, so the parity test above skips in exactly the environment that matters most. This one never skips.""" @@ -150,6 +158,10 @@ def test_decimal_scalars_keep_their_python_types_without_pyyaml(token: str): assert isinstance(value, _EXPECTED_TYPES[token]), (token, value) assert not isinstance(value, bool) # `15` must not arrive as True + if token in _KNOWN_PYYAML_DIVERGENCES: + # The divergent forms stay verbatim strings — assertable without PyYAML, + # so it does not belong behind the parity test's importorskip. + assert value == token def test_inline_and_block_lists(): diff --git a/scripts/tests/test_portability.py b/scripts/tests/test_portability.py index 8d0dda1..38885d7 100644 --- a/scripts/tests/test_portability.py +++ b/scripts/tests/test_portability.py @@ -1122,26 +1122,79 @@ def test_migration_adds_every_review_key_exactly_once(tmp_path: Path) -> None: assert yaml.safe_load(text)["review"]["unavailable_markers"] == ["mine"] -def test_migration_says_so_when_it_cannot_act(tmp_path: Path) -> None: - """A migration that silently no-ops is indistinguishable from one that ran. +@pytest.mark.parametrize( + ("style", "config", "wanted", "unwanted"), + [ + # Block list — a `- ` item is the right thing to add. + ( + "block", + 'review:\n unavailable_markers:\n - "mine"\n', + '- "review rate limited"', + "brackets", + ), + # Inline flow list — telling them to add a `- ` item would hang a block + # item off a flow scalar, i.e. walk them into corrupting their config. + ( + "flow", + 'review:\n unavailable_markers: ["mine"]\n', + "brackets", + '- "review rate limited"', + ), + # Flow list whose VALUE is on the next line. A key-line bracket test + # sees no `[` here and hands out the block-item advice. + ( + "flow_next_line", + 'review:\n unavailable_markers:\n ["mine", "other"]\n', + "brackets", + '- "review rate limited"', + ), + ], +) +def test_the_instruction_matches_the_list_style( + tmp_path: Path, style: str, config: str, wanted: str, unwanted: str +) -> None: + """The advice must be correct for the shape the adopter actually has. - The marker can't be inserted safely into every list shape, so it isn't - inserted at all — the adopter is told exactly what to add instead. + Without a per-style assertion the whole branch is dead-codeable: hardwiring + the style test to a constant still passes a suite that only ever checks the + block-list direction. """ - _, proc = _run_init( - tmp_path, - "instructs", - 'review:\n bots: [bugbot]\n unavailable_markers:\n - "mine"\n', - ) + _, proc = _run_init(tmp_path, f"instructs_{style}", config) assert "ACTION NEEDED" in proc.stderr - assert '- "review rate limited"' in proc.stderr + assert wanted in proc.stderr + assert unwanted not in proc.stderr + - # …and stays quiet once the adopter has added it. - _, quiet = _run_init( +@pytest.mark.parametrize( + ("case", "config"), + [ + # Already present — nothing to ask for. + ("block", 'review:\n unavailable_markers:\n - "mine"\n - "review rate limited"\n'), + ("flow", 'review:\n unavailable_markers: ["mine", "review rate limited"]\n'), + # A key with no value falls back to the engine defaults, which already + # contain the marker — so asking for it would be noise. + ("empty_key", "review:\n unavailable_markers:\n"), + # Absent entirely: `ensure_review_key` writes the full default list. + ("absent", "review:\n bots: [bugbot]\n"), + ], +) +def test_the_instruction_stays_quiet_when_there_is_nothing_to_add( + tmp_path: Path, case: str, config: str +) -> None: + _, proc = _run_init(tmp_path, f"quiet_{case}", config) + + assert "ACTION NEEDED" not in proc.stderr + + +def test_a_marker_named_only_in_a_comment_does_not_count(tmp_path: Path) -> None: + """The kit's own shipped config carries a trailing comment on that very + line, so a raw-line grep is satisfied by a config whose LIST lacks it.""" + _, proc = _run_init( tmp_path, - "quiet", - 'review:\n bots: [bugbot]\n unavailable_markers:\n' - ' - "mine"\n - "review rate limited"\n', + "comment_only", + 'review:\n unavailable_markers:\n' + ' - "mine" # unlike review rate limited, this one is ours\n', ) - assert "ACTION NEEDED" not in quiet.stderr + + assert "ACTION NEEDED" in proc.stderr From 321907ea52cf2c5ac12d7729a28f0b50d6a392b5 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 17:13:23 +0300 Subject: [PATCH 9/9] Don't give confident advice about a list the reader can't read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 6 found that the previous round promoted a list spelling to "supported" in the instruction table without checking whether the kit's own reader parses it. `kitconfig` — what every engine uses — resolves a flow list whose brackets don't close on the key line to `{}`, and a multi-line one to the string `"["`: unavailable_markers: ["a", "b"] -> ['a', 'b'] unavailable_markers:\n ["a", "b"] -> {} unavailable_markers: [\n "a",\n "b"\n] -> '[' So an adopter in either of the latter shapes has NO markers in effect at all, and "add the string inside the brackets" is advice that fixes nothing. Those shapes now get told what is actually wrong — the whole list is inert — which is considerably bigger news than one missing marker. The style detection is unchanged for the shapes that do work. Two smaller ones from the same pass: - **Comment-stripping was quote-unaware**, so a marker containing an issue number (`- "tracked in #23: review rate limited"`) was truncated before the match and the adopter was asked for a marker they already had. Now scans character-wise and only cuts a `#` outside quotes. - **`grep -qi`'s case-insensitivity had no test** — dropping `-i` passed all 281 — which is the same untested-branch class the previous round existed to close. Also corrected in the PR body: the baseline is 202 tests, not 204 (measured at the merge-base); "four ways to corrupt" is three, matching what init.sh's own comment enumerates — the fifth round's finding was about the replacement message, not surgery; and the migration matrix is 8 shapes × 2 tests plus a separate 5-row instruction table, not 8 × 4. Tests 281 → 285. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --- init.sh | 33 +++++++++++++++++++++++++++---- scripts/tests/test_portability.py | 29 ++++++++++++++++++++++++--- 2 files changed, 55 insertions(+), 7 deletions(-) diff --git a/init.sh b/init.sh index 3ad1eae..9d1544c 100755 --- a/init.sh +++ b/init.sh @@ -416,8 +416,20 @@ migrate_kit_schema() { # Match the marker in the VALUES only. The kit's own shipped config carries a # trailing `# the status-check wording of …` comment on that very line, so a # raw-line grep would also be satisfied by an adopter who has the phrase in a - # comment and not in the list. - marker_values=$(printf '%s\n' "$markers" | sed 's/[[:space:]]*#.*$//') + # comment and not in the list. Quote-aware: a plain `s/#.*//` would also cut a + # marker containing an issue number (`- "tracked in #23: review rate limited"`) + # and then ask for a marker that is already there. + marker_values=$(printf '%s\n' "$markers" | awk '{ + out = ""; qc = "" + for (i = 1; i <= length($0); i++) { + c = substr($0, i, 1) + if (qc == "" && (c == "\"" || c == "'\''")) { qc = c } + else if (qc != "" && c == qc) { qc = "" } + else if (qc == "" && c == "#") { break } + out = out c + } + print out + }') if [ -n "$markers" ] && ! printf '%s\n' "$marker_values" | grep -qi 'review rate limited'; then # The instruction has to match the list style the adopter actually uses. # Telling someone with an inline list to add a `- ` item would have them @@ -429,14 +441,27 @@ migrate_kit_schema() { # sees no bracket and hands out the corrupting advice. if printf '%s\n' "$markers" | grep -qE '^[[:space:]]*- '; then style=block - elif printf '%s\n' "$markers" | grep -q '\['; then + elif printf '%s\n' "$markers" | head -n 1 | grep -qE ':[[:space:]]*\[.*\]'; then style=flow + elif printf '%s\n' "$markers" | grep -q '\['; then + # A flow list whose brackets are NOT closed on the key line. Valid YAML, + # but `scripts/lib/kitconfig.py` — the reader every engine uses — parses + # it to `{}` or `"["`, so the adopter's ENTIRE marker list is already + # being ignored. Asking them to add one more string to it would be + # confident, inert advice; the list not working at all is the bigger news. + style=unreadable else # A key with no value at all: the reader falls back to the engine # defaults, which already contain the marker. Nothing to ask for. style=none fi - if [ "$style" != none ]; then + if [ "$style" = unreadable ]; then + echo "ACTION NEEDED: review.unavailable_markers in $CONFIG_FILE is a flow list" >&2 + echo " spanning more than one line. The kit's config reader cannot parse that" >&2 + echo " spelling, so NONE of your markers are in effect. Put the whole list on the" >&2 + echo " key's own line (\`unavailable_markers: [\"a\", \"b\"]\`) or use a block list," >&2 + echo ' and include "review rate limited" — see issue #23.' >&2 + elif [ "$style" != none ]; then echo "ACTION NEEDED: add \"review rate limited\" to review.unavailable_markers" >&2 echo " in $CONFIG_FILE." >&2 if [ "$style" = flow ]; then diff --git a/scripts/tests/test_portability.py b/scripts/tests/test_portability.py index 38885d7..1da2cc8 100644 --- a/scripts/tests/test_portability.py +++ b/scripts/tests/test_portability.py @@ -1140,14 +1140,30 @@ def test_migration_adds_every_review_key_exactly_once(tmp_path: Path) -> None: "brackets", '- "review rate limited"', ), - # Flow list whose VALUE is on the next line. A key-line bracket test - # sees no `[` here and hands out the block-item advice. + # Flow list whose brackets do not close on the key line. Valid YAML, but + # `kitconfig` parses it to {} / "[" — so the adopter's WHOLE list is + # already inert, and "add it inside the brackets" would be confident, + # useless advice. Say what's actually wrong instead. ( "flow_next_line", 'review:\n unavailable_markers:\n ["mine", "other"]\n', - "brackets", + "cannot parse that", '- "review rate limited"', ), + ( + "flow_multi_line", + 'review:\n unavailable_markers: [\n "mine",\n "other"\n ]\n', + "cannot parse that", + "add the string inside the brackets", + ), + # A marker containing a `#` must not be cut short by comment-stripping — + # doing so asks for a marker the adopter already has. + ( + "hash_in_value", + 'review:\n unavailable_markers:\n - "see #23 for why"\n', + '- "review rate limited"', + "brackets", + ), ], ) def test_the_instruction_matches_the_list_style( @@ -1172,6 +1188,13 @@ def test_the_instruction_matches_the_list_style( # Already present — nothing to ask for. ("block", 'review:\n unavailable_markers:\n - "mine"\n - "review rate limited"\n'), ("flow", 'review:\n unavailable_markers: ["mine", "review rate limited"]\n'), + # Case-insensitively: an adopter may have written it capitalized. + ("block_mixed_case", 'review:\n unavailable_markers:\n - "Review Rate Limited"\n'), + # …and a marker whose text contains a `#` must still count as present. + ( + "hash_in_value", + 'review:\n unavailable_markers:\n - "tracked in #23: review rate limited"\n', + ), # A key with no value falls back to the engine defaults, which already # contain the marker — so asking for it would be noise. ("empty_key", "review:\n unavailable_markers:\n"),