-
Notifications
You must be signed in to change notification settings - Fork 0
Make the fallback review a panel, not a command (#26) #31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
7000b05
Make the fallback review a panel, not a command (#26)
27f9f96
Address CodeRabbit: make the panel's claims enforceable, not documented
44792df
The panel gate did not gate: show coverage at merge time instead
57a09ab
The warning fired on every honest receipt, so it exposed nothing
c9c1cd2
Remove the default-lens-roster trap on the unreadable-config path
456b9b7
Two disjoint lenses is the floor, not the ceiling — make that true
30c7948
Delete the lens gate: it could not do what it claimed (#32 filed)
baa68cb
Deleting the gate reintroduced a bug it was masking
59bdb8d
Mutation testing here reports false kills — and it hid three unpinned…
6d56440
Document the gap the lane merge path leaves open
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,158 @@ | ||
| # The fallback review panel | ||
|
|
||
| What to run when your configured review bot can't. Read this whenever | ||
| `pr_watch` reports a reviewer as unavailable, and before recording any | ||
| `fallback:` receipt. | ||
|
|
||
| ## Why a panel and not a command | ||
|
|
||
| `review.fallback_commands.<runtime>` runs the runtime's own review command — in | ||
| the cockpit's context. When the cockpit *authored* the diff, that is the author | ||
| re-reading their own work. It finds things; it does not find the things the | ||
| author is blind to, which are exactly the ones that survive to production. | ||
|
|
||
| `safety-critical-changes.md` rule 2 already says a single-lens verdict "is an | ||
| incomplete review, not a green light". A single command cannot satisfy a rule | ||
| that asks for two disjoint lenses, so the mechanism was violating the doctrine | ||
| it was meant to serve. | ||
|
|
||
| **The evidence is disjointness, not volume.** Run as two fresh-context lenses | ||
| over one merge-gate change, the panel found a stale outage comment that | ||
| reintroduced the exact bug the PR existed to fix; a future-dated clock that | ||
| wedged the gate for 30 days *after* the fix for that class shipped; and a | ||
| section-scoping fix applied to one of three guards in the same function. The two | ||
| lenses overlapped on almost nothing, and the author's own passes had found none | ||
| of it. | ||
|
|
||
| ## The lenses | ||
|
|
||
| Configured in `review.fallback_panel.lenses` — **that is where each lens's | ||
| brief lives**, as its `focus`, and it is what you hand the reviewer. Restating | ||
| the briefs here would give the kit two copies to drift apart, so this section | ||
| describes only what the two shipped lenses are *for*: | ||
|
|
||
| - **adversarial** — starts from "this is wrong" and tries to prove it. | ||
| - **correctness** — starts from "this works" and asks what it *says*. | ||
|
|
||
| They are the two the doctrine names, and they are chosen to overlap as little | ||
| as possible. Add or replace lenses for your own risk profile (a data-migration | ||
| lens, a performance lens): two disjoint lenses is the floor, not the ceiling. | ||
|
|
||
| ## The contract every lens gets | ||
|
|
||
| These are why the panel works. Drop any of them and it degrades toward the | ||
| author re-reading their own diff. | ||
|
|
||
| 1. **Fresh context.** A lens that watched the change being written inherits the | ||
| author's model of it, including the wrong parts. | ||
| 2. **The raw diff, no framing.** Do not tell the lens what the change is *for*, | ||
| what you already fixed, or what you think is risky. That is the anchoring | ||
| the panel exists to escape. | ||
| 3. **Say it did not write the code.** Cheap, and it measurably changes what a | ||
| lens is willing to call wrong. | ||
| 4. **Execute, don't only read.** The highest-value findings came from *running* | ||
| the changed paths against hostile input, not from reading them. A guard was | ||
| dead code for the exact bot it was written for, because that bot reports a | ||
| zero timestamp — no amount of re-reading surfaced it; one live poll did. | ||
| 5. **Mutation-test new branches.** Break the new behaviour deliberately and | ||
| confirm a test fails. Repeatedly, properties were *named* by a test and | ||
| pinned by nothing — hardwiring a branch to a constant still passed the whole | ||
| suite. | ||
|
|
||
| **Beware false kills.** If your repo has a checksum/drift test over the files | ||
| you are mutating (this kit has one: `kit_doctor`'s self-check), *every* | ||
| mutation fails it regardless of behaviour, and a whole run can report 100% | ||
| killed while nothing behavioural caught anything. One lens's first pass here | ||
| reported 17/17 killed; re-run with that test excluded, 7 had survived. | ||
| Exclude the drift test — or regenerate the manifest after each mutation — | ||
| before believing a kill. | ||
| 6. **Report, do not fix.** A lens that edits loses the disjointness: it starts | ||
| defending its own changes on the next round. | ||
| 7. **Mutate in an isolated copy, never the shared tree.** Mutation testing needs | ||
| temporary writes, and lenses run concurrently. Discovered the hard way on the | ||
| PR that added this file: one lens's mutations appeared to the other as an | ||
| external process corrupting the repo, and it "restored" them mid-run — so one | ||
| lens's results were unreliable and the other nearly destroyed live work. Give | ||
| each lens a scratch copy or its own git worktree, and require it to leave the | ||
| shared tree byte-identical. | ||
| 8. **State what was verified clean, and how.** Absence of findings is only | ||
| evidence if you know what was actually checked. | ||
|
|
||
| ## Running it | ||
|
|
||
| 1. Read the change: `git diff <base>...HEAD`. | ||
| 2. Launch **one isolated reviewer per lens**, concurrently, each with the | ||
| contract above and its lens focus. Use whatever isolation your runtime has | ||
| (a subagent, a separate session, a second person). If it has none, see | ||
| *Degraded mode*. | ||
| 3. Triage every finding against the *current* code — some go stale across | ||
| rounds. | ||
| 4. Fix real findings, reply-with-reason to the rest. | ||
| 5. **Re-run the panel after the fix round.** Not optional: see below. | ||
| 6. Record the receipt with the lenses that actually ran: | ||
|
|
||
| ```sh | ||
| uv run <engine-dir>/pr_watch.py <PR#> \ | ||
| --record-review "<review.fallback_panel.receipt_source>" \ | ||
| --lenses <names of the lenses that actually ran> \ | ||
| --head <polled-sha> | ||
| ``` | ||
|
|
||
| **`--lenses` is self-reported, and the engine does not verify it.** You write | ||
| the source and the lens names in one invocation; nothing binds either to a | ||
| review that happened. Four rounds of this PR tried to verify it from the | ||
| engine — matching the source, then the lens names, then a required roster, | ||
| then a counted one — | ||
| and each was defeated, the last by a single extra character in the source, | ||
| after which the render cheerfully affirmed the forgery. | ||
| `safety-critical-changes.md` rule 1 calls that a stopgap, not a fix. | ||
|
|
||
| So what you get is an **audit trail, not a gate**. The poll render states the | ||
| claim, labelled as a claim, on every poll once a current-head receipt exists | ||
| (no receipt, or one bound to an older head, prints nothing): | ||
|
|
||
| ```text | ||
| review evidence: fallback:codex — ⚠ ONE lens claimed (correctness) — not a dual-lens pass | ||
| ``` | ||
|
|
||
| That is genuinely useful — a one-lens pass is visible at merge time instead of | ||
| buried in the record command's stdout — and it is worth exactly what an honest | ||
| operator puts into it. Verifying coverage needs each lens to record its own | ||
| receipt from its own context: issue #32, not this. | ||
|
|
||
| ## Re-running, and when to stop | ||
|
|
||
| `safety-critical-changes.md` rule 3 says to re-review after every fix round | ||
| "until a full pass finds nothing new". Take that literally *and* know its limit: | ||
| across one session of 13 rounds on three PRs, **every round found something**, | ||
| and seven of those findings were defects introduced by the *previous round's | ||
| fix*. The termination condition may never arrive. | ||
|
|
||
| So the stopping criterion is **blast radius, not round count**: | ||
|
|
||
| - A **gate, send path, or destructive operation** — keep going. Worst case is an | ||
| unreviewed change landing, and the doctrine's operator-merge rule applies. | ||
| - Something **reported but never acted on** (a warning, a log line, a report | ||
| field) — a couple of rounds with the findings decaying in severity is | ||
| proportionate. Worst case is a wrong message. | ||
|
|
||
| Say which one you applied **in the PR**, where a human reads it — the receipt | ||
| carries what the review did *not* cover (`override`, `bot_signal`, | ||
| `bots_behind_head`) and what it did (`lenses`) — not a prose rationale for | ||
| stopping. "The last round found | ||
| nothing" is not available as a reason if it never happened. | ||
|
|
||
| ## Degraded mode | ||
|
|
||
| If the runtime cannot run isolated reviewers, fall back to | ||
| `review.fallback_commands.<runtime>` — one lens, in the author's context. It is | ||
| better than nothing and it is **not** a panel: | ||
|
|
||
| - record it as `fallback:<runtime>`, never the panel's `receipt_source`. The | ||
| engine will not stop you doing otherwise — it cannot tell what ran. The | ||
| honesty is entirely yours, which is the reason to write it down | ||
| - pass `--lenses` naming what actually ran, so the audit trail shows one lens | ||
| - for anything under `safety-critical-changes.md`, say plainly in the PR that | ||
| rule 2 was not satisfied | ||
|
|
||
| A receipt should never claim more coverage than the review it stands for. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.