Repository navigation
parity: record the two test-only call sites #2557 created - #2581
Merged
Merged
Conversation
`main` fails its own ratchet at 5de0519, on a clean tree, with no change applied: ERROR: Tools/parity_ledger_baseline.json: accepted finding group regressed test-only-callsite|Packages/StrandAnalytics ERROR: Tools/parity_ledger_baseline.json: accepted finding group regressed test-only-callsite|android/analytics I caused it by merging #2557. That change took the daily resting HR from the primary night's gated five-minute floor instead of the whole-session mean, which replaced the only production call to `primarySessionRestingHR` on both platforms: - ?? primarySessionRestingHR(physiologySessions, hr)?.roundToInt() + ?: primarySession?.restingHR The function is now reached from four test files and nothing else, so the lexical call graph emits one new `test-only-callsite` finding per platform. The checked-in baseline did not record them. This records them: 43 to 44 in `Packages/StrandAnalytics`, 64 to 65 in `android/analytics`, two lines, nothing else moved. Why nothing caught it: #2557 touched no `Tools/parity_*` path, so parity-governance never ran on it. Its seventeen green checks were real and did not include the gate that governs call-site structure. #2575 and #2576 landed the same way today and happened to be harmless. Why my review did not catch it: I checked that `primarySessionRestingHRWithCoverage` still had callers and concluded the old mean was not dead code. That is true of the `WithCoverage` variant and false of the plain function, which is the one #2557 orphaned. I verified the neighbouring symbol. Recording rather than deleting, deliberately. The findings are true, and the baseline exists to hold accepted findings. Whether `primarySessionRestingHR` should survive as a tested-but-unused function, or be deleted with its tests repointed at the `WithCoverage` variant, is a change to code merged an hour ago and belongs in its own review. A governance repair that also deletes product code gets neither part looked at properly. This does not make the debt go away, it makes it visible. The next change in either scope inherits 44 and 65 as the ceiling, so a second orphaned function there fires the gate again. Refs #2557, #2567
ryanbr
added a commit
that referenced
this pull request
Sep 29, 2026
`Tools/tests/test_parity_governance_acceptance.py` does not test the scanner. It asserts that the CHECKED-IN authority and baseline still reproduce from the CURRENT product source, so a product change is what invalidates it. It ran only in parity-governance.yml, whose filter names `Tools/parity_*`. So a change could move the declaration and call-site graph the ledger measures without the gate that measures it ever running. That is not hypothetical. Earlier today #2557 took the only production call to `primarySessionRestingHR` on both platforms, turning it test-only, and the two resulting findings went unrecorded. Seventeen green checks, none of them this one. `main` carried a red ratchet until #2581, and it surfaced on #2577, an outside contributor's PR that had caused none of it. parity-governance.yml's own header notes that shape had already happened four times, three of them on an outside contributor. Here, and not behind a filter naming product paths, for the reason the guard test below already gives about a different suite: the list would be whatever the assertions happen to read today, so a filter only moves the trap. #1691 is the precedent, where a `Tools/**` filter silenced a suite that reads `TodayScreen.kt` and `AppModel.swift`, and main carried it red through nine merges. Its own step and its own floor, so the core floor of 114 stays independent, which is the reasoning already written beside that floor. Cost, measured on this runner rather than estimated: 51 s for 49 tests, against roughly 25 s for the rest of the leg. I first assumed this suite cost 4 to 6 minutes and designed a separate parallel workflow around that; the 4 to 6 minutes is `parity_ratchet.py`, which checks out the base tree and derives twice. This suite derives once. The guard is extended to assert the unfiltered leg actually invokes the module. The failure mode is not someone arguing against this, it is someone dropping the step or hanging a filter off it later, which is exactly what #1691 was. Mutation-verified: pointing the step at a different module makes the guard fail. The assertion reads the dotted module path, which appears only in the invocation and not in the prose above it, so a comment cannot satisfy it. Verified: `tests.test_parity_governance_acceptance` plus `tests.test_parity_disposition_kinds`, 51 tests, pass. The workflow parses. Refs #2567, #2557, #1691
ryanbr
added a commit
that referenced
this pull request
Sep 29, 2026
`Tools/tests/test_parity_governance_acceptance.py` does not test the scanner. It asserts that the CHECKED-IN authority and baseline still reproduce from the CURRENT product source, so a product change is what invalidates it. It ran only in parity-governance.yml, whose filter names `Tools/parity_*`. So a change could move the declaration and call-site graph the ledger measures without the gate that measures it ever running. That is not hypothetical. Earlier today #2557 took the only production call to `primarySessionRestingHR` on both platforms, turning it test-only, and the two resulting findings went unrecorded. Seventeen green checks, none of them this one. `main` carried a red ratchet until #2581, and it surfaced on #2577, an outside contributor's PR that had caused none of it. parity-governance.yml's own header notes that shape had already happened four times, three of them on an outside contributor. Here, and not behind a filter naming product paths, for the reason the guard test below already gives about a different suite: the list would be whatever the assertions happen to read today, so a filter only moves the trap. #1691 is the precedent, where a `Tools/**` filter silenced a suite that reads `TodayScreen.kt` and `AppModel.swift`, and main carried it red through nine merges. Its own step and its own floor, so the core floor of 114 stays independent, which is the reasoning already written beside that floor. Cost, measured on this runner rather than estimated: 51 s for 49 tests, against roughly 25 s for the rest of the leg. I first assumed this suite cost 4 to 6 minutes and designed a separate parallel workflow around that; the 4 to 6 minutes is `parity_ratchet.py`, which checks out the base tree and derives twice. This suite derives once. The guard is extended to assert the unfiltered leg actually invokes the module. The failure mode is not someone arguing against this, it is someone dropping the step or hanging a filter off it later, which is exactly what #1691 was. Mutation-verified: pointing the step at a different module makes the guard fail. The assertion reads the dotted module path, which appears only in the invocation and not in the prose above it, so a comment cannot satisfy it. Verified: `tests.test_parity_governance_acceptance` plus `tests.test_parity_disposition_kinds`, 51 tests, pass. The workflow parses. Refs #2567, #2557, #1691
This was referenced Sep 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
mainfails its own ratchet at5de051909, on a clean tree, with nothing applied:I caused that by merging #2557.
What happened
#2557 took the daily resting HR from the primary night's gated five-minute floor instead of the whole-session mean. That replaced the only production call to
primarySessionRestingHRon both platforms:The function is now reached from four test files and nothing else, so the conservative lexical call graph emits one new
test-only-callsitefinding per platform. The checked-in baseline did not record them.This records them. Two lines: 43 to 44 in
Packages/StrandAnalytics, 64 to 65 inandroid/analytics. Nothing else moves.Why nothing caught it
#2557 touched no
Tools/parity_*path, soparity-governancenever ran on it. Its seventeen green checks were real; they did not include the gate that governs call-site structure. #2575 and #2576 landed the same way earlier today and happened to be harmless.This is the shape
parity-governance.yml's own header describes, and it names the count: it had happened four times, three of them landing on an outside contributor's PR that caused none of it. This is the next one, and it surfaced on #2577, which is innocent and will pass once this lands.Why my review of #2557 did not catch it
I checked that
primarySessionRestingHRWithCoveragestill had production callers and concluded the old mean was not dead code. That is true of theWithCoveragevariant and false of the plainprimarySessionRestingHR, which is the one #2557 orphaned. I verified the neighbouring symbol and reported the wrong conclusion.Recording, not deleting
Deliberate. The findings are true, and the baseline is where accepted findings live. Whether
primarySessionRestingHRshould survive as a tested-but-unused function, or be deleted with its tests repointed at theWithCoveragevariant, is a change to code merged an hour ago and deserves its own review. A governance repair that also deletes product code gets neither half looked at properly.Recording does not make the debt go away, it makes it visible. The next change in either scope inherits 44 and 65 as the ceiling, so a second orphaned declaration there fires the gate again. That is the ratchet working as intended.
Verification
parity_ratchet.py --base origin/mainon this branch:errors=0, no warnings. The branch was pinned identical before and after the run.Refs #2557, #2567