tools(parity): give the stale-base dead end an exit, then take it - #2233
Merged
Merged
Conversation
The refusal on a stale base says "migration required" and no migration existed. This adds it. ## What was actually wrong Two things I wrote in #2229 were incorrect, and the situation is worse than the issue describes rather than milder: * `--bootstrap-map --write-baseline` was never an available lever. It refuses outright when an authority is already checked in: "for initial authority creation only; use the reviewed refresh workflow for existing authority". * it could not have discarded the reviewed typed dispositions anyway. No refresh path writes `Tools/parity_dispositions.json`; the ledger writes only the twin map and the baseline, and reads dispositions from their own file. So there was no lever at all. I stopped for a reason that does not hold. Also disproved, at the cost of two full runs: the theory that a better base would work. `8039ad166` is the commit that last refreshed the twin map, and its authority still cannot be reproduced, so base selection was never the fix. ## The remedy `--migrate-authority` re-bases the comparison onto a freshly derived base authority when the base's stored manifest cannot be reproduced by the current derivation. `--repair-stale-base` cannot serve here by design: repair is for a base whose GOVERNED STATE is unchanged, and a tree carrying new files is exactly the case it refuses. Migration waives ONE thing, the reproducibility of the base's stored manifest. It waives no semantic debt. `required` is still computed from the freshly derived base and current sets, so every new one-sided declaration still needs its own issue-bound disposition and an undeclared one still fails. That is the load-bearing guard and it is pinned: if it ever inverts, the flag becomes a way to launder undeclared declarations onto main. The flag requires `--refresh-derived`, is mutually exclusive with `--repair-stale-base`, and the ordinary refusal now names it, because a dead end that does not name its exit is what made this take a day. ## Tests Six, in the existing synthetic fixture so they stay fast: * migration accepts a stale base when the new debt IS declared * migration still rejects undeclared new debt * migration rejects a hand-edited current authority * the flag requires the guarded refresh * the two remedies are mutually exclusive * the ordinary refusal names the flag Collected count measured, not assumed: 121. Floor raised from 115 to match. ## What this does not do It does not make main green. The 55 requirements from Lift Log still have to be declared, which is design follow-through rather than bookkeeping and remains the open half of #2229. Two `RepositoryBaselineTests` still fail on the 490 against 495 file-count mismatch; verified in a pristine worktree at eb34f3c that they fail identically without this change.
The refresh the previous commit unblocks, run and committed.
python3 Tools/parity_ledger.py --refresh-derived --migrate-authority
WROTE reviewed derived snapshots (299 known findings)
This is the ordinary maintenance action, the same one as fe0f5c9 after the
Oura merges. The authority now records 495 scanned files against the 490 it
carried, and `RepositoryBaselineTests` goes from two failures to ten passes.
A correction to how I have been describing this. The "55 requirements" I quoted
on #2229 and in the Lift Log twin were an artifact of the base I chose to
measure against, `3c8bfe6595`, which predates Lift Log. Against the default base
the tool actually uses, `origin/main`, the delta is ZERO: that debt is already
on main, so there was never anything to disposition. The ratchet governs debt
that is new RELATIVE TO BASE, and re-deriving records what main already carries
rather than waiving anything.
That also means the "decide which spreadsheet internals are Apple-only" step I
proposed is not a prerequisite for a green board. It may still be worth doing on
its own merits, as design follow-through on Lift Log, but it was never what was
blocking this.
`PARITY_GOVERNANCE.md` names `--repair-stale-base` as the remedy when "the exact base already contains stale metadata". That is the one case it cannot serve: repair requires the governed state to be UNCHANGED between base and tree, and a base carrying merged product files is exactly a base whose governed state differs, so the equality it insists on can never hold. The maintainer following the documented recovery reaches the same dead end the flag was meant to clear. Adds the migration variant next to it, with what it does and does not waive, and states plainly what a post-merge refresh means: it ADOPTS the one-sided declarations already on `main` into the authority, because the ratchet governs debt relative to the base and merged debt is the base. That is the intended maintenance action rather than a loophole, and it is why the daily schedule earns its place. Found by re-reading my own change rather than by the suite: no test reads this file, so nothing would have gone red with the remedy left undocumented, and #2229 asked for the DOCUMENTED recovery to work.
Found by mutating the guard rather than by re-reading it, which is the only way
the difference showed up.
`test_authority_migration_rejects_a_hand_edited_current_authority` was passing
for the wrong reason. It tampers by incrementing a count, which matches neither
the tree nor the base, so the PRE-EXISTING protection caught it and the
migration-specific check was never exercised. Removing the migration check left
that test green.
The shape that check actually exists for is narrower. The general protection
only ERRORS when the authority matches neither side:
if current_map["authority"] != current_manifest:
if current_map["authority"] == old_map["authority"]:
warnings.append("debt decreased; ...") # only a warning
else:
errors.append("authority matches neither ...")
So a current authority equal to the STALE BASE merely warns. Under migration
that would sail through and adopt an unrefreshed authority, which is the one
thing migration must not do.
The new test pins that shape and also asserts no migration-success warning is
emitted, so a future change cannot both refuse the authority and report the
migration as done. Removing the migration check now turns BOTH tests red.
Floor raised 121 to 122, measured after the test landed rather than incremented
by hand. A discovery guard sitting one below the real count has quietly lost a
test's worth of protection, which is exactly what it exists to prevent.
CI caught what three local re-review passes could not, because every one of them ran against the same wrong tree. `test_checked_in_inventory_and_baseline_match_current_sources` failed on `test-only-callsite|Packages/StrandAnalytics` after passing locally. The ledger walks the FILESYSTEM, not git, and this working tree carries gitignored `.build/` directories under every package. One of them supplies a callsite that the repository does not have, so a declaration reached only from tests looked reachable from product code. The committed baseline therefore recorded 43 test-only findings for `StrandAnalytics` where the repository has 44, 299 in total against 300. One group, one count, and the kind of wrong that CI finds and local runs never will. Re-derived in a detached worktree with no build artifacts. Only the baseline moves; the twin map was already right, since the authority sets were unaffected and only the findings differed. The full governance job in that pristine tree: 122 tests, OK. CLAUDE.md says to verify this class of result in a pristine clone. I ran the refresh in the working tree anyway, then validated it three times in the same place.
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
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.
parity-governancehas been red onmainsince Lift Log landed, and thedocumented recovery refused. This makes the recovery work, then runs it.
RepositoryBaselineTestsgoes from two failures to ten passes. The fullgovernance job as CI runs it: 121 tests, OK, no failures.
What was actually wrong
Two things I wrote on #2229 are incorrect, and the situation was worse than the
issue describes rather than milder:
--bootstrap-map --write-baselinewas never an available lever. It refuseswhen an authority exists: "for initial authority creation only; use the
reviewed refresh workflow for existing authority".
refresh path writes
Tools/parity_dispositions.json. The ledger writes thetwin map and the baseline and reads dispositions from their own file.
So I stopped for a reason that does not hold, and there was no lever at all. The
refusal said "migration required" and no migration existed.
Also disproved, at the cost of two full runs: the idea that a better base would
work.
8039ad166is the commit that last refreshed the twin map and itsauthority still cannot be reproduced, so base selection was never the fix.
The remedy
--migrate-authorityre-bases the comparison onto a freshly derived baseauthority when the base's stored manifest cannot be reproduced by the current
derivation.
--repair-stale-basecannot serve here by design: repair is for abase whose GOVERNED STATE is unchanged, which is exactly what a tree carrying
new files is not.
Migration waives ONE thing, the reproducibility of the base's stored manifest.
It waives no semantic debt:
requiredis still computed from the freshlyderived base and current sets, so a new one-sided declaration still needs its
issue-bound disposition. That guard is the load-bearing one and it is pinned. If
it ever inverts, the flag becomes a way to launder undeclared declarations onto
main.The flag requires
--refresh-derived, is mutually exclusive with--repair-stale-base, and the ordinary refusal now names it, because a dead endthat does not name its exit is what made this take a day.
The correction that matters most
The "55 requirements" I quoted on #2229 and in #2232 were an artifact of the
base I measured against. I used
3c8bfe6595, which predates Lift Log, so LiftLog's own declarations appeared as new debt needing dispositions. Against
origin/main, the base the tool actually defaults to, the delta is ZERO. Thatdebt is already on
main, so there was never anything to disposition.The ratchet governs debt that is new RELATIVE TO BASE. Re-deriving records what
mainalready carries; it waives nothing.That also retires the step I proposed and repeatedly presented as needing a
decision: "decide which spreadsheet internals in
XlsxSheetandLiftProgramSheetImporterare genuinely Apple-only". It is not a prerequisitefor a green board and never was. It may still be worth doing as design
follow-through on Lift Log, on its own merits.
The second commit
The refresh itself, which is the actual repair:
The ordinary maintenance action, the same one as
fe0f5c962after the Ouramerges. The authority now records 495 scanned files against the 490 it carried.
Tests
Six, in the existing synthetic fixture so they stay fast:
still_rejects_undeclared_new_debtrejects_a_hand_edited_current_authorityaccepts_a_stale_base_when_new_debt_is_disposedflag_requires_guarded_refresh--refresh-derivedand_repair_are_mutually_exclusiveordinary_refusal_names_the_migration_flagrejects_a_current_authority_equal_to_the_stale_baseCollected count measured rather than assumed: 122. Floor raised from 115, since
a floor that guards against broken discovery is the wrong place for a guess.
One of those tests was passing for the wrong reason
rejects_a_hand_edited_current_authoritytampers by incrementing a count, whichmatches neither the tree nor the base, so the PRE-EXISTING protection caught it
and the migration-specific check was never exercised. Removing that check left
the test green. Mutation testing found it; re-reading the code had not.
The shape the check actually exists for is narrower. The general protection only
ERRORS when the authority matches neither side:
A current authority equal to the STALE BASE therefore only warns, and under
migration would sail through and adopt an unrefreshed authority. The added test
pins that shape and asserts no migration-success warning is emitted alongside
the refusal. Removing the migration check now turns both tests red.
Documentation
PARITY_GOVERNANCE.mdnamed--repair-stale-baseas the remedy for "the exactbase already contains stale metadata", which is the one case it cannot serve.
Since #2229 asked for the DOCUMENTED recovery to work, the doc now carries the
migration variant, what it does and does not waive, and a plain statement of
what a post-merge refresh adopts. No test reads that file, so nothing would have
gone red with the remedy left undocumented.
What the refresh actually adopts, in numbers
Re-review of my own body: saying migration "waives no semantic debt" is true of
the FLAG and misleading about the PR. The flag enforces
requiredexactly asbefore. The refresh it enables then adopts the declarations already on
maininto the authority:
filesfunction_pairsproperty_pairsconstant_pairsunpaired_functionsunpaired_constantsunpaired_propertiesunpaired_filesSo roughly 56 one-sided declarations enter the authority undispositioned. They
are Lift Log's, already merged, and the ratchet governs debt RELATIVE TO BASE,
which is why nothing is
required. The accepted findings move both ways for thesame reason:
WhoopStoredrops 51 to 37 as merged code began callingpreviously test-only declarations, while
android/analyticsrises 66 to 70.Worth a reviewer's scrutiny
Two things, stated rather than buried.
Is this laundering? My reading is no: the debt is on
main, the ratchet'scontract is about the delta from base, and
fe0f5c962did exactly this afterthe Oura merges. But the honest shape of it is that product debt can enter the
authority without dispositions whenever it was merged through paths the
governance filter excludes, and this PR restores the action that does the
adopting. The daily schedule is what keeps that window to a day.
Four of the adopted findings are mine. The
android/analyticsrise isperExercise,rpeProfile,sessionLoadandmuscleCountsfrom #2232. Iwrote the code that created them and then the refresh that accepts them. Each
step is legitimate on its own and the combination deserves a second pair of
eyes.