Repository navigation
Add fail-closed cross-platform parity governance - #1534
Conversation
|
Reviewed as a draft — no merge decision, and I've respected the stacking (#1531 first). I ran the tool rather than reading it, including trying to defeat it. It works, and I checked that adversariallyStale twin reference. I renamed File, line, and target. That is the message a contributor can act on. New one-sided declaration, with the authority regenerated to hide it. This is the claim I most wanted to test, since it's the one that decides whether the whole thing is worth having. I added a Kotlin-only function, hit the ledger failure, then ran the remedy your own doc gives — The ratchet was not fooled: Exact kind, language, file, symbol — after both checked files were regenerated. "Regenerating both checked files cannot hide new debt" is a real property, not a claim. That is the part worth building on. What I'd want before it leaves draft1. Nothing runs the ratchet. No workflow is touched, and neither script is referenced anywhere in 2. The CI floor isn't raised, and this is your own failure mode. 3. The ledger's diagnostic for new debt is undiagnosable. The stale-twin message is excellent; this one is not: No file, no symbol, no kind. Cost, stated neutrally2,373-line scanner, 567-line ratchet, 1,531 lines of tests, and the Tools suite goes from 3s to 55s. Stdlib-only with no new dependency, which is the right call for this repo. Whether that maintenance surface is worth the drift protection is the maintainer's call, not mine — I'm recording the number rather than arguing it either way. Two things I checked and found clean: the tool touches no product code paths, and the JSON authority really is 65 lines rather than a generated inventory dump. |
|
Thanks for the adversarial review — all three pre-ready items are now addressed in the draft:
Verification on the amended candidate: 138/138 Tools tests, ledger green, offline ratchet This remains a Draft stacked on #1531. After that dependency merges, it still needs the planned rebase, authority regeneration, exact discovery-floor refresh against the new base, and final review before it can be considered ready. |
d580b22 to
a4c68ee
Compare
|
Follow-up on the failed refresh CI: GitHub tested the stacked merge ref after The draft is now rebuilt from current Final local evidence: 167/167 Tools tests; 219 baselined findings; 424/424 declared references; 73 function pairs and 590 constant pairs; offline and online ratchets This remains a Draft. It still depends on #1531 and will be rebased/regenerated once that PR merges; parity-gate CI invocation remains deliberately reserved for the final stack PR. |
|
Re-reviewed at The three, confirmed
Also verified on the pristine tree: 219 baselined, 424/424 declared references resolved, ledger exit 0 — matching your numbers, on a tree rebuilt over #1535. New: the governance suite is flaky here
Across five more runs the failing set rotates rather than repeating — drawn from about five tests: Full-discovery runs showed it too (1 failure, then 3). What I ruled out: not hash randomisation — Your CI is 15/15 green and you report 167/167 locally, so this is environment-sensitive — I am on WSL2, and coarse mtime granularity there is a plausible ingredient. I am not claiming your evidence is wrong; I am reporting that the suite does not reproduce green on a second machine. It matters more than a normal flake because of what this tool is for. A gate whose own tests fail one run in two teaches exactly the habit the PR exists to break: re-run until green. Worth pinning before the enforcement layer lands, since after that a flaky suite blocks merges at random. The tool itself is deterministic — three runs of the ledger over the same modified tree produced 1104 findings each time with an identical first entry. So this is the tests, not the scanner, which is the better of the two. Signal-to-noise on the thing being reportedRe-running my one-sided-function test: the format is fixed, but adding one Kotlin helper produces 1,104 actionable ledger lines, with the one I caused at position 590. The first line names So the message is actionable and the finding is present; what is missing is filtering the new debt from the tree's existing one-sided inventory. Given the ratchet is the real gate and it does still name the right symbol, I would treat this as polish rather than a blocker — but a contributor's first encounter with this tool will be that wall of output. Nothing here changes my read that the ratchet's core property is real and worth having. |
|
Im waiting on #1531 before addressing the new findings, so i can un-draft this PR. |
a4c68ee to
bbf9dd3
Compare
|
Confirmed and fixed on the refreshed draft. The WSL/coarse-mtime suspicion was correct, but the stale identity existed at four layers: The regression is deterministic now: tests rewrite the same resolved path with the same byte size and restore the identical The fix removes all persistent file-metadata caches. One top-level scan owns an operation-local immutable source snapshot, including masks and parse results, and releases it afterward; direct parse APIs still read fresh on every call. No Evidence on frozen head The draft still deliberately does not invoke the parity gate from CI; that remains final-stack scope. |
|
Reviewed, @bhelm. #1531 is merged ( Since this is stdlib Python, I could actually run it rather than read it — which is rare for this repo, and worth doing given the flakiness reported earlier. The tooling worksRun against the branch's own tree:
Pointed at today's The earlier flakiness looks fixedI re-tested it directly. The one failure is deterministic — 5/5 runs, byte-identical output, not the rotating failure reported before. Whatever the per-scan snapshot change did, it held. One real blocker: the frozen authority is already stale against its own tip
The baseline records 33 test-only callsites in A design question worth answering in PARITY_GOVERNANCE.mdThe ratchet fails on a debt decrease as well as an increase — 33 → 32 is what broke it. That is defensible for an exact authority, but it means a contributor who removes parity debt gets a red build and no obvious hint that regeneration is the fix. Worth one sentence in the doc saying so, or the first person to improve something will assume they broke the gate. The rebase is larger than a refreshThe branch is 104 files and −3,670 lines behind
Two of the ten comment files also still differ from One useful confirmation for the rebase: your discovery floor of 170 still holds. Today's RuntimeThe Tools suite took ~390–400 s here for 170 tests, against the ~150 s in your notes. Machine-dependent, but it roughly triples that job. You already flagged it as a transparent P2 trade-off for dropping the persistent cache, and I think that is the right call — I would rather a slow gate than a stale one — but the maintainer should know the CI-minute cost before it is wired up. SummaryDesign and execution look sound, and the fail-closed behaviour is real. The blockers are both staleness, not correctness: rebase onto current |
|
@bhelm — draft flag is off and the checks read green, but that green is stale, so please rebase and regenerate before this lands. Why the tick is not evidenceThe check runs on this head completed 2026-08-22T16:34Z. GitHub validates the merge result, so they reflect this branch against yesterday's What the real merge doesI merged your branch onto today's That is the tool doing its job — declarations landed today that the frozen authority has never seen, refused rather than silently absorbed. The consequence is what makes it blocking: Separately, and independent of the rebase: on your current tip the same acceptance test already fails, deterministically (5/5 runs, identical output) — the baseline records 33 The fix converges, and it is one commandI ran your own bootstrap on the merge result: → I deliberately did not push that. Regenerating the frozen authority is a judgement call that belongs to you — for a tool whose entire claim is that the authority is reviewed rather than auto-accepted, having the maintainer's reviewer regenerate it on your behalf would undercut the point. What still stands from the earlier reviewEverything positive, and it is worth repeating because none of the above is a design criticism:
One small thing you can stop worrying about: your discovery floor of 170 still holds after rebase. Today's And one doc request from the earlier review, still open: the ratchet fails on a debt decrease as well as an increase — 33 → 32 is what broke your tip. Worth a sentence in Rebase, regenerate, push, and I will re-review promptly. |
bbf9dd3 to
256af7f
Compare
|
Rebased conflict-free onto current main ( |
|
Ran this three ways to see how it behaves against a moving Measurements (
Your base The regeneration is small, and that is the interesting part. plus one accepted finding — Read as behaviour rather than churn: those 21 commits added two twin pairs, paired one more file, and took parity debt down by one. The harness tracked all of it correctly, and the diff is legible enough to review by eye. That is a better result than I expected going in. Two things before this can land: 1. Regenerate on top of current 2. The merge protocol, which is the real design question. Every merge that adds a function invalidates the baseline, so a fail-closed gate risks spending most of its life blocking work that is entirely correct. The asymmetry worth weighing: a removed or retargeted twin is a genuine parity regression and should fail the build; a newly added one-sided function is usually just new code whose twin has not been written yet, and failing on that makes every unrelated PR pay for someone else's addition. If additions warned — or updated the counters in place — while only removals and retargets blocked, the gate would fire on exactly what it exists to catch and stay quiet otherwise. Interested in your view on that, since you have thought about the fail-closed stance more than I have. Enabling "require branches to be up to date before merging" on the repo would also make the staleness visible rather than silent, which is worth doing regardless of how this lands. None of this is a criticism of the approach — the harness does what it claims. It is the protocol around it that needs settling. |
256af7f to
4cb1faa
Compare
|
Thanks — my take is a proposal, not a prescription; as maintainer, the final policy call is yours. The main project risk I see is accidental Android/iOS divergence, especially when a contributor does not realize that a shared change needs both sides. “Parity Guard” might be a useful concise name for that intent. I would therefore make shared bug fixes and final shared features parity-required, while allowing one-sided experimental work or genuinely platform-specific behavior only through an explicit typed disposition. Improvements and obsolete dispositions should warn, not block; regeneration should compute the new state but never silently approve new debt. The branch is now rebased and implements that model, with 203 tests, a green ledger, and |
4cb1faa to
333665e
Compare
|
Rebased onto current The rebase did exactly what this PR is meant to do: it refused to refresh while ten newly encountered twin declarations were unresolved. I fixed those before regenerating any snapshots. Ten hard scan errors found after the rebaseThese were declaration/scanner-contract problems, not demonstrated Android/iOS runtime divergences:
Both scanner gaps have focused red-before/green-after tests. New inherited parity debt since the previous snapshotCurrent
“Finding” here means the conservative lexical guard sees a test-only surface or a mirrored expression it cannot prove equal; it does not by itself prove a user-visible platform mismatch. Each identity is nevertheless retained in the reviewed baseline, remains visible, and cannot be replaced or increased silently. Verification on the refreshed head:
This is also a concrete example of why landing the guard matters: these declarations accumulated while the guard was still outside |
c757874 to
55eacad
Compare
|
Follow-up on the remaining red Android check: The failing test is not introduced by this PR. I reproduced the exact failure on a detached, unchanged current The test came from #1631. It stores a journal row dated For completeness on provenance: the parity-debt list above intentionally includes our own work as well—#1709 is the source of the inherited raw-IMU unpack findings. No contributor or PR was exempted from attribution. All checks owned by this PR, including |
|
Reviewed this properly — merged it onto current It proved itself on live codeRunning the ledger against the merged tree produced two findings, both on one line — mine, written the day before: I'd written "The Long twin of Swift's I also checked your The failing check is not yours
What's neededRebase onto current Worth doing after #1794 lands so the dead-twin line isn't captured into the regenerated baseline. One number for @ryanbr to decide
The ledger itself is 68s / ~526 MB; the rest is the acceptance suite (284s locally). A 19× increase on the cheapest always-on gate is a real change to contributor feedback time. Options: path-filter the parity suite to On the designThe scoping is honest and I checked it against the diff: the doc says CI enforcement is deferred to a later stack PR, and the workflow change does only run the tests — no blocking parity gate on contributors yet. Splitting the core and parity suites with independent floors, because "a single total lets growth in one suite hide accidental deletion in the other", is the right instinct. The parts that address what actually goes wrong in parity audits — selector labels disambiguating overloads, "name-only suggestions are not authority" — are the parts I'd have worried about, since naming divergence between the two layers ( |
|
Clarifying my own figure above, because if you try to reproduce it you'll see something different — and the reason is worth knowing. The ledger halts on scan errors before it ever evaluates the baseline. On the merged tree today:
So both numbers are real, at different stages. My "~1299 findings" only appears after the two scan errors are fixed; run it before and you get 2 errors and no baseline information at all. That makes the sequencing advice stronger than I put it: it isn't just tidier to land #1794 first, it's that you cannot see your baseline's state while any scan error exists. One dead reference anywhere in scope hides whether the baseline is current. Worth considering whether that's the behaviour you want. Fail-fast on scan errors is defensible — a claim that doesn't resolve means the inventory is untrustworthy, so comparing against it would be comparing to noise. But the operator cost is that "my baseline is stale" and "someone wrote a bad twin reference" produce completely different output, and only the second is visible. A one-line summary at the end — "baseline not evaluated: N scan errors" — would make the masking explicit without changing the fail-closed semantics. Also worth recording: I checked the 1299 against the source. |
I wrote this in #1785: /** `a - b`, or null when it would overflow. The Long twin of Swift's `subtractingReportingOverflow`. */ subtractingReportingOverflow is Swift STDLIB on FixedWidthInteger, not a declaration in this repository, so the reference resolves to nothing. A reader chasing it finds no such function here, and neither does an audit. Found by the parity ledger in #1534, which reported it as a dead-twin-reference within minutes of being run against a merged tree - and that same PR fixes the identical mistake in AICoach.swift, where "the twin of Kotlin's LocalDate.now().toEpochDay()" became "Kotlin computes the same value with". Two of us reached for that phrasing independently, which says it is easy to reach for rather than a one-off slip. Reworded to say what is true: Apple expresses the same contract with that method, and it is stdlib rather than a twin. The file's PARITY note a few lines up already had this right - it contrasts Math.subtractExact with subtractingReportingOverflow without claiming a pairing - so this brings one line into line with the rest of the file. Fixed rather than baselined, deliberately. Accepting it as known debt would launder a defect I introduced into the accepted set, which is the opposite of what a ledger is for. Verified against #1534's ledger that this specific finding is gone: two findings on PreSleepHeartRateFeedback.kt:239 before, none after. It also unblocks that PR's rebase - the ledger halts on scan errors before it ever evaluates the baseline, so while this line stood, its baseline state could not be seen at all. Comment only; no behaviour change.
|
#1794 is merged, so the scan error is gone from Rebase order that should get you green:
The dead-twin line your ledger found is fixed rather than baselined, so it won't be captured into the regenerated set. Still worth a decision from @ryanbr before this lands: |
55eacad to
72f4d3c
Compare
|
Rebased onto current I also took the suggested scan-error UX cleanup: after listing concrete scan errors, the ledger now ends with Verified on final head
The Tools workflow remains deliberately unfiltered, matching the current acceptance contract; I did not silently turn the runtime trade-off into a path-filtering policy change in this refresh. |
|
Follow-up on the CI-runtime decision: I have now split the parity-governance suite into its own path-filtered workflow on
Full parity-governance discovery remains green: 102/102. |
The add-unpaired-function diagnostic decided "new" from added diff lines, so a pre-existing declaration whose line was rewritten was reported as newly unpaired. It now compares the base and current declaration inventories (platform, owner, name, arity); authority drift only fails closed on removals and newly unpaired identities, additions and debt reductions pass. Red test: the rewritten existing helper was reported; green after the change, a genuinely new one-sided function is still reported. 67 ledger tests and the full Tools suite pass; the PR 1979 reproduction goes from exit 1 to exit 0.
Two attached twin claims stopped resolving on current main: the SleepStager trace claimed a bare round1 that now matches twelve Kotlin functions, and the BLE client claimed BLEManager.didConnect where the Swift method is centralManager(_:didConnect:). Qualify both; no logic changes.
Regenerated through the guarded refresh against main 7055def: 466 files, 141 function pairs, 657 constant pairs, 270 baselined findings. Ledger green, ratchet errors=0 against the rebase base.
main now path-filters tools-python.yml to Tools/** plus itself, so the acceptance test asserting an unfiltered core workflow described a state that no longer exists. Assert instead that the core filter covers every governance tool path and that the governance workflow stays filtered to its own files. Discovery floor raised to the measured 103 tests.
2bd1f4f to
c8e3cbf
Compare
|
The failed parity-governance run was another moved-base race, not a scanner regression: GitHub built the merge ref after I have rebased once more onto the new current Verified before push:
No product or comment changes were needed in this round. |
PR #1534 landed with snapshots derived from its own branch base, which sat 26 commits behind main. On main the ratchet re-derives the authority from the base tree and compares it against the checked-in manifest (parity_ratchet.py:438), so the stale pin failed the push-triggered gate: 482 files against 484 actual, plus test-only-callsite drift in Packages/StrandAnalytics and Packages/WhoopStore. The guarded refresh cannot repair that state by design. It rejects with "base authority cannot be reproduced with the current derivation; migration required", because the checked-in authority never matched a derivation of any tree that also carries it. Regenerated from main's own tree instead. No new debt was accepted: known findings stay at 288, with StrandAnalytics 42 to 41 and WhoopStore 36 to 37 netting out. The authority manifest tracks only the two source files main gained (484 files, 4282 functions, 662 constant pairs). 103 of 103 parity-governance tests pass.
A one-sided PROPERTY could never be dispositioned, so once one existed the authority refresh refused permanently. Lift Log added two, which is how it surfaced. _required_v3_exemptions built its kind strings with name[:-1], so unpaired_properties stemmed to add-unpaired-propertie, while _validate_dispositions accepts add-unpaired-property. The two never meet: the stemmed spelling is rejected by validation, and the accepted spelling never matches the requirement, which is compared as an exact (kind, identity) key. The debt could be neither waived nor cleared. Only the -ies plural breaks; files, functions and constants survive the strip, and the removal side is fine because "pairs" strips to "pair". It went unnoticed since #1534 because the path was unreachable. No test declared a one-sided property and none existed in the tree, so nothing ever generated the bad kind. Replaces the strip with explicit singulars and adds a test asserting 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 per required kind and runs it through the validator, so a future set name that stems badly fails immediately. Reverting the fix fails it on the exact string. Also wires that test into the governance job, which runs an explicit module list rather than discovery: a new file under Tools/tests is matched by the workflow's paths filter, so it would have TRIGGERED the job while never being collected by it. The collected-count floor moves 113 to 115 in step, because the three existing modules collect exactly 113 and the floor tracks the real total. Verified locally, since this PR's own parity-governance cannot report on it: the exact CI block runs, the continuations assemble, the count extraction yields 115 and clears the floor. Three modules give 113 and four give 115, with the same two failures either way. Merged with parity-governance red. That failure is the stale checked-in authority described in #2229, reproduces identically on unmodified main, and is inherited by any pull request touching Tools/parity_*. Nothing here can clear it, and while it persists the check cannot validate this change either, since GitHub runs the step under bash -e and aborts before the floor check on a failing suite.
…lidation Adds the portable behavioural-test contract that the static inventory gate (#1534) does not provide: cases carrying shared inputs and literal expected results, deterministic expansion, an authoritative shard and operation registry, exact split and remerge, and input-bound result artifacts. A platform validates its own artifact against the shared expectations without building the other platform or waiting for its output; a later comparison takes both result files and checks their agreement. Both sides are checked against the expected results as well, so two implementations returning the same wrong answer cannot pass merely by agreeing. The expectations in `Tools/parity_case_specs/core_reference.json` are hand-written literals rather than values emitted by the Python reference runner, which is what makes that property real. Keep it that way in later specs: generating them would remove the property silently while every test still passed. Each case hash binds its inputs, identity, function, shard, expected result and suite version. Output metadata must match those cases and the git revision the caller requires, so a stale or mixed revision, a changed case, malformed output or a missing result fails closed. Revision metadata is a consistency contract, not proof of which code ran. Scope is deliberate and stated by the author: the only operation exercised is `Portable.clamp/3`, so this is contract and scaffolding whose value arrives with the native runners and the first real migration. Python agreement is not evidence of Swift or Kotlin product parity, and no native build or enforcement is coupled in here. The governance discovery floor moves 122 to 159 alongside `tests.test_parity_harness`, and `Tools/parity_case_specs/**` joins both workflow path filters and the acceptance test's baseline list. Verified by merging onto current main in a clean worktree and running CI's own command: 159 tests, OK. Contributed by @bhelm
2 participants
Why
Swift/Kotlin parity debt was tracked informally, so regenerating a snapshot could hide a newly one-sided implementation. This PR adds a product-free, fail-closed static governance foundation for cross-platform review.
What this PR contains
Tools/tests/**workflow coverageScope is 17 files (+5,772/-16): governance/workflow files plus two doc-comment twin claims that stopped resolving on current main (
SleepStager+Trace.swift,WhoopBleClient.kt); no product logic. Current authority: 141 function pairs, 270 baselined findings, 768/768 references resolved, and 657 constant pairs.Verification
origin/main:errors=0Deliberate boundary
This is the foundation, executable demo, and self-test layer. Even if merged, it does not enforce parity on product-source changes. PR2 adds the portable case layer, PR3 adds corpora and native runners, and only PR4 enables product-source CI enforcement after the intermediate layers have demonstrated reliable behavior.