diff --git a/.agents/skills/pr-watch/SKILL.md b/.agents/skills/pr-watch/SKILL.md index d480376..59bc370 100644 --- a/.agents/skills/pr-watch/SKILL.md +++ b/.agents/skills/pr-watch/SKILL.md @@ -9,5 +9,5 @@ description: Drive a pull request through the deterministic poll, fix, acknowled 2. Read `config/dev-model.yaml` and `docs/agentic-dev-kit/workflows/pr-watch.md` completely. 3. Follow the workflow for the PR number in the user's request, or the current branch's PR when none is supplied. 4. Resolve the engine path from the repository root; support both `scripts/pr_watch.py` and a namespaced adopted path such as `scripts/devkit/pr_watch.py`. -5. Use `review.fallback_commands` for the current runtime when configured. Never treat an unavailable review bot as a review waiver. +5. When a bot is unavailable, run the `review.fallback_panel` pass — one isolated, fresh-context reviewer per lens, per `docs/agentic-dev-kit/fallback-review-panel.md`. Fall back to `review.fallback_commands` only if the runtime cannot isolate a reviewer, and record that as one lens, never as the configured `review.fallback_panel.receipt_source`. Never treat an unavailable review bot as a review waiver. 6. For safety-critical changes, also read and apply `docs/agentic-dev-kit/safety-critical-changes.md` before recommending merge. diff --git a/.gitignore b/.gitignore index 33db858..fb26a01 100644 --- a/.gitignore +++ b/.gitignore @@ -8,3 +8,6 @@ __pycache__/ # appends these to an adopting repo's .gitignore. state/ .devkit_state_root + +# Agent worktrees (created by isolated review lenses) are never committed. +.claude/worktrees/ diff --git a/config/dev-model.yaml b/config/dev-model.yaml index 5498b55..30748d8 100644 --- a/config/dev-model.yaml +++ b/config/dev-model.yaml @@ -93,6 +93,29 @@ review: # `CodeRabbit` and the author `coderabbitai` either way. Keep entries specific # enough not to collide with a CI job name. bots: [coderabbit] + # The independent pass to run when a configured bot can't review. TWO shapes, + # and the difference is whether the reviewer is independent of the author: + # + # fallback_panel one isolated, fresh-context reviewer PER LENS. This is + # the real substitute — `safety-critical-changes.md` + # rule 2 wants two disjoint lenses, which a single + # command cannot be. + # fallback_commands one command in the cockpit's own context. When the + # cockpit authored the diff that is the author + # re-reading their own work — kept as the DEGRADED mode + # for a runtime with no way to isolate a reviewer. + # + # Which lenses is adopter knowledge and lives here; how a lens must be run + # (fresh context, raw diff, no author framing, execute rather than only read, + # mutation-test new branches) is kit doctrine and lives in + # docs/agentic-dev-kit/fallback-review-panel.md. + fallback_panel: + receipt_source: "fallback:panel" # never the same as a single-lens receipt + lenses: + - name: adversarial + focus: "assume the change is wrong and try to prove it — bypasses, fail-open paths, wedges, and whether the new guard actually guards" + - name: correctness + focus: "assume it works and ask what it says — stale comments, claims that overstate what is verified, tests whose names promise more than their bodies check" fallback_commands: claude: "/code-review" codex: "/review" diff --git a/docs/CLAUDE-sections.md b/docs/CLAUDE-sections.md index c8203ac..dd5191a 100644 --- a/docs/CLAUDE-sections.md +++ b/docs/CLAUDE-sections.md @@ -65,10 +65,14 @@ infra failure) — then report the specific blocker and ask. **A review bot being unavailable is not a reason to skip review.** When a configured bot can't review an otherwise-ready PR — rate-limited, out of credits, or it silently skipped -the PR — run `review.fallback_commands.claude` as the independent review pass and triage its +the PR — run the `review.fallback_panel` pass (one isolated, fresh-context reviewer per +lens; see `docs/agentic-dev-kit/fallback-review-panel.md`) and triage its findings the same way (fix if confident and small, reply-with-reason otherwise). A blocked review bot is **not** a waiver: the "clean" bar still requires one independent review -pass — bot *or* the fallback command. +pass — a configured bot, or a completed `review.fallback_panel` pass. +`review.fallback_commands` is the DEGRADED one-lens mode for a runtime that cannot +isolate a reviewer; it is the author re-reading their own diff, which +`safety-critical-changes.md` rule 2 does not accept as a green light. --- diff --git a/docs/agentic-dev-kit/fallback-review-panel.md b/docs/agentic-dev-kit/fallback-review-panel.md new file mode 100644 index 0000000..d84672c --- /dev/null +++ b/docs/agentic-dev-kit/fallback-review-panel.md @@ -0,0 +1,158 @@ +# The fallback review panel + +What to run when your configured review bot can't. Read this whenever +`pr_watch` reports a reviewer as unavailable, and before recording any +`fallback:` receipt. + +## Why a panel and not a command + +`review.fallback_commands.` runs the runtime's own review command — in +the cockpit's context. When the cockpit *authored* the diff, that is the author +re-reading their own work. It finds things; it does not find the things the +author is blind to, which are exactly the ones that survive to production. + +`safety-critical-changes.md` rule 2 already says a single-lens verdict "is an +incomplete review, not a green light". A single command cannot satisfy a rule +that asks for two disjoint lenses, so the mechanism was violating the doctrine +it was meant to serve. + +**The evidence is disjointness, not volume.** Run as two fresh-context lenses +over one merge-gate change, the panel found a stale outage comment that +reintroduced the exact bug the PR existed to fix; a future-dated clock that +wedged the gate for 30 days *after* the fix for that class shipped; and a +section-scoping fix applied to one of three guards in the same function. The two +lenses overlapped on almost nothing, and the author's own passes had found none +of it. + +## The lenses + +Configured in `review.fallback_panel.lenses` — **that is where each lens's +brief lives**, as its `focus`, and it is what you hand the reviewer. Restating +the briefs here would give the kit two copies to drift apart, so this section +describes only what the two shipped lenses are *for*: + +- **adversarial** — starts from "this is wrong" and tries to prove it. +- **correctness** — starts from "this works" and asks what it *says*. + +They are the two the doctrine names, and they are chosen to overlap as little +as possible. Add or replace lenses for your own risk profile (a data-migration +lens, a performance lens): two disjoint lenses is the floor, not the ceiling. + +## The contract every lens gets + +These are why the panel works. Drop any of them and it degrades toward the +author re-reading their own diff. + +1. **Fresh context.** A lens that watched the change being written inherits the + author's model of it, including the wrong parts. +2. **The raw diff, no framing.** Do not tell the lens what the change is *for*, + what you already fixed, or what you think is risky. That is the anchoring + the panel exists to escape. +3. **Say it did not write the code.** Cheap, and it measurably changes what a + lens is willing to call wrong. +4. **Execute, don't only read.** The highest-value findings came from *running* + the changed paths against hostile input, not from reading them. A guard was + dead code for the exact bot it was written for, because that bot reports a + zero timestamp — no amount of re-reading surfaced it; one live poll did. +5. **Mutation-test new branches.** Break the new behaviour deliberately and + confirm a test fails. Repeatedly, properties were *named* by a test and + pinned by nothing — hardwiring a branch to a constant still passed the whole + suite. + + **Beware false kills.** If your repo has a checksum/drift test over the files + you are mutating (this kit has one: `kit_doctor`'s self-check), *every* + mutation fails it regardless of behaviour, and a whole run can report 100% + killed while nothing behavioural caught anything. One lens's first pass here + reported 17/17 killed; re-run with that test excluded, 7 had survived. + Exclude the drift test — or regenerate the manifest after each mutation — + before believing a kill. +6. **Report, do not fix.** A lens that edits loses the disjointness: it starts + defending its own changes on the next round. +7. **Mutate in an isolated copy, never the shared tree.** Mutation testing needs + temporary writes, and lenses run concurrently. Discovered the hard way on the + PR that added this file: one lens's mutations appeared to the other as an + external process corrupting the repo, and it "restored" them mid-run — so one + lens's results were unreliable and the other nearly destroyed live work. Give + each lens a scratch copy or its own git worktree, and require it to leave the + shared tree byte-identical. +8. **State what was verified clean, and how.** Absence of findings is only + evidence if you know what was actually checked. + +## Running it + +1. Read the change: `git diff ...HEAD`. +2. Launch **one isolated reviewer per lens**, concurrently, each with the + contract above and its lens focus. Use whatever isolation your runtime has + (a subagent, a separate session, a second person). If it has none, see + *Degraded mode*. +3. Triage every finding against the *current* code — some go stale across + rounds. +4. Fix real findings, reply-with-reason to the rest. +5. **Re-run the panel after the fix round.** Not optional: see below. +6. Record the receipt with the lenses that actually ran: + + ```sh + uv run /pr_watch.py \ + --record-review "" \ + --lenses \ + --head + ``` + + **`--lenses` is self-reported, and the engine does not verify it.** You write + the source and the lens names in one invocation; nothing binds either to a + review that happened. Four rounds of this PR tried to verify it from the + engine — matching the source, then the lens names, then a required roster, + then a counted one — + and each was defeated, the last by a single extra character in the source, + after which the render cheerfully affirmed the forgery. + `safety-critical-changes.md` rule 1 calls that a stopgap, not a fix. + + So what you get is an **audit trail, not a gate**. The poll render states the + claim, labelled as a claim, on every poll once a current-head receipt exists + (no receipt, or one bound to an older head, prints nothing): + + ```text + review evidence: fallback:codex — ⚠ ONE lens claimed (correctness) — not a dual-lens pass + ``` + + That is genuinely useful — a one-lens pass is visible at merge time instead of + buried in the record command's stdout — and it is worth exactly what an honest + operator puts into it. Verifying coverage needs each lens to record its own + receipt from its own context: issue #32, not this. + +## Re-running, and when to stop + +`safety-critical-changes.md` rule 3 says to re-review after every fix round +"until a full pass finds nothing new". Take that literally *and* know its limit: +across one session of 13 rounds on three PRs, **every round found something**, +and seven of those findings were defects introduced by the *previous round's +fix*. The termination condition may never arrive. + +So the stopping criterion is **blast radius, not round count**: + +- A **gate, send path, or destructive operation** — keep going. Worst case is an + unreviewed change landing, and the doctrine's operator-merge rule applies. +- Something **reported but never acted on** (a warning, a log line, a report + field) — a couple of rounds with the findings decaying in severity is + proportionate. Worst case is a wrong message. + +Say which one you applied **in the PR**, where a human reads it — the receipt +carries what the review did *not* cover (`override`, `bot_signal`, +`bots_behind_head`) and what it did (`lenses`) — not a prose rationale for +stopping. "The last round found +nothing" is not available as a reason if it never happened. + +## Degraded mode + +If the runtime cannot run isolated reviewers, fall back to +`review.fallback_commands.` — one lens, in the author's context. It is +better than nothing and it is **not** a panel: + +- record it as `fallback:`, never the panel's `receipt_source`. The + engine will not stop you doing otherwise — it cannot tell what ran. The + honesty is entirely yours, which is the reason to write it down +- pass `--lenses` naming what actually ran, so the audit trail shows one lens +- for anything under `safety-critical-changes.md`, say plainly in the PR that + rule 2 was not satisfied + +A receipt should never claim more coverage than the review it stands for. diff --git a/docs/agentic-dev-kit/safety-critical-changes.md b/docs/agentic-dev-kit/safety-critical-changes.md index 1997e55..270e328 100644 --- a/docs/agentic-dev-kit/safety-critical-changes.md +++ b/docs/agentic-dev-kit/safety-critical-changes.md @@ -21,12 +21,17 @@ skill; do not fork the doctrine into runtime-specific copies. is not enough: an adversarial/bypass-focused pass and a general-correctness pass routinely find **disjoint** holes. A send/publish gate needs BOTH lenses before merge. A single-lens "converged" verdict is an incomplete review, not a green - light. + light. When your review bot is unavailable, the substitute that satisfies this + rule is the panel in [`fallback-review-panel.md`](fallback-review-panel.md) — + a single fallback command run in the author's own context does not. 1. **Adversarial review to convergence, not one pass.** Re-review after every fix round until a full pass finds nothing new. Fix rounds on gate logic routinely introduce their own regressions — treat "the last round found nothing" as - provisional, not proof of safety. + provisional, not proof of safety. Be aware that "finds nothing new" may never + arrive: see [`fallback-review-panel.md`](fallback-review-panel.md) for the + observed base rate and for the stopping criterion to use instead — blast + radius, not round count. 1. **Kill/recovery paths need an integration test.** Unit tests on the handler are insufficient — a kill-path can pass unit tests while the wrapper-level behavior is diff --git a/docs/agentic-dev-kit/workflows/pr-watch.md b/docs/agentic-dev-kit/workflows/pr-watch.md index d3f488b..fe57691 100644 --- a/docs/agentic-dev-kit/workflows/pr-watch.md +++ b/docs/agentic-dev-kit/workflows/pr-watch.md @@ -7,9 +7,12 @@ run it without being asked. **Input:** an optional PR number. With none, the current branch's open PR is used. -Read `config/dev-model.yaml` first. Resolve `` from `paths.engines`, -and select the current runtime's independent fallback from -`review.fallback_commands` when needed. +Read `config/dev-model.yaml` first. Resolve `` from `paths.engines`. +When a configured review bot is unavailable, the substitute pass is the +**panel** in `review.fallback_panel` — read +[`../fallback-review-panel.md`](../fallback-review-panel.md) before running it. +`review.fallback_commands` is the degraded one-lens mode for a runtime that +cannot isolate a reviewer. If the diff affects a customer-facing gate, destructive operation, recovery path, or other configured high-risk file, read and apply @@ -65,14 +68,26 @@ Repeat until the report says **converged**: 1. **If there are `new_comments`:** handle each with judgment — - **Reviewer unavailable** (`review_unavailable_reason` is set — rate limit, - skipped review, no credits): run the current runtime's configured - `review.fallback_commands` pass. A blocked bot is an action signal, never - auto-noise or a review waiver. Acknowledge the notice only after the fallback - review has completed and every finding from it is handled. Then record the - pass against the exact `head` from the poll you reviewed with `uv run - /pr_watch.py --record-review "fallback:" --head - `. For a lane, use `/dev_session.sh pr-watch - --record-review "fallback:" --head ` instead. + skipped review, no credits): run the **fallback review panel** — + `review.fallback_panel`, one isolated fresh-context reviewer per lens. Read + [`../fallback-review-panel.md`](../fallback-review-panel.md) for the contract + each lens gets; it is what makes the pass independent of you. A blocked bot + is an action signal, never auto-noise or a review waiver. Acknowledge the + notice only after every finding is handled, then bind the pass to the exact + `head` from the poll you reviewed: + + ```sh + uv run /pr_watch.py \ + --record-review "" \ + --lenses --head + ``` + + `--lenses` names what actually ran, so a degraded one-lens pass is + distinguishable from a panel in the audit trail. It is **self-reported** — + the engine records it and shows it at merge time, but cannot verify it. If your + runtime cannot isolate a reviewer, run `review.fallback_commands` instead + and record it as `fallback:` with the single lens named. For a + lane, use `/dev_session.sh pr-watch ` with the same flags. - **Real finding** (a bug, a missing guard, a correctness/clarity issue): fix it in the code, commit, push. Re-running the local gate first. @@ -141,8 +156,8 @@ Self-pace on a bounded cadence — don't busy-wait: summaries) is filtered out by the engine. Reviewer-unavailable notices are deliberately *not* noise: they surface as new comments and so block `converged`; acknowledging one clears `converged` but still leaves the current-head - review-evidence blocker on `mergeable` until the configured fallback runs and - records its receipt. + review-evidence blocker on `mergeable` until the panel 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 @@ -151,7 +166,7 @@ Self-pace on a bounded cadence — don't busy-wait: 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 + to run the `review.fallback_panel` pass. It stays visible after you `--mark-seen` the notice comment, so the gap is still readable at merge time. Only a **check**-surface outage cancels the pending block below: a check describes the bot's state now, while comments are the whole PR history unscoped by @@ -204,7 +219,26 @@ Self-pace on a bounded cadence — don't busy-wait: invalidating a receipt on a shape change risks wedging a repo whose bot is permanently unavailable. + Once a current-head receipt exists, every poll also prints what that receipt + **claims** to cover: + + ``` + review evidence: fallback:panel — 2 lenses claimed (adversarial, correctness) + review evidence: fallback:codex — ⚠ ONE lens claimed (correctness) — not a dual-lens pass + ``` + + Self-reported: `--lenses` is written by whoever ran `--record-review`, and the + engine records it without verifying it (issue #32). It is shown here so a + one-lens pass is visible when a merge is considered rather than only in the + record command's output. Entries that read as prose rather than a lens name + are recorded but not counted. + **Known gaps, so you don't mistake them for coverage:** + - The `review evidence:` line prints on the **poll render**, which a human + reads. `dev_session.sh merge` consumes the JSON and gates on `mergeable` + alone — so on an autonomous self-merge path nobody sees it. That is the gap + issue #32 exists to close properly; until then, an unattended lane's review + coverage is only as good as what it recorded. - `coverage` reports only bots that have reviewed *and* whose review carried a commit SHA. A bot that has never reviewed at all produces no entry and no warning — that case is the pending/unavailable machinery's, not this one's. diff --git a/docs/autonomous-session-playbook.md b/docs/autonomous-session-playbook.md index 82c8943..8c399d4 100644 --- a/docs/autonomous-session-playbook.md +++ b/docs/autonomous-session-playbook.md @@ -70,12 +70,15 @@ Follow this top to bottom, per ticket. bot's last review rather than waiting indefinitely for a re-review that may not come. Fix real findings (commit + push → re-watch); reply-with-reason to nitpicks you keep. If every configured bot is unavailable (rate-limited, no credits, skipped the PR), run - the current runtime's `review.fallback_commands` value as the substitute independent - pass, handle its findings, then bind that evidence to the current head with - `uv run /pr_watch.py --record-review "fallback:" --head - ` (use the `head` from the exact reviewed poll; a lane uses the - scope-aware `dev_session.sh pr-watch` wrapper) — a blocked bot is not a waiver on - review. + the `review.fallback_panel` pass — one isolated fresh-context reviewer per lens, per + `docs/agentic-dev-kit/fallback-review-panel.md`. Handle its findings, then bind that + evidence to the current head with `uv run /pr_watch.py + --record-review "" --lenses --head + ` (use the `head` + from the exact reviewed poll; a lane uses the scope-aware `dev_session.sh pr-watch` + wrapper). Unattended, this matters more than it does interactively: there is no + operator reading the findings, so a one-lens pass in your own context is the author + approving their own diff. A blocked bot is not a waiver on review. ### Merge + close out diff --git a/init.sh b/init.sh index 9d1544c..e21b06a 100755 --- a/init.sh +++ b/init.sh @@ -374,6 +374,21 @@ migrate_kit_schema() { - "review skipped" - "no review credits"' || true + ensure_review_key fallback_panel ' # The independent pass when a configured bot cannot review. One isolated, + # fresh-context reviewer PER LENS — `safety-critical-changes.md` rule 2 wants two + # disjoint lenses, which a single command cannot be. `fallback_commands` elsewhere + # in this section is the DEGRADED one-lens mode for a runtime that cannot isolate + # a reviewer (a migration appends this block, so the two can land in either order). + # Which lenses is yours; how to run one is kit doctrine, in + # docs/agentic-dev-kit/fallback-review-panel.md. + fallback_panel: + receipt_source: "fallback:panel" + lenses: + - name: adversarial + focus: "assume the change is wrong and try to prove it — bypasses, fail-open paths, wedges, and whether the new guard actually guards" + - name: correctness + focus: "assume it works and ask what it says — stale comments, claims that overstate what is verified, tests whose names promise more than their bodies check"' || true + ensure_review_key informational_checks ' informational_checks: [coderabbit]' || true ensure_review_key require_ci ' # False only for a repo with NO CI at all — otherwise pr-watch never converges. @@ -715,6 +730,9 @@ add_ignore_line() { } add_ignore_line "state/" add_ignore_line ".devkit_state_root" +# Isolated review lenses (fallback-review-panel.md contract item 7) run in their +# own worktrees; those must never be committed back into the repo. +add_ignore_line ".claude/worktrees/" # dev_session.sh copies a repo-root .mcp.json into each lane worktree so lanes # inherit MCP access. If yours holds literal credentials rather than ${ENV} # references, that copy must never be committable from a lane. @@ -733,7 +751,7 @@ install_hooks echo "" echo "agentic-dev-kit is bootstrapped (kit schema v2)." echo "Review config/dev-model.yaml for any remaining values (paths, doc_budgets," -echo "models, review.fallback_commands) and edit to taste." +echo "models, review.fallback_panel.lenses) and edit to taste." echo "" echo "Upgrading later: pull the new kit files, then re-run ./init.sh — it" echo "migrates an older config forward in place and never touches a narrative" diff --git a/kit-manifest.json b/kit-manifest.json index fea5b7f..bee30d7 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -13,9 +13,13 @@ ".gitignore" ], "files": { + "docs/agentic-dev-kit/fallback-review-panel.md": { + "role": "doctrine", + "sha256": "cf460b82d9c0e4f55691ca23a77f0a46911af3282d9f317da46b51e81b9227d2" + }, "docs/agentic-dev-kit/safety-critical-changes.md": { "role": "doctrine", - "sha256": "cc3f3a76a22ba0a8552539bd2b85f07e10e59485eae2c3749f06036bf7db2e91" + "sha256": "b889d9dddb9c3b5b7441162406094530532288a996af232120ca2b22c822d312" }, "docs/agentic-dev-kit/workflows/parallel.md": { "role": "workflow", @@ -23,7 +27,7 @@ }, "docs/agentic-dev-kit/workflows/pr-watch.md": { "role": "workflow", - "sha256": "7f116a78706776feb68e9065f434eedd63a1eec420f3b8314899b8e4e58dfbfe" + "sha256": "358a3eae1277920974a4a2752cfb3112b5cadefc1e9cf5e3399dc57a9aa2912e" }, "docs/agentic-dev-kit/workflows/session-start.md": { "role": "workflow", @@ -67,7 +71,7 @@ }, "scripts/kit_doctor.py": { "role": "engine", - "sha256": "d2e4fcaf3e4814a340b1c0ac499e3a79a977122004d593629cbb562a9213bbd0" + "sha256": "7268ec94aa5b4c44311a57e34447f92b09a64eba34bf7b0e9fecf2e099a68f9f" }, "scripts/lib/devmodel_config.py": { "role": "engine", @@ -99,7 +103,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "b04cc9a678a28c9d292b8f4d98062fd24f82e1a5bbdc5ac380dbca7e9394639b" + "sha256": "b8ee2baf65375d9422e2bbfce7ba4ce5de965a4f331a3b82ef948606fcc3a670" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/hooks/pr_followup_hook.py b/scripts/hooks/pr_followup_hook.py index 28876ee..9db0a20 100755 --- a/scripts/hooks/pr_followup_hook.py +++ b/scripts/hooks/pr_followup_hook.py @@ -10,7 +10,8 @@ unasked (Principle #8: "a rule that lives only in a doc is a wish"). Generalized from a project-specific version: the reminder names whichever review -bot(s) and fallback command this repo actually configures, read from +bot(s) and the fallback PANEL (or, when fewer than two lenses are configured, +the single fallback command) this repo actually configures, read from `config/dev-model.yaml` via `scripts/lib/kitconfig.py` — never a hardcoded bot name (Principle #10, "No hardcoding"). `paths.engines` resolves where `pr_watch.py` lives so the reminder's poll command is correct even when the kit is vendored under a @@ -41,10 +42,12 @@ _DEFAULT_FALLBACK_COMMAND = "/code-review" _DEFAULT_ENGINES_DIR = "scripts" +_DEFAULT_PANEL_RECEIPT_SOURCE = "fallback:panel" -def _load_review_config() -> tuple[list[str], str, str]: - """Read ``(review.bots, review.fallback_commands.claude, paths.engines)``. +def _load_review_config() -> tuple[list[str], str, str, list[str], str]: + """Read ``(review.bots, review.fallback_commands.claude, paths.engines, + review.fallback_panel lens names, review.fallback_panel.receipt_source)``. Best-effort: any failure (missing config, kitconfig unimportable, malformed values) falls back to generic defaults rather than raising — this hook must @@ -64,9 +67,39 @@ def _load_review_config() -> tuple[list[str], str, str]: fallback = _DEFAULT_FALLBACK_COMMAND if not isinstance(engines, str) or not engines: engines = _DEFAULT_ENGINES_DIR - return bots, fallback, engines + panel = kitconfig.get(config, "review.fallback_panel.lenses", []) + lenses = ( + [ + lens["name"].strip() + for lens in panel + if isinstance(lens, dict) + and isinstance(lens.get("name"), str) + # A blank name would have the hook advertise a panel with an + # unnameable lens — worse than advertising no panel at all. + and lens["name"].strip() + ] + if isinstance(panel, list) + else [] + ) + panel_source = kitconfig.get( + config, "review.fallback_panel.receipt_source", _DEFAULT_PANEL_RECEIPT_SOURCE + ) + if not isinstance(panel_source, str) or not panel_source.strip(): + panel_source = _DEFAULT_PANEL_RECEIPT_SOURCE + return bots, fallback, engines, lenses, panel_source.strip() except Exception: - return [], _DEFAULT_FALLBACK_COMMAND, _DEFAULT_ENGINES_DIR + return ( + [], + _DEFAULT_FALLBACK_COMMAND, + _DEFAULT_ENGINES_DIR, + # NO default lens roster, deliberately: this path means the config + # could not be read, so nothing has confirmed a panel exists. A + # non-empty default here would have the hook tell the operator to + # claim panel coverage on the strength of a config it just failed to + # load — and nothing downstream would catch that (issue #32). + [], + _DEFAULT_PANEL_RECEIPT_SOURCE, + ) def _bot_description(bots: list[str]) -> str: @@ -78,8 +111,43 @@ def _bot_description(bots: list[str]) -> str: return " / ".join(names) +def _fallback_instruction( + fallback_command: str, + lenses: list[str], + panel_source: str = _DEFAULT_PANEL_RECEIPT_SOURCE, + engines_dir: str = _DEFAULT_ENGINES_DIR, +) -> str: + """What to run when a bot is unavailable. + + Names the PANEL when one is configured, because a single command in this + session's own context is the author reviewing their own diff — which + `safety-critical-changes.md` rule 2 says is not a green light. This hook + fires on every `gh pr create`/`ready`, so it is the most-read statement of + the fallback policy in the kit; pointing it at the degraded mode taught the + wrong habit every time. + """ + # Two DISTINCT lenses is the panel's floor (see fallback-review-panel.md). + # A one-lens `fallback_panel` is not a panel, so advertising one would tell + # the operator to claim coverage they cannot have. Nothing downstream will + # stop them: the engine records `--lenses` without verifying it (issue #32), + # so this wording is the only thing steering it. + if len({lens.casefold() for lens in lenses}) >= 2: + return ( + "If a review bot is unavailable, run the fallback review PANEL — one " + f"isolated, fresh-context reviewer per lens ({', '.join(lenses)}), per " + "docs/agentic-dev-kit/fallback-review-panel.md — and record it with " + f"`uv run {engines_dir}/pr_watch.py " + f'--record-review "{panel_source}" --lenses --head `. ' + "Never treat the outage as a review waiver." + ) + return ( + f"If a review bot is unavailable, run the configured fallback " + f"(`{fallback_command}`) instead of treating the outage as a review waiver." + ) + + def build_reminder() -> str: - bots, fallback_command, engines_dir = _load_review_config() + bots, fallback_command, engines_dir, lenses, panel_source = _load_review_config() bot_desc = _bot_description(bots) return ( "A pull request was just opened or marked ready for review. Per the kit's " @@ -90,9 +158,9 @@ def build_reminder() -> str: f"until CI is fully green AND every {bot_desc} finding is fixed or " "replied-to with a reason. Fix real findings, reply-with-reason to nitpicks " "you disagree with, `--mark-seen` each handled round, and keep polling (CI " - "can take 20-30 min). If a review bot is unavailable, run the configured " - f"fallback (`{fallback_command}`) instead of treating the outage as a review " - "waiver. Only stop early if you hit something that genuinely needs an " + "can take 20-30 min). " + + _fallback_instruction(fallback_command, lenses, panel_source, engines_dir) + + " Only stop early if you hit something that genuinely needs an " "operator decision." ) diff --git a/scripts/kit_doctor.py b/scripts/kit_doctor.py index 8a15934..d8d358a 100755 --- a/scripts/kit_doctor.py +++ b/scripts/kit_doctor.py @@ -94,6 +94,11 @@ ("docs/agentic-dev-kit/workflows/pr-watch.md", "workflow"), ("docs/agentic-dev-kit/workflows/parallel.md", "workflow"), ("docs/agentic-dev-kit/safety-critical-changes.md", "doctrine"), + # Tracked because safety-critical-changes.md — which IS refreshed by + # /upgrade — links to it from rules 2 and 3. An untracked target means an + # upgrading adopter gets doctrine pointing at a file they do not have, and + # kit_doctor cannot report it missing because it is not tracked. + ("docs/agentic-dev-kit/fallback-review-panel.md", "doctrine"), # narrative-doc templates (the rendered outputs are adopter-owned) ("docs/templates/handoff.md.tmpl", "template"), ("docs/templates/handoff-history.md.tmpl", "template"), diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index d9d7aec..02c30b6 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -86,9 +86,11 @@ import hashlib import json import os +import re import subprocess import sys import time +import unicodedata from pathlib import Path from typing import NamedTuple @@ -255,6 +257,7 @@ def _resolve_state_root(start: Path, repo_root: Path, state_root_env: str | None # 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 @@ -975,7 +978,8 @@ 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. + signal: run the fallback review panel + (docs/agentic-dev-kit/fallback-review-panel.md). Only a **check**-surface hit suppresses the pending block below. A check describes the bot's state now; a comment describes the past, and @@ -1371,6 +1375,7 @@ def record_review( expected_head: str, *, allow_pending_bot: bool = False, + lenses: str | None = None, now: datetime | None = None, ) -> dict: """Persist independent-review evidence bound to the PR's current head SHA. @@ -1400,6 +1405,12 @@ def record_review( check read (``bot_signal``; but not "no bots configured" — nothing was unreadable in that case) and any bot whose last review predates this head (``bots_behind_head``). All three say what the receipt does NOT stand for. + + ``lenses`` names the review lenses that actually ran (see + ``docs/agentic-dev-kit/fallback-review-panel.md``). Recorded verbatim so a + one-lens pass is distinguishable from a panel in the audit trail: the + doctrine holds that a single-lens verdict is not a green light, and without + this a degraded `fallback:` receipt reads exactly like a full one. """ source = source.strip() if not source: @@ -1469,6 +1480,9 @@ def record_review( "source": source, "recorded_at": now.isoformat(), } + named_lenses = [part.strip() for part in (lenses or "").split(",") if part.strip()] + if named_lenses: + receipt["lenses"] = named_lenses 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 @@ -1501,8 +1515,58 @@ def record_review( # ----------------------------------------------------------------------- main +# A lens NAME, not a sentence: letters, digits, and the usual separators. +# `--lenses` splits on `,`, which is also ordinary punctuation — so +# "adversarial, focused on the new merge gate" arrived as two entries and +# rendered as a two-lens panel, suppressing the one-lens warning that is the +# whole remaining value of the field. That is an HONEST input misreported, not +# only a forgery. Counting only entries that look like names fixes both without +# a roster and without a gate: prose is still recorded verbatim, it just does +# not count toward "how many lenses ran". +_LENS_NAME_RE = re.compile(r"^[\w.+-]{1,40}$") + + +def _countable_lenses(names: list[str]) -> set[str]: + """Case-folded lens names from ``names``, ignoring prose.""" + return {n.casefold() for n in names if _LENS_NAME_RE.match(n)} + + +def _flat(text: object, n: int = 120) -> str: + """One line, bounded. For any receipt/config value entering a render. + + Both `source` and the lens names are free text chosen by whoever ran + `--record-review`. Interpolated raw, a single newline splits the coverage + line in two and leaves the first half reading as a completed panel: + + review evidence: fallback:panel — 2 lenses claimed (adversarial, + correctness) (recorded) — ⚠ ONE lens claimed … + + `_excerpt` already established this convention for comment bodies; the + receipt fields skipped it. + """ + # Strip C0/C1 control characters BEFORE collapsing whitespace. `split()` + # normalizes whitespace but `\x1b` is not whitespace, so ANSI cursor + # control survived — and that is strictly worse than the newline this + # function was written for: `\x1b[1A\x1b[2K` *erases* lines that exist + # rather than appending ones that don't, so a receipt could delete the + # merge blockers printed above it. + cleaned = "".join(" " if unicodedata.category(c) == "Cc" else c for c in str(text)) + flat = " ".join(cleaned.split()) + return flat if len(flat) <= n else flat[: n - 1] + "…" + + def _excerpt(body: str, n: int = 140) -> str: - flat = " ".join((body or "").split()) + # Same control-character strip as :func:`_flat`, and for a stronger reason: + # this renders a COMMENT BODY, which on a public repo any account can write. + # It also renders last, so cursor-up sequences here walk over every merge + # blocker above them. Pre-existing on main; fixed here because `_flat`'s + # docstring cites this function as the convention it copies, and copying a + # defect forward is how the convention stops being one. + flat = " ".join( + "".join( + " " if unicodedata.category(c) == "Cc" else c for c in (body or "") + ).split() + ) return flat if len(flat) <= n else flat[: n - 1] + "…" @@ -1614,6 +1678,34 @@ def build_report( else None ), "head": receipt_head, + # Carried into the report so the poll render can state what the receipt + # stands for. Previously the one-lens warning printed exactly once — on + # the stdout of the `--record-review` call the agent itself chose to + # make — and never again at the moment a merge is authorized. + # `isinstance(..., list)` first: a bare string is iterable, so a + # hand-edited `"lenses": "adversarial"` would otherwise be read as + # eleven single-character lenses and render as ample coverage. + "lenses": ( + [ + lens + for lens in review_receipt["lenses"] + if isinstance(lens, str) and lens.strip() + ] + if isinstance(review_receipt, dict) + and receipt_head == head + and isinstance(review_receipt.get("lenses"), list) + else [] + ), + "override": ( + review_receipt.get("override") + if isinstance(review_receipt, dict) and receipt_head == head + else None + ), + "bot_signal": ( + review_receipt.get("bot_signal") + if isinstance(review_receipt, dict) and receipt_head == head + else None + ), } merge_blockers: list[str] = [] if pr_state != "OPEN": @@ -1748,7 +1840,7 @@ def render(report: dict) -> str: for entry in bots.get("unavailable") or []: lines.append( f" ⚠ review unavailable [{entry['surface']}] {entry['where']}: " - f"{entry['reason']} — run the configured fallback review" + f"{entry['reason']} — run the fallback review panel (docs/agentic-dev-kit/fallback-review-panel.md)" ) grace = bots.get("grace_minutes") for entry in bots.get("pending") or []: @@ -1760,7 +1852,47 @@ def render(report: dict) -> str: lines.append( f" ⚠ review bot {entry['bot']} check {entry['check']} still pending after " f"{entry['age_minutes']}m (past the {grace:g}m grace) — " - "treated as not coming; run the configured fallback review" + "treated as not coming; run the fallback review panel (docs/agentic-dev-kit/fallback-review-panel.md)" + ) + # What the current-head receipt actually stands for. The gate cannot judge + # this — `source` is free text an agent chooses — so the honest move is to + # SHOW it at the moment a merge is considered, rather than to pattern-match + # a label and hope. A relabelled one-lens receipt now reads as one lens + # regardless of what it is called. + evidence = report.get("review_evidence") or {} + if evidence.get("valid"): + # SELF-REPORTED, and labelled as such. Whoever ran `--record-review` + # wrote both the source and the lens names in one invocation, with + # nothing binding either to a review that happened — so this engine + # cannot verify coverage, and four rounds of trying to (matching the + # source, then the lens names, then a configured roster) produced a + # check defeated by one extra character while the render affirmed the + # forgery. `safety-critical-changes.md` rule 1: treat "we tightened the + # matcher" as a stopgap, not a fix. So: report the claim, name it a + # claim, and let a reader judge it. Verifying it needs each lens to + # record its own receipt from its own context — see issue #32. + named = [_flat(lens, 40) for lens in evidence.get("lenses") or []] + distinct = len(_countable_lenses(named)) + source = _flat(evidence.get("source")) + if distinct >= 2: + detail = f"{distinct} lenses claimed ({', '.join(named)})" + elif named: + detail = f"⚠ ONE lens claimed ({named[0]}) — not a dual-lens pass" + else: + detail = "no lenses recorded" + lines.append(f" review evidence: {source} — {detail}") + # The same argument that moved `lenses` to the poll render applies to + # its siblings: a caveat printed only on the stdout of the + # `--record-review` call the agent itself made is not visible at the + # moment a merge is considered. + if evidence.get("override"): + lines.append( + f" ⚠ recorded over an active override ({_flat(evidence['override'])})" + ) + if evidence.get("bot_signal"): + lines.append( + f" ⚠ review-bot state was unreadable ({_flat(evidence['bot_signal'])}) " + "when this receipt was taken" ) for blocker in report.get("merge_blockers") or []: lines.append(f" ✗ merge blocker: {blocker}") @@ -1771,7 +1903,7 @@ def render(report: dict) -> str: if c.get("review_unavailable_reason"): lines.append( f" • [review unavailable] @{c['author']}{loc}: " - f"{c['review_unavailable_reason']} — run the configured fallback review" + f"{c['review_unavailable_reason']} — run the fallback review panel (docs/agentic-dev-kit/fallback-review-panel.md)" ) else: lines.append(f" • [{c['kind']}] @{c['author']}{loc}: {c['excerpt']}") @@ -1782,7 +1914,7 @@ def render_record_review(report: dict) -> str: receipt = report["review_receipt"] lines = [ f"PR #{report['pr']} — recorded independent review from " - f"{receipt['source']} for head {receipt['head']}" + f"{_flat(receipt['source'])} for head {_flat(receipt['head'], 60)}" ] if receipt.get("override"): lines.append( @@ -1794,9 +1926,27 @@ def render_record_review(report: dict) -> str: f" ⚠ review-bot state was unreadable ({receipt['bot_signal']}) when this " "receipt was taken — the queued-reviewer guard did not run" ) - for bot, sha in (receipt.get("bots_behind_head") or {}).items(): + # Type-guarded like the sibling receipt fields: the state file is plain + # JSON on disk and anything that can run this engine can edit it, so a + # non-list or a list of non-strings must not crash the render. + raw_lenses = receipt.get("lenses") + named = ( + [lens for lens in raw_lenses if isinstance(lens, str) and lens.strip()] + if isinstance(raw_lenses, list) + else [] + ) + named = [_flat(lens, 40) for lens in named] + if len(_countable_lenses(named)) == 1: + lines.append( + f" ⚠ one lens only ({named[0]}) — `safety-critical-changes.md` rule 2 " + "holds that a single-lens verdict is not a green light" + ) + elif named: + lines.append(f" lenses: {', '.join(named)}") + behind_map = receipt.get("bots_behind_head") + for bot, sha in (behind_map if isinstance(behind_map, dict) else {}).items(): lines.append( - f" ⚠ {bot}'s last review was of {sha[:7]}, not this head — this receipt " + f" ⚠ {bot}'s last review was of {_flat(sha, 12)}, not this head — this receipt " "does not stand for its review of this design" ) return "\n".join(lines) @@ -1880,6 +2030,16 @@ def main(argv: list[str] | None = None) -> int: "will never arrive — a queued bot is SLOW, not unavailable (issue #19)" ), ) + parser.add_argument( + "--lenses", + metavar="NAMES", + help=( + "with --record-review: comma-separated review lenses that actually ran " + "(e.g. adversarial,correctness). Recorded on the receipt so a one-lens " + "pass is distinguishable from a panel — see " + "docs/agentic-dev-kit/fallback-review-panel.md" + ), + ) mode_group = parser.add_mutually_exclusive_group() mode_group.add_argument( "--mark-seen", @@ -1920,6 +2080,8 @@ def main(argv: list[str] | None = None) -> int: 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") + if args.lenses and args.record_review is None: + parser.error("--lenses is only valid with --record-review") try: pr = resolve_pr(args.pr) @@ -1945,6 +2107,7 @@ def main(argv: list[str] | None = None) -> int: args.record_review, args.head, allow_pending_bot=args.allow_pending_bot_review, + lenses=args.lenses, ) except (RuntimeError, KeyError, ValueError) as exc: print(f"error: {exc}", file=sys.stderr) diff --git a/scripts/tests/test_portability.py b/scripts/tests/test_portability.py index 32512b5..fca8eb9 100644 --- a/scripts/tests/test_portability.py +++ b/scripts/tests/test_portability.py @@ -916,6 +916,17 @@ def test_init_migrates_the_previous_runtime_schema(tmp_path: Path) -> None: assert config["runtime"]["default"] == "claude" assert config["runtime"]["launchers"]["codex"] == "codex" assert config["review"]["fallback_commands"]["codex"] == "/review" + # The panel is what the fallback actually IS now; deleting its whole + # migration block from init.sh previously passed the entire suite. + panel = config["review"]["fallback_panel"] + assert panel["receipt_source"] == "fallback:panel" + assert [lens["name"] for lens in panel["lenses"]] == ["adversarial", "correctness"] + # The migrated focus text must match what a fresh install ships, or an + # upgrading adopter runs a materially weaker lens prompt than a new one. + shipped = yaml.safe_load( + (REPO_ROOT / "config" / "dev-model.yaml").read_text(encoding="utf-8") + )["review"]["fallback_panel"] + assert panel["lenses"] == shipped["lenses"] assert config["models"]["runtime_mappings"]["claude"] == { "cheap": "tiny", "default": "normal", diff --git a/scripts/tests/test_pr_followup_hook.py b/scripts/tests/test_pr_followup_hook.py index d7d465f..23a956c 100644 --- a/scripts/tests/test_pr_followup_hook.py +++ b/scripts/tests/test_pr_followup_hook.py @@ -104,18 +104,145 @@ def test_exits_zero_on_malformed_or_unexpected_stdin(monkeypatch, capsys, stdin_ assert out == "" -def test_reminder_names_configured_bots_and_fallback_not_a_hardcoded_bot(monkeypatch): - """The reminder must be sourced from config (review.bots / fallback_commands), +def test_reminder_names_configured_bots_not_a_hardcoded_bot(): + """The reminder must be sourced from config (review.bots / fallback_panel), never a hardcoded bot literal — this is the whole point of generalizing the reference implementation (Principle #10, "No hardcoding").""" hook = _load_hook() reminder = hook.build_reminder() assert "coderabbit" in reminder # from this repo's config/dev-model.yaml review.bots - assert "/code-review" in reminder # review.fallback_commands.claude - assert "codex" not in reminder.lower() assert "bugbot" not in reminder.lower() +def test_reminder_points_at_the_panel_not_the_degraded_one_lens_mode(monkeypatch): + """This hook fires on every `gh pr create`/`ready`, so it is the most-read + statement of the fallback policy in the kit. + + Pointing it at `fallback_commands` — a single command in the author's own + context — taught the wrong habit every time it fired, against + `safety-critical-changes.md` rule 2. With a panel configured it must name + the panel and its lenses. + """ + hook = _load_hook() + + reminder = hook.build_reminder() + + assert "PANEL" in reminder + assert "adversarial" in reminder and "correctness" in reminder + assert "fallback-review-panel.md" in reminder + assert "--lenses" in reminder + # …and it must NOT advertise the degraded command as the thing to run. + assert "/code-review" not in reminder + + +def test_the_reminder_reads_the_receipt_source_from_config(monkeypatch): + """`review.fallback_panel.receipt_source` exists so an adopter can rename it. + + Driven through `load_config`, not through a positional argument: passing the + value in only proves the formatter interpolates it. A mutation that made the + loader ignore config entirely and return the default survived the whole + suite — which is adding a config key and then not reading it (Principle #10), + the exact failure the docstring claims to guard. + """ + hook = _load_hook() + hook._load_review_config() # prime the kitconfig import + import kitconfig # noqa: PLC0415 + + monkeypatch.setattr( + kitconfig, + "load_config", + lambda *a, **k: { + "review": { + "bots": ["somebot"], + "fallback_panel": { + "receipt_source": "fallback:my-panel", + "lenses": [{"name": "adversarial"}, {"name": "correctness"}], + }, + } + }, + ) + + reminder = hook.build_reminder() + + assert '"fallback:my-panel"' in reminder + assert "fallback:panel" not in reminder + + +def test_a_one_lens_panel_config_degrades_instead_of_advertising_a_refusal( + monkeypatch, +): + """Two distinct lenses is the panel's floor, and `record_review` enforces it. + + An adopter who configures one lens would otherwise be told to run "the + PANEL" and record it with a command the engine refuses every single time. + """ + hook = _load_hook() + hook._load_review_config() + import kitconfig # noqa: PLC0415 + + monkeypatch.setattr( + kitconfig, + "load_config", + lambda *a, **k: { + "review": { + "bots": ["somebot"], + "fallback_commands": {"claude": "/solo-review"}, + "fallback_panel": {"lenses": [{"name": "adversarial"}]}, + } + }, + ) + + reminder = hook.build_reminder() + + assert "PANEL" not in reminder + assert "/solo-review" in reminder + assert "review waiver" in reminder + + +def test_blank_lens_names_are_discarded_and_never_advertised(monkeypatch): + """A whitespace-only `name` would have the hook advertise a panel with an + unnameable lens — worse than advertising no panel at all.""" + hook = _load_hook() + hook._load_review_config() + import kitconfig # noqa: PLC0415 + + monkeypatch.setattr( + kitconfig, + "load_config", + lambda *a, **k: { + "review": { + "bots": ["somebot"], + "fallback_panel": { + "lenses": [{"name": " "}, {"name": "adversarial"}, {"name": ""}] + }, + } + }, + ) + + _bots, _fb, _eng, lenses, _src = hook._load_review_config() + + assert lenses == ["adversarial"] + # "never advertised" was the half the name promised and the body skipped: + # one usable lens is below the panel floor, so the reminder must not name + # a panel at all. + assert "PANEL" not in hook.build_reminder() + + +def test_reminder_falls_back_to_the_single_command_with_no_panel_configured(): + """An adopter who has not configured a panel — or a runtime that cannot + isolate a reviewer — must still get actionable wording, not a dangling + reference to lenses that do not exist.""" + hook = _load_hook() + + degraded = hook._fallback_instruction("/my-review", []) + panel = hook._fallback_instruction("/my-review", ["adversarial", "correctness"]) + + assert "`/my-review`" in degraded + assert "PANEL" not in degraded + assert "review waiver" in degraded # the invariant survives either way + assert "/my-review" not in panel + + def test_load_review_config_degrades_gracefully_when_config_unreadable(monkeypatch): """A missing/unreadable config must never raise — the hook falls back to generic wording rather than failing the session.""" @@ -127,7 +254,37 @@ def _boom(*_args, **_kwargs): raise FileNotFoundError("no config here") monkeypatch.setattr(kitconfig, "load_config", _boom) - bots, fallback, engines = hook._load_review_config() + bots, fallback, engines, lenses, panel_source = hook._load_review_config() assert bots == [] assert fallback == "/code-review" assert engines == "scripts" + # No panel known → the reminder degrades to the single command rather than + # naming lenses that were never read. + assert lenses == [] + assert panel_source == "fallback:panel" + assert "PANEL" not in hook._fallback_instruction( + fallback, lenses, panel_source, engines + ) + + +def test_an_unreadable_config_never_advertises_a_panel(monkeypatch): + """This path means nothing confirmed a panel exists. + + A default lens roster here would have the hook advertise a panel — and a + `--record-review` command the engine then refuses — on the strength of a + config it had just failed to read. + """ + hook = _load_hook() + hook._load_review_config() + import kitconfig # noqa: PLC0415 + + def _boom(*_a, **_k): + raise FileNotFoundError("no config") + + monkeypatch.setattr(kitconfig, "load_config", _boom) + + reminder = hook.build_reminder() + + assert "PANEL" not in reminder + assert "/code-review" in reminder # the compatible single-command wording + assert "review waiver" in reminder diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index 8b584bd..253f85f 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 +import sys from datetime import datetime, timedelta, timezone from pathlib import Path from types import ModuleType @@ -360,6 +361,9 @@ def test_review_receipt_must_match_current_head() -> None: assert stale["mergeable"] is False assert current["mergeable"] is True assert current["review_evidence"] == { + "lenses": [], + "override": None, + "bot_signal": None, "valid": True, "source": "fallback:codex", "head": "abc123", @@ -1248,9 +1252,9 @@ def test_record_review_allows_the_fallback_when_the_bot_is_unavailable( 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) + pr_watch.record_review(22, "fallback:codex", "32f3e4f", now=NOW) - assert recorded[0]["review_receipt"]["source"] == "fallback:panel" + assert recorded[0]["review_receipt"]["source"] == "fallback:codex" assert "override" not in recorded[0]["review_receipt"] @@ -1313,7 +1317,7 @@ def test_a_failed_check_read_is_recorded_on_the_receipt( 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) + report = pr_watch.record_review(9, "fallback:codex", "abc123", now=NOW) assert recorded[0]["review_receipt"]["bot_signal"] == "unavailable" assert "guard did not run" in pr_watch.render_record_review(report) @@ -1334,7 +1338,7 @@ def test_the_override_is_recorded_on_the_receipt( monkeypatch.setattr(pr_watch, "load_state", lambda pr: {}) pr_watch.record_review( - 9, "fallback:panel", "abc123", allow_pending_bot=True, now=NOW + 9, "fallback:codex", "abc123", allow_pending_bot=True, now=NOW ) assert recorded[0]["review_receipt"]["override"] == "pending-bot" @@ -1859,7 +1863,7 @@ def test_a_receipt_records_which_bots_were_behind_the_head( 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) + report = pr_watch.record_review(9, "fallback:codex", "abc123", now=NOW) assert recorded[0]["review_receipt"]["bots_behind_head"] == {"coderabbit": "0ldc0de"} assert "does not stand for its review" in pr_watch.render_record_review(report) @@ -1891,7 +1895,7 @@ def test_the_override_path_still_records_which_bots_were_behind( monkeypatch.setattr(pr_watch, "load_state", lambda pr: {}) report = pr_watch.record_review( - 9, "fallback:panel", "abc123", allow_pending_bot=True, now=NOW + 9, "fallback:codex", "abc123", allow_pending_bot=True, now=NOW ) receipt = recorded[0]["review_receipt"] @@ -2092,3 +2096,399 @@ def test_a_non_string_sha_or_timestamp_cannot_break_the_poll() -> None: ) assert report["review_bots"]["coverage"] == [] pr_watch.render(report) # would raise on `sha[:7]` without the type check + + +# --------------------------------------------------------------------------- # +# the fallback review panel: a receipt must not claim more than it stands for +# --------------------------------------------------------------------------- # + + +def _record(monkeypatch, pr_watch, **kwargs) -> tuple[dict, dict]: + 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([], "ok") + ) + recorded: list[dict] = [] + monkeypatch.setattr(pr_watch, "save_state", lambda pr, state: recorded.append(state)) + monkeypatch.setattr(pr_watch, "load_state", lambda pr: {}) + source = kwargs.pop("source", "fallback:panel") + report = pr_watch.record_review(9, source, "abc123", now=NOW, **kwargs) + return recorded[0]["review_receipt"], report + + +def test_a_single_lens_receipt_does_not_read_like_a_panel( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """`safety-critical-changes.md` rule 2: a single-lens verdict is not a green + light. + + Without this, a degraded one-lens fallback and a full panel produce + byte-identical receipts — so the audit trail cannot show which one a merge + actually rested on, which is the whole reason the panel exists. + """ + pr_watch = _load_pr_watch() + + receipt, report = _record( + monkeypatch, pr_watch, source="fallback:codex", lenses="correctness" + ) + + assert receipt["lenses"] == ["correctness"] + rendered = pr_watch.render_record_review(report) + assert "one lens only (correctness)" in rendered + assert "not a green light" in rendered + + +def test_a_panel_receipt_names_every_lens_that_ran( + monkeypatch: pytest.MonkeyPatch, +) -> None: + pr_watch = _load_pr_watch() + + receipt, report = _record( + monkeypatch, pr_watch, lenses="adversarial, correctness" + ) + + assert receipt["lenses"] == ["adversarial", "correctness"] + rendered = pr_watch.render_record_review(report) + assert "lenses: adversarial, correctness" in rendered + assert "one lens only" not in rendered +def test_the_poll_render_surfaces_override_and_unreadable_bot_state() -> None: + """Same argument that moved `lenses` to the poll render applies to its + siblings: a caveat printed only at record time is not visible when a merge + is considered. `lenses` was the only one of the family that had moved.""" + pr_watch = _load_pr_watch() + + report = pr_watch.build_report( + _green_view(), [], set(), + review_receipt={"head": "abc123", "source": "fallback:codex", + "lenses": ["correctness"], "override": "pending-bot", + "bot_signal": "unavailable"}, + ) + rendered = pr_watch.render(report) + + assert "recorded over an active override (pending-bot)" in rendered + assert "review-bot state was unreadable (unavailable)" in rendered + + +def test_a_hand_edited_bots_behind_head_cannot_break_the_receipt_render() -> None: + """The commit that added the `lenses` guard claimed parity with "the sibling + receipt fields" — which were not guarded. This one raised AttributeError on + a string or a list.""" + pr_watch = _load_pr_watch() + + for junk in ("coderabbit", ["coderabbit"], 5, None): + pr_watch.render_record_review( + {"pr": 9, "review_receipt": {"head": "abc", "source": "s", + "bots_behind_head": junk}} + ) + + +def test_every_unavailability_line_points_at_the_panel_not_the_degraded_mode() -> None: + """This PR redefines `review.fallback_commands` as the DEGRADED mode, so + "the configured fallback" now names the wrong thing. + + The strings were changed at two of three sites and pinned by none — the + existing assertions matched the `review unavailable` prefix, never the + pointer, so reverting the change passed the whole suite. That is the + "named by a test and pinned by nothing" class this PR ships doctrine about. + """ + pr_watch = _load_pr_watch() + + outage_comment = pr_watch.build_report( + _green_view( + comments=[{"id": "c1", "author": {"login": "coderabbitai"}, + "body": "Review limit reached."}] + ), + [], set(), now=NOW, + ) + outage_check = pr_watch.build_report( + _green_view(), [], set(), + check_details=[_bot_check(description="Review rate limited")], now=NOW, + ) + aged_out = pr_watch.build_report( + _green_view(), [], set(), + check_details=[ + _bot_check(state="PENDING", bucket="pending", startedAt=_minutes_ago(600)) + ], + now=NOW, + ) + + for label, report in ( + ("comment surface", outage_comment), + ("check surface", outage_check), + ("grace-cancelled", aged_out), + ): + rendered = pr_watch.render(report) + assert "fallback review panel" in rendered, label + assert "configured fallback review" not in rendered, label + + +def test_the_poll_render_reports_the_receipt_as_a_CLAIM_not_a_verdict() -> None: + """Coverage is self-reported and the render says so. + + Whoever ran `--record-review` wrote both the source and the lens names in + one invocation, with nothing binding either to a review that happened. Four + rounds went into trying to verify it from here — matching the source, then + the lens names, then a configured roster — and each was defeated: the last + by a single extra character in the source, after which the render *affirmed* + the forgery. Rule 1 calls that a stopgap, not a fix. + + So the render states what the receipt claims, labelled as a claim. That is + honest and still useful: a one-lens receipt is visible at merge time. + Verifying it needs each lens to record its own receipt (issue #32). + """ + pr_watch = _load_pr_watch() + + def _line(receipt): + report = pr_watch.build_report(_green_view(), [], set(), review_receipt=receipt) + # Prefix match: the merge-blocker line legitimately contains the same + # phrase, so a substring test picks it up when no receipt exists. + return next( + (l.strip() for l in pr_watch.render(report).splitlines() + if l.strip().startswith("review evidence:")), + "", + ) + + two = _line({"head": "abc123", "source": "fallback:panel", + "lenses": ["adversarial", "correctness"]}) + one = _line({"head": "abc123", "source": "fallback:codex", + "lenses": ["correctness"]}) + none = _line({"head": "abc123", "source": "coderabbit"}) + + assert "2 lenses claimed (adversarial, correctness)" in two + assert "ONE lens claimed (correctness)" in one + assert "no lenses recorded" in none + # No line at all without a current-head receipt. + assert _line({"head": "0ldc0de", "source": "fallback:panel"}) == "" + + +def test_the_render_cannot_be_forged_into_extra_lines_or_extra_lenses() -> None: + """It reports a claim, but it must report it accurately. + + A newline in `source` split the line and left the first half reading as a + completed panel; a comma inside a lens description counted as two lenses; + duplicates counted twice. + """ + pr_watch = _load_pr_watch() + + def _render(receipt): + return pr_watch.render( + pr_watch.build_report(_green_view(), [], set(), review_receipt=receipt) + ) + + forged = _render({ + "head": "abc123", + "source": "fallback:panel — 2 lenses (adversarial, correctness)\n (recorded)", + }) + assert len([l for l in forged.splitlines() + if l.strip().startswith("review evidence:")]) == 1 + assert not any(l.strip().startswith("(recorded)") for l in forged.splitlines()) + + duped = _render({"head": "abc123", "source": "fallback:codex", + "lenses": ["adversarial", "Adversarial"]}) + assert "ONE lens claimed" in duped + + +def test_a_receipt_records_lenses_without_the_engine_judging_them( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """`--record-review` no longer refuses anything on lens grounds. + + The refusal was a matcher over caller-supplied text in the same invocation + as the claim it checked. Removing it is the honest state: the receipt is an + audit trail, and nothing here pretends otherwise. + """ + pr_watch = _load_pr_watch() + + for lenses in ("adversarial", "adversarial,correctness", "a,b,c", None): + receipt, _ = _record(monkeypatch, pr_watch, lenses=lenses) + expected = [p.strip() for p in (lenses or "").split(",") if p.strip()] + assert receipt.get("lenses", []) == expected, lenses + + +def test_a_lens_described_in_prose_counts_as_one_lens() -> None: + """`--lenses` splits on `,`, which is also ordinary punctuation. + + "adversarial, focused on the new merge gate" is an HONEST way to record one + lens, and it arrived as two entries and rendered as a two-lens panel — + suppressing the one-lens warning that is the whole remaining value of the + field. Deleting the roster check (which had caught this class) reintroduced + it. Counting only entries that look like NAMES fixes the honest case and the + forgery together, without a roster and without a gate. + """ + pr_watch = _load_pr_watch() + + def _line(lenses): + report = pr_watch.build_report( + _green_view(), [], set(), + review_receipt={"head": "abc123", "source": "fallback:codex", + "lenses": lenses}, + ) + return next(l.strip() for l in pr_watch.render(report).splitlines() + if l.strip().startswith("review evidence:")) + + assert "ONE lens claimed" in _line(["adversarial", " focused on the merge gate"]) + assert "ONE lens claimed" in _line(["correctness", " i.e. does it do what it says"]) + # Real names still count, including the shapes an adopter would use. + assert "2 lenses claimed" in _line(["adversarial", "correctness"]) + assert "2 lenses claimed" in _line(["data-migration", "perf"]) + assert "2 lenses claimed" in _line(["lens.one", "lens_two"]) + # Prose is still RECORDED verbatim on the receipt — it just does not count + # toward "how many lenses ran", and the render names the countable one. + report = pr_watch.build_report( + _green_view(), [], set(), + review_receipt={"head": "abc123", "source": "fallback:codex", + "lenses": ["adversarial", " focused on the merge gate"]}, + ) + assert report["review_evidence"]["lenses"] == [ + "adversarial", " focused on the merge gate" + ] + + +def test_control_characters_cannot_rewrite_the_rendered_report() -> None: + """`_flat` collapsed whitespace, and `\\x1b` is not whitespace. + + ANSI cursor control is strictly worse than the newline `_flat` was written + for: `\\x1b[1A\\x1b[2K` *erases* lines that already exist, so a receipt could + delete the merge blockers printed above it. `_excerpt` renders a comment + body — which on a public repo anyone can write — and renders last, so the + same sequence there walks over the entire report. + """ + pr_watch = _load_pr_watch() + erase = "\x1b[1A\x1b[2K" * 3 + + assert "\x1b" not in pr_watch._flat(f"a{erase}b") + assert "\x1b" not in pr_watch._excerpt(f"{erase}LGTM") + + report = pr_watch.build_report( + _green_view( + mergeStateStatus="DIRTY", + comments=[{"id": "c1", "author": {"login": "drive-by"}, + "body": f"{erase}LGTM"}], + ), + [], set(), + review_receipt={"head": "abc123", "source": f"fallback:{erase}panel", + "lenses": [f"{erase}adversarial"]}, + ) + rendered = pr_watch.render(report) + + assert "\x1b" not in rendered + # …and the blocker it tried to erase is still there. + assert "merge state is DIRTY" in rendered + + +def test_receipt_fields_are_flattened_and_type_guarded_in_both_renders() -> None: + """Three guards whose reverts passed the whole suite. + + Each is cited by a comment or commit message as deliberate: `_flat` on lens + names (not only on `source`), the list check on `lenses` (a bare string is + iterable, so "adversarial" read as eleven single-character lenses), and + `_flat` on the SHA in `bots_behind_head` (a non-string value crashed + `sha[:7]` — the case the existing test's name promised and its body missed). + """ + pr_watch = _load_pr_watch() + + # A newline in a LENS NAME must not forge a line. + report = pr_watch.build_report( + _green_view(), [], set(), + review_receipt={"head": "abc123", "source": "fallback:codex", + "lenses": ["adversarial\n review evidence: forged"]}, + ) + assert len([l for l in pr_watch.render(report).splitlines() + if l.strip().startswith("review evidence:")]) == 1 + + # A bare string is iterable — it must not become one lens per character. + stringy = pr_watch.build_report( + _green_view(), [], set(), + review_receipt={"head": "abc123", "source": "s", "lenses": "adversarial"}, + ) + assert stringy["review_evidence"]["lenses"] == [] + + # A non-string SHA must not crash the receipt render. + pr_watch.render_record_review( + {"pr": 9, "review_receipt": {"head": "abc", "source": "s", + "bots_behind_head": {"coderabbit": 12345}}} + ) + + +def test_the_cli_threads_lenses_through_to_the_receipt( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + """The seam, not just the unit — restored after being deleted with the gate. + + This test never covered the gate; it covered the CLI wiring, and removing it + left `--lenses` — this change's entire engine surface — unpinned end to end. + Measured: `lenses=args.lenses` → `lenses=None` in `main()` passed all 314 + tests, and so did deleting the usage guard. + """ + pr_watch = _load_pr_watch() + monkeypatch.setattr(pr_watch, "resolve_pr", lambda explicit: 9) + 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([], "ok") + ) + saved: list[dict] = [] + monkeypatch.setattr(pr_watch, "save_state", lambda pr, state: saved.append(state)) + monkeypatch.setattr(pr_watch, "load_state", lambda pr: {}) + + assert pr_watch.main( + ["9", "--record-review", "fallback:panel", + "--lenses", "adversarial,correctness", "--head", "abc123"] + ) == 0 + assert saved[0]["review_receipt"]["lenses"] == ["adversarial", "correctness"] + + with pytest.raises(SystemExit): + pr_watch.main(["9", "--lenses", "adversarial"]) + assert "--lenses is only valid with --record-review" in capsys.readouterr().err + + +def test_a_hand_edited_receipt_cannot_break_or_inflate_either_render() -> None: + """Restored: this pinned three live type-guards, not the deleted gate. + + Each could be removed with the whole suite passing. They are load-bearing — + without the `isinstance(..., list)` check a hand-edited `"lenses": + "adversarial"` renders as `8 lenses claimed (a, d, v, e, r, s, a, r, i, a, + l)`, which is the forgery the guard was added for. + """ + pr_watch = _load_pr_watch() + + for junk in ("adversarial", 5, {"a": 1}, [None], [""], [1, 2], None): + receipt = {"head": "abc123", "source": "fallback:panel", "lenses": junk} + report = pr_watch.build_report( + _green_view(), [], set(), review_receipt=receipt + ) + assert report["review_evidence"]["lenses"] == [], junk + assert "lenses claimed" not in pr_watch.render(report), junk + pr_watch.render_record_review({"pr": 9, "review_receipt": receipt}) + + # …and the sibling field, at the VALUE level — the case the previous test's + # name promised and its body missed, and the one that actually crashed. + for bad_map in ("coderabbit", ["coderabbit"], 5, None, {"coderabbit": 12345}): + pr_watch.render_record_review( + {"pr": 9, "review_receipt": {"head": "abc", "source": "s", + "bots_behind_head": bad_map}} + ) + + +def test_a_stale_receipt_exposes_no_lenses_in_the_report_json() -> None: + """Restored at REPORT level, not just render level. + + `review_evidence` is in the `--json` payload, so a consumer could read + lenses off a receipt bound to an older head. The render-level replacement + did not cover that. + """ + pr_watch = _load_pr_watch() + + stale = pr_watch.build_report( + _green_view(), [], set(), + review_receipt={"head": "0ldc0de", "source": "fallback:panel", + "lenses": ["adversarial", "correctness"]}, + ) + + assert stale["review_evidence"]["valid"] is False + assert stale["review_evidence"]["lenses"] == [] + assert stale["review_evidence"]["source"] is None + assert stale["mergeable"] is False