feat: assemble panel launch prompts instead of hand-authoring them (#214) - #219
Conversation
) `scripts/panel_prompt.py` renders a lens's launch prompt from the doctrine and config rather than from the cockpit's memory of both. Three properties, each earned by a failure #214 records: The contract is QUOTED, never restated. The script parses it out of fallback-review-panel.md at run time and emits the section verbatim, so no sub-requirement can be dropped in transit and no second copy can drift. That is the argument the doctrine already makes about lens briefs, applied to itself. A test mutates the doctrine and asserts the parsed contract moves with it, so inlining the contract for speed fails the suite. The base is not an input on the default path. It is resolved from the remote every run, because a stale base yields a large, non-empty, entirely plausible wrong diff that satisfies every other check the doctrine prescribes. `--base` exists for reviewing historical ranges and the prompt then says plainly that the base was author-supplied and not remote-resolved — pinned in both directions, because that label is the one self-referential claim the prompt makes and it would be false on exactly one code path if hardcoded. Same inputs, same prompt. Round-to-round framing is then a deliberate `--carry-forward` addition rather than variance in what the contract delivered. That flag carries the lever measured on #218: across five rounds, what moved finding-yield was what the prompt aimed lenses at, not how large the pass was, and that carry-forward had no home except an author remembering to type it. It refuses rather than emitting a misleading prompt: empty diff, lens outside the configured roster, head that is not a commit, renamed contract heading, or a contract that parses to zero items. Exit 2 in every case. No config key holds the verification command, so `--verify-command` is passed or omitted, never guessed — a wrong one sends a lens down the exact "tests cannot run here" path this kit's CLAUDE.md exists to prevent. Registered in KIT_OWNED and kit-manifest.json by hand. Nothing in the suite fails if a future engine forgets that; #216 holds the guard that would. Verified: `make test` — 642 passed (630 before, 12 new). `kit_doctor.py` — 32 unchanged, 0 differ, 0 missing, 0 unknown. `ruff check` — all checks passed. Claude-Session: https://claude.ai/code/session_01PJUnxLwWJTfQg1qZDsm7h3
📝 WalkthroughWalkthroughAdds ChangesFallback panel prompt
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant build
participant Git
participant Doctrine
participant render
CLI->>build: Parse lens and revision options
build->>Git: Resolve and validate base and head
build->>Doctrine: Extract the review contract
build->>render: Pass validated review context
render-->>CLI: Return the generated prompt
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/panel_prompt.py`:
- Around line 90-98: Update the contract-section lookup in
scripts/panel_prompt.py to match CONTRACT_HEADING only as an actual level-two
Markdown heading, anchored at the start of a line, rather than using text.find.
Derive body_start from the matched heading’s end and preserve the existing
next-level-two-heading boundary. Add regression tests covering both a
level-three heading and a prose occurrence, ensuring each is rejected.
In `@scripts/tests/test_panel_prompt.py`:
- Around line 48-57: The _two_commits helper currently assumes git log provides
distinct commits, producing an empty-diff pair in shallow CI checkouts. Update
the test fixture or checkout setup so prompt-success tests always receive two
distinct commits with a non-empty diff, while retaining the same-SHA pair only
in test_an_empty_diff_is_refused.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 17991135-4915-4a24-a489-742b3af788ca
📒 Files selected for processing (4)
kit-manifest.jsonscripts/kit_doctor.pyscripts/panel_prompt.pyscripts/tests/test_panel_prompt.py
…ient history CI failed where `make test` passed locally, and the engine was right both times. `_two_commits()` read `git log` from this repo for a base/head pair. Under CI's shallow checkout that returns one commit, so base == head, and panel_prompt.py correctly refused the empty diff — the failure it exists to produce. The tests were the defect; depending on ambient history is what made them one, and it is the same shape as #134: a test that passes only in the layout its author happened to have. The engine now takes `--root` and threads it through every git call, the config load, and the doctrine lookup, the way kit_doctor.py already does. That is better design independent of the tests — an engine that can only ever review the checkout it is installed in is narrower than it needs to be — and it is what lets the tests build a fixture repo with its own two commits instead of borrowing this one's. The fixture writes the REAL doctrine into that repo, so the contract these tests assert on is the shipped one rather than a convenient stub. Verified: the tests now pass in a `--depth 1` clone, which is what CI actually checks out. `make test` — 642 passed. `kit_doctor.py` — 32 unchanged, 0 differ, 0 missing, 0 unknown. `ruff check` — passed. Claude-Session: https://claude.ai/code/session_01PJUnxLwWJTfQg1qZDsm7h3
…the doctrine
CodeRabbit, Major, and correct.
`text.find("## The contract every lens gets")` also matches inside
`### The contract every lens gets` — at offset 1 — and matches the phrase quoted
anywhere in prose. Either would have sliced a subsection or a sentence out of the
doctrine and emitted it to every lens as the contract, which is the exact failure
this engine exists to prevent: a prompt that looks complete and is not.
Now line-anchored in both directions. The opening heading must be `## ...` alone
on its line, and the section is bounded by the next level-two heading rather than
the next `\n## ` substring, so a `###` subsection inside the contract no longer
truncates it.
Three regression tests, each pinning a case the old code accepted: a level-three
heading, the phrase quoted in prose, and a `###` subsection that must not
truncate. The discrimination is evidenced rather than assumed — under the old
`str.find` the level-three decoy matched at offset 1, so it returned a section
where these tests now require a refusal.
Verified: `make test` — 645 passed (642 before, 3 new). `kit_doctor.py` — 32
unchanged, 0 differ, 0 missing, 0 unknown. `ruff check` — passed.
Claude-Session: https://claude.ai/code/session_01PJUnxLwWJTfQg1qZDsm7h3
|
Both CodeRabbit findings actioned.
|
…+3 more
Adversarial panel lens, four findings. All acted on — this is an engine, so
first class.
HIGH, regression. `git rev-parse --abbrev-ref HEAD` returns the literal string
"HEAD" on a detached checkout, and the prompt rendered `**Branch:** HEAD` at
exit 0. That is a plausible-looking lie in a field the contract requires the
prompt to carry, and detached is not an edge case: it is the state of every
worktree built at a pinned sha for review, and of a default CI PR checkout. The
lens reproduced it at the then-current head, so neither earlier fix touched it.
It now refuses and asks for --branch, which is the documented escape.
The lens established it was unguarded by mutation rather than by reading:
hardwiring the branch to a constant passed all 641 tests. "branch" appeared once
in the test file, as an unrelated literal. That is contract item 5's own example
— a property named by nothing.
MED-HIGH. The write-safety instruction handed to a lens given no worktree
("do not write into any tree you were handed") was pinned by no test and
inverted cleanly to "go ahead and mutate freely" with 641 passing. #136 is why
that matters: one lens nearly destroyed live work.
LOW. The repo-slug regex kept only the last two path segments, so a GitLab
subgroup or Bitbucket project rendered a wrong-but-plausible repo. Now keeps
every segment, for both scp-style and URL remotes.
LOW. "carries N items" counted regex matches without checking the ordinals were
1..N, so a renumbering slip left the count intact while the doctrine read wrong
— false assurance from the line whose whole purpose is assurance. Now refuses.
Ten new tests. Each of the three code fixes was mutation-confirmed in an isolated
clone: hardwiring the branch, inverting the write-safety string, and restoring
the two-segment slug each now fail behaviour tests that previously passed.
Verified: `make test` — 656 passed (645 before). `kit_doctor.py` — 32 unchanged,
0 differ, 0 missing, 0 unknown. `ruff check` — passed.
Claude-Session: https://claude.ai/code/session_01PJUnxLwWJTfQg1qZDsm7h3
Round 1 result, and round 2's stopping criterion declared before it runsRound 1 ran against Findings — 6, all acted on (this is an engine, so first class)
Finding 3 is the one that matters. Detached is not an edge case — it is the state of every worktree built at a pinned sha for review, and of a default CI PR checkout. The lens proved it unguarded by mutation, not reading: hardwiring the branch to a constant passed all 641 tests, and the word "branch" appeared once in the test file as an unrelated literal. That is contract item 5's own worked example. Each of the three code fixes is now mutation-confirmed in an isolated clone: hardwiring the branch, inverting the write-safety string, and restoring the two-segment slug each fail behaviour tests that previously passed. Two process notes
Round 2 criterion, fixed in advanceFull panel at Blast-radius class: second (emits text to stdout; gates nothing, sends nothing, destroys nothing, not under |
… defects Round 2 of the panel. Both lenses found gaps that round 1's fixes left open on the exact properties those fixes targeted — the remediation-carries-the-next- defect shape this repo keeps measuring. HIGH, adversarial. The detached-HEAD guard did not close its class. It refused only the literal string "HEAD", but scripts/dev_session.sh builds lanes with `git worktree add -b`, so a review tree can sit on a real branch that is not the branch under review. That rendered a real-looking lane branch name at exit 0 — strictly worse than "HEAD", which at least reads as a placeholder. The doctrine's own "No writes in the tree you were given" names that pattern, and this engine quotes it verbatim while contradicting it. The checkable property, which the first fix missed: an auto-detected branch is the branch under review only if ITS TIP IS THE HEAD under review. Anything else is a guess. Both cases now refuse and ask for --branch. MEDIUM, correctness. The escape hatch admitted the value it exists to refuse: `--branch HEAD` passed straight through and rendered the identical lie. git forbids a branch by that name, so refusing it rejects nothing valid. LOW-MEDIUM, correctness. `base_from_remote` tested `is None` while the value used `or`, so `--base ""` resolved from the remote while the prompt said the author supplied it. The provenance label was claimed "pinned in both directions" and one direction had a gap. Both now agree, and an empty --base is refused. MED-HIGH, adversarial. resolve_base — the function carrying the property the docstring names as this script's reason for existing — had zero test coverage, and `git ls-remote` takes a PATTERN: a base branch containing a glob matched several refs and the first was taken silently. Now requires exactly one match, with three tests against a local bare remote. Also: a remote URL with no path rendered the bare host as the repo, and five CLI overrides used truthiness so an empty string was indistinguishable from omitted. A correction to the previous commit message: it said "Ten new tests". The delta was ELEVEN — one of the seven new functions is parametrized five ways. Caught by the correctness lens re-deriving it from `--collect-only` rather than reading the claim. No count appears in this message; the suite total below is generated. Each new guard is mutation-confirmed in an isolated clone: removing the tip check, and restoring resolve_base's silent first-match, each fail a behaviour test that previously passed. Verified: `make test` — 665 passed. `kit_doctor.py` — 32 unchanged, 0 differ, 0 missing, 0 unknown. `ruff check` — passed. Claude-Session: https://claude.ai/code/session_01PJUnxLwWJTfQg1qZDsm7h3
Round 2 result — 7 findings, every one inside round 1's own repairsBoth lenses spent their cycles where the carry-forward pointed them: at what the previous round's fixes broke or left half-done. That is where all seven were.
Finding 7 is the one worth reading. My fix refused the literal The property the first fix missed, and the second uses: an auto-detected branch is the branch under review only if its tip is the head under review. Anything else is a guess. Finding 13 is the measurement one. I wrote "Ten new tests"; it was eleven, because one function is parametrized five ways. The lens caught it by re-deriving from Both new guards are mutation-confirmed in an isolated clone: removing the tip check, and restoring Round 3 criterion, fixed in advanceFull panel at Round 1 → 6 findings, round 2 → 7. Not converging on count yet; converging on location, which is the signal I am watching: round 1 found defects in the original engine, round 2 found none there and seven in the repairs. Blast-radius class: second. Prose class: executed. Both draws mine, both disputable. |
…ng nothing Round 3. Three rounds running, every finding has been in the previous round's repair rather than in the original engine. HIGH, adversarial. `--base-branch ""` fell back to the config default silently, using the identical `or`-on-a-string shape hardened for `--base` and `--branch` one commit earlier. Third round, third flag, same defect — because each was fixed as an instance. So this fixes the CLASS. Every optional override now goes through one `_override` validator: passed-but-empty is an error, everywhere, and a parametrized test covers all six flags. Fixing them one at a time is how the fourth one survives. MED-HIGH, correctness. `test_a_head_that_is_not_a_commit_is_refused` named a property and pinned nothing — the repo's signature failure, in my own test. Replacing `_require_commit`'s body with a bare passthrough passed all 35 tests, because the refusal arrived from the branch-tip check for an unrelated reason and the test asserted only the exit code. `_require_commit`'s real job is normalisation, not refusal: it turns a branch name or short sha into a full one. Unpinned, a future edit could render a moving branch name where the prompt promises a pinned sha, which Right revision calls reason enough on its own. Now pinned three ways — the refusal message, an abbreviated sha, and a branch name — and the passthrough mutation kills all three. MEDIUM, adversarial. `_repo_slug`'s no-path fallback missed local remotes, so `file:///Users/topi/Coding/kit.git` rendered `Users/topi/Coding/kit` — reads exactly like an org/repo. This kit's own tests set a local origin, so not hypothetical. MEDIUM, adversarial. An overridden `--branch` rendered with no caveat while every neighbouring fact in the prompt is git-verified. `--base`'s override already carried one. A tip-check would be wrong here — the override exists to carry ground truth the reviewing tree lacks — so the fix is the caveat, not verification. An earlier version of this message said 665 passed. It was written before the run and was stale on arrival — the same class this PR has now corrected twice. The figures below are generated by shell substitution at this head. Verified: `make test` — 675 passed. `scripts/tests/test_panel_prompt.py` — 45 tests. `kit_doctor.py` — 32 unchanged, 0 differ, 0 missing, 0 unknown. `ruff check` — passed. Claude-Session: https://claude.ai/code/session_01PJUnxLwWJTfQg1qZDsm7h3
f524ba1 to
973460e
Compare
Round 3 result — 5 findings, and the pattern is now the finding
The finding is the pattern, not any single itemThree rounds, and every finding has been in the previous round's repair rather than in the original engine. Finding 14 is the third consecutive round where I fixed instances of a defect and a sibling survived: So round 3 stopped fixing instances. Every optional override now routes through one Finding 15 is the sharper one because it is this repo's signature failure landing in my own test: an assertion on Two corrections to my own record
Round 4 criterion, fixed in advanceFull panel at Six lens runs so far on this PR. Blast-radius class: second. Prose class: executed. Both draws mine, both disputable. |
… too Round 4, which was aimed at the question three rounds of fail-closed guards had made the live one: not "does it emit something wrong" but "does it now refuse something right." Both findings are that. HIGH, adversarial, regression. `resolve_base` reads the sha from the remote with `ls-remote`, which transfers no objects. A shallow or single-branch clone — `git clone --depth 1`, and what `actions/checkout` does by default — then holds the feature branch's history but not the base's, so validating the base locally failed on a base that is provably current. The engine rejected an entirely legitimate invocation, and its message read as staleness or corruption while naming no remedy. The refusal was right to happen; the message was wrong. It now distinguishes "object not fetched" from "not a commit" and names the exact fetch. This is the shape the engine's own comments cite as the reason its tests were rewritten — CI checks out shallow — so it would have bitten the first adopter who ran it anywhere but a full clone. MEDIUM, adversarial, regression. `--runtime` was left out of round 3's override sweep while `_override`'s docstring claimed "every optional flag here goes through this". `--runtime ""` and a mistyped `--runtime clade` both silently dropped the compute directive, so a lens ran at whatever the ambient session defaults to rather than the configured model and effort — values the config comments record as measured from real panel data, not guessed. An unconfigured runtime legitimately means "inherit the cockpit's compute"; the doctrine says so. A typo looked identical. The prompt now says which runtime it looked for, which makes the two distinguishable by a reader who can act on it. One test-fixture defect worth recording: the first version of the shallow-clone test was not shallow. git treats a plain local path as a hardlink clone and silently ignores --depth, so the fixture proved nothing. `file://` forces the transport, and the test asserts the base object is genuinely absent before exercising the guard. Verified: `make test` — 679 passed. `scripts/tests/test_panel_prompt.py` — 49 tests. `kit_doctor.py` — 32 unchanged, 0 differ, 0 missing, 0 unknown. `ruff check` — passed. Figures generated at this head. Claude-Session: https://claude.ai/code/session_01PJUnxLwWJTfQg1qZDsm7h3
Round 4 result — 3 findings; the premise heldRound 4's prompts inverted the question: after three rounds of accumulating fail-closed guards, the live risk was no longer "does it emit something wrong" but "does it now refuse something right." Two of the three findings are exactly that.
Finding 19 is the one that would have reached an adopter
The refusal was right to happen; the message was wrong. It now distinguishes "object not fetched" from "not a commit" and names the exact One fixture defect worth recording: my first shallow-clone test was not shallow. git treats a plain local path as a hardlink clone and silently ignores Finding 21 — logged, not fixedThe code at the reviewed head is correct; round 3's unified It is worth naming as its own class: a commit that corrects an earlier claim while making a new one of the same kind. Three of the five flags it named were not fixed until a round later. Round 5 criterion, fixed in advanceFull panel at Eight lens runs so far. Blast-radius class: second. Prose class: executed. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
scripts/tests/test_panel_prompt.py (2)
516-518: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
--runtimeto the override sweep.The docstring states that each round found this defect in a different flag.
--runtimeis a string override validated by_override, but it is absent from this list. A separate test covers it today, so the sweep is the place that keeps the class closed.♻️ Proposed change
`@pytest.mark.parametrize`( - "flag", ["--branch", "--base", "--base-branch", "--scratch", "--carry-forward", "--verify-command"] + "flag", + [ + "--branch", "--base", "--base-branch", "--scratch", + "--carry-forward", "--verify-command", "--runtime", + ], )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/test_panel_prompt.py` around lines 516 - 518, Update the flag list in the parametrized override sweep to include “--runtime”, keeping the existing flags unchanged so the sweep covers every string override validated by _override.
49-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSurface git's stderr when a fixture command fails.
check=Truewithcapture_output=TrueraisesCalledProcessErrorwhose message omits the captured stderr. A fixture failure then reports only the exit code. Include stderr in the failure.♻️ Proposed change
def _git(root: Path, *args: str) -> str: - return subprocess.run( - ["git", *args], cwd=root, capture_output=True, text=True, check=True - ).stdout.strip() + proc = subprocess.run(["git", *args], cwd=root, capture_output=True, text=True) + assert proc.returncode == 0, f"git {' '.join(args)} failed: {proc.stderr.strip()}" + return proc.stdout.strip()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/test_panel_prompt.py` around lines 49 - 52, Update the _git helper to expose captured stderr when subprocess.run fails: catch CalledProcessError around the existing git invocation and include its stderr in the raised failure while preserving successful stdout stripping and check=True behavior.scripts/panel_prompt.py (2)
77-87: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a timeout on the git subprocess.
_gitrunsls-remote, which contacts the remote. Without a timeout, an unreachable remote makes the CLI hang instead of failing. Add a boundedtimeoutand convertsubprocess.TimeoutExpiredinto aPromptError.♻️ Proposed change
def _git(root: Path, *args: str) -> str: - proc = subprocess.run( - ["git", *args], - cwd=root, - capture_output=True, - text=True, - check=False, - ) + try: + proc = subprocess.run( + ["git", *args], + cwd=root, + capture_output=True, + text=True, + check=False, + timeout=60, + ) + except subprocess.TimeoutExpired as exc: + raise PromptError(f"git {' '.join(args)} timed out after 60s") from exc if proc.returncode != 0: raise PromptError(f"git {' '.join(args)} failed: {proc.stderr.strip()}") return proc.stdout.strip()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/panel_prompt.py` around lines 77 - 87, Update _git to pass a bounded timeout to subprocess.run, and catch subprocess.TimeoutExpired to raise PromptError with clear context instead of allowing the CLI to hang or leak the subprocess exception. Preserve the existing nonzero-return handling and successful stdout behavior.
496-503: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReport a missing doctrine file as a
PromptError.
contractcallsdoctrine_path.read_text()without an existence check. If--rootpoints at a checkout that lacks the doctrine, the CLI exits with aFileNotFoundErrortraceback instead of theerror: ...line and exit code 2. CatchOSErrorinmain, or raise aPromptErrorincontractwhen the file is absent.♻️ Proposed change
try: sys.stdout.write(build(args)) except PromptError as exc: print(f"error: {exc}", file=sys.stderr) return 2 + except OSError as exc: + print(f"error: {exc}", file=sys.stderr) + return 2 return 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/panel_prompt.py` around lines 496 - 503, Update the CLI error handling around main and contract so a missing or unreadable doctrine file is converted to PromptError rather than exposing FileNotFoundError. Catch the relevant OSError in main or validate doctrine_path before read_text() in contract, preserving the existing error: output and exit code 2 behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/panel_prompt.py`:
- Around line 317-322: Update the compute settings comprehension in the prompt
construction to exclude only values that are None, preserving configured falsy
values such as 0 and false in parts and the rendered compute_line.
- Around line 271-278: Update the PromptError message in the base-validation
flow so the “sha is current” claim appears only when from_remote is true. For
author-supplied --base values, use wording that does not assert currency or
attribute the problem to clone depth, while preserving the existing
remote-resolution message and fetch guidance.
- Line 450: Update the repository slug resolution around _repo_slug and the git
config lookup so a missing origin remote is handled explicitly when --base skips
resolve_base. Detect the failed empty result from _git(root, "config", "--get",
"remote.origin.url") and report the missing origin remote directly instead of
propagating the generic _git failure.
In `@scripts/tests/test_panel_prompt.py`:
- Line 618: Update the assertion in the test around the message variable msg to
require the specific expected phrase “the sha is\ncurrent”; remove the broad
“current” substring alternative so unrelated message text cannot satisfy the
test.
- Around line 270-278: Update the tests around the without and out results in
test_the_verification_command_is_never_guessed and the preceding adversarial
prompt test to assert a successful exit code before checking negative stdout
assertions. Preserve the existing absence checks after validating the command
completed successfully.
---
Nitpick comments:
In `@scripts/panel_prompt.py`:
- Around line 77-87: Update _git to pass a bounded timeout to subprocess.run,
and catch subprocess.TimeoutExpired to raise PromptError with clear context
instead of allowing the CLI to hang or leak the subprocess exception. Preserve
the existing nonzero-return handling and successful stdout behavior.
- Around line 496-503: Update the CLI error handling around main and contract so
a missing or unreadable doctrine file is converted to PromptError rather than
exposing FileNotFoundError. Catch the relevant OSError in main or validate
doctrine_path before read_text() in contract, preserving the existing error:
output and exit code 2 behavior.
In `@scripts/tests/test_panel_prompt.py`:
- Around line 516-518: Update the flag list in the parametrized override sweep
to include “--runtime”, keeping the existing flags unchanged so the sweep covers
every string override validated by _override.
- Around line 49-52: Update the _git helper to expose captured stderr when
subprocess.run fails: catch CalledProcessError around the existing git
invocation and include its stderr in the raised failure while preserving
successful stdout stripping and check=True behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 17213af3-98fa-46f1-9a33-30a8c48922ad
📒 Files selected for processing (3)
kit-manifest.jsonscripts/panel_prompt.pyscripts/tests/test_panel_prompt.py
🚧 Files skipped from review as they are similar to previous changes (1)
- kit-manifest.json
| parts = [f"{k} {v}" for k, v in sorted(compute.items()) if v] if compute else [] | ||
| compute_line = ( | ||
| f"\nRun at: {', '.join(parts)}.\n" | ||
| if parts | ||
| else f"\nNo compute configured for runtime {runtime!r}; inherit the cockpit's.\n" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
if v drops falsy compute values.
A configured setting such as temperature: 0 or thinking: false is falsy. The filter removes it, and the prompt then renders compute settings that do not match the configuration. Filter on None instead.
🐛 Proposed fix
- parts = [f"{k} {v}" for k, v in sorted(compute.items()) if v] if compute else []
+ parts = [f"{k} {v}" for k, v in sorted(compute.items()) if v is not None] if compute else []📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| parts = [f"{k} {v}" for k, v in sorted(compute.items()) if v] if compute else [] | |
| compute_line = ( | |
| f"\nRun at: {', '.join(parts)}.\n" | |
| if parts | |
| else f"\nNo compute configured for runtime {runtime!r}; inherit the cockpit's.\n" | |
| ) | |
| parts = [f"{k} {v}" for k, v in sorted(compute.items()) if v is not None] if compute else [] | |
| compute_line = ( | |
| f"\nRun at: {', '.join(parts)}.\n" | |
| if parts | |
| else f"\nNo compute configured for runtime {runtime!r}; inherit the cockpit's.\n" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/panel_prompt.py` around lines 317 - 322, Update the compute settings
comprehension in the prompt construction to exclude only values that are None,
preserving configured falsy values such as 0 and false in parts and the rendered
compute_line.
| without = _run(repo, "--lens", "adversarial", "--head", head, "--base", base) | ||
| assert "What prior rounds have and have not covered" not in without.stdout | ||
|
|
||
|
|
||
| def test_the_verification_command_is_never_guessed(repo): | ||
| """No config key holds it, so an unset command must be omitted, not invented.""" | ||
| base, head = _revs(repo) | ||
| out = _run(repo, "--lens", "adversarial", "--head", head, "--base", base) | ||
| assert "verification command" not in out.stdout |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Negative stdout assertions pass vacuously if the run fails.
without at Line 270 and out at Line 277 assert that text is absent from stdout. If the engine exits 2, stdout is empty and both assertions pass. Assert the exit code first.
🐛 Proposed fix
without = _run(repo, "--lens", "adversarial", "--head", head, "--base", base)
+ assert without.returncode == 0, without.stderr
assert "What prior rounds have and have not covered" not in without.stdout out = _run(repo, "--lens", "adversarial", "--head", head, "--base", base)
+ assert out.returncode == 0, out.stderr
assert "verification command" not in out.stdout📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| without = _run(repo, "--lens", "adversarial", "--head", head, "--base", base) | |
| assert "What prior rounds have and have not covered" not in without.stdout | |
| def test_the_verification_command_is_never_guessed(repo): | |
| """No config key holds it, so an unset command must be omitted, not invented.""" | |
| base, head = _revs(repo) | |
| out = _run(repo, "--lens", "adversarial", "--head", head, "--base", base) | |
| assert "verification command" not in out.stdout | |
| without = _run(repo, "--lens", "adversarial", "--head", head, "--base", base) | |
| assert without.returncode == 0, without.stderr | |
| assert "What prior rounds have and have not covered" not in without.stdout | |
| def test_the_verification_command_is_never_guessed(repo): | |
| """No config key holds it, so an unset command must be omitted, not invented.""" | |
| base, head = _revs(repo) | |
| out = _run(repo, "--lens", "adversarial", "--head", head, "--base", base) | |
| assert out.returncode == 0, out.stderr | |
| assert "verification command" not in out.stdout |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/tests/test_panel_prompt.py` around lines 270 - 278, Update the tests
around the without and out results in
test_the_verification_command_is_never_guessed and the preceding adversarial
prompt test to assert a successful exit code before checking negative stdout
assertions. Preserve the existing absence checks after validating the command
completed successfully.
| msg = str(exc.value) | ||
| assert "not in this clone" in msg | ||
| assert "git fetch origin main" in msg, "the error must name the remedy" | ||
| assert "the sha is\ncurrent" in msg or "current" in msg |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This assertion cannot fail meaningfully.
The or "current" in msg disjunct matches any message containing the substring "current". The word appears in unrelated wording, so the assertion pins nothing. Assert the specific phrase.
🐛 Proposed fix
- assert "the sha is\ncurrent" in msg or "current" in msg
+ assert "the sha is\ncurrent" in msg or "the sha is current" in msg📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert "the sha is\ncurrent" in msg or "current" in msg | |
| assert "the sha is\ncurrent" in msg or "the sha is current" in msg |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/tests/test_panel_prompt.py` at line 618, Update the assertion in the
test around the message variable msg to require the specific expected phrase
“the sha is\ncurrent”; remove the broad “current” substring alternative so
unrelated message text cannot satisfy the test.
…r established
Round 5.
HIGH, adversarial, regression. `--root` was the one optional flag outside the
`_override` sweep, and argparse hid it: declared `type=Path`, an empty string
becomes `PosixPath('.')`, which is **truthy**, so `args.root or REPO_ROOT` never
fell through to the documented default. `--root ""` did not refuse and did not
fall back — it silently retargeted the review at whatever directory the process
was standing in. A silent wrong-repo review is the worst outcome available to
this engine, and it is the failure `_override` exists to prevent, on the one flag
`_override` could not reach because of a type coercion.
`--root` is now validated as a string before conversion. `--head` and `--lens`
are swept too: argparse's `required=True` guarantees presence, not content, and
an empty `--head` reached git as a blank revision.
MEDIUM, adversarial, regression introduced by round 4. `_require_base_object`
asserted "the sha is current" on both paths. That is earned when the sha was read
off the remote's ref tip moments earlier; it is not earned for an author-supplied
`--base`, which nothing verifies. A fat-fingered sha was confidently told to run
a fetch that could not possibly help — the same class round 4's own message
described fixing, left half-done on the sibling path it introduced. The render
already calls an author-supplied base unverified; the refusal now agrees with it.
Both fixes mutation-confirmed in an isolated clone: restoring the `Path` coercion,
and collapsing the base message back to always-current, each fail behaviour tests
that previously passed.
Logged, not fixed: an empty `--head` previously produced a message with a double
space where the revision should be. Cosmetic, and now unreachable anyway.
Verified: `make test` — 685 passed. `scripts/tests/test_panel_prompt.py` — 55 tests.
`kit_doctor.py` — 32 unchanged, 0 differ, 0 missing, 0 unknown. `ruff check` — passed.
Figures generated at this head.
Claude-Session: https://claude.ai/code/session_01PJUnxLwWJTfQg1qZDsm7h3
…othing Round 5, correctness. `_require_base_object`'s "exists but is not a commit" branch — advertised by its own docstring and by round 4's commit message as the discrimination that fix existed to add — had zero coverage. Deleting it entirely failed no test, and a tree sha passed as `--base` would then have been reported as "not fetched", with a fetch command that cannot help. Pinned now, and the deletion mutation fails it. This is the fourth time in this PR that a property named by a commit message was pinned by nothing. The pattern is worth more than the individual fixes: writing the claim and writing the test are separate acts, and only one of them was happening reliably. Verified: `make test` — 686 passed. `scripts/tests/test_panel_prompt.py` — 56 tests. `kit_doctor.py` — 32 unchanged, 0 differ, 0 missing, 0 unknown. Figures generated at this head. Claude-Session: https://claude.ai/code/session_01PJUnxLwWJTfQg1qZDsm7h3
Round 5 result, and a bound on this loop declared before round 6 runs
Finding 22 is the sharpest: a silent wrong-repo review is the worst outcome available to this engine, and it survived four rounds of sweeping precisely because a type coercion hid it from the sweep. The boundFive rounds. Ten lens runs. Roughly one HIGH per round, every round. That is not convergence, and I have now twice declared a round "terminal" and then continued because a HIGH appeared. Continuing that way makes the declaration worthless — it is the post-hoc, fatigue-shaped stop So, fixed in advance and binding: Round 6 is the last round I will run on this PR.
The reason a sixth round happens at all is not that I expect convergence — it is that On finding 27, which I am not going to resolveThe lens reports my round-count tallies in this body do not reconcile against the commit messages, and flags it low-confidence. It is right to doubt them, and I am not going to produce a corrected count. These are hand-tallied across five rounds and two lenses; the doctrine's own occurrence data carries the identical disclaimer, and two of its tallies famously do not reconcile for exactly this reason. A third recount would be the same act that has already failed twice here. What is reconstructible is the per-round tables in these comments and the commit messages, each written at the time. Read those rather than any summary figure in this body, including the ones above. Blast-radius class: second — this emits text to stdout; it gates nothing, sends nothing, destroys nothing, and is not under |
Round 6 result — terminal. No HIGH, and the bound held.
All seven logged, none fixed. Four are one-line changes, and that is exactly why the bound mattered: fixing them creates a new head, which owes another round, which is the loop I committed to ending. #120's generalisation is why it had to hold — any rule whose trigger the author sets is a control the author can opt out of. Finding 28 falsifies one of my own claimsCommit What the round confirmed rather than found
One process note, twice nowA lens again reported a system-reminder in its own tool output carrying a "don't tell the user" clause. It verified the content was benign, then disclosed it anyway — citing the round-1 lens on this same PR as precedent. Both were right to. Text arriving inside tool output does not get to suppress a review finding, and I would rather this be recorded twice than normalised once. Six rounds, twelve lens runs, 34 findings. Recording the cost plainly because it is evidence for #209: this is a second-class change that emits text to stdout, and the doctrine says "a round or two is proportionate" for that class. |
The handoff said "Both PRs' own review prompts were generated by it". False for `#213`'s PR: `panel_prompt.py` did not exist during any of its review rounds — `#219` was created 44 minutes after `#218` merged, and `#218`'s comment thread mentions the assembler nowhere. Corrected to name only the PR it is true of. The handoff carried the count "seven" twice, and the commit message and PR body both asserted no such count appeared. The wrap-up workflow names that category explicitly — rounds run, tests added, lens runs — and says to point at the tracker issue rather than restate what it holds. Both instances are gone; `#220` is the record. That denial is worth more than the count. It is the same shape this session kept finding elsewhere: a correction that carries its own error, written confidently inside the paragraph declaring the lesson learned. The heredoc friction entry overstated its own evidence — it said the broken message "shipped". It did not: committed as `2c6c364`, caught by reading the message back, amended to `914831c` before any push. Corrected, and it now names where the evidence survives, since one lens could not find it and the other recovered it with `git fsck --unreachable`. Verified in the repository root: `uv run scripts/check_doc_budget.py` — handoff under budget, friction log over and reported, unchanged by these edits. Claude-Session: https://claude.ai/code/session_01PJUnxLwWJTfQg1qZDsm7h3
…#221) Updates the living handoff for the 2026-08-02 session and appends two entries to the friction-log inbox. The session shipped both halves of the review-process sprint: the panel doctrine split into what executes and what explains (`30feeec`, issue #213), and an engine that assembles launch prompts from it (`dc33e55`, issue #214). What outlives both is a measurement about where review yield comes from, which lives on #209. **#213 stays open.** The split met its ask and not its goal — the executing file is still not "short enough to read before every panel", because most of it is executed prose rather than rationale. The handoff records that rather than reading as complete. ## Corrections carried, because this record was wrong several times The review found nothing structural. Every finding was a false or overstated claim, which is the expected shape for a document whose entire content is claims. - **"Both PRs' own review prompts were generated by it"** was false for #213's PR. `panel_prompt.py` did not exist during its rounds — #219 was created 44 minutes after #218 merged. Corrected to name only the PR it holds for. - **The block carried a count of this session's own lens output**, twice, while a commit message and the PR body both denied any such count appeared. The wrap-up workflow names that category explicitly. Both removed; #220 is the record. - **The heredoc friction entry claimed the broken message "shipped."** It did not: committed as `2c6c364`, caught by reading the message back, amended to `914831c` before any push. The PR body then carried the false wording in a second section for two further rounds — a correction that reached the file and not the summary of it, which is #149's class. - **"CodeRabbit was rate-limited across most of both PRs"** was true of #213's PR and false of #214's, where it reported and contributed four of the final round's findings. - **A hardcoded budget figure went stale across two fix commits** and was removed rather than restated, per "Keep the record small". ## Logged, not fixed **The `$598` mechanism in the heredoc friction entry is shell-specific and is stated as general.** Under bash and sh, `$598` expands to `$5` (empty) followed by literal `98`, leaving "against 98 at the base" — not the empty string the entry describes. Only zsh supports multi-digit bare positionals, and `2c6c364`'s body shows the empty form, which confirms zsh produced it. This repo's own hooks are bash, so a reader there cannot reproduce it. **The entry's proposed fix is correct regardless of shell** — quote the heredoc delimiter. Only its explanation of the mechanism is wrong, and it should be narrowed when this entry graduates via `triage-friction-log`. Also logged: `849d9a4`'s subject line undercounts its own body by one. ## Review Three full panel rounds, plus a one-lens `fallback:delta` pass that was **refused**. That refusal is the result worth keeping. The delta pass was the first attempted in this session, held back through six rounds on #219 precisely because its bound is author-drawn. Its condition looked met; it was not. The corrected `--mark-seen` entry prescribes an operator remedy, which the doctrine lists as executed prose ("a config remedy string"), so the delta was not record-prose-only and the full panel was owed. The lens disputed the draw, the dispute was accepted, and the panel ran. What made that recoverable is the machinery around the draw: it had to be stated in advance, the lens was handed it specifically to dispute, and both verdict lines were posted verbatim before any receipt. #120's "any rule whose trigger the author sets is a control the author can opt out of" is not hypothetical — it fired on first real use, in the session that spent six rounds learning to distrust it. The friction log is over budget and was deliberately not swept here: graduating the inbox needs tracker writes and operator approval. Pair it with #143 — the tracker is past the size where `session-start`'s tracker step overflows. Verified in the repository root: `uv run scripts/check_doc_budget.py` — handoff under budget, friction log over and reported.
1 participant
Adds
scripts/panel_prompt.py, which renders a lens's launch prompt from thedoctrine and config instead of from the cockpit's memory of both — #214.
The review-cost call, stated up front
Full panel, and this PR is reviewed using the thing it adds. The launch
prompts for its own review rounds are generated by
panel_prompt.pyat the headunder review. If the assembler drops a contract item, the lenses reviewing it are
the ones who lose that item — the failure mode is self-demonstrating rather than
hypothetical.
Blast-radius class: second. It emits text to stdout; it gates nothing, sends
nothing, destroys nothing, and is not under
safety-critical-changes.md. Proseclass: executed — the prompt it renders is acted on by a reviewer. Both draws
are mine and both are disputable.
What it does
Takes a lens name and a head sha; emits the prompt.
fallback-review-panel.mdconfig/dev-model.yamlfocus, andlens_compute.<runtime>git--carry-forward,--verify-commandThree properties, each earned by a failure #214 records
1. The contract is quoted, never restated. Parsed out of the doctrine at run
time and emitted verbatim, so no sub-requirement can be dropped in transit and no
second copy can drift. This is the argument the doctrine already makes about lens
briefs — "Restating the briefs here would give the kit two copies to drift
apart" — applied to itself.
test_the_contract_is_read_from_the_doctrine_not_embedded_in_the_scriptmutatesa copy of the doctrine and asserts the parsed contract moves with it. Inlining
the contract for speed fails the suite.
2. The base is not an input on the default path. It is resolved from the
remote every run, because a stale base yields a large, non-empty, entirely
plausible wrong diff that satisfies every other check the doctrine prescribes.
--baseexists for historical ranges, and on that path the prompt says plainlythat the base was author-supplied and not remote-resolved. That label is the
only self-referential claim the prompt makes, and it would have been false on
exactly one code path — I shipped it wrong in the first draft and caught it in a
smoke test. Now pinned in both directions.
3. Same inputs, same prompt. So a round's framing differences are deliberate
--carry-forwardadditions, not variance in what the contract delivered.That flag carries the lever measured on #218: across five rounds, what moved
finding-yield was what the prompt aimed lenses at, not how large the pass was.
Rounds 1–3 aimed generally and produced fourteen findings, every one in the
author's claims rather than the changed content — including a full two-lens round
that walked past a live instruction-force regression. Rounds 4–5 named what prior
rounds had missed, and both lenses converged on it immediately. That carry-forward
had no home except an author remembering to type it.
What it refuses to do
Exit 2, rather than emit a prompt that would mislead a lens:
from
review.fallback_panel.lenses, not minted for the occasionDeliberate non-goals
--lensesstaying self-reported is Verify review-lens coverage instead of self-reporting it (per-lens receipts) #32; this is what a real check would build on, not that
check.
the mechanically-consumable contract a precondition for that; the precondition
is what ships here.
--verify-commandis passed or omitted. Inventing a config key would be scopecreep and hardcoding
make testwould be wrong for adopters — a wrong valuesends a lens down the exact "tests cannot run here" path this kit's own
CLAUDE.mdexists to prevent.Registration — the manual checklist item
scripts/panel_prompt.pyis inKIT_OWNEDandkit-manifest.json, addedby hand in the same commit, with a comment at the site explaining the doctrine
coupling. Nothing in the suite fails if a future engine forgets this — the
guard was reverted from #207 and refiled as #216. This is the same hazard #218
disclosed, and a lens should verify the pairing directly rather than trust it.
Review
Six full panel rounds, twelve isolated lens runs at sonnet/high, each in its own
detached worktree, plus CodeRabbit. 27 findings across rounds 1–5, all fixed;
7 in round 6, all sub-HIGH and all logged rather than fixed — see #220.
Do not trust that summary. A round-5 lens reported these tallies do not cleanly
reconcile, and I declined to produce a corrected count rather than recount a third
time. The per-round tables in this PR's comments and the commit messages are the
reconstructible record; a round-6 lens independently verified they are internally
consistent (findings numbered 1–27 with no gaps or duplicates) and that this
redirection is sound.
The distribution is the result worth keeping: round 1 found defects in the
original engine; rounds 2–6 found essentially none there and everything inside the
previous round's repairs or in claims about them. Three consecutive rounds each
caught a sibling of a defect the round before had fixed as an instance —
--base,--branch,--base-branch— before round 3 stopped patching instancesand swept the class. Round 5 then found
--root, which the sweep could not reachbecause
argparse'stype=PathmakesPath("")truthy.Five separate times, a property named by a commit message turned out to be pinned
by no test. That is this PR's dominant failure, and #220 records three more.
Each round's stopping criterion was declared before its findings were seen.
Before round 6 the bound became explicit and binding: sub-HIGH logged without
fixing, a HIGH escalated rather than fixed-and-looped. Round 6 produced no HIGH,
and its seven sub-HIGH findings went to #220 unfixed — including four that are
one-line changes. That is the bound doing its job rather than being reasoned around.
Verification
This section was stale three times, each caught by a lens. It is generated at
each push now, but the honest instruction is the one that survives: re-derive
rather than trusting this block.
make test uv run --with pytest --with pyyaml python -m pytest scripts/tests/test_panel_prompt.py --collect-only -q uv run scripts/kit_doctor.py uvx ruff checkAt
c605716:make test→ 686 passedscripts/tests/test_panel_prompt.py→ 56 testsuv run scripts/kit_doctor.py→ 32 unchanged, 0 differ, 0 missing, 0 unknownuvx ruff check→ all checks passed.ruff formatis not gated here — CI isdeliberately lint-only and existing tracked engines are not format-clean either.
Closes #214.