Repository navigation
guard(reach): pin every declared floor to its population (#15928) - #16048
Conversation
A floor far below what its own discovery returns fires only against total collapse, and total collapse is not the failure that happens. Partial loss is: a narrowed glob, a moved directory, a change propagated to some call sites and not others. Every floor examined in review during #15896, #15901 and #15913 fired correctly against an empty tree while tolerating the loss of most of a real one. Adoption did not fix this. Both guards that had already adopted _reach declared a number chosen by feel -- 1000 against 5,599 files and 500 against 3,152, 18% and 16% of their own populations. The mechanism was never the missing part. Adds Reach.growth and a meta-test asserting the live population sits within [floor, floor + growth]. growth=0 -- equality -- is the default because a floor at its population turns shrinkage into a deliberate line in a diff. A population ordinary work grows states a band instead, where a reviewer sees it, rather than being pinned low and quietly meaning nothing. Validated in the meta-test rather than in declare(): declaring would walk the tree once per imported guard, and the meta-test already enumerates every declaration, so the guarantee is the same and the cost is paid once. Both existing floors ratcheted to a band of 250, which absorbs ordinary churn in both directions while staying far below the size of any subtree whose loss the floor exists to catch.
… discovered (#15928) The pre-push hook rejected the first version and the rejection is the finding: one floor serves two populations. `examined()` bounds what `discover` returned -- 5,599 tracked python files -- while `completed()` bounds what the guard actually finished, 5,337, because 262 are skipped as unreadable or unparseable. A floor pinned to the discovered count clears the first check and fails the second. So the meta-test added here is necessary and not sufficient: it can only see the higher population, and `completed()` remains the binding constraint. Both declarations carry a band of 500 rather than the 250 ordinary churn alone would need, precisely to absorb that gap. Separating the two floors is the fuller fix and is deliberately not this change. Stating the limitation in the test rather than leaving it for the next author to rediscover by the same hook failure.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 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 |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
|
Review: the ratchet is the right mechanism and the docstrings are unusually honest about its limits. Two findings, one of which is in the failure message rather than the logic. I read this as the necessary complement to I also want to note the docstring saying this is necessary and not sufficient, that 1. The failure message tells the reader to defeat the guard
assert slack >= 0, (
f"[{reach.name}] floor {reach.floor} exceeds the live population of "
f"{count} {reach.what}. The guard cannot pass; lower the floor to "
f"{count} only if the population genuinely shrank."
)Consider when this fires. The The second assertion has the same shape ( Suggestion: on the negative branch, lead with the diagnosis and make the reader do the work — the sweep is reaching N fewer items than when this floor was set; find out what stopped being reached before touching the floor — and drop the pre-computed replacement value. Right or wrong, the number should not be sitting there ready to paste. 2.
|
|
The Measured against current base: It fails by one. The immediate fix is the one your own assertion names, and here it is the correct branch of it — slack is positive, so the population genuinely grew and ratcheting the floor to 5601 is right. (This is the case I said the second assertion handles correctly; only the negative-slack message worries me.) One correction to a diagnosis in circulation: this is not the Worth more than the fix. I flagged in review that
So I would not just ratchet. Either widen the band substantially with the reasoning stated where a reviewer meets it, or take the separate Caveat on my evidence: the CI logs for that run have expired — |
Do not merge — the band in this PR is wrong, not unluckyMarking this explicitly because the failure is subtle enough to read as flaky, The red on shard 12/12 is mine and the test is behaving correctlyThe tree grew by two files between my measurement and CI. The meta-test did Why the number was unchoosable, which is the actual defectOne floor has to satisfy both populations at once:
That forces A number that silently spends most of itself on a different quantity cannot be The fix, and why it is not in this pushSplit the two jobs: That change lands in Not the same root as #16057, for the recordThat collision fires in Shard 8/12 is not mineStale base. The same two assertions in The part I would keep from this if the rest changes: the mechanism caught its |
…15928) The previous push failed by one file, and the failure is the argument. One floor serves two populations: `examined()` bounds what `discover` returned (5,601 tracked python files), `completed()` bounds what the guard finished (5,337, with 262 skipped as unreadable or unparseable). The floor has to clear the lower one while the meta-test measures the higher, so a single band of 500 spent 264 of itself on the skip gap before buying any growth headroom. The remainder lasted an hour. Raising it to 800 would have been the fix this change exists to prevent: a number chosen by feel. Splitting it is the fix -- `skips` for what a guard cannot complete, `growth` for what it may grow before a deliberate ratchet. Each is then chosen against one quantity and a reviewer can see which. audio-extension-allowlist declares skips=300, measured from the hook's own `completed 5337` against 5,599. env-var-bare-cast declares skips=200 and says in the comment that it is a guess, because that guard's `completed` count has never failed and the gap is unknown -- recorded as unmeasured rather than presented as measured. The failure message now names both numbers and says which to raise, since the whole defect was one number silently doing two jobs. `examined()` is deliberately untouched: #16057 documents an EmptyEnumeration collision in that function and the fix belongs to whoever takes it.
Pushed
|
| before | after | |
|---|---|---|
growth |
500, silently part skip-gap | 400, meaning only growth |
skips |
— | 300 (audio) / 200 (env-var) |
The meta-test checks slack <= skips + growth and its failure message now names
both, and says which to raise:
skips= items this guard cannot COMPLETE (unreadable, unparseable).
Measure it from a `completed` failure; do not estimate it.
growth= ordinary growth tolerated before a deliberate ratchet.
If neither is short, the floor is stale: ratchet it toward N.
Why not just raise the band to 800
That is the habit this PR exists to end. A number chosen because the last one
was too small is the same number chosen by feel, one iteration later. A
quantity that silently spends most of itself on a different quantity cannot be
chosen well — the fix is two names, not a bigger one.
The two declarations, and which numbers are measured
audio-extension-allowlist:skips=300is measured — the pre-push hook
reportedcompleted 5337againstdiscover 5599, a gap of 262, rounded up
for churn.growth=400is a maintenance interval and is the only number here
chosen by judgement.env-var-bare-cast:skips=200is a guess and the comment says so. That
guard'scompletedcount has never failed, so its gap is unknown. Recorded as
unmeasured rather than presented as measured; the nextcompletedfailure
gives the real number and whoever sees it should replace this.
Current slack: audio 501 against an allowance of 700; env-var 302 against 600.
examined() is deliberately untouched
#16057 documents an EmptyEnumeration collision in that function — tracked_paths
raises where test_no_guard_can_succeed_against_an_empty_tree needs an empty
result as evidence. Both mechanisms are right and the resolution belongs to
whoever takes it. Verified untouched: the diff to _reach.py adds a dataclass
field and a parameter, and changes no function body.
Status
Shard 12/12 was the red this fixes. Shard 8/12 was stale base — the same two
assertions in glob_declared_reads_15900_test.py were red on #15978, #15980 and
#16052, and #16012 fixed the record they check; merging base in clears it.
Green on the pre-push suite at c1a8b714. No reds on the PR; 27 checks pending.
mrveiss
left a comment
mrveiss
left a comment
There was a problem hiding this comment.
Reviewed. The split is the right abstraction, and I took up your invitation to push back on env-var's unmeasured number — it is measurable, and the measurement says 0.
The split
growth was carrying two quantities, so a 500 band bought ~236 of actual headroom and the repo ate it in an hour. skips naming the discover/completed gap separately is right, and the failure message naming both and saying which to raise is what makes the next choice a decision rather than a guess. Raising 500 → 800 would have been the same act one iteration later, and you said so.
examined() is untouched — confirmed in the diff, a dataclass field and a parameter, no function body changed. 46's EmptyEnumeration resolution stays available.
The pushback you asked for: EXPECTED_SKIPS = 200 should be 0
I replicated the guard's own population and parse loop rather than reasoning about it — git ls-files "*.py", the _EXCLUDED_PATH_FRAGMENTS filter, _is_scanned, then ast.parse catching the same (SyntaxError, UnicodeDecodeError, OSError) triple that measurement() catches at :233:
discovered : 3152
completed : 3152
GAP : 0
Every file this guard discovers, it parses. So 200 is not a conservative guess — it is 200 units of slack buying nothing, and it widens the band by 200 for a population that has no gap at all.
That matters more than the arithmetic: a number chosen by feel is exactly what this PR exists to end, and an unmeasured skips reintroduces one under a name that now looks measured because its sibling is. The comment saying it is a guess is honest, and honesty about a wrong number is weaker than the right number.
Recommend EXPECTED_SKIPS = 0 with the derivation and the command recorded, so the next person re-derives rather than re-guesses. If a completed failure ever appears there, that is the signal to raise it — with the number the failure hands you, which is precisely the mechanism you built.
Contrast worth keeping in the comment: audio's 262 is real because that guard's population contains files it genuinely cannot finish. Two guards, two answers, and only measurement distinguishes them — which is the argument for the field existing.
Not blocking on
growth=400as a maintenance-interval judgement. It is judgement, it is labelled as judgement, and safety comes from the floor rather than the band.MIN_FILES_SCANNED500 → 2850 on the env-var guard: a floor at 500 against a 3,152 population tolerated losing four-fifths of the tree, which is the same defect this PR is about, one declaration over.
Verdict: approve with one change. Set EXPECTED_SKIPS = 0 and record the measurement; everything else stands. I am not the ideal reviewer for whether the abstraction is right — I endorsed the ordering that produced it — so a second pass from 46 is still worth having, since they hit the adjacent half of this tension from the other side.
It is 0. That guard discovers 3,152 files and parses 3,152 -- every file it enumerates, it completes -- so the 200 it shipped with was slack buying nothing. Worse than useless: it sat beside audio's measured 300, where it inherited an authority it had not earned. A number chosen by feel, under the name this change added to stop numbers being chosen by feel. Declaring it honestly as a guess was weaker than measuring it, which is the pushback I asked for and got. With no gap the floor can pin near the real population rather than below a phantom one, so MIN_FILES_SCANNED goes 2850 -> 3000: slack 152 against a growth band of 400, and the floor still clears completed by 152. The derivation and the command to repeat it are recorded beside the constant, so the next person re-derives rather than re-guesses. If a completed failure ever appears there, that is the moment to raise it -- with the number the failure hands you, which is the mechanism this PR builds.
1 participant
Thinking Path
repo_tests/_reach.pygives a guard a reach floor and proves it fires(
reach_declarations_testdrives every declaration against an empty tree). Itdoes not prove the floor is tight, and that is the half that was missing.
The evidence is in the two guards that had already adopted it:
audio-extension-allowlistenv-var-bare-castBoth fire correctly against an empty tree. Both tolerate losing four-fifths of a
real one. Adoption was never the missing part — the number was, and the same
is true of every floor examined in review during #15896 (five of them), #15901
and #15913.
A floor far below its population catches only total collapse. Total collapse is
not the failure that happens; partial loss is — a narrowed glob, a moved
directory, a change propagated to some call sites and not others. Measured on
the file-size guard: a floor of 3,000 against 5,241 files detects only the loss
of the one tree holding 75% of them. The other five can vanish silently.
What Changed
Reach.growth(_reach.py) — how far the live population may sit abovethe floor before the ratchet demands it be raised.
growth=0(equality) isthe default, because a floor at its population turns shrinkage into a
deliberate line in a diff, which is the correct price for shrinking a guarded
population.
test_every_declared_floor_is_pinned_to_its_population(
reach_declarations_test.py) — asserts the live count sits in[floor, floor + growth], and names both numbers plus the remedy when itfails.
them rather than chosen by feel.
Validated in the meta-test rather than in
declare(): declaring would walk thetree once per imported guard, while the meta-test already enumerates every
declaration — same guarantee, cost paid once. The issue's AC said "declare()
rejects"; that was the wrong place and the PR says so.
Verification
The pre-push hook rejected the first version, and the rejection is the second
finding.
One floor serves two populations.
examined()bounds whatdiscoverreturned (5,599);
completed()bounds what the guard actually finished (5,337)— 262 files are skipped as unreadable or unparseable. A floor pinned to the
discovered count clears the first check and fails the second.
So the new meta-test is necessary and not sufficient: it can only see the
higher population, and
completed()remains the binding constraint. Both bandsare 500 rather than the 250 ordinary churn alone would need, precisely to
absorb that gap. The limitation is stated in the test rather than left for the
next author to rediscover by the same hook failure. Giving the two populations
separate floors is the fuller fix and is deliberately not this change.
Mutation-checked: restoring the old floor of 1000 yields slack 4599 against
a band of 500, so the new test fails on exactly the defect it exists for.
Populations measured independently, not by running the guards' own
discovercallables:git ls-files '*.py'plus each guard's own filter,replicated. The 5,337 figure is the hook's, which is the one that binds.
Both guards pass the full pre-push suite at
715b5eb1.Model Used
Opus 5 (1M context)
Closes #15928
Single-issue rationale: this is one mechanism in one module. Its siblings under
#15923 are #15926 (one
tracked_files()) and #15927 (an exclusion registry),which touch different files and carry different risk — #15927 is a
policy-visible change and this is not. Batching them would put three
independent migrations behind one review pass.