fix(verify): make the collection floor an EQUALITY so drift cannot be silent - #56
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 41 minutes Limit details: You’ve used the included review currently available. Your 75 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
770814c to
aed3d69
Compare
aed3d69 to
531790a
Compare
Workflow source detectedPR #56 now has valid workflow source context (origin=local_request). No linked GitHub issue is required for this PR. |
Automated Status SummaryHead SHA: cc76763
Updated automatically; will refresh on subsequent CI/Docker completions. Keepalive checklistScopeNo scope information available Tasks
Acceptance criteria
|
|
Runner dispatch state for autofix on PR #56. Do not edit. |
|
Runner dispatch state for codex on PR #56. Do not edit. |
|
Workflow state fingerprint for Agents Gate Followups. Do not edit. |
|
Workflow state fingerprint for Keepalive Loop Reporter. Do not edit. |
|
Autofix updated these files:
|
482adce to
4b88cbf
Compare
… silent `.verify-floor.json` has been found BELOW reality four times -- 21 low at the worst, then 8, then 1, then 2 -- and each was hand-raised after the fact by whoever happened to look. The stale number was never the defect. `_floor_problems` fired only on `collected < floor`, so a PR could add tests and never touch the file: silently green, with the floor left permissive by exactly the number added. #34 and #37 each did precisely that, which is what PR #50 then had to clean up by hand. `collected` is now an equality. Too few tests still fails; too many fails as well, printing the two integers to write and telling you to rebase first. That also makes the concurrent case self-enforcing, which is the part discipline could not fix. Once every test-adding branch must edit these same two lines, two concurrent branches CONFLICT IN GIT -- the second cannot merge without rebasing onto the first and re-measuring on the actual merge result. This change demonstrated that on itself, at length: six merges landed on main during the two hours it took to write, moving the floor 368 -> 387 -> 391 -> 402 and the ceiling 24 -> 26. Under the old one-directional rule every one of them would have left the floor below reality and merged green. Git's own conflict detection is what enforces "measure on the merge result, not on the branch", the rule the note in that file had to restate three times with nothing behind it. `passed` deliberately stays a MINIMUM on passed+skipped. Only collection is machine-invariant, measured across machines at 391 collected on both, with pass/skip splits of 365/26 on CI against 391/0 locally. Making that one strict too would fail a machine for honestly naming a missing prerequisite. Two further fixes in the same change: * `--update-floor` no longer REPLACES the note. It appends. The note is the only record of which prerequisite justifies each ceiling, so overwriting it destroyed the rationale on every use -- the file had to carry a warning about its own tool. * Drift does not block `--update-floor`. The first draft made it a latched gate: a floor behind reality became a problem, and the guard was `not problems`, so the one remedy the error message named was refused for the existence of the very condition it clears. `_blocks_floor_update` lets drift through while real failures still block, sharing `DRIFT_PREFIX` so message and predicate cannot diverge. Selftests cover both directions, skip-invariance, the unset-floor case, the unblock predicate and note preservation, each with a deliberate-break demonstration. Two of those tests were themselves defective and only the break demo caught it: the latched-gate assert exercised `_blocks_floor_update` in isolation and stayed green when the CALL SITE was reverted (built-but-not-wired), and the source-text assert written to fix that searched for a literal that appears in its own line, so it could never fail. The needle is now built from two fragments and the wiring is asserted at the guard. Verified on the merge result: 402 passed, 0 failed, 0/26 skipped, 402 collected (floor 402), 84/84 selftests, 43/43 can-fire, 5/5 gates. Ceilings untouched at 26/7/2; the floor stays at main's 402, since this adds selftest assertions rather than pytest tests. black clean at line-length 100. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1a4852c to
bbbd8ae
Compare
…anch Fourth `.verify-floor.json` conflict for this branch, and the first under the EQUALITY that #56 introduced — which is the mechanism working as designed, not friction: that change's own rationale says git conflict detection is what enforces "measure on the merge result, not on the branch". Measured accordingly: 405 = main's 402 + this branch's 3. Under the old minimum, keeping the larger number would have passed; under the equality it has to be measured, and it was. Union of both rationales retained in the note. No ceiling moved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#56 makes `collected` an EQUALITY rather than a minimum, so a floor that sits below reality now goes RED instead of passing quietly. That turns "assume 402+1" from a bad habit into a hard failure, which is the right direction and the reason every round of this branch measured instead of deriving. Union as before: main's note kept whole, this branch's entry appended, and the count MEASURED on the merge result -- `pytest --collect-only` reports 403. Main stayed at 402 across #56, so #56 added no collected tests and 403 is main's 402 plus this branch's one test. No ceiling moved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
One lint finding this PR will hit now that #60 has merged and Measured by overlaying this PR's fc, fp = int(floor.get("collected", 0)), int(floor.get("passed", 0))
problems += _floor_problems(floor, py)It looks like a leftover of this PR's own refactor: extracting Worth stating why this is new information rather than pre-existing debt: Two things that will matter when you rebase, both from #60 and after:
|
Seventh floor union on this branch. main a247a55 records 410; the merge result MEASURES 411 with `pytest --collect-only -q` -- main's 410 plus this branch's one test. Measured, never derived: #56 made `collected` an EQUALITY, so an assumed number is a hard RED. #69 also flips CI coverage ON, so this branch's merge is the first to be measured under it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…70) * test(diagnostics): salvage PR #43's pytest coverage and fetch-first pointer Rebuilt on current main (PR #43's branch was CONFLICTING and #46/#60 have since reshaped both files). #43 and #46 built the entrypoint-absence diagnostic independently and in parallel; #46 merged first, so this lifts the three things #43 had that #46 lacked rather than losing them with the closed PR. 1. `test_an_absent_entrypoint_diagnoses_itself_differently_from_a_real_defect` — #46 put the equivalent checks in `capability_activation_audit --selftest`, and a selftest is NOT guarded by `.verify-floor.json`. Only a COLLECTED test is, so this is the half that makes the behaviour hold. 2. `test_the_capability_gates_all_consult_the_entrypoint_diagnosis` — the sharper idea, with no equivalent in #46 at all: nothing else notices if one of the three gates quietly stops calling the helper and reverts to a bare capability id. It matches the CALL, not the bare name, because all three files also MENTION the helper in a comment — a name-only grep would keep passing after someone deleted the call and left the comment behind. 3. The `git fetch --all && git log --all --oneline -- <module>` pointer, WITH its caveat. `git log --all` is the natural next probe and on 2026-08-22 it came back empty for a module that existed, because the branch holding it had never been fetched into that checkout. An empty result there means "not fetched", never "does not exist", so the command and its precondition travel together or the pointer reproduces the misreading it exists to prevent. Also adds `absent_entrypoint_report(..., ledger=)` / `absent_entrypoint_note(..., ledger=)` so both tests inject their rows instead of reading the running instance's ledger. That is why neither test skips anywhere: the interesting case is one row present and one absent, and no real ledger is reliably both. A machine-independent test is the preferred way to grow this suite — see the 24 -> 26 -> 24 ceiling episode in `.verify-floor.json`. FLOOR 402 -> 404, measured on the merge result, and it must be exact now that #56 made `collected` an EQUALITY. No ceiling moved and nothing new is skipped. Deliberate break -> revert, each caught then reverted byte-identically: * stripped one gate's call to the helper while leaving the comment behind — the drift guard fired and named the file; * dropped the fetch-first caveat from the pointer — the pointer assertion fired. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(diagnostics): two CodeRabbit findings — a short-circuit and a vacuous guard Both from the review on PR #51, both real, and the second is embarrassing in a useful way. 1. THE FETCH COMMAND OMITTED THE MODULES THAT MATTERED MOST. The module list behind `git log --all` was `{sibling hits} or {missing candidates}`. `or` short-circuits, so as soon as ONE absent row was found in a sibling checkout, the candidates of every row found NOWHERE were dropped — and those are precisely the rows the pointer exists for, because a module in no sibling checkout is the one most likely to sit on an unfetched remote branch. The command said "to check every branch" while silently omitting the hardest cases. Now a union, with the truncation counted rather than silent (`+N more module(s) not shown`), because a cap that hides modules would put the same lie back in a smaller place. 2. THE DRIFT GUARD WAS VACUOUS FOR ITS OWN FILE. It searched each whole FILE for the substring `audit.absent_entrypoint_note(` — which appears SIX times in `test_capability_set_coverage.py` alone: in the docstring, in sibling tests, and inside the assertion itself. So deleting the real call from the recurrence-fixture gate left the guard passing. A guard written specifically to catch a check that cannot fail, that itself could not fail. That is this repo's founding defect wearing the uniform of its own countermeasure. It now walks the AST of ONE NAMED FUNCTION per file, from an explicit `GATE_CALL_SITES` mapping. A string literal or a comment cannot satisfy an AST call match at all, and a RENAMED gate asserts rather than passing quietly — "some call somewhere in the file" was the whole weakness. Deliberate break -> revert, each caught then reverted byte-identically: * union -> `or`: the new regression test failed naming the exact omission (`b_lane.py` absent from the command while `a_lane.py` was present); * deleted the real call from this file's gate, leaving all six literals in place: the AST guard failed where the substring version had passed — the specific hole CodeRabbit found; * renamed a gate function: caught, with a message saying to update `GATE_CALL_SITES` deliberately. FLOOR 404 -> 405: one new test, `test_the_fetch_command_names_every_absent_module_not_just_the_found_ones`. It needs BOTH kinds of absent row present at once, which is why it is its own test rather than more assertions on an existing one — with only one kind, `or` and `|` are indistinguishable. It injects its ledger rows, so it skips nowhere. No ceiling moved. Verified with `python3 verify.py`: 405 passed, 0 failed, 0 skipped, 84/84 selftests, 43/43 can-fire, 5/5 gates. `ruff check .` and `black --check --line-length 100` both clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(diagnostics): address PR #70 CodeRabbit threads Add truncation regression for seven-plus absent modules and tighten the AST gate predicate to require audit.absent_entrypoint_note in the named function body only, ignoring nested defs and non-audit receivers. Co-authored-by: Cursor <cursoragent@cursor.com> * chore(autofix): formatting/lint * chore(verify): raise floor to 415 on PR #70 merge result CI measured 415 collected (389 passed + 26 skipped) after merging main into the diagnostics salvage branch. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Tim Stranske <tim@stranskemo.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
The recurrence, not the instance
.verify-floor.jsonhas been found below reality four times — 21 low at the worst, then 8, then 1, then 2 — each hand-raised after the fact by whoever happened to look (#15, #18, #50, plus the entry the backlog logged as a one-off follow-up).The stale number was never the defect.
_floor_problemsfired only oncollected < floor:So a PR could add tests and never touch the file — silently green, floor left permissive by exactly the number added. #34 and #37 each did that, which is what #50 then cleaned up by hand.
Fix
collectedis now an equality. Too few still fails; too many fails as well, printing the two integers to write and telling you to rebase first.That makes the concurrent case self-enforcing, which is the part discipline could not fix. Once every test-adding branch must edit these same two lines, two concurrent branches conflict in git — the second cannot merge without rebasing onto the first and re-measuring on the actual merge result.
This PR demonstrated that on itself, repeatedly. In one afternoon #42 landed underneath it (floor 368 → 387, ceiling 24 → 26,
verify.pyreformatted), then #59 (387 → 391), then #61. Each conflict forced a rebase and a fresh measurement. Under the old one-directional rule, every one of them would have merged green with a floor below reality. Git's conflict detection is what enforces "measure on the merge result, not on the branch" — the rule that file's note had to restate three times with nothing behind it.passeddeliberately stays a minimum onpassed + skipped. Only collection is machine-invariant, now measured across machines:Making
passedstrict would fail a machine for honestly naming a missing prerequisite.Two further fixes in the same change
--update-floorappends to the note instead of replacing it. The note is the only record of which prerequisite justifies each ceiling; overwriting it destroyed that on every use, so the file had to carry a warning about its own tool.--update-floor. The first draft made it a latched gate: drift became a problem, the guard wasnot problems, so the one remedy the error message named was refused for the existence of the condition it clears._blocks_floor_updatelets drift through while real failures still block, sharingDRIFT_PREFIXso message and predicate cannot diverge.Two of my own tests were defective, and only the break demo caught it
Worth stating plainly, because both are this repo's own failure modes and all three selftests passed while both were broken:
_blocks_floor_updatein isolation, so reverting the call site left it green — built-but-not-wired, in the test for the fix._srcalways contained it and the assert could never fail.The needle is now built from two concatenated fragments, so the joined form exists only at the real guard. All three break demos — no-upward-check, latched gate, note clobbered — fail as intended and were reverted.
Verification
Locally, on the merge result:
CI on a clean runner passed on the previous head:
365 passed, 26/26 max skipped (391 collected; floor 391), 77/84 selftests, 3/5 gates with 2 skipped for named absent prerequisites.End-to-end: with the floor set 2 low the run exits 1 and reports
floor 366 — 2 BEHIND. black clean at line-length 100; ruff at main's baseline of 6 pre-existing findings, none added.Ceilings untouched at 26/7/2 — nothing new is skipped. Floor stays at main's 391: this adds assertions inside
verify.py --selftest, not new pytest tests.Note on the autofix commits
Four bot commits (
improvement_log.py,test_improvement_log.py, a JSON artifact) were dropped in the rebase. They touched only files from #59, none belonging to this PR, and their content is now byte-identical to main — redundant, not lost work. Keeping them would have carried 212 lines of another PR's code through this one.Cost, stated plainly
A test-adding PR must bump two integers that CI prints for it. Stale worktrees and in-flight branches go red until they rebase — including #51, which recorded 370 against a base now at 391. That red is the mechanism working.
🤖 Generated with Claude Code