source_year reviewed exclusion + schema-2 approval receipts with expiry (#630, #610) - #658
Conversation
4de107e to
b5b94dc
Compare
vahid-ahmadi
left a comment
There was a problem hiding this comment.
The shape here is right, and the part I most expected to be fudged isn't: moving the frozen policy digest because the register changed, and pinning the superseded digest to exactly the grandfathered release ids, keeps the "every release matches exactly one reviewed policy digest" property instead of loosening the check. The comment in contract.py saying the legacy branch is currently defensive and becomes load-bearing later is honest about its own status, which is rarer than it should be.
The register entry itself is a good reviewed exclusion — it says why household.source_year is constant by construction and what the column is actually for, rather than just naming it.
Two substantive notes.
1. The register is a shared mutable dict, which undercuts the tripwire.
UK_DEFAULT_DEGENERATE_REVIEWED_EXCLUSIONS: dict[str, str] = (
load_uk_reviewed_exclusion_register(None, resource=...)
)load_uk_reviewed_exclusion_register returns a fresh dict[str, str], so this is a mutable module-level object that is then handed straight to the gate:
if reviewed_degenerate_exclusions is None:
reviewed_degenerate_exclusions = UK_DEFAULT_DEGENERATE_REVIEWED_EXCLUSIONSAnything that mutates that mapping — a test that pops an entry and doesn't restore it, a caller that treats its argument as owned — changes the policy of record for the rest of the process, while UK_TERMINAL_GATE_POLICY_SHA256 was computed once at import and stays stale. The digest would keep attesting a policy that is no longer in force, which is the one failure this whole mechanism exists to prevent.
MappingProxyType(...) around it (or returning a copy at the use site) closes that off for one line of code. Given the register is explicitly "the policy of record", I'd make it immutable.
2. Reading a package resource at import time.
The same assignment turns importing terminal_gates into file I/O: if degenerate_reviewed_exclusions.json is missing, unreadable, or malformed, the module fails to import, and everything downstream reports a confusing ImportError rather than a clear "the committed register is invalid". It also means UK_TERMINAL_GATE_POLICY_SHA256 — a constant other modules compare against — is now derived from the filesystem at import.
The neighbouring UK_DEFAULT_ZERO_WEIGHT_STRATA is a literal, so this is a new kind of dependency for this module. Sealing the digest over the committed file is clearly the intent and I don't think that's wrong; a lazy functools.cached accessor would get you the same sealing without the import-time coupling, if it's easy.
3. Minor
test_committed_degenerate_register_is_the_policy_of_recordasserts"lineage plumbing" in reason. That pins prose — rewording the register entry for clarity breaks the test for no semantic reason. The"microcosm#630" in reasonassertion is the durable half; I'd keep that one and drop the other.- There's an unrelated formatting change in
contract.py(thefailures.append(...)reflow around line 952) that isn't part of this change. --degenerate-exclusionshelp text is good — explicitly saying the override "changes the run's policy digest away from the certified pin" is the right warning to put in front of someone reaching for it.
4. Process
This targets uk-scale-ladder-627 (#656), which is still a draft, so this can't land until that does. Worth confirming that's deliberate stacking rather than a base-branch left over from development — the change itself doesn't obviously depend on the scale ladder.
a1154ec to
4f5ad7f
Compare
vahid-ahmadi
left a comment
There was a problem hiding this comment.
The schema-2 upgrade is a real improvement on what I reviewed. Making every register entry a complete approval receipt — reason, approver, adjudication, approval date, expiry — and sealing the whole record into the digest means "who said this was fine, and when does that lapse" is now answerable from the artifact. expires_on > approved_on validated at load, expired() honouring the entry through its date, and the {column: reason} projection preserving the shared US-consumed gate layer are all careful.
Recording exclusions_evaluated_on in the gate details is the detail that makes the expiry mechanism auditable rather than mysterious — a reader of a failed report can see which clock produced the verdict. Good.
Three things, one of them concrete.
1. The type annotation on the default register is now wrong.
UK_DEFAULT_DEGENERATE_REVIEWED_EXCLUSIONS: dict[str, str] = (
load_uk_reviewed_exclusion_register(None, resource=...)
)The loader's return type moved to dict[str, UKReviewedExclusion] in this same PR, and the values are dataclasses — your own test reads record = UK_DEFAULT_DEGENERATE_REVIEWED_EXCLUSIONS["household.source_year"] and then goes at .reason/.adjudication. So dict[str, str] is a stale annotation that now actively lies about the shape.
Worth fixing on its own, but also worth asking why nothing caught it — if this package is type-checked in CI, that should have been an error, and if it isn't, that's useful to know given how much of this design leans on declared shapes.
2. My mutability point from the first pass is still open.
UKReviewedExclusion being @dataclass(frozen=True) covers the values — good, and it wasn't frozen in what I reviewed. But the container is still a plain module-level dict handed straight to the gate:
if reviewed_degenerate_exclusions is None:
reviewed_degenerate_exclusions = UK_DEFAULT_DEGENERATE_REVIEWED_EXCLUSIONSso a pop anywhere in the process still changes the policy of record while UK_TERMINAL_GATE_POLICY_SHA256, computed once at import, keeps attesting the original. MappingProxyType is already imported in weighted_integrity.py and used twice in it, so the idiom is right there.
This matters a little more now than it did: with schema-2 the register is the approval record, so a mutated container means the digest attests approvals that aren't in force.
3. Expiry renewal will invalidate the certified digest — is that planned for?
The whole record is sealed into policy_sha256, expires_on included. So on 2027-02-10, renewing household.source_year isn't just a register edit: it moves the frozen digest, which means updating UK_TERMINAL_GATE_POLICY_SHA256, the microcosm-data pin, and pushing the current digest onto the legacy-vintage list the way this PR does for the June release. Meanwhile any build after 2027-02-10 that hasn't renewed fails the degenerate gate.
That's the mechanism working as designed — "cannot rot" is the point, and I'm not arguing for a softer expiry. But it's a dated operational obligation that currently exists only inside a JSON file, and the person who hits it will be whoever runs a build that day, not whoever approved the exclusion. Worth an issue with the date on it so the renewal is scheduled rather than discovered.
4. Scope.
This started as a 12-file, +100/-9 change adding one reviewed exclusion, and is now +568/-107 carrying the #610 schema-2 upgrade across all three registers plus the expiry machinery. The #610 half is independently reviewable and is the part with the new failure modes; the #630 half is the one-line adjudication. Not asking you to unpick it if the two genuinely co-designed, but the PR title still only mentions #630, and a reader coming to this from the #630 issue will not expect to find a register schema migration in it.
…610) Addresses vahid-ahmadi's review of #658: - UK_DEFAULT_DEGENERATE_REVIEWED_EXCLUSIONS and UK_TERMINAL_GATE_POLICY_SHA256 become cached accessors (uk_default_degenerate_reviewed_exclusions / uk_terminal_gate_policy_sha256) returning a MappingProxyType-wrapped register: the policy of record can no longer be mutated out from under the already-computed digest, and importing terminal_gates no longer reads the filesystem — a broken committed register surfaces as a clear ValueError at first use instead of an ImportError. The digest value is unchanged (ae93bd10…): laziness moves when the payload is read, not what it contains. - The register-of-record test drops its prose pins ("schema symmetry", "derivable") — rewording the reason still moves the frozen digest, so the durable assertions are the structural receipt fields plus the microcosm#630 adjudication. - New regression test: the register is loaded once, is immutable, and the digest is stable across calls. - The unrelated failures.append reflow in contract.py is reverted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks @vahid-ahmadi — your points land in 2ccaa01, and a three-lens adversarial review of the whole PR followed in 98daf62. The policy digest is unchanged throughout ( Your review (2ccaa01): 1 + 2. Mutable shared dict / import-time file I/O — fixed together, your suggested shape. Both module-level assignments became cached accessors: 3. Minor — both taken. The prose pins ( 4. Process — deliberate stacking. #658's driver hunks are written against #656's rewritten driver ( Adversarial pass (98daf62): three independent reviewers (expiry semantics; digest/contract isolation; register loading/driver) tried to refute the PR's claims. Cross-validated findings, all fixed:
Two attacks were refuted with no change needed: duplicate JSON keys are rejected at every nesting level by the loader's Suites: 386 tests across the affected files plus the full two-package battery green (three known environmental regeneration/live-load failures unrelated to this PR; CI adjudicates). |
The committed uk/degenerate_reviewed_exclusions.json register is the degenerate-release-surface policy of record, carrying the #630 adjudication: the column is constant at the build vintage by construction — lineage plumbing for the rowwise clone's source_household_key that documents the vintage at the row level, not signal. A None argument to uk_terminal_gate_report resolves to the register (pass {} to run bare); stale entries still fail the gate, so the register cannot rot. Interim schema-1, matching the two existing registers; all three upgrade together when the #610 approval-identity/ receipt/expiry design lands. The frozen policy digest moved with the register — the intended tripwire — and the microcosm-data pin is now vintage-aware: new releases must attest the #630 policy while the grandfathered June release keeps attesting its own pre-#630 digest (defensive today, since the terminal-report checker only runs for exact-k ids). The driver gains --degenerate-exclusions as a review-time override, wired through the distinct-path check. Live receipt: the full-scale evidence run at seed 7 evaluated the battery with the register active and degenerate_release_surface passed with the exclusion recorded; only weight_ratio (1592.18 vs the 1151.25 maximum, its own #630 adjudication) still fails at full scale. Refs #630, #627, #610. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Resolves the TODO(PR #610 review): every admitted exclusion now records who approved it, the reasoning, the adjudication it descends from, when it was approved, and when it expires. One frozen UKReviewedExclusion record, one validator, one loader — shared by all three registers and any future entry; nothing in the schema knows about source_year. The whole record is sealed into the policy digest (74c9cd -> 2dbd78 -> ae93bd10 across this PR's commits), so editing an approver or extending an expiry moves the pinned literal. Expiry is enforced at gate evaluation with an injected clock — never at load, where the committed registers import at module load and a lapse date would brick every build at once. An entry is honored through expires_on; strictly after it, the exclusion stops suppressing and the gate fails with one combined renew-or-remove message naming the approver, the adjudication, and the lapse date. Details gain additive expired_exclusions and exclusions_evaluated_on keys on all three gates, and the contract requires expired_exclusions to be empty on published reports (absent fields default to empty, so grandfathered reports stay total). The shared US-consumed gate layer and the contract's flat str->str pin on QRF exclusion details are untouched: the UK wrappers project records down to plain reasons for non-expired entries before delegating, and withhold expired ones so the underlying failure fires beside the expiry context. The degenerate gate consumes records directly and its nested details entries carry the approver, adjudication, and expiry. The source_year entry keeps Max's #630 adjudication with the reason corrected to the verified mechanics: the column is derivable from the artifact's time_period on a single-vintage build and is retained for row-level vintage documentation and UK/US schema symmetry (the US pools three ASEC vintages, where the same column is irreducible per-row identity). approved_by juaristi22, expires 2027-02-10 — a plain six-month review, since multi-vintage stacking would auto-retire the entry through the stale discipline regardless. Refs #610, #630, #609. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…610) Addresses vahid-ahmadi's review of #658: - UK_DEFAULT_DEGENERATE_REVIEWED_EXCLUSIONS and UK_TERMINAL_GATE_POLICY_SHA256 become cached accessors (uk_default_degenerate_reviewed_exclusions / uk_terminal_gate_policy_sha256) returning a MappingProxyType-wrapped register: the policy of record can no longer be mutated out from under the already-computed digest, and importing terminal_gates no longer reads the filesystem — a broken committed register surfaces as a clear ValueError at first use instead of an ImportError. The digest value is unchanged (ae93bd10…): laziness moves when the payload is read, not what it contains. - The register-of-record test drops its prose pins ("schema symmetry", "derivable") — rewording the reason still moves the frozen digest, so the durable assertions are the structural receipt fields plus the microcosm#630 adjudication. - New regression test: the register is loaded once, is immutable, and the digest is stable across calls. - The unrelated failures.append reflow in contract.py is reverted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…610) Three independent adversarial lenses reviewed the PR; every cross-validated finding lands here. The policy digest is unchanged (ae93bd10…) — these fix behavior and validation, not the sealed payload. - The degenerate gate now fails ANY out-of-force exclusion — dormant, signal-regained, or live — with receipt context, matching the input-mass and QRF wrappers (all three lenses found the dormant-expired silent pass: a green build would publish a report every consumer then rejects). - Receipts gain an in-force window: an entry whose approved_on is still in the future is not an approval and never suppresses (previously fail-open for a typo'd future year). New premature_exclusions details key on all three gates, with contract expectations. - Receipt dates must be canonical YYYY-MM-DD (fromisoformat also accepts compact and week-date forms; the raw string is sealed, so two spellings of one date minted two digests) and text fields must be trimmed. - uk_terminal_gate_report coerces and freezes the register once at entry: the gate and the attested digest can no longer observe different contents when caller-controlled evaluators mutate the mapping mid-report. - The driver loads the degenerate register (override or committed preflight) before the destructive sidecar unlinks — a typo'd --degenerate-exclusions path no longer destroys the previous build's evidence first — and the build record notes committed-vs-override register provenance (content-addressed digests can't show it). - Contract: the three new detail fields join the required schema (a key-signed report can no longer omit them to dodge the empty-list expectations, and the absent-field defaults that had silently weakened four pre-existing checks revert to strict); exclusion-consuming gates must share one exclusions_evaluated_on; the legacy policy pin gains a lockstep test (it was asserted nowhere). - exclusion_evaluation_date() refuses datetimes (a date subclass that compared timestamps against dates or leaked a timestamp into details). - Stale Mapping[str, str] annotation on build_uk_national_dataset fixed. Refuted during review (no change needed): duplicate JSON keys are rejected at every nesting level by the loader's object_pairs_hook; the attested policy_sha256 is recomputed from the actual register in force, so an override cannot ride the certified pin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
98daf62 to
544dfe6
Compare
vahid-ahmadi
left a comment
There was a problem hiding this comment.
2ccaa01 closes my points in exactly the shape I'd have written, and 98daf62 goes considerably further than I asked.
The accessor pair is right: @functools.cache + MappingProxyType, lazy so importing the module never touches the filesystem, cached so every caller seals the same load, read-only so the policy of record can't drift from the already-computed digest. The docstring naming all three properties is what makes it maintainable. The immutability test asserting both TypeError on __setitem__ and AttributeError on .pop pins it properly.
I checked the one thing that could have quietly undermined this, since MappingProxyType is a view rather than a copy: if the caller's mapping were wrapped directly, a caller holding a reference could still mutate it mid-report and defeat the freeze. It doesn't — coerce_reviewed_exclusions always populates a fresh records dict, including on the already-typed pass-through path, so the proxy wraps something the caller has no handle on. The freeze is genuine.
Also confirmed the tripwire survived the constant-to-function move: uk_terminal_gate_policy_sha256() is still pinned to a literal in the build tests, and ae93bd10… appears in three places in lockstep (build test, contract.py, test_contract.py). That was the property most at risk in this refactor and it's intact.
On the adversarial pass — the expired-but-absent-column hole is the one I'd single out. An out-of-force entry producing no gate failure because the column had gone absent, caught only by the contract at publish time, is precisely the "cannot rot" promise failing in the one state nobody thinks to test. Good that it's now enforced at every column state. The date.fromisoformat finding is a nice catch too: it accepting 20270210 and week-date forms while the raw string is sealed means two spellings of one date mint two digests, which would have been baffling to debug.
The residual you name — a key-holding builder injecting a past now to revive expired approvals, with the coherence check catching mixed dates but not a consistent lie — is correctly scoped as a trust-boundary question rather than something the gate can close. Agreed it belongs on the #611 consumer side.
Nothing further from me. Base is retargeted to main now that #656 has landed, and it reports mergeable.
The exclusion_evaluation_date helper took over the module's only datetime.now(UTC) call; only date remains in use. Local ruff missed the F401 (version drift against CI's pin). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rebasing onto main surfaced a real divergence the differential tests caught: #658 made the committed schema-2 register the terminal report's default degenerate policy, so the legacy path ran the policy of record while the battery binding ran bare — dormant/expired state differed (household.source_year dormant in one, absent in the other). The binding now follows the spec's own idiom for the other two exclusion gates: uk/gates.json declares reviewed_exclusions_resource, the evaluator pins the declared name against the runtime's register constant, and the gate runs uk_default_degenerate_reviewed_exclusions() — the exact policy of record the legacy report resolves for None. The entry's note is updated: the anticipated #630 approval landed via #658. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Vahid's follow-up on the override-label fix: the signed report's evidence
labeled by content ("committed" for a register-identical review file)
while the #658 build record labeled by presence ("override" whenever the
flag was passed) — opposite answers under near-identical names in two
artifacts of the same build. Both questions are real, so each now carries
its own name: the evidence payload's exclusions_policy answers "which
register content governed this run", and the build record's boolean
degenerate_exclusions_override_supplied answers "did the operator invoke
the override path". The two can honestly disagree, and both artifacts now
say so in their comments.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
S4a of the #627 lane (originally stacked on #656, retargeted to main after its merge) — four commits: the first of #630's adjudications encoded as declarative policy, the #610 TODO resolved on top of it, then two hardening commits from review — vahid-ahmadi's points and a three-lens adversarial pass (details in the PR comments; the policy digest
ae93bd10…is unchanged by both).Commit 1 —
household.source_yearbecomes a reviewed degenerate exclusion (#630)uk/degenerate_reviewed_exclusions.jsonis the committed register and the degenerate-surface policy of record (Noneresolves to it;{}runs bare); the gate's stale-entry discipline still applies — ifsource_yearever regains signal, the entry fails the gate.--degenerate-exclusionsreview-time override, registered in the distinct-path fence.degenerate_release_surfacepassed with the exclusion recorded; onlyweight_ratiostill fails at full scale (1590.53/1592.18 across the seed pair vs the 1151.25 maximum — National staging rebuild fails the current terminal gates: constant source_year release column and SPI weight tail above the reviewed maximum #630 finding 2, the remaining owner-held adjudication).Commit 2 — exclusion registers become schema-2 approval receipts with expiry (#610)
Resolves the
TODO(PR #610 review): every admitted exclusion is a complete, case-independent approval receipt — reason, approver, adjudication reference, approval date, expiry — one frozenUKReviewedExclusionrecord, one validator, one loader, shared by all three registers and any future entry.74c9cd…→2dbd78…→ae93bd10…across this PR): editing an approver or extending an expiry moves the pinned literal.approved_onthroughexpires_on; outside that window it stops suppressing and every gate fails with receipt context at any column state — expired, not-yet-in-force, dormant, or signal-regained — naming the approver, adjudication, and boundary date. Details gain additiveexpired_exclusions/exclusions_evaluated_onkeys, and the contract requiresexpired_exclusionsto be empty on published reports (absent fields default to empty, keeping grandfathered reports total).gates.pylayer and the contract's flatstr → strpin on QRF exclusion details are untouched — the UK wrappers project records to plain reasons for non-expired entries and withhold expired ones so the underlying failure fires beside the expiry context. The UK-only degenerate gate consumes records directly; its nested details entries carry the approver, adjudication, and expiry.source_yearentry keeps the National staging rebuild fails the current terminal gates: constant source_year release column and SPI weight tail above the reviewed maximum #630 adjudication with the reason corrected to the verified mechanics: derivable from the artifact'stime_periodon a single-vintage build (the rowwise clone path re-derives it when absent; the write-onlysource_household_keyis its only reader), retained for row-level vintage documentation and UK/US schema symmetry — the US pools three ASEC vintages, where the same column is irreducible per-row identity.approved_by: juaristi22,adjudication: microcosm#630,expires_on: 2027-02-10— a plain six-month review; multi-vintage stacking would auto-retire the entry through the stale discipline regardless.Closes #627 — the ladder itself (identity-bound
sample_fraction, the three rungs, the boring-before-billed discipline) merged in #656; this PR is that lane's outstanding tail, the adjudication receipts the rung evidence demanded, so the issue closes here.Refs #630, #610, #609.
🤖 Generated with Claude Code