tools(parity): make a one-sided property declarable - #2231
Merged
Merged
Conversation
`_required_v3_exemptions` built its kind strings with `name[:-1]`, so `unpaired_properties` stemmed to `add-unpaired-propertie`. The vocabulary `_validate_dispositions` accepts is `add-unpaired-property`. Those two never meet. A disposition written as `add-unpaired-propertie` is rejected by validation, and one written as `add-unpaired-property` never matches the requirement, so a one-sided property can be neither waived nor cleared and `--refresh-derived` refuses for as long as it exists. Files, functions and constants all survive the naive strip; only the `-ies` plural does not, which is why nothing caught it. No existing test declares a one-sided property, and none had appeared in the tree until Lift Log added two. Replaces the strip with explicit singulars and adds a test asserting the invariant directly: every kind the ratchet can REQUIRE must be a kind a disposition may DECLARE. Reverting the fix fails it on the exact string. Nothing else referenced the misspelling, and the removal side already read correctly because "pairs" strips to "pair". The workflow runs an explicit module list rather than discovery, so the new test would not have run in CI. Adding it to that list and raising the collected-count floor from 113 to 115: the three existing modules produce exactly 113, so the floor tracks the real total rather than sitting loosely below it, and leaving it alone would have let these two tests disappear later without anything going red.
ryanbr
force-pushed
the
fix/parity-property-disposition
branch
from
September 15, 2026 07:55
7f5b6a7 to
069cffa
Compare
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.
A one-sided property could never be dispositioned, so once one existed
--refresh-derivedrefused permanently. Lift Log added two, which is how it surfaced.The mismatch
_required_v3_exemptionsbuilt kind strings withname[:-1]:_validate_dispositionsacceptsadd-unpaired-property. The two never meet:add-unpaired-propertieand validation rejects the registry outrightadd-unpaired-propertyand it never matches the requirement, which stays unsatisfiedcheckedis keyed on(item["kind"], item["identity"])and compared directly againstrequired, so the strings have to be equal. The debt can be neither waived nor cleared, and the refresh refuses for as long as the declaration exists.Only the
-iesplural breaks. Files, functions and constants all survive the strip, and the removal side is fine becausepairsstrips topair.Why it went unnoticed since #1534
No test ever declared a one-sided property. Every existing case in
test_parity_governance_acceptance.pyandtest_parity_ledger.pyusesadd-unpaired-functionoradd-unpaired-constant, and until Lift Log there was no one-sided property in the tree either. The path was unreachable, so the typo was invisible.The fix
Explicit singulars instead of
name[:-1], with a comment recording why the naive strip is a trap here.The test asserts the invariant rather than the spelling: every kind the ratchet can REQUIRE must be a kind a disposition may DECLARE. It builds a real disposition for each required kind and runs it through
_validate_dispositions, so any future set name that stems badly fails immediately.Mutation-checked. With the fix reverted:
Restored, both pass.
Nothing anywhere referenced the misspelling, so there is no migration: the dispositions registry is currently empty.
Scope
This unblocks the mechanism only.
mainis still red forparity-governancebecause the authority is stale (#2229), and clearing that needs dispositions for the one-sided declarations Lift Log introduced, which is a larger set than I first reported and is still being enumerated. That work follows separately; this lands on its own because the vocabulary defect is real regardless of how the Lift Log drift is resolved.Second pass: the test was not going to run
Re-reviewing my own change caught that the governance job uses an explicit module list rather than discovery:
A new file under
Tools/tests/is matched by the workflow'spaths:filter, so it would have triggered the job while never being collected by it. The regression test would have sat there proving nothing. Addedtests.test_parity_disposition_kindsto the list.That is the same defect I raised on #2103, where a new test file was never collected by
discover, and on the watch-build trigger gap. Having shipped it myself in the first push is worth recording rather than quietly patching.The floor moves 113 to 115 in step. The three existing modules collect exactly 113, so that number tracks the real total rather than sitting loosely underneath it; leaving it alone would have let these two tests disappear later with nothing going red, which is precisely what the floor exists to prevent.
Verified locally, since this PR's own
parity-governancecannot report on it:Same two failures either way, so they are the inherited authority staleness rather than anything here.
test_core_tools_filter_covers_every_governance_tool_pathstill passes against the edited workflow, which matters because it reads that file's text and pins the path list.About the red check
parity-governanceis failing on this PR for the reason described in #2229:main's checked-in authority is stale, and any PR touchingTools/parity_*inherits it. It is the same red @bhelm is sitting behind on #2221. Nothing in this change can clear it, and the check cannot vouch for this change either way until the authority question is settled.