From 7000b059e602ceecb6e05f1e827224180ee85f2a Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 23:49:18 +0300 Subject: [PATCH 01/10] Make the fallback review a panel, not a command (#26) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #26. `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 — so the mechanism meant to satisfy `safety-critical-changes.md` rule 2 ("a single-lens verdict is an incomplete review, not a green light") structurally could not. `review.fallback_panel` replaces it as the primary: one isolated, fresh-context reviewer per lens, shipping the two the doctrine names (adversarial, correctness). `fallback_commands` stays as the explicitly DEGRADED mode for a runtime that cannot isolate a reviewer, and must be recorded as one lens rather than as a panel. The split follows Principle #10: *which* lenses is adopter knowledge and lives in config; *how a lens must be run* is kit doctrine and lives in the new `docs/agentic-dev-kit/fallback-review-panel.md`. That contract is the part worth having written down — fresh context, the raw diff with no author framing, and the two things the trials showed matter most: - **execute, don't only read** — a guard turned out to be 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 - **mutation-test new branches** — five properties across the trial session were *named* by a test and pinned by nothing, including one where hardwiring a branch to a constant passed the entire suite The doc also records what the trials showed about stopping: across 13 rounds on three PRs every round found something, so rule 3's "until a full pass finds nothing new" may never terminate. The criterion it gives instead is blast radius — keep going on a gate, stop earlier on something reported but never acted on — and rule 3 now points at it. Engine change is small and is the audit half: `--record-review --lenses adversarial,correctness` records what actually ran, so a degraded one-lens pass no longer produces a receipt byte-identical to a panel. `render_record_review` calls out a single-lens receipt explicitly. Also repointed the surface that mattered most: `pr_followup_hook` fires on every `gh pr create`/`ready`, making it the most-read statement of fallback policy in the kit — and it was advertising the degraded mode. It now names the panel and its configured lenses, and falls back to the single command only when no panel is configured. Tests 300 → 306. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --- .agents/skills/pr-watch/SKILL.md | 2 +- config/dev-model.yaml | 23 ++++ docs/CLAUDE-sections.md | 3 +- docs/agentic-dev-kit/fallback-review-panel.md | 117 ++++++++++++++++++ .../safety-critical-changes.md | 9 +- docs/agentic-dev-kit/workflows/pr-watch.md | 39 ++++-- docs/autonomous-session-playbook.md | 14 ++- init.sh | 14 +++ kit-manifest.json | 6 +- scripts/hooks/pr_followup_hook.py | 52 ++++++-- scripts/pr_watch.py | 31 +++++ scripts/tests/test_pr_followup_hook.py | 46 ++++++- scripts/tests/test_pr_watch.py | 96 ++++++++++++++ 13 files changed, 415 insertions(+), 37 deletions(-) create mode 100644 docs/agentic-dev-kit/fallback-review-panel.md diff --git a/.agents/skills/pr-watch/SKILL.md b/.agents/skills/pr-watch/SKILL.md index d480376..7fe3981 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 `fallback:panel`. 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/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..f8cad21 100644 --- a/docs/CLAUDE-sections.md +++ b/docs/CLAUDE-sections.md @@ -65,7 +65,8 @@ 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. 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..eaf5312 --- /dev/null +++ b/docs/agentic-dev-kit/fallback-review-panel.md @@ -0,0 +1,117 @@ +# 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`. The kit ships two, because they +are the two the doctrine names: + +- **adversarial** — assume the change is wrong and try to prove it. Bypasses, + fail-open paths, wedges, inputs the author did not consider, and *whether the + new guard actually guards*. +- **correctness** — assume the change works and ask what it *says*. Stale + comments and docstrings, claims that overstate what is verified, tests whose + names promise more than their bodies check, drift between the diff and its PR + body. + +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. +6. **Report, do not fix.** A lens that edits loses the disjointness: it starts + defending its own changes on the next round. +7. **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: + + ``` + uv run /pr_watch.py \ + --record-review "fallback:panel" --lenses adversarial,correctness \ + --head + ``` + +## 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 when you record the receipt. "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 `fallback:panel` +- 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..bdbb2f8 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: + + ``` + uv run /pr_watch.py \ + --record-review "fallback:panel" --lenses adversarial,correctness \ + --head + ``` + + `--lenses` names what actually ran, so a degraded one-lens pass is + distinguishable from a panel in the audit trail. If your runtime cannot + isolate a reviewer, run `review.fallback_commands` instead and record it as + `fallback:` with the single lens named — never as `fallback:panel`. + 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. @@ -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 diff --git a/docs/autonomous-session-playbook.md b/docs/autonomous-session-playbook.md index 82c8943..824fb5d 100644 --- a/docs/autonomous-session-playbook.md +++ b/docs/autonomous-session-playbook.md @@ -70,12 +70,14 @@ 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 "fallback:panel" --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..d20d20b 100755 --- a/init.sh +++ b/init.sh @@ -374,6 +374,20 @@ 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` below is + # the DEGRADED one-lens mode for a runtime that cannot isolate a reviewer. + # 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" + - name: correctness + focus: "assume it works and ask what it says"' || 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. diff --git a/kit-manifest.json b/kit-manifest.json index fea5b7f..1ca5646 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -15,7 +15,7 @@ "files": { "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 +23,7 @@ }, "docs/agentic-dev-kit/workflows/pr-watch.md": { "role": "workflow", - "sha256": "7f116a78706776feb68e9065f434eedd63a1eec420f3b8314899b8e4e58dfbfe" + "sha256": "8262dbeb481cb757461c1faf5b695d64eecd84f210438cec131e3eec32d6b1dc" }, "docs/agentic-dev-kit/workflows/session-start.md": { "role": "workflow", @@ -99,7 +99,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "b04cc9a678a28c9d292b8f4d98062fd24f82e1a5bbdc5ac380dbca7e9394639b" + "sha256": "7999559dcff41c37b82176cefdcd3175a903252c964ef6ea797ef021bf3f7278" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/hooks/pr_followup_hook.py b/scripts/hooks/pr_followup_hook.py index 28876ee..85611b2 100755 --- a/scripts/hooks/pr_followup_hook.py +++ b/scripts/hooks/pr_followup_hook.py @@ -41,10 +41,12 @@ _DEFAULT_FALLBACK_COMMAND = "/code-review" _DEFAULT_ENGINES_DIR = "scripts" +_DEFAULT_LENSES: list[str] = [] -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]]: + """Read ``(review.bots, review.fallback_commands.claude, paths.engines, + review.fallback_panel lens names)``. Best-effort: any failure (missing config, kitconfig unimportable, malformed values) falls back to generic defaults rather than raising — this hook must @@ -64,9 +66,19 @@ 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"] + for lens in panel + if isinstance(lens, dict) and isinstance(lens.get("name"), str) + ] + if isinstance(panel, list) + else [] + ) + return bots, fallback, engines, lenses except Exception: - return [], _DEFAULT_FALLBACK_COMMAND, _DEFAULT_ENGINES_DIR + return [], _DEFAULT_FALLBACK_COMMAND, _DEFAULT_ENGINES_DIR, list(_DEFAULT_LENSES) def _bot_description(bots: list[str]) -> str: @@ -78,8 +90,32 @@ def _bot_description(bots: list[str]) -> str: return " / ".join(names) +def _fallback_instruction(fallback_command: str, lenses: list[str]) -> 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. + """ + if lenses: + 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 " + "`--record-review \"fallback:panel\" --lenses `. 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 = _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 +126,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) + + " Only stop early if you hit something that genuinely needs an " "operator decision." ) diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index d9d7aec..b496b28 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -1371,6 +1371,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 +1401,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 +1476,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 @@ -1794,6 +1804,14 @@ 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" ) + named = receipt.get("lenses") or [] + if len(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)}") for bot, sha in (receipt.get("bots_behind_head") or {}).items(): lines.append( f" ⚠ {bot}'s last review was of {sha[:7]}, not this head — this receipt " @@ -1880,6 +1898,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 +1948,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 +1975,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_pr_followup_hook.py b/scripts/tests/test_pr_followup_hook.py index d7d465f..e7a017c 100644 --- a/scripts/tests/test_pr_followup_hook.py +++ b/scripts/tests/test_pr_followup_hook.py @@ -105,17 +105,51 @@ def test_exits_zero_on_malformed_or_unexpected_stdin(monkeypatch, capsys, stdin_ def test_reminder_names_configured_bots_and_fallback_not_a_hardcoded_bot(monkeypatch): - """The reminder must be sourced from config (review.bots / fallback_commands), + """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_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 +161,11 @@ 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 = 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" not in hook._fallback_instruction(fallback, lenses) diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index 8b584bd..eb2db9d 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 @@ -2092,3 +2093,98 @@ 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: {}) + report = pr_watch.record_review(9, "fallback:panel", "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, 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_omitting_lenses_records_nothing_rather_than_guessing( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """An older caller, or a receipt from a human reviewer, has no lens list. + + Absent must mean "not stated" — inventing one would make an unknown look + like a verified panel, which is the failure direction this whole family of + fields exists to close. + """ + pr_watch = _load_pr_watch() + + for value in (None, "", " ", ",", " , "): + receipt, report = _record(monkeypatch, pr_watch, lenses=value) + assert "lenses" not in receipt, value + assert "one lens only" not in pr_watch.render_record_review(report), value + + +def test_the_shipped_config_ships_two_disjoint_lenses() -> None: + """The doctrine's floor is TWO lenses, and it must survive being config. + + A kit that ships a one-lens panel would violate rule 2 out of the box while + appearing to satisfy it. + """ + pr_watch = _load_pr_watch() + sys.path.insert(0, str(ENGINE_DIR / "lib")) + import kitconfig # noqa: PLC0415 + + config = kitconfig.load_config(ENGINE_DIR.parent / "config" / "dev-model.yaml") + panel = kitconfig.get(config, "review.fallback_panel") + + assert len({lens["name"] for lens in panel["lenses"]}) >= 2 + assert all(lens.get("focus") for lens in panel["lenses"]) + # The panel's receipt source must differ from a single-lens one, or the + # audit trail cannot tell them apart. + assert panel["receipt_source"] == "fallback:panel" + assert panel["receipt_source"] not in set( + kitconfig.get(config, "review.fallback_commands", {}).values() + ) + assert pr_watch is not None From 27f9f964a86b0b74fa37675556e07e9032f47589 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sat, 25 Jul 2026 23:58:51 +0300 Subject: [PATCH 02/10] Address CodeRabbit: make the panel's claims enforceable, not documented MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eight findings, four Major, all valid. Two are the classes this session keeps producing. **I added `review.fallback_panel.receipt_source` and then ignored it.** The hook and both workflow docs prescribed the literal `fallback:panel`, so an adopter who renamed it would be handed a command writing a differently-labelled receipt than their own config declares — a config key that exists and does nothing (Principle #10). Now loaded, validated, and interpolated everywhere. **A panel receipt asserted two lenses on trust.** `--lenses adversarial,adversarial` rendered as multi-lens coverage, and omitting `--lenses` still produced a receipt labelled `fallback:panel`. That is prose where a deterministic gate belongs — the exact thing `safety-critical-changes.md` rule 1 says to stop doing. `--record-review` now refuses a panel-sourced receipt naming fewer than two *distinct* (case-folded) lenses, and the check follows the configured source rather than a literal. The escape is not a flag: it is recording what actually ran under a single-lens source. That gate immediately rejected six of my own existing tests, which had been using `fallback:panel` as a convenient string for scenarios that were really single-lens. They now say what they mean. **The clean bar still allowed the degraded path.** `CLAUDE-sections.md` required a panel two lines above, then said "bot *or* the fallback command" satisfies the clean bar — which let an agent treat one lens in its own context as sufficient while isolated reviewers were available. Also: blank lens names are discarded rather than advertised (a panel with an unnameable lens is worse than no panel); the stopping rationale now points at the PR, since receipts carry what a review did *not* cover rather than prose; docs use `` and placeholder lens names instead of this repo's defaults; and two fences got languages. Tests 306 → 310. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --- docs/agentic-dev-kit/fallback-review-panel.md | 19 +++-- docs/agentic-dev-kit/workflows/pr-watch.md | 16 ++--- docs/autonomous-session-playbook.md | 3 +- kit-manifest.json | 4 +- scripts/hooks/pr_followup_hook.py | 40 ++++++++--- scripts/pr_watch.py | 34 +++++++++ scripts/tests/test_pr_followup_hook.py | 47 +++++++++++- scripts/tests/test_pr_watch.py | 72 ++++++++++++++++--- 8 files changed, 199 insertions(+), 36 deletions(-) diff --git a/docs/agentic-dev-kit/fallback-review-panel.md b/docs/agentic-dev-kit/fallback-review-panel.md index eaf5312..04aa780 100644 --- a/docs/agentic-dev-kit/fallback-review-panel.md +++ b/docs/agentic-dev-kit/fallback-review-panel.md @@ -78,12 +78,19 @@ author re-reading their own diff. 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 "fallback:panel" --lenses adversarial,correctness \ + --record-review "" \ + --lenses \ --head ``` + `--record-review` **refuses** a panel-sourced receipt naming fewer than two + distinct lenses. That is deliberate: the claim "a panel reviewed this" is + the one thing a receipt should not be able to assert on trust. The way past + it is to run the second lens, or to record what actually ran under a + single-lens source. + ## Re-running, and when to stop `safety-critical-changes.md` rule 3 says to re-review after every fix round @@ -100,7 +107,9 @@ So the stopping criterion is **blast radius, not round count**: field) — a couple of rounds with the findings decaying in severity is proportionate. Worst case is a wrong message. -Say which one you applied when you record the receipt. "The last round found +Say which one you applied **in the PR**, where a human reads it — the receipt +carries what the review *did not* cover (lenses, `override`, `bot_signal`, +`bots_behind_head`), not a prose rationale for stopping. "The last round found nothing" is not available as a reason if it never happened. ## Degraded mode @@ -109,7 +118,9 @@ 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 `fallback:panel` +- record it as `fallback:`, never the panel's `receipt_source` — the + engine enforces this, so recording it honestly is also the only thing that + works - 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 diff --git a/docs/agentic-dev-kit/workflows/pr-watch.md b/docs/agentic-dev-kit/workflows/pr-watch.md index bdbb2f8..791c77a 100644 --- a/docs/agentic-dev-kit/workflows/pr-watch.md +++ b/docs/agentic-dev-kit/workflows/pr-watch.md @@ -76,18 +76,18 @@ Repeat until the report says **converged**: 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 "fallback:panel" --lenses adversarial,correctness \ - --head + --record-review "" \ + --lenses --head ``` `--lenses` names what actually ran, so a degraded one-lens pass is - distinguishable from a panel in the audit trail. If your runtime cannot - isolate a reviewer, run `review.fallback_commands` instead and record it as - `fallback:` with the single lens named — never as `fallback:panel`. - For a lane, use `/dev_session.sh pr-watch ` with the same - flags. + distinguishable from a panel in the audit trail — and a panel-sourced + receipt naming fewer than two distinct lenses is **refused**. 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. diff --git a/docs/autonomous-session-playbook.md b/docs/autonomous-session-playbook.md index 824fb5d..8c399d4 100644 --- a/docs/autonomous-session-playbook.md +++ b/docs/autonomous-session-playbook.md @@ -73,7 +73,8 @@ Follow this top to bottom, per ticket. 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 "fallback:panel" --lenses --head ` (use the `head` + --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 diff --git a/kit-manifest.json b/kit-manifest.json index 1ca5646..ecb21ff 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -23,7 +23,7 @@ }, "docs/agentic-dev-kit/workflows/pr-watch.md": { "role": "workflow", - "sha256": "8262dbeb481cb757461c1faf5b695d64eecd84f210438cec131e3eec32d6b1dc" + "sha256": "d1e047a6d8881116cc5615ad8a41865d3e2645dd53de1b4922164e9a6885c21f" }, "docs/agentic-dev-kit/workflows/session-start.md": { "role": "workflow", @@ -99,7 +99,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "7999559dcff41c37b82176cefdcd3175a903252c964ef6ea797ef021bf3f7278" + "sha256": "c472f3768bc453f158d5a226b3e0fcc6add1beea9eb9ea188147a3961a8aafea" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/hooks/pr_followup_hook.py b/scripts/hooks/pr_followup_hook.py index 85611b2..ceba8ad 100755 --- a/scripts/hooks/pr_followup_hook.py +++ b/scripts/hooks/pr_followup_hook.py @@ -42,11 +42,12 @@ _DEFAULT_FALLBACK_COMMAND = "/code-review" _DEFAULT_ENGINES_DIR = "scripts" _DEFAULT_LENSES: list[str] = [] +_DEFAULT_PANEL_RECEIPT_SOURCE = "fallback:panel" -def _load_review_config() -> tuple[list[str], str, str, list[str]]: +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 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 @@ -69,16 +70,31 @@ def _load_review_config() -> tuple[list[str], str, str, list[str]]: panel = kitconfig.get(config, "review.fallback_panel.lenses", []) lenses = ( [ - lens["name"] + lens["name"].strip() for lens in panel - if isinstance(lens, dict) and isinstance(lens.get("name"), str) + 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 [] ) - return bots, fallback, engines, lenses + 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, list(_DEFAULT_LENSES) + return ( + [], + _DEFAULT_FALLBACK_COMMAND, + _DEFAULT_ENGINES_DIR, + list(_DEFAULT_LENSES), + _DEFAULT_PANEL_RECEIPT_SOURCE, + ) def _bot_description(bots: list[str]) -> str: @@ -90,7 +106,11 @@ def _bot_description(bots: list[str]) -> str: return " / ".join(names) -def _fallback_instruction(fallback_command: str, lenses: list[str]) -> str: +def _fallback_instruction( + fallback_command: str, + lenses: list[str], + panel_source: str = _DEFAULT_PANEL_RECEIPT_SOURCE, +) -> str: """What to run when a bot is unavailable. Names the PANEL when one is configured, because a single command in this @@ -105,7 +125,7 @@ def _fallback_instruction(fallback_command: str, lenses: list[str]) -> str: "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 " - "`--record-review \"fallback:panel\" --lenses `. Never treat the " + f'`--record-review "{panel_source}" --lenses `. Never treat the ' "outage as a review waiver." ) return ( @@ -115,7 +135,7 @@ def _fallback_instruction(fallback_command: str, lenses: list[str]) -> str: def build_reminder() -> str: - bots, fallback_command, engines_dir, lenses = _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 " @@ -127,7 +147,7 @@ def build_reminder() -> str: "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). " - + _fallback_instruction(fallback_command, lenses) + + _fallback_instruction(fallback_command, lenses, panel_source) + " Only stop early if you hit something that genuinely needs an " "operator decision." ) diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index b496b28..b58565d 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -255,6 +255,13 @@ 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 +# The receipt source a full panel records under. Adopter-configurable +# (`review.fallback_panel.receipt_source`) because the whole point of the key is +# that a panel receipt is DISTINGUISHABLE from a single-lens one — an adopter who +# renames it must still get that property, and the validation below must follow +# their name rather than a literal. +_DEFAULT_PANEL_RECEIPT_SOURCE = "fallback:panel" + # 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 @@ -281,6 +288,7 @@ class ReviewConfig(NamedTuple): require_ci: bool bots: tuple[str, ...] bot_pending_grace_minutes: float + panel_receipt_source: str def _load_review_config(config_path: str | Path | None = None) -> ReviewConfig: @@ -326,6 +334,7 @@ def _load_review_config(config_path: str | Path | None = None) -> ReviewConfig: require_ci=_DEFAULT_REQUIRE_CI, bots=_DEFAULT_REVIEW_BOTS, bot_pending_grace_minutes=_DEFAULT_BOT_PENDING_GRACE_MINUTES, + panel_receipt_source=_DEFAULT_PANEL_RECEIPT_SOURCE, ) try: from kitconfig import get, get_str_list, load_config @@ -353,6 +362,13 @@ def _load_review_config(config_path: str | Path | None = None) -> ReviewConfig: ) if isinstance(grace, bool) or not isinstance(grace, (int, float)) or grace < 0: grace = _DEFAULT_BOT_PENDING_GRACE_MINUTES + panel_source = 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 except FileNotFoundError: # `load_config` raises this for an absent config file — a standalone # engine run. Defaults are exactly right; stay quiet. @@ -377,6 +393,7 @@ def _load_review_config(config_path: str | Path | None = None) -> ReviewConfig: require_ci=require_ci, bots=tuple(bot.strip().lower() for bot in bots if bot.strip()), bot_pending_grace_minutes=float(grace), + panel_receipt_source=panel_source.strip(), ) @@ -387,6 +404,7 @@ def _load_review_config(config_path: str | Path | None = None) -> ReviewConfig: _REQUIRE_CI = _REVIEW_CONFIG.require_ci _REVIEW_BOTS = _REVIEW_CONFIG.bots _BOT_PENDING_GRACE_MINUTES = _REVIEW_CONFIG.bot_pending_grace_minutes +_PANEL_RECEIPT_SOURCE = _REVIEW_CONFIG.panel_receipt_source # --------------------------------------------------------------------------- gh @@ -1477,6 +1495,22 @@ def record_review( "recorded_at": now.isoformat(), } named_lenses = [part.strip() for part in (lenses or "").split(",") if part.strip()] + # A panel receipt asserts two INDEPENDENT lenses. Left as prose, that claim + # was checkable by nobody: `--lenses adversarial,adversarial` rendered as + # multi-lens coverage, and omitting `--lenses` entirely still produced a + # receipt labelled `fallback:panel`. Principle: a deterministic gate beats a + # documented expectation. The escape is not a flag — it is recording the + # truth, under a single-lens source. + if source == _PANEL_RECEIPT_SOURCE: + distinct = {lens.casefold() for lens in named_lenses} + if len(distinct) < 2: + raise ValueError( + f"a {source!r} receipt asserts two independent lenses, but " + f"--lenses named {len(distinct)} distinct ({', '.join(named_lenses) or 'none'}). " + "Run the second lens, or record what actually ran under a " + "single-lens source (e.g. fallback:codex) — see " + "docs/agentic-dev-kit/fallback-review-panel.md" + ) if named_lenses: receipt["lenses"] = named_lenses if allow_pending_bot: diff --git a/scripts/tests/test_pr_followup_hook.py b/scripts/tests/test_pr_followup_hook.py index e7a017c..f20394d 100644 --- a/scripts/tests/test_pr_followup_hook.py +++ b/scripts/tests/test_pr_followup_hook.py @@ -135,6 +135,48 @@ def test_reminder_points_at_the_panel_not_the_degraded_one_lens_mode(monkeypatch assert "/code-review" not in reminder +def test_the_reminder_uses_the_configured_receipt_source_not_a_literal(): + """`review.fallback_panel.receipt_source` exists so an adopter can rename it. + + Prescribing the literal in the instruction would hand them a command that + writes a differently-labelled receipt than their own config declares — + adding a config key and then ignoring it (Principle #10). + """ + hook = _load_hook() + + renamed = hook._fallback_instruction( + "/x", ["adversarial", "correctness"], "fallback:my-panel" + ) + + assert '"fallback:my-panel"' in renamed + assert "fallback:panel" not in renamed + + +def test_blank_lens_names_are_discarded_rather_than_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"] + + 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 @@ -161,11 +203,12 @@ def _boom(*_args, **_kwargs): raise FileNotFoundError("no config here") monkeypatch.setattr(kitconfig, "load_config", _boom) - bots, fallback, engines, lenses = 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" not in hook._fallback_instruction(fallback, lenses) + assert panel_source == "fallback:panel" + assert "PANEL" not in hook._fallback_instruction(fallback, lenses, panel_source) diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index eb2db9d..f769b1f 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -1249,9 +1249,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"] @@ -1314,7 +1314,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) @@ -1335,7 +1335,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" @@ -1545,6 +1545,7 @@ def test_missing_config_falls_back_to_defaults_silently( True, pr_watch._DEFAULT_REVIEW_BOTS, pr_watch._DEFAULT_BOT_PENDING_GRACE_MINUTES, + pr_watch._DEFAULT_PANEL_RECEIPT_SOURCE, ) assert capsys.readouterr().err == "" @@ -1860,7 +1861,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) @@ -1892,7 +1893,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"] @@ -2110,7 +2111,8 @@ def _record(monkeypatch, pr_watch, **kwargs) -> tuple[dict, dict]: 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, **kwargs) + source = kwargs.pop("source", "fallback:panel") + report = pr_watch.record_review(9, source, "abc123", now=NOW, **kwargs) return recorded[0]["review_receipt"], report @@ -2126,7 +2128,9 @@ def test_a_single_lens_receipt_does_not_read_like_a_panel( """ pr_watch = _load_pr_watch() - receipt, report = _record(monkeypatch, pr_watch, lenses="correctness") + receipt, report = _record( + monkeypatch, pr_watch, source="fallback:codex", lenses="correctness" + ) assert receipt["lenses"] == ["correctness"] rendered = pr_watch.render_record_review(report) @@ -2161,7 +2165,9 @@ def test_omitting_lenses_records_nothing_rather_than_guessing( pr_watch = _load_pr_watch() for value in (None, "", " ", ",", " , "): - receipt, report = _record(monkeypatch, pr_watch, lenses=value) + receipt, report = _record( + monkeypatch, pr_watch, source="fallback:codex", lenses=value + ) assert "lenses" not in receipt, value assert "one lens only" not in pr_watch.render_record_review(report), value @@ -2188,3 +2194,51 @@ def test_the_shipped_config_ships_two_disjoint_lenses() -> None: kitconfig.get(config, "review.fallback_commands", {}).values() ) assert pr_watch is not None + + +def test_a_panel_receipt_is_refused_without_two_distinct_lenses( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The claim "a panel reviewed this" is the one a receipt must not assert on + trust. + + Left as prose, `--lenses adversarial,adversarial` rendered as multi-lens + coverage and omitting `--lenses` entirely still produced a receipt labelled + `fallback:panel`. A deterministic gate beats a documented expectation — and + the escape is not a flag, it is recording the truth under a single-lens + source. + """ + pr_watch = _load_pr_watch() + + for bad in (None, "", "adversarial", "adversarial,adversarial", "a, A , a"): + with pytest.raises(ValueError, match="two independent lenses"): + _record(monkeypatch, pr_watch, lenses=bad) + + # Two genuinely distinct lenses go through. + receipt, _ = _record(monkeypatch, pr_watch, lenses="adversarial,correctness") + assert receipt["lenses"] == ["adversarial", "correctness"] + + # …and the honest single-lens record is always available. + single, _ = _record( + monkeypatch, pr_watch, source="fallback:codex", lenses="correctness" + ) + assert single["source"] == "fallback:codex" + + +def test_the_panel_gate_follows_a_renamed_receipt_source( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """`receipt_source` is adopter-configurable, so the validation has to follow + their name — checking a literal would leave a renamed panel unguarded while + rejecting a source they never use.""" + pr_watch = _load_pr_watch() + monkeypatch.setattr(pr_watch, "_PANEL_RECEIPT_SOURCE", "fallback:my-panel") + + with pytest.raises(ValueError, match="two independent lenses"): + _record(monkeypatch, pr_watch, source="fallback:my-panel", lenses="one") + + # The old literal is now just an ordinary single-lens source. + receipt, _ = _record( + monkeypatch, pr_watch, source="fallback:panel", lenses="one" + ) + assert receipt["lenses"] == ["one"] From 44792dfc180bd92eed78cd78fde367fe32248fdc Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sun, 26 Jul 2026 00:34:44 +0300 Subject: [PATCH 03/10] The panel gate did not gate: show coverage at merge time instead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both lenses ran on this PR (the bot was rate-limited, so the panel reviewed itself). The adversarial lens found the thing that matters, and it invalidates how the previous commit described its own change. **`decide_mergeable` never looks at `source` or `lenses`.** So the one-lens ⚠ 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. The previous commit's message said "make the panel's claims enforceable". What it made enforceable was the LABEL, not the merge. **And the incentive gradient ran backwards.** `source` is free text an agent picks, so equality against it is a matcher over attacker-chosen input — exactly what `safety-critical-changes.md` rule 1 says to stop building. Measured: ACCEPTED 'fallback:panel (adversarial + correctness)' no lenses, NO warning ACCEPTED 'fallback: panel' no lenses, NO warning ACCEPTED 'fallback-panel' no lenses, NO warning while the honest escape — `fallback:codex --lenses correctness` — produced the ⚠. The quietest receipt was the dishonest one. The fix is not a tighter matcher. `review_evidence` now carries the recorded lenses, and the POLL render states them every time the PR is polled: review evidence: fallback:panel — 2 lenses (adversarial, correctness) review evidence: fallback:codex — ⚠ ONE lens (correctness) — not a dual-lens pass review evidence: fallback:panel (…) — ⚠ lenses not stated A relabelled receipt now reads as unstated coverage instead of buying silence, and the source check is documented as what it actually is: a catch for the accidental mislabel, not proof a panel ran. Also from this round: - **A string `lenses` was read as one lens per character.** `"adversarial"` is iterable, so a hand-edited state file rendered as eleven lenses' worth of coverage. Both render paths are now list-guarded, like the sibling receipt fields already were. - Stale receipts report no coverage — evidence bound to an older head is not evidence for this one. - The two `receipt_source` validation branches were unpinned, and both fail toward switching the gate OFF (an unmatchable or unstripped source silently accepts a lens-free panel receipt). Tests 313 → 317. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --- docs/CLAUDE-sections.md | 5 +- docs/agentic-dev-kit/fallback-review-panel.md | 27 ++- init.sh | 11 +- kit-manifest.json | 8 +- scripts/hooks/pr_followup_hook.py | 13 +- scripts/kit_doctor.py | 5 + scripts/pr_watch.py | 63 ++++++- scripts/tests/test_portability.py | 11 ++ scripts/tests/test_pr_followup_hook.py | 63 ++++++- scripts/tests/test_pr_watch.py | 175 +++++++++++++++++- 10 files changed, 344 insertions(+), 37 deletions(-) diff --git a/docs/CLAUDE-sections.md b/docs/CLAUDE-sections.md index f8cad21..dd5191a 100644 --- a/docs/CLAUDE-sections.md +++ b/docs/CLAUDE-sections.md @@ -69,7 +69,10 @@ the PR — run the `review.fallback_panel` pass (one isolated, fresh-context rev 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 index 04aa780..fd4a862 100644 --- a/docs/agentic-dev-kit/fallback-review-panel.md +++ b/docs/agentic-dev-kit/fallback-review-panel.md @@ -85,11 +85,21 @@ author re-reading their own diff. --head ``` - `--record-review` **refuses** a panel-sourced receipt naming fewer than two - distinct lenses. That is deliberate: the claim "a panel reviewed this" is - the one thing a receipt should not be able to assert on trust. The way past - it is to run the second lens, or to record what actually ran under a - single-lens source. + `--record-review` **refuses** a receipt whose source is exactly the + configured `receipt_source` unless `--lenses` names two distinct lenses. Know + what that is and is not: `source` is free text, so the check catches an + accidental mislabel, not a determined one — `fallback:panel (2 lenses)` is + just another string. + + The defence that does not depend on the label is the **poll render**, which + states the recorded lens count every time the PR is polled: + + ```text + review evidence: fallback:codex — ⚠ ONE lens (correctness) — not a dual-lens pass + ``` + + So a one-lens pass reads as one lens at merge time whatever it was called, + and `--lenses` is what determines that — not the source string. ## Re-running, and when to stop @@ -118,9 +128,10 @@ 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 enforces this, so recording it honestly is also the only thing that - works +- record it as `fallback:`, never the panel's `receipt_source`. The + engine refuses a panel-sourced receipt naming fewer than two distinct lenses, + so the shortest path through it is to record what actually ran — but it cannot + tell whether a panel really ran, only what you named. The honesty is yours - 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 diff --git a/init.sh b/init.sh index d20d20b..9036277 100755 --- a/init.sh +++ b/init.sh @@ -376,17 +376,18 @@ migrate_kit_schema() { 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` below is - # the DEGRADED one-lens mode for a runtime that cannot isolate a reviewer. + # 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" + 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"' || true + 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 @@ -747,7 +748,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 ecb21ff..6e9784e 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -13,6 +13,10 @@ ".gitignore" ], "files": { + "docs/agentic-dev-kit/fallback-review-panel.md": { + "role": "doctrine", + "sha256": "02d9dd45c06a641f8822dc50c52f513e3f3860c3a144841d50f95a6ef93c1cf3" + }, "docs/agentic-dev-kit/safety-critical-changes.md": { "role": "doctrine", "sha256": "b889d9dddb9c3b5b7441162406094530532288a996af232120ca2b22c822d312" @@ -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": "c472f3768bc453f158d5a226b3e0fcc6add1beea9eb9ea188147a3961a8aafea" + "sha256": "e90ca1a3eb03673d810f14046b9cce52c5d41c7c8bdf68ccbaf874d941cb5036" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/hooks/pr_followup_hook.py b/scripts/hooks/pr_followup_hook.py index ceba8ad..f1b4367 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, with none 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 @@ -120,13 +121,17 @@ def _fallback_instruction( the fallback policy in the kit; pointing it at the degraded mode taught the wrong habit every time. """ - if lenses: + # Two DISTINCT lenses is the panel's floor (see fallback-review-panel.md). + # A one-lens `fallback_panel` would otherwise have this hook advertise a + # command that `record_review` refuses every single time — so a sub-floor + # config degrades to the single-command wording instead. + 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'`--record-review "{panel_source}" --lenses `. Never treat the ' - "outage as a review waiver." + 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 " 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 b58565d..d7667ff 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -1495,13 +1495,19 @@ def record_review( "recorded_at": now.isoformat(), } named_lenses = [part.strip() for part in (lenses or "").split(",") if part.strip()] - # A panel receipt asserts two INDEPENDENT lenses. Left as prose, that claim - # was checkable by nobody: `--lenses adversarial,adversarial` rendered as - # multi-lens coverage, and omitting `--lenses` entirely still produced a - # receipt labelled `fallback:panel`. Principle: a deterministic gate beats a - # documented expectation. The escape is not a flag — it is recording the - # truth, under a single-lens source. - if source == _PANEL_RECEIPT_SOURCE: + # A cheap guard against the ACCIDENTAL mislabel, and no more than that. + # `source` is free text an agent picks, so this can only catch a receipt + # that names the configured panel source exactly; `fallback:panel (2 lenses)` + # sails past it. That is why the real defence is not here but in the poll + # render, which states the recorded lens count at merge time whatever the + # receipt is called — a deterministic artifact rather than a matcher over a + # label (`safety-critical-changes.md` rule 1). + # Both sides case-folded. Folding the lens names to defeat + # `adversarial,Adversarial` while comparing the SOURCE case-sensitively left + # `Fallback:Panel` bypassing the gate entirely — and producing a receipt that + # reads as a panel with no single-lens warning at all, i.e. worse than before + # this check existed. + if source.casefold() == _PANEL_RECEIPT_SOURCE.casefold(): distinct = {lens.casefold() for lens in named_lenses} if len(distinct) < 2: raise ValueError( @@ -1658,6 +1664,24 @@ 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 [] + ), } merge_blockers: list[str] = [] if pr_state != "OPEN": @@ -1806,6 +1830,21 @@ def render(report: dict) -> str: f"{entry['age_minutes']}m (past the {grace:g}m grace) — " "treated as not coming; run the configured fallback review" ) + # 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"): + named = evidence.get("lenses") or [] + if len(named) >= 2: + detail = f"{len(named)} lenses ({', '.join(named)})" + elif named: + detail = f"⚠ ONE lens ({named[0]}) — not a dual-lens pass" + else: + detail = "⚠ lenses not stated" + lines.append(f" review evidence: {evidence.get('source')} — {detail}") for blocker in report.get("merge_blockers") or []: lines.append(f" ✗ merge blocker: {blocker}") if report["new_comments"]: @@ -1838,7 +1877,15 @@ 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" ) - named = receipt.get("lenses") or [] + # 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 [] + ) if len(named) == 1: lines.append( f" ⚠ one lens only ({named[0]}) — `safety-critical-changes.md` rule 2 " 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 f20394d..72dc4b8 100644 --- a/scripts/tests/test_pr_followup_hook.py +++ b/scripts/tests/test_pr_followup_hook.py @@ -135,21 +135,68 @@ def test_reminder_points_at_the_panel_not_the_degraded_one_lens_mode(monkeypatch assert "/code-review" not in reminder -def test_the_reminder_uses_the_configured_receipt_source_not_a_literal(): +def test_the_reminder_reads_the_receipt_source_from_config(monkeypatch): """`review.fallback_panel.receipt_source` exists so an adopter can rename it. - Prescribing the literal in the instruction would hand them a command that - writes a differently-labelled receipt than their own config declares — - adding a config key and then ignoring it (Principle #10). + 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 - renamed = hook._fallback_instruction( - "/x", ["adversarial", "correctness"], "fallback:my-panel" + monkeypatch.setattr( + kitconfig, + "load_config", + lambda *a, **k: { + "review": { + "bots": ["somebot"], + "fallback_panel": { + "receipt_source": "fallback:my-panel", + "lenses": [{"name": "adversarial"}, {"name": "correctness"}], + }, + } + }, ) - assert '"fallback:my-panel"' in renamed - assert "fallback:panel" not in renamed + 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_rather_than_advertised(monkeypatch): diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index f769b1f..2fdf481 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -361,6 +361,7 @@ def test_review_receipt_must_match_current_head() -> None: assert stale["mergeable"] is False assert current["mergeable"] is True assert current["review_evidence"] == { + "lenses": [], "valid": True, "source": "fallback:codex", "head": "abc123", @@ -2193,7 +2194,74 @@ def test_the_shipped_config_ships_two_disjoint_lenses() -> None: assert panel["receipt_source"] not in set( kitconfig.get(config, "review.fallback_commands", {}).values() ) - assert pr_watch is not None + # "Disjoint" means the FOCUS differs, not just the label — two lenses with + # the same brief are one lens run twice. + focuses = [lens["focus"] for lens in panel["lenses"]] + assert len(set(focuses)) == len(focuses) + # …and the engine must have read this file, not its own default. + assert pr_watch._PANEL_RECEIPT_SOURCE == panel["receipt_source"] + + +def test_the_panel_receipt_source_is_read_from_config_not_the_engine_default( + tmp_path: Path, +) -> None: + """A mutation making `_load_review_config` ignore the key entirely survived + the suite — a config key added and then not read (Principle #10).""" + pr_watch = _load_pr_watch() + + resolved = pr_watch._load_review_config( + _write_config( + tmp_path, + 'review:\n fallback_panel:\n receipt_source: "fallback:my-panel"\n', + ) + ) + + assert resolved.panel_receipt_source == "fallback:my-panel" + + # Blank / wrong-typed values keep the default rather than disabling the gate + # by making the source unmatchable. + for bad in ('""', '" "', "[]", "true"): + degraded = pr_watch._load_review_config( + _write_config( + tmp_path / bad.strip('"[]'), + f"review:\n fallback_panel:\n receipt_source: {bad}\n", + ) + ) + assert degraded.panel_receipt_source == pr_watch._DEFAULT_PANEL_RECEIPT_SOURCE, bad + + +def test_the_cli_threads_lenses_through_to_the_receipt( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + """The seam, not just the unit. + + Dropping `lenses=args.lenses` in `main()` passed every test — the flag + parsed, the function accepted it, and nothing checked they were connected. + Also pins that `--lenses` without `--record-review` is a usage error. + """ + 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: {}) + + exit_code = pr_watch.main( + ["9", "--record-review", "fallback:panel", + "--lenses", "adversarial,correctness", "--head", "abc123"] + ) + + assert exit_code == 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_panel_receipt_is_refused_without_two_distinct_lenses( @@ -2242,3 +2310,108 @@ def test_the_panel_gate_follows_a_renamed_receipt_source( monkeypatch, pr_watch, source="fallback:panel", lenses="one" ) assert receipt["lenses"] == ["one"] + + +def test_the_poll_render_states_what_the_receipt_covers() -> None: + """The one-lens warning used to print 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. + + `source` is free text an agent picks, so no matcher over it can be + trustworthy: `fallback:panel (adversarial + correctness)` sails past an + equality check. The defence that does not depend on the label is showing the + recorded LENS COUNT every poll. A relabelled receipt then reads as what it + actually is, and the quietest receipt is no longer the dishonest one. + """ + pr_watch = _load_pr_watch() + + def _evidence_line(receipt): + report = pr_watch.build_report( + _green_view(), [], set(), review_receipt=receipt + ) + return next( + (l for l in pr_watch.render(report).splitlines() if "review evidence" in l), + "", + ) + + panel = _evidence_line( + {"head": "abc123", "source": "fallback:panel", + "lenses": ["adversarial", "correctness"]} + ) + single = _evidence_line( + {"head": "abc123", "source": "fallback:codex", "lenses": ["correctness"]} + ) + relabelled = _evidence_line( + {"head": "abc123", "source": "fallback:panel (adversarial + correctness)"} + ) + + assert "2 lenses (adversarial, correctness)" in panel and "⚠" not in panel + assert "ONE lens (correctness)" in single + # The bypass no longer buys silence — it reads as unstated coverage. + assert "lenses not stated" in relabelled + + +def test_a_stale_receipt_reports_no_coverage_at_all() -> None: + """Evidence bound to an older head is not evidence for this one, so the + render must not describe its lenses as covering the current design.""" + pr_watch = _load_pr_watch() + + report = pr_watch.build_report( + _green_view(), + [], + set(), + review_receipt={"head": "0ldc0de", "source": "fallback:panel", + "lenses": ["adversarial", "correctness"]}, + ) + rendered = pr_watch.render(report) + + assert report["review_evidence"]["lenses"] == [] + # The blocker line legitimately contains the phrase, so match the render's + # own coverage prefix rather than a substring that appears in both. + assert not any(l.strip().startswith("review evidence:") for l in rendered.splitlines()) + assert report["mergeable"] is False + + +def test_a_hand_edited_receipt_cannot_break_either_render() -> None: + """The state file is plain JSON on disk and anything that can run this + engine can edit it. The sibling receipt fields are type-guarded; this one + skipped it.""" + pr_watch = _load_pr_watch() + + for junk in ("adversarial", 5, {"a": 1}, [None], [""], [1, 2]): + 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 + pr_watch.render(report) # must not raise + pr_watch.render_record_review({"pr": 9, "review_receipt": receipt}) + + +def test_a_blank_or_wrong_typed_receipt_source_keeps_the_gate_armed( + tmp_path: Path, +) -> None: + """Both validation branches on `receipt_source` were unpinned, and both fail + in the direction that switches the gate OFF: an unmatchable source (a list, + a blank string) or an unstripped one silently accepts a lens-free panel + receipt.""" + pr_watch = _load_pr_watch() + + for bad in ('""', '" "', "[]", "true", "12"): + resolved = pr_watch._load_review_config( + _write_config( + tmp_path / f"c{abs(hash(bad))}", + f"review:\n fallback_panel:\n receipt_source: {bad}\n", + ) + ) + assert resolved.panel_receipt_source == pr_watch._DEFAULT_PANEL_RECEIPT_SOURCE, bad + + # Surrounding whitespace is stripped, so the configured source still matches + # the (already stripped) source a caller passes in. + padded = pr_watch._load_review_config( + _write_config( + tmp_path / "padded", + 'review:\n fallback_panel:\n receipt_source: " fallback:panel "\n', + ) + ) + assert padded.panel_receipt_source == "fallback:panel" From 57a09ab9016c569f40eddf36f5c4cff393a54585 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sun, 26 Jul 2026 00:56:03 +0300 Subject: [PATCH 04/10] The warning fired on every honest receipt, so it exposed nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Panel round 3. Both lenses again, both found real defects in round 2's fix, and the correctness lens found the one that matters. **`⚠ lenses not stated` fired on every lens-free receipt** — including the ordinary bot receipt, which no workflow tells you to record with `--lenses`. Measured against this repo's own 14 stored receipts: all 14 warned. So the relabelled bypass round 2 set out to expose produced a line byte-identical to the healthy default, and the change achieved nothing. It also repeated the exact mistake this engine deliberately avoids 300 lines earlier for `bot_signal: "skipped"` — a permanent false warning on a correct receipt. Now only a receipt CLAIMING to be a panel owes a lens list. Of those same 14, 5 warn — and those 5 genuinely did claim a panel without naming lenses. **Two more forgeries in round 2's render.** A newline in `source` split the coverage line and left the first half reading as a completed panel; and `,` is both the lens separator and ordinary punctuation, so "correctness, i.e. does it do what it says" counted as two lenses. Untrusted text is flattened (the convention `_excerpt` already set for comment bodies) and the count deduped case-folded. **The lens roster is now checked against config.** That is the one part of the panel claim that is genuinely checkable — the lenses are declared in `review.fallback_panel.lenses`, not invented at record time. It does not prove a panel ran; nothing here can. The docs say so plainly rather than implying otherwise. **Claims of mine that were false, now corrected:** - "Both render paths are now list-guarded, like the sibling receipt fields already were" — the sibling was NOT guarded and raised `AttributeError` on a string `bots_behind_head`. Guarded now, and the test that promised to cover "either render" actually does. - "interpolated everywhere" — `SKILL.md` still hardcoded the literal. - The poll render still said "run the configured fallback review", naming the degraded mode this PR redefines, while the workflow doc described that line as pointing at the panel. Doc described output the code did not produce. - A test asserted `_PANEL_RECEIPT_SOURCE == panel["receipt_source"]` where both sides are the same string — a tautology that passed with config reading removed entirely. **And the family is consistent again.** Round 2's own argument — a caveat printed only on the stdout of the `--record-review` call the agent itself made is not visible at merge time — applied equally to `override` and `bot_signal`, which had not moved. Both now surface on the poll render too. The source-side casefold, unpinned for three rounds against this PR's own "mutation-test new branches" doctrine, is finally pinned. Tests 317 → 323. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --- .agents/skills/pr-watch/SKILL.md | 2 +- docs/agentic-dev-kit/fallback-review-panel.md | 9 +- kit-manifest.json | 4 +- scripts/hooks/pr_followup_hook.py | 6 +- scripts/pr_watch.py | 105 ++++++++++-- scripts/tests/test_pr_followup_hook.py | 12 +- scripts/tests/test_pr_watch.py | 158 +++++++++++++++++- 7 files changed, 267 insertions(+), 29 deletions(-) diff --git a/.agents/skills/pr-watch/SKILL.md b/.agents/skills/pr-watch/SKILL.md index 7fe3981..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. 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 `fallback:panel`. 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/docs/agentic-dev-kit/fallback-review-panel.md b/docs/agentic-dev-kit/fallback-review-panel.md index fd4a862..43c0e63 100644 --- a/docs/agentic-dev-kit/fallback-review-panel.md +++ b/docs/agentic-dev-kit/fallback-review-panel.md @@ -85,8 +85,8 @@ author re-reading their own diff. --head ``` - `--record-review` **refuses** a receipt whose source is exactly the - configured `receipt_source` unless `--lenses` names two distinct lenses. Know + `--record-review` **refuses** a receipt whose source is the configured + `receipt_source` (compared case-insensitively, after stripping) unless `--lenses` names two distinct lenses. Know what that is and is not: `source` is free text, so the check catches an accidental mislabel, not a determined one — `fallback:panel (2 lenses)` is just another string. @@ -118,8 +118,9 @@ So the stopping criterion is **blast radius, not round count**: 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 (lenses, `override`, `bot_signal`, -`bots_behind_head`), not a prose rationale for stopping. "The last round found +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 diff --git a/kit-manifest.json b/kit-manifest.json index 6e9784e..f26e1a9 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -15,7 +15,7 @@ "files": { "docs/agentic-dev-kit/fallback-review-panel.md": { "role": "doctrine", - "sha256": "02d9dd45c06a641f8822dc50c52f513e3f3860c3a144841d50f95a6ef93c1cf3" + "sha256": "82245a16caf8173d93b59a6e004e14a75d1c591cf0b1dcf38a8eddb0b47f2304" }, "docs/agentic-dev-kit/safety-critical-changes.md": { "role": "doctrine", @@ -103,7 +103,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "e90ca1a3eb03673d810f14046b9cce52c5d41c7c8bdf68ccbaf874d941cb5036" + "sha256": "43be6fa9379cb40291b46e1348bf851c3616db2000797bdc9dfddaf651f641bb" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/hooks/pr_followup_hook.py b/scripts/hooks/pr_followup_hook.py index f1b4367..f52706b 100755 --- a/scripts/hooks/pr_followup_hook.py +++ b/scripts/hooks/pr_followup_hook.py @@ -111,6 +111,7 @@ 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. @@ -130,7 +131,8 @@ def _fallback_instruction( "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'`--record-review "{panel_source}" --lenses --head `. ' + f"`uv run {engines_dir}/pr_watch.py " + f'--record-review "{panel_source}" --lenses --head `. ' "Never treat the outage as a review waiver." ) return ( @@ -152,7 +154,7 @@ def build_reminder() -> str: "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). " - + _fallback_instruction(fallback_command, lenses, panel_source) + + _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/pr_watch.py b/scripts/pr_watch.py index d7667ff..049982e 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -289,6 +289,7 @@ class ReviewConfig(NamedTuple): bots: tuple[str, ...] bot_pending_grace_minutes: float panel_receipt_source: str + panel_lens_names: frozenset[str] def _load_review_config(config_path: str | Path | None = None) -> ReviewConfig: @@ -335,6 +336,7 @@ def _load_review_config(config_path: str | Path | None = None) -> ReviewConfig: bots=_DEFAULT_REVIEW_BOTS, bot_pending_grace_minutes=_DEFAULT_BOT_PENDING_GRACE_MINUTES, panel_receipt_source=_DEFAULT_PANEL_RECEIPT_SOURCE, + panel_lens_names=frozenset(), ) try: from kitconfig import get, get_str_list, load_config @@ -369,6 +371,18 @@ def _load_review_config(config_path: str | Path | None = None) -> ReviewConfig: ) if not isinstance(panel_source, str) or not panel_source.strip(): panel_source = _DEFAULT_PANEL_RECEIPT_SOURCE + declared = get(config, "review.fallback_panel.lenses", []) + panel_lenses = ( + frozenset( + lens["name"].strip().casefold() + for lens in declared + if isinstance(lens, dict) + and isinstance(lens.get("name"), str) + and lens["name"].strip() + ) + if isinstance(declared, list) + else frozenset() + ) except FileNotFoundError: # `load_config` raises this for an absent config file — a standalone # engine run. Defaults are exactly right; stay quiet. @@ -394,6 +408,7 @@ def _load_review_config(config_path: str | Path | None = None) -> ReviewConfig: bots=tuple(bot.strip().lower() for bot in bots if bot.strip()), bot_pending_grace_minutes=float(grace), panel_receipt_source=panel_source.strip(), + panel_lens_names=panel_lenses, ) @@ -405,6 +420,7 @@ def _load_review_config(config_path: str | Path | None = None) -> ReviewConfig: _REVIEW_BOTS = _REVIEW_CONFIG.bots _BOT_PENDING_GRACE_MINUTES = _REVIEW_CONFIG.bot_pending_grace_minutes _PANEL_RECEIPT_SOURCE = _REVIEW_CONFIG.panel_receipt_source +_PANEL_LENS_NAMES = _REVIEW_CONFIG.panel_lens_names # --------------------------------------------------------------------------- gh @@ -1509,6 +1525,19 @@ def record_review( # this check existed. if source.casefold() == _PANEL_RECEIPT_SOURCE.casefold(): distinct = {lens.casefold() for lens in named_lenses} + # Names must be ones the config declares. Free text made `,` both the + # separator and ordinary punctuation, so "correctness, i.e. does it do + # what it says" counted as two lenses. This is the one part of the + # claim that IS checkable: the lens roster is configuration, not + # something the caller invents at record time. + if _PANEL_LENS_NAMES: + unknown = sorted(distinct - _PANEL_LENS_NAMES) + if unknown: + raise ValueError( + f"--lenses named {', '.join(unknown)}, which is not in " + f"review.fallback_panel.lenses ({', '.join(sorted(_PANEL_LENS_NAMES))}). " + "Name the configured lenses, or record under a single-lens source" + ) if len(distinct) < 2: raise ValueError( f"a {source!r} receipt asserts two independent lenses, but " @@ -1551,6 +1580,23 @@ def record_review( # ----------------------------------------------------------------------- main +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 (adversarial, correctness) + (recorded) — ⚠ lenses not stated + + `_excerpt` already established this convention for comment bodies; the + receipt fields skipped it. + """ + flat = " ".join(str(text).split()) + return flat if len(flat) <= n else flat[: n - 1] + "…" + + def _excerpt(body: str, n: int = 140) -> str: flat = " ".join((body or "").split()) return flat if len(flat) <= n else flat[: n - 1] + "…" @@ -1682,6 +1728,16 @@ def build_report( 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": @@ -1816,7 +1872,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 []: @@ -1837,14 +1893,41 @@ def render(report: dict) -> str: # regardless of what it is called. evidence = report.get("review_evidence") or {} if evidence.get("valid"): - named = evidence.get("lenses") or [] - if len(named) >= 2: - detail = f"{len(named)} lenses ({', '.join(named)})" + named = [_flat(lens, 40) for lens in evidence.get("lenses") or []] + # Counted case-folded and deduped, like the record-time gate. Counting + # raw list length let `adversarial,adversarial` — and an honest one-lens + # note written as "correctness, i.e. …", where the comma is punctuation + # rather than a separator — render as a full panel. + distinct = len({lens.casefold() for lens in named}) + source = _flat(evidence.get("source")) + claims_panel = source.casefold().startswith(_PANEL_RECEIPT_SOURCE.casefold()) + if distinct >= 2: + detail = f"{distinct} lenses ({', '.join(named)})" elif named: detail = f"⚠ ONE lens ({named[0]}) — not a dual-lens pass" + elif claims_panel: + # Only a receipt CLAIMING to be a panel owes a lens list. Warning on + # every lens-free receipt made the dishonest line byte-identical to + # the ordinary bot receipt — so it exposed nothing, and it repeated + # the mistake this file avoids for `bot_signal: skipped`: a + # permanent false warning on a correct receipt. + detail = "⚠ claims a panel but states no lenses" else: - detail = "⚠ lenses not stated" - lines.append(f" review evidence: {evidence.get('source')} — {detail}") + detail = "no lens list (not a panel receipt)" + 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}") if report["new_comments"]: @@ -1854,7 +1937,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']}") @@ -1865,7 +1948,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( @@ -1886,14 +1969,16 @@ def render_record_review(report: dict) -> str: if isinstance(raw_lenses, list) else [] ) - if len(named) == 1: + named = [_flat(lens, 40) for lens in named] + if len({lens.casefold() for lens in 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)}") - for bot, sha in (receipt.get("bots_behind_head") or {}).items(): + 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 " "does not stand for its review of this design" diff --git a/scripts/tests/test_pr_followup_hook.py b/scripts/tests/test_pr_followup_hook.py index 72dc4b8..cb71117 100644 --- a/scripts/tests/test_pr_followup_hook.py +++ b/scripts/tests/test_pr_followup_hook.py @@ -104,7 +104,7 @@ 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): +def test_reminder_names_configured_bots_not_a_hardcoded_bot(monkeypatch): """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").""" @@ -199,7 +199,7 @@ def test_a_one_lens_panel_config_degrades_instead_of_advertising_a_refusal( assert "review waiver" in reminder -def test_blank_lens_names_are_discarded_rather_than_advertised(monkeypatch): +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() @@ -222,6 +222,10 @@ def test_blank_lens_names_are_discarded_rather_than_advertised(monkeypatch): _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(): @@ -258,4 +262,6 @@ def _boom(*_args, **_kwargs): # naming lenses that were never read. assert lenses == [] assert panel_source == "fallback:panel" - assert "PANEL" not in hook._fallback_instruction(fallback, lenses, panel_source) + assert "PANEL" not in hook._fallback_instruction( + fallback, lenses, panel_source, engines + ) diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index 2fdf481..f965a25 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -362,6 +362,8 @@ def test_review_receipt_must_match_current_head() -> None: assert current["mergeable"] is True assert current["review_evidence"] == { "lenses": [], + "override": None, + "bot_signal": None, "valid": True, "source": "fallback:codex", "head": "abc123", @@ -1547,6 +1549,7 @@ def test_missing_config_falls_back_to_defaults_silently( pr_watch._DEFAULT_REVIEW_BOTS, pr_watch._DEFAULT_BOT_PENDING_GRACE_MINUTES, pr_watch._DEFAULT_PANEL_RECEIPT_SOURCE, + frozenset(), ) assert capsys.readouterr().err == "" @@ -2198,8 +2201,13 @@ def test_the_shipped_config_ships_two_disjoint_lenses() -> None: # the same brief are one lens run twice. focuses = [lens["focus"] for lens in panel["lenses"]] assert len(set(focuses)) == len(focuses) - # …and the engine must have read this file, not its own default. - assert pr_watch._PANEL_RECEIPT_SOURCE == panel["receipt_source"] + # (That the engine READS this rather than using its default is pinned by + # test_the_panel_receipt_source_is_read_from_config_not_the_engine_default — + # asserting it here would be a tautology, since the shipped value and the + # engine default are deliberately the same string.) + assert pr_watch._PANEL_LENS_NAMES == { + lens["name"].casefold() for lens in panel["lenses"] + } def test_the_panel_receipt_source_is_read_from_config_not_the_engine_default( @@ -2278,7 +2286,8 @@ def test_a_panel_receipt_is_refused_without_two_distinct_lenses( """ pr_watch = _load_pr_watch() - for bad in (None, "", "adversarial", "adversarial,adversarial", "a, A , a"): + for bad in (None, "", "adversarial", "adversarial,adversarial", + "correctness, Correctness"): with pytest.raises(ValueError, match="two independent lenses"): _record(monkeypatch, pr_watch, lenses=bad) @@ -2303,9 +2312,11 @@ def test_the_panel_gate_follows_a_renamed_receipt_source( monkeypatch.setattr(pr_watch, "_PANEL_RECEIPT_SOURCE", "fallback:my-panel") with pytest.raises(ValueError, match="two independent lenses"): - _record(monkeypatch, pr_watch, source="fallback:my-panel", lenses="one") + _record(monkeypatch, pr_watch, source="fallback:my-panel", + lenses="adversarial") - # The old literal is now just an ordinary single-lens source. + # The old literal is now just an ordinary single-lens source, so a free-text + # lens name is accepted there — the roster check is panel-only. receipt, _ = _record( monkeypatch, pr_watch, source="fallback:panel", lenses="one" ) @@ -2347,8 +2358,9 @@ def _evidence_line(receipt): assert "2 lenses (adversarial, correctness)" in panel and "⚠" not in panel assert "ONE lens (correctness)" in single - # The bypass no longer buys silence — it reads as unstated coverage. - assert "lenses not stated" in relabelled + # The bypass no longer buys silence — and it is now the ONLY lens-free + # line that warns, so it no longer hides behind the healthy default. + assert "claims a panel but states no lenses" in relabelled def test_a_stale_receipt_reports_no_coverage_at_all() -> None: @@ -2415,3 +2427,135 @@ def test_a_blank_or_wrong_typed_receipt_source_keeps_the_gate_armed( ) ) assert padded.panel_receipt_source == "fallback:panel" + + +def test_the_coverage_line_is_quiet_on_receipts_that_owe_no_lens_list() -> None: + """A warning that fires on every healthy receipt exposes nothing. + + Warning whenever `lenses` was absent made the ordinary bot receipt — which + no workflow tells you to record with `--lenses` — render identically to a + receipt relabelled to fake a panel. Measured against this repo's own stored + receipts, all 14 warned. It also repeated the mistake this engine + deliberately avoids for `bot_signal: "skipped"`: a permanent false warning + on a correct receipt. + + Only a receipt CLAIMING to be a panel owes a lens list. + """ + pr_watch = _load_pr_watch() + + def _line(receipt): + report = pr_watch.build_report( + _green_view(), [], set(), review_receipt=receipt + ) + return next( + l.strip() + for l in pr_watch.render(report).splitlines() + if "review evidence" in l + ) + + bot = _line({"head": "abc123", "source": "coderabbit"}) + panel = _line({"head": "abc123", "source": "fallback:panel", + "lenses": ["adversarial", "correctness"]}) + one = _line({"head": "abc123", "source": "fallback:codex", + "lenses": ["correctness"]}) + relabelled = _line({"head": "abc123", + "source": "fallback:panel (adversarial + correctness)"}) + + assert "⚠" not in bot and "not a panel receipt" in bot + assert "⚠" not in panel and "2 lenses" in panel + assert "ONE lens" in one + # The bypass is now the only lens-free line that warns. + assert "claims a panel but states no lenses" in relabelled + + +def test_the_coverage_line_cannot_be_forged_with_a_newline_or_a_comma() -> None: + """Both fields are free text chosen by whoever ran `--record-review`. + + A newline in `source` split the line in two and left the first half reading + as a completed panel; a comma inside a lens *description* ("correctness, + i.e. does it do what it says") counted as two lenses. Untrusted text is + flattened and the count is deduped case-folded. + """ + pr_watch = _load_pr_watch() + + def _lines(receipt): + report = pr_watch.build_report( + _green_view(), [], set(), review_receipt=receipt + ) + return pr_watch.render(report).splitlines() + + forged = _lines({ + "head": "abc123", + "source": "fallback:panel — 2 lenses (adversarial, correctness)\n (recorded)", + }) + evidence = [l for l in forged if "review evidence" in l] + assert len(evidence) == 1 + assert "claims a panel but states no lenses" in evidence[0] + # No orphaned continuation line masquerading as report output. + assert not any(l.strip().startswith("(recorded)") for l in forged) + + duped = _lines({"head": "abc123", "source": "fallback:codex", + "lenses": ["adversarial", "Adversarial"]}) + assert "ONE lens" in next(l for l in duped if "review evidence" in l) + + +def test_a_case_variant_panel_source_cannot_evade_the_lens_requirement( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The source-side casefold survived three rounds unpinned. + + Before it, `Fallback:Panel` skipped the check entirely — producing a receipt + that reads as a panel with no warning at all, which is worse than before the + check existed. + """ + pr_watch = _load_pr_watch() + + for variant in ("fallback:panel", "Fallback:Panel", "FALLBACK:PANEL"): + with pytest.raises(ValueError, match="two independent lenses"): + _record(monkeypatch, pr_watch, source=variant, lenses="adversarial") + + +def test_an_invented_lens_name_is_refused_for_a_panel_receipt( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The lens roster is configuration, not something a caller invents at + record time — which is the one part of the panel claim that IS checkable.""" + pr_watch = _load_pr_watch() + + with pytest.raises(ValueError, match="not in review.fallback_panel.lenses"): + _record(monkeypatch, pr_watch, + lenses="correctness, i.e. does it do what it says") + + receipt, _ = _record(monkeypatch, pr_watch, lenses="Adversarial, correctness") + assert receipt["lenses"] == ["Adversarial", "correctness"] + + +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}} + ) From c9c1cd26f0e2725d4bed4aa9e77811b4adf2b345 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sun, 26 Jul 2026 01:02:11 +0300 Subject: [PATCH 05/10] Remove the default-lens-roster trap on the unreadable-config path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit. `_DEFAULT_LENSES` was `[]`, so the hazard was latent rather than live — the degraded wording already fired. But the name implied a default panel exists, and populating it would have made 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. The constant is gone; that path now returns `[]` with the reason written down, and a test pins that an unreadable config produces the single-command wording. Tests 323 → 324. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --- scripts/hooks/pr_followup_hook.py | 8 ++++++-- scripts/tests/test_pr_followup_hook.py | 23 +++++++++++++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/scripts/hooks/pr_followup_hook.py b/scripts/hooks/pr_followup_hook.py index f52706b..b6354dd 100755 --- a/scripts/hooks/pr_followup_hook.py +++ b/scripts/hooks/pr_followup_hook.py @@ -42,7 +42,6 @@ _DEFAULT_FALLBACK_COMMAND = "/code-review" _DEFAULT_ENGINES_DIR = "scripts" -_DEFAULT_LENSES: list[str] = [] _DEFAULT_PANEL_RECEIPT_SOURCE = "fallback:panel" @@ -93,7 +92,12 @@ def _load_review_config() -> tuple[list[str], str, str, list[str], str]: [], _DEFAULT_FALLBACK_COMMAND, _DEFAULT_ENGINES_DIR, - list(_DEFAULT_LENSES), + # 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 advertise a panel — + # and a `--record-review` command the engine then refuses — on the + # strength of a config it just failed to load. + [], _DEFAULT_PANEL_RECEIPT_SOURCE, ) diff --git a/scripts/tests/test_pr_followup_hook.py b/scripts/tests/test_pr_followup_hook.py index cb71117..48de6eb 100644 --- a/scripts/tests/test_pr_followup_hook.py +++ b/scripts/tests/test_pr_followup_hook.py @@ -265,3 +265,26 @@ def _boom(*_args, **_kwargs): 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 From 456b9b7b634e6f2daf692f6a6fec5f0f218b8cbd Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sun, 26 Jul 2026 01:43:40 +0300 Subject: [PATCH 06/10] =?UTF-8?q?Two=20disjoint=20lenses=20is=20the=20floo?= =?UTF-8?q?r,=20not=20the=20ceiling=20=E2=80=94=20make=20that=20true?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Panel round 4. The correctness lens found three High; the adversarial lens had to be stopped mid-run (see below). **The roster check refused a genuine third lens.** Requiring every `--lenses` name to appear in `review.fallback_panel.lenses` made this PR's own doc false: `adversarial,correctness,security` was rejected, and the error told the operator to record a three-lens review under a *single-lens* source — pushing them to UNDER-claim. Roster names are now COUNTED rather than required, so extras pass while the forgery this replaced still fails (`"correctness, i.e. does it do what it says"` names one roster lens, not two). **Round 3's render fix landed on two of three sites.** The grace-cancelled branch and a docstring still said "run the configured fallback review" — naming the degraded mode this PR redefines, while the workflow doc described that same line as pointing at the panel. **Neither changed string was pinned.** The existing assertions matched the `review unavailable` prefix, never the pointer, so reverting round 3's change passed all 324 tests. That is the "named by a test and pinned by nothing" class this PR ships doctrine about, on this PR's own change. Now pinned across all three sites. Also: `_flat`'s docstring quoted a warning string round 3 deleted; the doc claimed the coverage line prints "every time the PR is polled" when it needs a current-head receipt; and the doc restated the lens briefs that config already owns — two copies which had already drifted (the doc's adversarial brief mentioned "inputs the author did not consider", config's did not). The doc now points at config for the briefs rather than duplicating them. **A finding about the panel itself, discovered by running it.** The two lenses ran concurrently in the same working tree. The adversarial lens mutates files to test them; the correctness lens saw those mutations as an external process corrupting the repo and ran `git checkout --` to "restore" it. They clobbered each other for ten minutes: one lens's results were unreliable, and when I stopped the adversarial lens it left a live mutant behind — `if False and _PANEL_LENS_NAMES:` — which silently disabled the roster check in my working tree. I caught it only because a test failed with an error string that should no longer have existed. `fallback-review-panel.md` gains contract item 7: mutate in an isolated copy, never the shared tree. That is a real gap in the doc this PR ships, found by using it. Tests 324 → 326. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --- docs/agentic-dev-kit/fallback-review-panel.md | 40 ++++++---- kit-manifest.json | 4 +- scripts/hooks/pr_followup_hook.py | 4 +- scripts/pr_watch.py | 32 ++++---- scripts/tests/test_pr_followup_hook.py | 2 +- scripts/tests/test_pr_watch.py | 73 ++++++++++++++++++- 6 files changed, 120 insertions(+), 35 deletions(-) diff --git a/docs/agentic-dev-kit/fallback-review-panel.md b/docs/agentic-dev-kit/fallback-review-panel.md index 43c0e63..e627511 100644 --- a/docs/agentic-dev-kit/fallback-review-panel.md +++ b/docs/agentic-dev-kit/fallback-review-panel.md @@ -26,19 +26,18 @@ of it. ## The lenses -Configured in `review.fallback_panel.lenses`. The kit ships two, because they -are the two the doctrine names: +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** — assume the change is wrong and try to prove it. Bypasses, - fail-open paths, wedges, inputs the author did not consider, and *whether the - new guard actually guards*. -- **correctness** — assume the change works and ask what it *says*. Stale - comments and docstrings, claims that overstate what is verified, tests whose - names promise more than their bodies check, drift between the diff and its PR - body. +- **adversarial** — starts from "this is wrong" and tries to prove it. +- **correctness** — starts from "this works" and asks what it *says*. -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. +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, +and `--record-review` counts roster lenses without forbidding extras. ## The contract every lens gets @@ -62,7 +61,21 @@ author re-reading their own diff. suite. 6. **Report, do not fix.** A lens that edits loses the disjointness: it starts defending its own changes on the next round. -7. **State what was verified clean, and how.** Absence of findings is only +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. +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 @@ -92,7 +105,8 @@ author re-reading their own diff. just another string. The defence that does not depend on the label is the **poll render**, which - states the recorded lens count every time the PR is polled: + states the recorded lens count 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 (correctness) — not a dual-lens pass diff --git a/kit-manifest.json b/kit-manifest.json index f26e1a9..dec3071 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -15,7 +15,7 @@ "files": { "docs/agentic-dev-kit/fallback-review-panel.md": { "role": "doctrine", - "sha256": "82245a16caf8173d93b59a6e004e14a75d1c591cf0b1dcf38a8eddb0b47f2304" + "sha256": "661e7e6ad32d97871d0de0593c5dace81e74e111c2f53218aafb99da86acd3b9" }, "docs/agentic-dev-kit/safety-critical-changes.md": { "role": "doctrine", @@ -103,7 +103,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "43be6fa9379cb40291b46e1348bf851c3616db2000797bdc9dfddaf651f641bb" + "sha256": "2a783b67d83620b1fab785c27c9e6d78d6d87e43fd7e80409a621b9b7d298e87" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/hooks/pr_followup_hook.py b/scripts/hooks/pr_followup_hook.py index b6354dd..68eac88 100755 --- a/scripts/hooks/pr_followup_hook.py +++ b/scripts/hooks/pr_followup_hook.py @@ -10,8 +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 the fallback PANEL (or, with none configured, the single 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 diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index 049982e..e1f7e64 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -1009,7 +1009,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 @@ -1530,21 +1531,24 @@ def record_review( # what it says" counted as two lenses. This is the one part of the # claim that IS checkable: the lens roster is configuration, not # something the caller invents at record time. + # Count only lenses the config DECLARES, but do not forbid extras. + # Requiring every name to be in the roster made "two disjoint lenses is + # the floor, not the ceiling" false: a genuine third ad-hoc lens was + # refused, and the error pushed the operator to UNDER-claim a three-lens + # review. Counting roster hits still blocks the forgery this replaced, + # where `,` is punctuation rather than a separator ("correctness, i.e. + # does it do what it says" names one roster lens). if _PANEL_LENS_NAMES: - unknown = sorted(distinct - _PANEL_LENS_NAMES) - if unknown: - raise ValueError( - f"--lenses named {', '.join(unknown)}, which is not in " - f"review.fallback_panel.lenses ({', '.join(sorted(_PANEL_LENS_NAMES))}). " - "Name the configured lenses, or record under a single-lens source" - ) + distinct &= _PANEL_LENS_NAMES if len(distinct) < 2: raise ValueError( f"a {source!r} receipt asserts two independent lenses, but " - f"--lenses named {len(distinct)} distinct ({', '.join(named_lenses) or 'none'}). " - "Run the second lens, or record what actually ran under a " - "single-lens source (e.g. fallback:codex) — see " - "docs/agentic-dev-kit/fallback-review-panel.md" + f"--lenses named {len(distinct)} of the configured roster " + f"({', '.join(sorted(_PANEL_LENS_NAMES)) or 'none declared'}); " + f"you gave {', '.join(named_lenses) or 'none'}. Run the second " + "lens, add it to review.fallback_panel.lenses if it is new, or " + "record what actually ran under a single-lens source (e.g. " + "fallback:codex) — see docs/agentic-dev-kit/fallback-review-panel.md" ) if named_lenses: receipt["lenses"] = named_lenses @@ -1588,7 +1592,7 @@ def _flat(text: object, n: int = 120) -> str: line in two and leaves the first half reading as a completed panel: review evidence: fallback:panel — 2 lenses (adversarial, correctness) - (recorded) — ⚠ lenses not stated + (recorded) — ⚠ claims a panel but states no lenses `_excerpt` already established this convention for comment bodies; the receipt fields skipped it. @@ -1884,7 +1888,7 @@ 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 diff --git a/scripts/tests/test_pr_followup_hook.py b/scripts/tests/test_pr_followup_hook.py index 48de6eb..23a956c 100644 --- a/scripts/tests/test_pr_followup_hook.py +++ b/scripts/tests/test_pr_followup_hook.py @@ -104,7 +104,7 @@ def test_exits_zero_on_malformed_or_unexpected_stdin(monkeypatch, capsys, stdin_ assert out == "" -def test_reminder_names_configured_bots_not_a_hardcoded_bot(monkeypatch): +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").""" diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index f965a25..c331d9b 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -2515,14 +2515,19 @@ def test_a_case_variant_panel_source_cannot_evade_the_lens_requirement( _record(monkeypatch, pr_watch, source=variant, lenses="adversarial") -def test_an_invented_lens_name_is_refused_for_a_panel_receipt( +def test_an_invented_lens_name_does_not_count_toward_the_panel_floor( monkeypatch: pytest.MonkeyPatch, ) -> None: """The lens roster is configuration, not something a caller invents at - record time — which is the one part of the panel claim that IS checkable.""" + record time — which is the one part of the panel claim that IS checkable. + + Names outside the roster are recorded but not counted. Forbidding them + outright broke "two disjoint lenses is the floor, not the ceiling": a + genuine third ad-hoc lens was refused. + """ pr_watch = _load_pr_watch() - with pytest.raises(ValueError, match="not in review.fallback_panel.lenses"): + with pytest.raises(ValueError, match="of the configured roster"): _record(monkeypatch, pr_watch, lenses="correctness, i.e. does it do what it says") @@ -2559,3 +2564,65 @@ def test_a_hand_edited_bots_behind_head_cannot_break_the_receipt_render() -> Non {"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_panel_gate_counts_roster_lenses_and_allows_extras( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """See `test_two_lenses_is_the_floor_not_the_ceiling` for why.""" + pr_watch = _load_pr_watch() + + # Two roster lenses, plus a genuine third: accepted, and all three recorded. + receipt, _ = _record( + monkeypatch, pr_watch, lenses="adversarial,correctness,security" + ) + assert receipt["lenses"] == ["adversarial", "correctness", "security"] + + # One roster lens plus punctuation is still one lens. + with pytest.raises(ValueError, match="of the configured roster"): + _record(monkeypatch, pr_watch, + lenses="correctness, i.e. does it do what it says") + + # Two invented names cannot fake a panel. + with pytest.raises(ValueError, match="of the configured roster"): + _record(monkeypatch, pr_watch, lenses="security,performance") From 30c7948202862f190ce04dab0aab476c7a464eea Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sun, 26 Jul 2026 02:04:53 +0300 Subject: [PATCH 07/10] Delete the lens gate: it could not do what it claimed (#32 filed) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Panel round 5, run with the lenses in isolated worktrees this time. The adversarial lens showed the gate is defeated by one extra character — and that the render then *affirms* the forgery: --record-review "fallback:panel (2 lenses)" \ --lenses "correctness, i.e. does it do what it says" → review evidence: fallback:panel (2 lenses) — 2 lenses (correctness, i.e. …) → ✅ DONE — green, reviewed, merge-ready That is verbatim the forgery the previous commit claimed to block. The record gate matched `source` with `==` while the render matched it with `startswith`, so one decorated character passed the first and satisfied the second. It also found the render never consulted the roster at all (so any source with two invented lens names rendered as a panel), and that an empty roster failed OPEN from four plausible config shapes, silently. **So the gate is gone, not tightened a fifth time.** Rule 1: "treat 'we tightened the matcher' as a stopgap, not a fix." Four tightenings — source equality, then lens names, then a required roster, then a counted roster — each defeated by the next round. The mechanism cannot work from here: the caller writes the source and the lens names in one invocation, with nothing binding either to a review that happened. What remains is honest and still useful: `--lenses` is recorded and the poll render states it as a **claim** ("2 lenses claimed", "⚠ ONE lens claimed"), so a one-lens pass is visible at merge time instead of buried in the record command's stdout. The docs say plainly that this is an audit trail, not a gate, and that the engine cannot verify it. The version that would actually verify — each lens recording its own receipt from its own context, with the panel derived by counting entries — is filed as **#32** with the four defeats written up, so the next attempt starts from why this one failed rather than rediscovering it. Also fixed from this round: contract item 7 shipped DUPLICATED verbatim in the doc (kit_doctor could not catch it — the manifest was rehashed in the same commit); and `sha[:7]` crashed on a non-string value in a guard whose own comment claimed to have handled exactly that. Net: pr_watch.py loses ~60 lines of unverifiable checking. Tests 326 → 314, which is the right direction — seven tests pinned a mechanism that did not work. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --- .gitignore | 3 + docs/agentic-dev-kit/fallback-review-panel.md | 41 +- docs/agentic-dev-kit/workflows/pr-watch.md | 4 +- kit-manifest.json | 6 +- scripts/pr_watch.py | 101 +--- scripts/tests/test_pr_watch.py | 475 +++--------------- 6 files changed, 117 insertions(+), 513 deletions(-) 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/docs/agentic-dev-kit/fallback-review-panel.md b/docs/agentic-dev-kit/fallback-review-panel.md index e627511..8f0365c 100644 --- a/docs/agentic-dev-kit/fallback-review-panel.md +++ b/docs/agentic-dev-kit/fallback-review-panel.md @@ -36,8 +36,7 @@ describes only what the two shipped lenses are *for*: 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, -and `--record-review` counts roster lenses without forbidding extras. +lens, a performance lens): two disjoint lenses is the floor, not the ceiling. ## The contract every lens gets @@ -68,13 +67,6 @@ author re-reading their own diff. 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. -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. @@ -98,22 +90,26 @@ author re-reading their own diff. --head ``` - `--record-review` **refuses** a receipt whose source is the configured - `receipt_source` (compared case-insensitively, after stripping) unless `--lenses` names two distinct lenses. Know - what that is and is not: `source` is free text, so the check catches an - accidental mislabel, not a determined one — `fallback:panel (2 lenses)` is - just another string. + **`--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 configured roster — + 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. - The defence that does not depend on the label is the **poll render**, which - states the recorded lens count on every poll once a current-head receipt - exists (no receipt, or one bound to an older head, prints nothing): + 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 (correctness) — not a dual-lens pass + review evidence: fallback:codex — ⚠ ONE lens claimed (correctness) — not a dual-lens pass ``` - So a one-lens pass reads as one lens at merge time whatever it was called, - and `--lenses` is what determines that — not the source string. + 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 @@ -144,9 +140,8 @@ If the runtime cannot run isolated reviewers, fall back to better than nothing and it is **not** a panel: - record it as `fallback:`, never the panel's `receipt_source`. The - engine refuses a panel-sourced receipt naming fewer than two distinct lenses, - so the shortest path through it is to record what actually ran — but it cannot - tell whether a panel really ran, only what you named. The honesty is yours + 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 diff --git a/docs/agentic-dev-kit/workflows/pr-watch.md b/docs/agentic-dev-kit/workflows/pr-watch.md index 791c77a..603640b 100644 --- a/docs/agentic-dev-kit/workflows/pr-watch.md +++ b/docs/agentic-dev-kit/workflows/pr-watch.md @@ -83,8 +83,8 @@ Repeat until the report says **converged**: ``` `--lenses` names what actually ran, so a degraded one-lens pass is - distinguishable from a panel in the audit trail — and a panel-sourced - receipt naming fewer than two distinct lenses is **refused**. If your + 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. diff --git a/kit-manifest.json b/kit-manifest.json index dec3071..61b6137 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -15,7 +15,7 @@ "files": { "docs/agentic-dev-kit/fallback-review-panel.md": { "role": "doctrine", - "sha256": "661e7e6ad32d97871d0de0593c5dace81e74e111c2f53218aafb99da86acd3b9" + "sha256": "421e5eed191375aed8a463a4b1d9a2ce8b52d9a6ea77e81ef026432f6eafaf2c" }, "docs/agentic-dev-kit/safety-critical-changes.md": { "role": "doctrine", @@ -27,7 +27,7 @@ }, "docs/agentic-dev-kit/workflows/pr-watch.md": { "role": "workflow", - "sha256": "d1e047a6d8881116cc5615ad8a41865d3e2645dd53de1b4922164e9a6885c21f" + "sha256": "9d19949ec94d47b2efbdd9d5258fb8a32cc17b48d9cc39faab3202d202a859e7" }, "docs/agentic-dev-kit/workflows/session-start.md": { "role": "workflow", @@ -103,7 +103,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "2a783b67d83620b1fab785c27c9e6d78d6d87e43fd7e80409a621b9b7d298e87" + "sha256": "69a9ea73e7b460e985c0d5a3b77b4028f9fd3ff75ba661393c395600ba6a5de3" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index e1f7e64..be4064f 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -255,12 +255,6 @@ 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 -# The receipt source a full panel records under. Adopter-configurable -# (`review.fallback_panel.receipt_source`) because the whole point of the key is -# that a panel receipt is DISTINGUISHABLE from a single-lens one — an adopter who -# renames it must still get that property, and the validation below must follow -# their name rather than a literal. -_DEFAULT_PANEL_RECEIPT_SOURCE = "fallback:panel" # 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`. @@ -288,8 +282,6 @@ class ReviewConfig(NamedTuple): require_ci: bool bots: tuple[str, ...] bot_pending_grace_minutes: float - panel_receipt_source: str - panel_lens_names: frozenset[str] def _load_review_config(config_path: str | Path | None = None) -> ReviewConfig: @@ -335,8 +327,6 @@ def _load_review_config(config_path: str | Path | None = None) -> ReviewConfig: require_ci=_DEFAULT_REQUIRE_CI, bots=_DEFAULT_REVIEW_BOTS, bot_pending_grace_minutes=_DEFAULT_BOT_PENDING_GRACE_MINUTES, - panel_receipt_source=_DEFAULT_PANEL_RECEIPT_SOURCE, - panel_lens_names=frozenset(), ) try: from kitconfig import get, get_str_list, load_config @@ -364,25 +354,6 @@ def _load_review_config(config_path: str | Path | None = None) -> ReviewConfig: ) if isinstance(grace, bool) or not isinstance(grace, (int, float)) or grace < 0: grace = _DEFAULT_BOT_PENDING_GRACE_MINUTES - panel_source = 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 - declared = get(config, "review.fallback_panel.lenses", []) - panel_lenses = ( - frozenset( - lens["name"].strip().casefold() - for lens in declared - if isinstance(lens, dict) - and isinstance(lens.get("name"), str) - and lens["name"].strip() - ) - if isinstance(declared, list) - else frozenset() - ) except FileNotFoundError: # `load_config` raises this for an absent config file — a standalone # engine run. Defaults are exactly right; stay quiet. @@ -407,8 +378,6 @@ def _load_review_config(config_path: str | Path | None = None) -> ReviewConfig: require_ci=require_ci, bots=tuple(bot.strip().lower() for bot in bots if bot.strip()), bot_pending_grace_minutes=float(grace), - panel_receipt_source=panel_source.strip(), - panel_lens_names=panel_lenses, ) @@ -419,8 +388,6 @@ def _load_review_config(config_path: str | Path | None = None) -> ReviewConfig: _REQUIRE_CI = _REVIEW_CONFIG.require_ci _REVIEW_BOTS = _REVIEW_CONFIG.bots _BOT_PENDING_GRACE_MINUTES = _REVIEW_CONFIG.bot_pending_grace_minutes -_PANEL_RECEIPT_SOURCE = _REVIEW_CONFIG.panel_receipt_source -_PANEL_LENS_NAMES = _REVIEW_CONFIG.panel_lens_names # --------------------------------------------------------------------------- gh @@ -1512,44 +1479,6 @@ def record_review( "recorded_at": now.isoformat(), } named_lenses = [part.strip() for part in (lenses or "").split(",") if part.strip()] - # A cheap guard against the ACCIDENTAL mislabel, and no more than that. - # `source` is free text an agent picks, so this can only catch a receipt - # that names the configured panel source exactly; `fallback:panel (2 lenses)` - # sails past it. That is why the real defence is not here but in the poll - # render, which states the recorded lens count at merge time whatever the - # receipt is called — a deterministic artifact rather than a matcher over a - # label (`safety-critical-changes.md` rule 1). - # Both sides case-folded. Folding the lens names to defeat - # `adversarial,Adversarial` while comparing the SOURCE case-sensitively left - # `Fallback:Panel` bypassing the gate entirely — and producing a receipt that - # reads as a panel with no single-lens warning at all, i.e. worse than before - # this check existed. - if source.casefold() == _PANEL_RECEIPT_SOURCE.casefold(): - distinct = {lens.casefold() for lens in named_lenses} - # Names must be ones the config declares. Free text made `,` both the - # separator and ordinary punctuation, so "correctness, i.e. does it do - # what it says" counted as two lenses. This is the one part of the - # claim that IS checkable: the lens roster is configuration, not - # something the caller invents at record time. - # Count only lenses the config DECLARES, but do not forbid extras. - # Requiring every name to be in the roster made "two disjoint lenses is - # the floor, not the ceiling" false: a genuine third ad-hoc lens was - # refused, and the error pushed the operator to UNDER-claim a three-lens - # review. Counting roster hits still blocks the forgery this replaced, - # where `,` is punctuation rather than a separator ("correctness, i.e. - # does it do what it says" names one roster lens). - if _PANEL_LENS_NAMES: - distinct &= _PANEL_LENS_NAMES - if len(distinct) < 2: - raise ValueError( - f"a {source!r} receipt asserts two independent lenses, but " - f"--lenses named {len(distinct)} of the configured roster " - f"({', '.join(sorted(_PANEL_LENS_NAMES)) or 'none declared'}); " - f"you gave {', '.join(named_lenses) or 'none'}. Run the second " - "lens, add it to review.fallback_panel.lenses if it is new, or " - "record what actually ran under a single-lens source (e.g. " - "fallback:codex) — see docs/agentic-dev-kit/fallback-review-panel.md" - ) if named_lenses: receipt["lenses"] = named_lenses if allow_pending_bot: @@ -1897,27 +1826,25 @@ def render(report: dict) -> str: # 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 []] - # Counted case-folded and deduped, like the record-time gate. Counting - # raw list length let `adversarial,adversarial` — and an honest one-lens - # note written as "correctness, i.e. …", where the comma is punctuation - # rather than a separator — render as a full panel. distinct = len({lens.casefold() for lens in named}) source = _flat(evidence.get("source")) - claims_panel = source.casefold().startswith(_PANEL_RECEIPT_SOURCE.casefold()) if distinct >= 2: - detail = f"{distinct} lenses ({', '.join(named)})" + detail = f"{distinct} lenses claimed ({', '.join(named)})" elif named: - detail = f"⚠ ONE lens ({named[0]}) — not a dual-lens pass" - elif claims_panel: - # Only a receipt CLAIMING to be a panel owes a lens list. Warning on - # every lens-free receipt made the dishonest line byte-identical to - # the ordinary bot receipt — so it exposed nothing, and it repeated - # the mistake this file avoids for `bot_signal: skipped`: a - # permanent false warning on a correct receipt. - detail = "⚠ claims a panel but states no lenses" + detail = f"⚠ ONE lens claimed ({named[0]}) — not a dual-lens pass" else: - detail = "no lens list (not a panel receipt)" + 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 @@ -1984,7 +1911,7 @@ def render_record_review(report: dict) -> str: 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) diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index c331d9b..9cbce82 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -1548,8 +1548,6 @@ def test_missing_config_falls_back_to_defaults_silently( True, pr_watch._DEFAULT_REVIEW_BOTS, pr_watch._DEFAULT_BOT_PENDING_GRACE_MINUTES, - pr_watch._DEFAULT_PANEL_RECEIPT_SOURCE, - frozenset(), ) assert capsys.readouterr().err == "" @@ -2155,386 +2153,6 @@ def test_a_panel_receipt_names_every_lens_that_ran( rendered = pr_watch.render_record_review(report) assert "lenses: adversarial, correctness" in rendered assert "one lens only" not in rendered - - -def test_omitting_lenses_records_nothing_rather_than_guessing( - monkeypatch: pytest.MonkeyPatch, -) -> None: - """An older caller, or a receipt from a human reviewer, has no lens list. - - Absent must mean "not stated" — inventing one would make an unknown look - like a verified panel, which is the failure direction this whole family of - fields exists to close. - """ - pr_watch = _load_pr_watch() - - for value in (None, "", " ", ",", " , "): - receipt, report = _record( - monkeypatch, pr_watch, source="fallback:codex", lenses=value - ) - assert "lenses" not in receipt, value - assert "one lens only" not in pr_watch.render_record_review(report), value - - -def test_the_shipped_config_ships_two_disjoint_lenses() -> None: - """The doctrine's floor is TWO lenses, and it must survive being config. - - A kit that ships a one-lens panel would violate rule 2 out of the box while - appearing to satisfy it. - """ - pr_watch = _load_pr_watch() - sys.path.insert(0, str(ENGINE_DIR / "lib")) - import kitconfig # noqa: PLC0415 - - config = kitconfig.load_config(ENGINE_DIR.parent / "config" / "dev-model.yaml") - panel = kitconfig.get(config, "review.fallback_panel") - - assert len({lens["name"] for lens in panel["lenses"]}) >= 2 - assert all(lens.get("focus") for lens in panel["lenses"]) - # The panel's receipt source must differ from a single-lens one, or the - # audit trail cannot tell them apart. - assert panel["receipt_source"] == "fallback:panel" - assert panel["receipt_source"] not in set( - kitconfig.get(config, "review.fallback_commands", {}).values() - ) - # "Disjoint" means the FOCUS differs, not just the label — two lenses with - # the same brief are one lens run twice. - focuses = [lens["focus"] for lens in panel["lenses"]] - assert len(set(focuses)) == len(focuses) - # (That the engine READS this rather than using its default is pinned by - # test_the_panel_receipt_source_is_read_from_config_not_the_engine_default — - # asserting it here would be a tautology, since the shipped value and the - # engine default are deliberately the same string.) - assert pr_watch._PANEL_LENS_NAMES == { - lens["name"].casefold() for lens in panel["lenses"] - } - - -def test_the_panel_receipt_source_is_read_from_config_not_the_engine_default( - tmp_path: Path, -) -> None: - """A mutation making `_load_review_config` ignore the key entirely survived - the suite — a config key added and then not read (Principle #10).""" - pr_watch = _load_pr_watch() - - resolved = pr_watch._load_review_config( - _write_config( - tmp_path, - 'review:\n fallback_panel:\n receipt_source: "fallback:my-panel"\n', - ) - ) - - assert resolved.panel_receipt_source == "fallback:my-panel" - - # Blank / wrong-typed values keep the default rather than disabling the gate - # by making the source unmatchable. - for bad in ('""', '" "', "[]", "true"): - degraded = pr_watch._load_review_config( - _write_config( - tmp_path / bad.strip('"[]'), - f"review:\n fallback_panel:\n receipt_source: {bad}\n", - ) - ) - assert degraded.panel_receipt_source == pr_watch._DEFAULT_PANEL_RECEIPT_SOURCE, bad - - -def test_the_cli_threads_lenses_through_to_the_receipt( - monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] -) -> None: - """The seam, not just the unit. - - Dropping `lenses=args.lenses` in `main()` passed every test — the flag - parsed, the function accepted it, and nothing checked they were connected. - Also pins that `--lenses` without `--record-review` is a usage error. - """ - 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: {}) - - exit_code = pr_watch.main( - ["9", "--record-review", "fallback:panel", - "--lenses", "adversarial,correctness", "--head", "abc123"] - ) - - assert exit_code == 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_panel_receipt_is_refused_without_two_distinct_lenses( - monkeypatch: pytest.MonkeyPatch, -) -> None: - """The claim "a panel reviewed this" is the one a receipt must not assert on - trust. - - Left as prose, `--lenses adversarial,adversarial` rendered as multi-lens - coverage and omitting `--lenses` entirely still produced a receipt labelled - `fallback:panel`. A deterministic gate beats a documented expectation — and - the escape is not a flag, it is recording the truth under a single-lens - source. - """ - pr_watch = _load_pr_watch() - - for bad in (None, "", "adversarial", "adversarial,adversarial", - "correctness, Correctness"): - with pytest.raises(ValueError, match="two independent lenses"): - _record(monkeypatch, pr_watch, lenses=bad) - - # Two genuinely distinct lenses go through. - receipt, _ = _record(monkeypatch, pr_watch, lenses="adversarial,correctness") - assert receipt["lenses"] == ["adversarial", "correctness"] - - # …and the honest single-lens record is always available. - single, _ = _record( - monkeypatch, pr_watch, source="fallback:codex", lenses="correctness" - ) - assert single["source"] == "fallback:codex" - - -def test_the_panel_gate_follows_a_renamed_receipt_source( - monkeypatch: pytest.MonkeyPatch, -) -> None: - """`receipt_source` is adopter-configurable, so the validation has to follow - their name — checking a literal would leave a renamed panel unguarded while - rejecting a source they never use.""" - pr_watch = _load_pr_watch() - monkeypatch.setattr(pr_watch, "_PANEL_RECEIPT_SOURCE", "fallback:my-panel") - - with pytest.raises(ValueError, match="two independent lenses"): - _record(monkeypatch, pr_watch, source="fallback:my-panel", - lenses="adversarial") - - # The old literal is now just an ordinary single-lens source, so a free-text - # lens name is accepted there — the roster check is panel-only. - receipt, _ = _record( - monkeypatch, pr_watch, source="fallback:panel", lenses="one" - ) - assert receipt["lenses"] == ["one"] - - -def test_the_poll_render_states_what_the_receipt_covers() -> None: - """The one-lens warning used to print 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. - - `source` is free text an agent picks, so no matcher over it can be - trustworthy: `fallback:panel (adversarial + correctness)` sails past an - equality check. The defence that does not depend on the label is showing the - recorded LENS COUNT every poll. A relabelled receipt then reads as what it - actually is, and the quietest receipt is no longer the dishonest one. - """ - pr_watch = _load_pr_watch() - - def _evidence_line(receipt): - report = pr_watch.build_report( - _green_view(), [], set(), review_receipt=receipt - ) - return next( - (l for l in pr_watch.render(report).splitlines() if "review evidence" in l), - "", - ) - - panel = _evidence_line( - {"head": "abc123", "source": "fallback:panel", - "lenses": ["adversarial", "correctness"]} - ) - single = _evidence_line( - {"head": "abc123", "source": "fallback:codex", "lenses": ["correctness"]} - ) - relabelled = _evidence_line( - {"head": "abc123", "source": "fallback:panel (adversarial + correctness)"} - ) - - assert "2 lenses (adversarial, correctness)" in panel and "⚠" not in panel - assert "ONE lens (correctness)" in single - # The bypass no longer buys silence — and it is now the ONLY lens-free - # line that warns, so it no longer hides behind the healthy default. - assert "claims a panel but states no lenses" in relabelled - - -def test_a_stale_receipt_reports_no_coverage_at_all() -> None: - """Evidence bound to an older head is not evidence for this one, so the - render must not describe its lenses as covering the current design.""" - pr_watch = _load_pr_watch() - - report = pr_watch.build_report( - _green_view(), - [], - set(), - review_receipt={"head": "0ldc0de", "source": "fallback:panel", - "lenses": ["adversarial", "correctness"]}, - ) - rendered = pr_watch.render(report) - - assert report["review_evidence"]["lenses"] == [] - # The blocker line legitimately contains the phrase, so match the render's - # own coverage prefix rather than a substring that appears in both. - assert not any(l.strip().startswith("review evidence:") for l in rendered.splitlines()) - assert report["mergeable"] is False - - -def test_a_hand_edited_receipt_cannot_break_either_render() -> None: - """The state file is plain JSON on disk and anything that can run this - engine can edit it. The sibling receipt fields are type-guarded; this one - skipped it.""" - pr_watch = _load_pr_watch() - - for junk in ("adversarial", 5, {"a": 1}, [None], [""], [1, 2]): - 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 - pr_watch.render(report) # must not raise - pr_watch.render_record_review({"pr": 9, "review_receipt": receipt}) - - -def test_a_blank_or_wrong_typed_receipt_source_keeps_the_gate_armed( - tmp_path: Path, -) -> None: - """Both validation branches on `receipt_source` were unpinned, and both fail - in the direction that switches the gate OFF: an unmatchable source (a list, - a blank string) or an unstripped one silently accepts a lens-free panel - receipt.""" - pr_watch = _load_pr_watch() - - for bad in ('""', '" "', "[]", "true", "12"): - resolved = pr_watch._load_review_config( - _write_config( - tmp_path / f"c{abs(hash(bad))}", - f"review:\n fallback_panel:\n receipt_source: {bad}\n", - ) - ) - assert resolved.panel_receipt_source == pr_watch._DEFAULT_PANEL_RECEIPT_SOURCE, bad - - # Surrounding whitespace is stripped, so the configured source still matches - # the (already stripped) source a caller passes in. - padded = pr_watch._load_review_config( - _write_config( - tmp_path / "padded", - 'review:\n fallback_panel:\n receipt_source: " fallback:panel "\n', - ) - ) - assert padded.panel_receipt_source == "fallback:panel" - - -def test_the_coverage_line_is_quiet_on_receipts_that_owe_no_lens_list() -> None: - """A warning that fires on every healthy receipt exposes nothing. - - Warning whenever `lenses` was absent made the ordinary bot receipt — which - no workflow tells you to record with `--lenses` — render identically to a - receipt relabelled to fake a panel. Measured against this repo's own stored - receipts, all 14 warned. It also repeated the mistake this engine - deliberately avoids for `bot_signal: "skipped"`: a permanent false warning - on a correct receipt. - - Only a receipt CLAIMING to be a panel owes a lens list. - """ - pr_watch = _load_pr_watch() - - def _line(receipt): - report = pr_watch.build_report( - _green_view(), [], set(), review_receipt=receipt - ) - return next( - l.strip() - for l in pr_watch.render(report).splitlines() - if "review evidence" in l - ) - - bot = _line({"head": "abc123", "source": "coderabbit"}) - panel = _line({"head": "abc123", "source": "fallback:panel", - "lenses": ["adversarial", "correctness"]}) - one = _line({"head": "abc123", "source": "fallback:codex", - "lenses": ["correctness"]}) - relabelled = _line({"head": "abc123", - "source": "fallback:panel (adversarial + correctness)"}) - - assert "⚠" not in bot and "not a panel receipt" in bot - assert "⚠" not in panel and "2 lenses" in panel - assert "ONE lens" in one - # The bypass is now the only lens-free line that warns. - assert "claims a panel but states no lenses" in relabelled - - -def test_the_coverage_line_cannot_be_forged_with_a_newline_or_a_comma() -> None: - """Both fields are free text chosen by whoever ran `--record-review`. - - A newline in `source` split the line in two and left the first half reading - as a completed panel; a comma inside a lens *description* ("correctness, - i.e. does it do what it says") counted as two lenses. Untrusted text is - flattened and the count is deduped case-folded. - """ - pr_watch = _load_pr_watch() - - def _lines(receipt): - report = pr_watch.build_report( - _green_view(), [], set(), review_receipt=receipt - ) - return pr_watch.render(report).splitlines() - - forged = _lines({ - "head": "abc123", - "source": "fallback:panel — 2 lenses (adversarial, correctness)\n (recorded)", - }) - evidence = [l for l in forged if "review evidence" in l] - assert len(evidence) == 1 - assert "claims a panel but states no lenses" in evidence[0] - # No orphaned continuation line masquerading as report output. - assert not any(l.strip().startswith("(recorded)") for l in forged) - - duped = _lines({"head": "abc123", "source": "fallback:codex", - "lenses": ["adversarial", "Adversarial"]}) - assert "ONE lens" in next(l for l in duped if "review evidence" in l) - - -def test_a_case_variant_panel_source_cannot_evade_the_lens_requirement( - monkeypatch: pytest.MonkeyPatch, -) -> None: - """The source-side casefold survived three rounds unpinned. - - Before it, `Fallback:Panel` skipped the check entirely — producing a receipt - that reads as a panel with no warning at all, which is worse than before the - check existed. - """ - pr_watch = _load_pr_watch() - - for variant in ("fallback:panel", "Fallback:Panel", "FALLBACK:PANEL"): - with pytest.raises(ValueError, match="two independent lenses"): - _record(monkeypatch, pr_watch, source=variant, lenses="adversarial") - - -def test_an_invented_lens_name_does_not_count_toward_the_panel_floor( - monkeypatch: pytest.MonkeyPatch, -) -> None: - """The lens roster is configuration, not something a caller invents at - record time — which is the one part of the panel claim that IS checkable. - - Names outside the roster are recorded but not counted. Forbidding them - outright broke "two disjoint lenses is the floor, not the ceiling": a - genuine third ad-hoc lens was refused. - """ - pr_watch = _load_pr_watch() - - with pytest.raises(ValueError, match="of the configured roster"): - _record(monkeypatch, pr_watch, - lenses="correctness, i.e. does it do what it says") - - receipt, _ = _record(monkeypatch, pr_watch, lenses="Adversarial, correctness") - assert receipt["lenses"] == ["Adversarial", "correctness"] - - 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 @@ -2606,23 +2224,84 @@ def test_every_unavailability_line_points_at_the_panel_not_the_degraded_mode() - assert "configured fallback review" not in rendered, label -def test_the_panel_gate_counts_roster_lenses_and_allows_extras( - monkeypatch: pytest.MonkeyPatch, -) -> None: - """See `test_two_lenses_is_the_floor_not_the_ceiling` for why.""" +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() - # Two roster lenses, plus a genuine third: accepted, and all three recorded. - receipt, _ = _record( - monkeypatch, pr_watch, lenses="adversarial,correctness,security" - ) - assert receipt["lenses"] == ["adversarial", "correctness", "security"] + 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 - # One roster lens plus punctuation is still one lens. - with pytest.raises(ValueError, match="of the configured roster"): - _record(monkeypatch, pr_watch, - lenses="correctness, i.e. does it do what it says") - # Two invented names cannot fake a panel. - with pytest.raises(ValueError, match="of the configured roster"): - _record(monkeypatch, pr_watch, lenses="security,performance") +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 From baa68cb9159e59e0f4a643ffc778ac8c4d1ad6a3 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sun, 26 Jul 2026 02:26:38 +0300 Subject: [PATCH 08/10] Deleting the gate reintroduced a bug it was masking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 6, adversarial lens (isolated worktree). Two real findings, one of them a regression the previous commit caused. **`--lenses` splits on `,`, which is also ordinary punctuation.** So `--lenses "adversarial, focused on the new merge gate"` — an HONEST way to record one lens — arrived as two entries and rendered "2 lenses claimed", suppressing the one-lens warning that is the entire remaining value of the field after the gate was removed. The deleted roster intersection had been catching this class; deleting it brought the bug back, and the same input is the forgery the previous commit's own message quoted as blocked. It was not. Fixed without reintroducing a roster or a gate: count only entries that look like lens NAMES (`adversarial`, `data-migration`, `lens.one`), not prose. Prose is still recorded verbatim on the receipt; it just does not count toward "how many lenses ran". That fixes the honest case and the forgery together, and it is a property of the value rather than a matcher over a label. **Control characters survived `_flat`.** It collapsed whitespace, and `\x1b` is not whitespace — so ANSI cursor control passed through. That is strictly worse than the newline `_flat` exists to stop: `\x1b[1A\x1b[2K` *erases* lines that already exist, so a receipt could delete the merge blockers printed above it. Fixed in `_flat` and also in `_excerpt`, which is pre-existing on main but worse: it renders a COMMENT BODY, which any account can write on a public repo, and it renders last, so cursor-up there walks over the whole report. `_flat`'s docstring cites `_excerpt` as the convention it copies — copying a defect forward is how a convention stops being one. Also pinned three guards whose reverts passed the entire suite, each cited by a comment or commit message as deliberate: `_flat` on lens names (not only on `source`); the list check that stops a bare string being read as one lens per character; and `_flat` on the SHA in `bots_behind_head`, which the previous commit advertised fixing while the test named for it checked only the wrapper type, never a dict with a non-string value. Corrected a hook comment that justified its behaviour by a refusal the engine no longer performs. Tests 314 → 317. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --- kit-manifest.json | 2 +- scripts/hooks/pr_followup_hook.py | 7 +- scripts/pr_watch.py | 43 ++++++++++-- scripts/tests/test_pr_watch.py | 105 ++++++++++++++++++++++++++++++ 4 files changed, 149 insertions(+), 8 deletions(-) diff --git a/kit-manifest.json b/kit-manifest.json index 61b6137..2d47f9a 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -103,7 +103,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "69a9ea73e7b460e985c0d5a3b77b4028f9fd3ff75ba661393c395600ba6a5de3" + "sha256": "e9d3e1cd01ba6ccc081e3241a787226789fb0d03b7aebd26ad321885bb944fb1" }, "scripts/reconcile_sessions.sh": { "role": "engine", diff --git a/scripts/hooks/pr_followup_hook.py b/scripts/hooks/pr_followup_hook.py index 68eac88..1898ce7 100755 --- a/scripts/hooks/pr_followup_hook.py +++ b/scripts/hooks/pr_followup_hook.py @@ -127,9 +127,10 @@ def _fallback_instruction( wrong habit every time. """ # Two DISTINCT lenses is the panel's floor (see fallback-review-panel.md). - # A one-lens `fallback_panel` would otherwise have this hook advertise a - # command that `record_review` refuses every single time — so a sub-floor - # config degrades to the single-command wording instead. + # A one-lens `fallback_panel` is not a panel, so advertising one would tell + # the operator to claim coverage they cannot have. (The engine no longer + # refuses such a receipt — that check was deleted as unverifiable, see + # issue #32 — which makes the wording here 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 " diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index be4064f..1294241 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 @@ -1513,6 +1515,22 @@ 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. @@ -1526,12 +1544,29 @@ def _flat(text: object, n: int = 120) -> str: `_excerpt` already established this convention for comment bodies; the receipt fields skipped it. """ - flat = " ".join(str(text).split()) + # 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] + "…" @@ -1837,7 +1872,7 @@ def render(report: dict) -> str: # 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({lens.casefold() for lens in named}) + distinct = len(_countable_lenses(named)) source = _flat(evidence.get("source")) if distinct >= 2: detail = f"{distinct} lenses claimed ({', '.join(named)})" @@ -1901,7 +1936,7 @@ def render_record_review(report: dict) -> str: else [] ) named = [_flat(lens, 40) for lens in named] - if len({lens.casefold() for lens in named}) == 1: + 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" diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index 9cbce82..56286c1 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -2305,3 +2305,108 @@ def test_a_receipt_records_lenses_without_the_engine_judging_them( 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}}} + ) From 59bdb8d0a2a272d54c956db9f2e3446ef3b605bc Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sun, 26 Jul 2026 02:38:19 +0300 Subject: [PATCH 09/10] =?UTF-8?q?Mutation=20testing=20here=20reports=20fal?= =?UTF-8?q?se=20kills=20=E2=80=94=20and=20it=20hid=20three=20unpinned=20gu?= =?UTF-8?q?ards?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 7, correctness lens. Its most valuable finding is about the method, not the code. **`kit_doctor`'s self-check rehashes every kit-owned file, so ANY byte change fails it.** Mutate `pr_watch.py` and that test goes red regardless of behaviour — so a mutation run reports "killed" when nothing behavioural caught anything. The lens's first pass here reported 17/17 killed; re-run with that test excluded, 7 had survived. I verified it directly: disabling `_flat`'s truncation entirely fails only the manifest test. That invalidates confidence from earlier rounds on this branch, and contract item 5 of the doc this PR ships said "mutation-test new branches" without warning about it. It now does. **The deletion took three tests that pinned LIVE code**, not the gate: - `test_the_cli_threads_lenses_through_to_the_receipt` covered the CLI seam. Without it, `lenses=args.lenses` → `None` in `main()` passed all 314 tests — `--lenses`, this change's entire engine surface, was unpinned end to end. - the hand-edited-receipt test pinned three type-guards; each could be removed with the suite green. Without the `isinstance(..., list)` check, a hand-edited `"lenses": "adversarial"` renders as `8 lenses claimed (a, d, v, e, r, s, …)`. - the stale-receipt test pinned report level; its replacement only pinned the render, leaving `review_evidence.lenses` readable off an old receipt in the `--json` payload. All three restored, and the `bots_behind_head` test now exercises the level that actually crashed — a dict with a non-string VALUE, not junk at the map level, which is what the previous commit advertised fixing while the test named for it checked only the wrapper. Re-ran the mutation battery with the drift test excluded: 7/7 caught by named behavioural tests. Also: `_flat`'s docstring quoted a warning string deleted two commits ago (a verbatim recurrence of a defect an earlier round fixed in the same docstring); two hook comments cited a refusal the engine no longer performs; a workflow doc still pointed at "the configured fallback" where the branch's own test pins all three engine sites; the new poll line was undocumented in the Notes reference that documents every other render line; and `init.sh` now ignores `.claude/worktrees/` for adopters, since contract item 7 tells them to give each lens its own worktree and the kit had protected only itself. Tests 317 → 320. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --- docs/agentic-dev-kit/fallback-review-panel.md | 11 ++- docs/agentic-dev-kit/workflows/pr-watch.md | 18 +++- init.sh | 3 + kit-manifest.json | 6 +- scripts/hooks/pr_followup_hook.py | 12 +-- scripts/pr_watch.py | 4 +- scripts/tests/test_pr_watch.py | 82 +++++++++++++++++++ 7 files changed, 122 insertions(+), 14 deletions(-) diff --git a/docs/agentic-dev-kit/fallback-review-panel.md b/docs/agentic-dev-kit/fallback-review-panel.md index 8f0365c..d84672c 100644 --- a/docs/agentic-dev-kit/fallback-review-panel.md +++ b/docs/agentic-dev-kit/fallback-review-panel.md @@ -58,6 +58,14 @@ author re-reading their own diff. 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 @@ -93,7 +101,8 @@ author re-reading their own diff. **`--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 configured roster — + 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. diff --git a/docs/agentic-dev-kit/workflows/pr-watch.md b/docs/agentic-dev-kit/workflows/pr-watch.md index 603640b..bf577b5 100644 --- a/docs/agentic-dev-kit/workflows/pr-watch.md +++ b/docs/agentic-dev-kit/workflows/pr-watch.md @@ -156,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 @@ -219,6 +219,20 @@ 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:** - `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 diff --git a/init.sh b/init.sh index 9036277..e21b06a 100755 --- a/init.sh +++ b/init.sh @@ -730,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. diff --git a/kit-manifest.json b/kit-manifest.json index 2d47f9a..d7d105d 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -15,7 +15,7 @@ "files": { "docs/agentic-dev-kit/fallback-review-panel.md": { "role": "doctrine", - "sha256": "421e5eed191375aed8a463a4b1d9a2ce8b52d9a6ea77e81ef026432f6eafaf2c" + "sha256": "cf460b82d9c0e4f55691ca23a77f0a46911af3282d9f317da46b51e81b9227d2" }, "docs/agentic-dev-kit/safety-critical-changes.md": { "role": "doctrine", @@ -27,7 +27,7 @@ }, "docs/agentic-dev-kit/workflows/pr-watch.md": { "role": "workflow", - "sha256": "9d19949ec94d47b2efbdd9d5258fb8a32cc17b48d9cc39faab3202d202a859e7" + "sha256": "c91857aae5525be9cc234fd73c0c3eb8cfd63f74c6dd56e854da63f79262870b" }, "docs/agentic-dev-kit/workflows/session-start.md": { "role": "workflow", @@ -103,7 +103,7 @@ }, "scripts/pr_watch.py": { "role": "engine", - "sha256": "e9d3e1cd01ba6ccc081e3241a787226789fb0d03b7aebd26ad321885bb944fb1" + "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 1898ce7..9db0a20 100755 --- a/scripts/hooks/pr_followup_hook.py +++ b/scripts/hooks/pr_followup_hook.py @@ -94,9 +94,9 @@ def _load_review_config() -> tuple[list[str], str, str, list[str], str]: _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 advertise a panel — - # and a `--record-review` command the engine then refuses — on the - # strength of a config it just failed to load. + # 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, ) @@ -128,9 +128,9 @@ def _fallback_instruction( """ # 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. (The engine no longer - # refuses such a receipt — that check was deleted as unverifiable, see - # issue #32 — which makes the wording here the only thing steering it.) + # 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 " diff --git a/scripts/pr_watch.py b/scripts/pr_watch.py index 1294241..02c30b6 100755 --- a/scripts/pr_watch.py +++ b/scripts/pr_watch.py @@ -1538,8 +1538,8 @@ def _flat(text: object, n: int = 120) -> str: `--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 (adversarial, correctness) - (recorded) — ⚠ claims a panel but states no lenses + 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. diff --git a/scripts/tests/test_pr_watch.py b/scripts/tests/test_pr_watch.py index 56286c1..253f85f 100644 --- a/scripts/tests/test_pr_watch.py +++ b/scripts/tests/test_pr_watch.py @@ -2410,3 +2410,85 @@ def test_receipt_fields_are_flattened_and_type_guarded_in_both_renders() -> None {"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 From 6d564402c644998fc554b497cf88b7157a570669 Mon Sep 17 00:00:00 2001 From: Topi Jarvinen Date: Sun, 26 Jul 2026 02:39:48 +0300 Subject: [PATCH 10/10] Document the gap the lane merge path leaves open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 — which is exactly the path `autonomous-session-playbook.md` argues review independence matters most on. Not fixed here: surfacing it would mean changing a second safety-critical file at the end of a seven-round review, and the real fix is #32's per-lens receipts rather than another render call. Stated as a known gap instead of left implicit. Claude-Session: https://claude.ai/code/session_011KVJzDj7Z2nYkUjqbGgSVp --- docs/agentic-dev-kit/workflows/pr-watch.md | 5 +++++ kit-manifest.json | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/docs/agentic-dev-kit/workflows/pr-watch.md b/docs/agentic-dev-kit/workflows/pr-watch.md index bf577b5..fe57691 100644 --- a/docs/agentic-dev-kit/workflows/pr-watch.md +++ b/docs/agentic-dev-kit/workflows/pr-watch.md @@ -234,6 +234,11 @@ Self-pace on a bounded cadence — don't busy-wait: 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/kit-manifest.json b/kit-manifest.json index d7d105d..bee30d7 100644 --- a/kit-manifest.json +++ b/kit-manifest.json @@ -27,7 +27,7 @@ }, "docs/agentic-dev-kit/workflows/pr-watch.md": { "role": "workflow", - "sha256": "c91857aae5525be9cc234fd73c0c3eb8cfd63f74c6dd56e854da63f79262870b" + "sha256": "358a3eae1277920974a4a2752cfb3112b5cadefc1e9cf5e3399dc57a9aa2912e" }, "docs/agentic-dev-kit/workflows/session-start.md": { "role": "workflow",