fix(review): six more — reporting honesty, surface parity, two vacuous tests

[henleda]

Sixth PR from the deep-dive review. 26 of 27 findings now closed.

Reporting honesty

1. report.html claimed "✓ code fix drafted" for a dependency upgrade. The badge keyed on the mere presence of a remediation. There is no file to patch in someone else's package and pr.py writes no diff for one — so the card described work that does not exist, contradicting the hero on the same page. Three outcomes now:

dependency upgrade   -> ↑ dependency upgrade — log4j-core → 2.17.1
real code fix        -> ✓ code fix drafted
plan with no patch   -> cure planned — no patch drafted

2. The blast-radius table reported an unmeasured replay as safe. simulate computes evaluated, errored, enforcement_confirmed and reason, and the table used none of them. A replay in which every request failed in transit rendered as green "within threshold" at 0.0% — a rate that means "we measured nothing", presented as "safe to promote". Now four verdicts (not measured / unconfirmed / within / over) plus a caveats column carrying the reason and the transit-failure count.

3. reconcile reported a transport failure as "no runnable probe". {} is what "no probe recorded" returns — a permanent condition an operator can only fix by re-scanning. A probe that exists and blew up in transport is transient. Reporting the second as the first sends someone to fix the wrong thing. It now has its own hold code that says the next pass will retry. (auth_failed already existed for exactly this reason — the pattern was one case short.)

Surface parity

4. MCP ignored VPCOPILOT_SIM_THRESHOLD. The threshold was a lookup each surface had to remember. The CLI, console and refiner remembered; MCP did not — so an operator who tightened the blast-radius threshold got the default silently applied to every simulation an agent ran, and the number they set was the number they believed was in force.

Now simulate.effective_threshold, one resolver all four call, pinned by a test that asserts no caller re-implements the lookup — because the defect was one caller not calling it. A malformed value is refused rather than silently defaulted: substituting the default would apply a threshold the operator did not choose, on the gate that decides whether a policy is safe to promote, and they would never be told. An empty value is treated as unset, not malformed.

Two vacuous tests

5. test_safe_rollback_restores_and_verifies never tested "verifies". FakeXC genuinely applies the PUT, so the restore succeeded on its own and verify returned True whatever it did — delete the verify call from safe_rollback entirely and the test still passed. It was asserting the behaviour of the fake.

The new test uses an appliance that accepts the rollback PUT and applies nothing — a 200 that changed nothing, which is what a partially degraded control plane looks like. Without the verify step, safe_rollback returns True and the caller reports a clean rollback while the band-aid is still attached to a live load balancer: the "silent half-rollback" the module docstring calls the worst outcome on a live LB.

6. The four-place registration guard was a whole-file substring search. It asserted "resolve" in report.py — which the unrelated word "resolved" elsewhere in that module already satisfied. It passed with the agent missing from the very list it guarded. report.py now exposes REPORTED_AGENTS and the test asserts the list; a second test asserts all four sites agree with each other, which holds for the next agent rather than for this one by name.

One fixture corrected, called out

tests/test_report.py's remediation carried empty diff and patched_content, so it was asserting the drafted-fix badge for a plan with no patch. Given a real patch — the fixture was unrealistic (a code_fix with no patch is nothing for pr.py to write), not a behaviour change I am papering over.

Verification

• 10 new tests, suite 1067 → 1077, ruff clean.
• Mutation-verified: reverting the four source fixes fails 7 of them; the two vacuous tests now fail against the mutations they previously survived (dropping verify from safe_rollback, removing resolve from REPORTED_AGENTS).
• Live suite: 18 passed against the real tenant and BIG-IP, with the estates confirmed restored afterwards — 0 ASM policies on the appliance, Larkspur LK-1001 at 48215, vpcopilot-lab back to baseline.

What is left

One finding remains open, and it deserves its own PR rather than being folded in here: the spec-vs-code orphan comparison (pipeline.py:152). It is not a small bug — served is never derived from code in any configuration, so with no in-repo spec every declared endpoint is reported as unserved, and with one the comparison is spec-vs-spec and a genuinely undeclared route is reported nowhere. It also synthesises a Finding that bypasses the verify agent and can draw a real api_schema band-aid, so a wrong answer there is load-bearing. The honest fix likely involves declining the comparison when route extraction found nothing, which is a design decision worth reviewing on its own.

The two remaining cli.py items (scan-path existence, run-dir existence — both "enforced only on MCP") go with it.

🤖 Generated with Claude Code