Seven sub-HIGH findings logged from #219's terminal round — two flags validate-but-discard, and three tests pin less than they claim

[topij]

Seven sub-HIGH findings from PR #219's terminal review round, logged rather than fixed under a bound declared on that PR before the round ran: anything sub-HIGH is logged, no commit, so the receipt at the merging head stands.

Recording them here because the doctrine's logged-disposition rule requires an artifact outside the repo tree, and because they are all real and all cheap. None is a live correctness hazard; every one is either a narrow behavioural gap or a test that pins less than it claims.

Why these were not simply fixed

They are trivial. That is exactly why the bound mattered.

PR #219 ran six full panel rounds and roughly one HIGH per round, every round. Twice I declared a round terminal and then continued because a HIGH appeared. Before round 6 I committed publicly to stopping — sub-HIGH logged, a HIGH escalated rather than fixed — and fixing seven cheap things would have created a new head, which owes another round, which is the loop the bound exists to end. #120's generalisation is the reason it had to hold: any rule whose trigger the author sets is a control the author can opt out of.

The findings

Behaviour

1. --head and --lens validate their value and discard the normalized result. build() stores _override's return for every other flag (o_root, o_branch, …) but calls _override("--head", args.head) for these two and then uses the raw args.head / args.lens. A value with real content plus incidental padding passes validation and is then used unstripped:

   $ panel_prompt.py --lens "adversarial " --head <sha> --branch <b>
   error: lens 'adversarial ' is not in review.fallback_panel.lenses (adversarial, correctness)
   

   Fails closed, so no wrong prompt — but it refuses a legitimate invocation with a misleading message. This also falsifies a claim in commit 2671103, which said an empty --head's double-space message was "now unreachable anyway". It is reachable; just not via an empty value. Fix: store the return, as every sibling flag does.

2. Falsy compute values are dropped from the rendered prompt. parts = [... for k, v in sorted(compute.items()) if v] filters on truthiness, so a lens_compute carrying temperature: 0 or thinking: false silently renders without them. Filter on is not None. (CodeRabbit.)

3. A repo with no origin remote produces an error with no cause. With --base supplied, resolve_base is skipped, so a checkout without origin is a valid invocation — but git config --get remote.origin.url exits non-zero with empty stderr, yielding error: git config --get remote.origin.url failed: and nothing else. Name the missing remote. (CodeRabbit.)

4. An invalid --root raises an uncaught FileNotFoundError at exit 1, not the documented exit 2. build() catches only PromptError; kitconfig.load_config raises FileNotFoundError straight through. No prompt is emitted, so nothing is misleading — but the module docstring's "Exits 2 on any condition that would produce a misleading prompt" overstates the guarantee.

Tests that pin less than they claim

This is the class that dominated PR #219 — it recurred four times there and these are three more.

5. contract()'s three refusal branches are never exercised through the CLI. All three are asserted by calling pp.contract() directly. Wrapping the contract() call inside build() in a try/except PromptError that substitutes a stub passes the entire suite. The current code is correct end-to-end — verified by subprocess against a fixture with a renamed heading — but the seam from contract() through build() to main()'s exit code is unpinned.

6. assert "the sha is\ncurrent" in msg or "current" in msg — the second disjunct matches almost any message, so the assertion pins nothing. Assert the specific phrase. (CodeRabbit.)

7. Negative stdout assertions can pass vacuously. Several tests assert text is absent from stdout without first asserting the exit code. If the engine exits 2, stdout is empty and the assertion passes for the wrong reason. Assert the exit code first. (CodeRabbit.)

Source

PR #219, round 6 (terminal). Findings 1 and 4 from the adversarial lens, 5 from correctness, 2/3/6/7 from CodeRabbit. Each was reproduced by direct execution before being logged — none is a read-only suspicion.