Repository navigation
Fold the review follow-ups from #1417, #1109 and #1416 - #1553
Merged
Merged
Conversation
ITL differential check (#1417): - Name the engine variables the cells read (TAX, TOTAL_INCOME, IS_SP_AGE, AGE) and hold them, and each bound cell's mask, to the contract's policyengine bindings: a structural test of the counting keys, a differential against the build's own binding interpreter on people at every edge, and twelve binding edits that each turn exactly their family red. - Refuse --current-income-for-annual-measures unless --diagnostics or --references names the in-sample basis. - Cite the vendored band counts in the tolerance note; keep 5% and 10%. - Import UK_HMRC_ITL_UK_GEOGRAPHY_ID instead of repeating K02000001. Fiscal refresh (#1109): the estate_income exclusion also covers a run whose raw base becomes the reference. Target materialization (#1416): the docstring limits the plain-row period refusal to a row declaring a period other than the calibration period. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review of 1f506fa (should-fix): the differential handed the build the check's own band edges, so a band_upper_bound or band_period_factor on the band binding left every test green. The differential now builds real TargetSpecs for the 35 bound references (stamped by the Ledger compile's _ledger_metadata from each reference's vendored fact, under _reference_metadata), puts the edge people on a real Frame and runs materialize_target_bindings on a UKFrameTargetAdapter, so the build reads each band from the spec and scopes each area by its geography predicate. People 0.4p below each band edge make a whole-pence reading visible. The structural test now holds every binding key but notes and metric_name, and three edits join the mutation list: a capped top band (the top band alone turns red), monthly band edges and a provider kind. Nits: the tolerance-note test reads the note from the module source and derives its numbers from the rows; the tool refuses an empty --current-income-for-annual-measures reason and records the reason in the receipt; "is above zero" reads "exceeds zero". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The review of 39b8a93 found that the harness stamped each bound spec from the window's first fact (2024-25), while the Ledger compile stamps it from _value_representative_fact, the window's latest fact (2025-26). The structural test's claim that require_matching_fact_period stays invisible held for the compile but not for the harness. The harness now picks the fact with the compile's own _value_representative_fact, keys it with _fact_key, and asserts each spec's ledger_fact_period equals its reference's period. Three latent cases pin the keys the differential cannot see on these rows (require_matching_fact_period, a declared measurement period and a band dimension beside the single band-like filter): each leaves every cell green, so the structural test alone holds them. The metric_name note names its three readers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The round-3 review of 9d1e62c approved it with three wording nits. Two land here. The metric_name note said reference authoring copies the name into these rows' measures; it falls back to metric_name only for an unnamed reference, and all 35 of these references carry fan-out names, so only the UK local metric map reads it. The structural test's docstring said any other changed key fails; a value respelled as Python compares it (65 as 65.0, 0 as false, true as 1) passes both tests and moves no count, since _compare casts numeric thresholds with float(). The third nit lives in the PR body. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The round-4 review of 3913048 found that the tolerance-note test read each count but not the band the note names for it, so a note that put 65,500 on £1m to £5m stayed green. The test now reads each "count at band" pair and compares it with the count and the label spelled from ITL_BANDS, in band order. It goes red when the note moves a count to the wrong band, raises the top band's edge, misstates a count or swaps two bands' labels, and stays green on the real note. The reviewer also saw test_the_verdict_is_the_tolerance exceed hypothesis's 200 ms default deadline at host load 166 to 221. It, its neighbour test_no_residual_lands_on_a_tolerance and test_the_window_weights_the_two_tax_years now take deadline=None, as every other property test in the file already does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MaxGhenis
marked this pull request as ready for review
November 11, 2026 22:06
Contributor
Author
|
Merged at 305452e as b427c0a (merge commit). Gates at that head:
🤖 Generated with Claude Code |
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.
This PR fixes the six low-severity follow-ups that independent reviews left on three PRs merged on 2026-11-09: #1417, #1109 and #1416.
Changes
From #1417 (ITL differential check)
The check now reads what the contract binds, and the tests fail when a binding changes.
uk_runtime/itl_differential.pynow names the four engine variables its cells read:TAX,TOTAL_INCOME,IS_SP_AGEandAGE.resolved_peoplereads the age columns through those names.New engine-free tests in
tests/engine_free/uk/test_uk_itl_differential.pyhold the check to the threepolicyenginebindings inuk/uk_population_targets.json, in two ways:notesandmetric_name;materialize_target_bindingson aUKFrameTargetAdapterand compares what it writes for each of the 35 bound references with the check's masks.The invariants are in the next section.
test_the_cells_read_the_bindings_at_their_edgesstill pins the masks by hand. Its docstring now points to the test that reads the bindings through the build.--current-income-for-annual-measuresnow needs--diagnosticsor--references. I chose refusal over a help-text warning, for these reasons:inat the default--year. That is a claim about the incumbent's calibration that nothing supports.--help. The receipt and the printed table would still carry the wrong flags.--diagnosticswith the incumbent run's diagnostics, or--referenceswith the references that build read. A references file that lists no reference ({"target_references": []}) marks every cell out of sample, so the basis is explicit.parser.errorrefuses with exit code 2 before the tool reads any input, so it writes no receipt. The module docstring and the option's help say so.The review also found that the option accepted an empty reason, which
measure_resolvertreats as no reason (if reason:). The route then went unused, and nothing showed it. The tool now refuses an empty reason too, and the receipt records the reason undercurrent_income_for_annual_measures(nullwithout the option).The tolerance comment now cites the vendored counts. The old comment justified the tighter national tolerance by saying national cells sum over millions of taxpayers. I recomputed the counts from
hmrc_itl_taxpayer_counts.jsonat the calendar-2025 window (0.25 × 2024-25 + 0.75 × 2025-26), and they match the review:The comment now says geography, not size, decides which tolerance a cell takes.
NATIONAL_TOLERANCEstays 5% andREGIONAL_TOLERANCEstays 10%.test_the_tolerance_note_cites_the_vendored_countsreads the comment from the module's source and derives every number it checks from the rows, so a change to the rows fails until the comment follows. I confirmed it fails when the comment says 65,501, says "top two", or names Wales.This PR deletes
UNITED_KINGDOM = "K02000001". The module and its test importUK_HMRC_ITL_UK_GEOGRAPHY_IDfromuk_runtime/hmrc_uprating.py, followingdocs/shared-constants.md.From #1109 (fiscal refresh)
estate_incomeexclusion intools/build_us_fiscal_refresh_release.pynow ends "…, or if the flag is absent and the raw base becomes the reference."_export_input_mass_gatefalls back to the base frame when the caller passes no reference frame.gates._reviewed_exclusion_reasonsstill accepts the register: I loaded it and checked all eight reasons.From #1416 (measurement periods)
materialize_target_bindingsdocstring now uses the qualifier from Run the measurement-period guard for provider rows too #1416's first invariant: a declared period that a read misses refuses the run "for a plain row that declares any period but the calibration period, on a frame with period metadata". The guard returns early for a plain row that declares the calibration period, so the old wording, "for a plain row on a frame with period metadata", claimed too much.Invariants the new tests check
The bindings bind what the check counts (
test_the_check_reads_what_the_contract_binds). For each of the three families inBOUND_BY, every key of the binding exceptnotesandmetric_nameequals exactly the following:person_countat person grain, mapped to the household;filtersofTAX > 0, plusIS_SP_AGEequal to true for the State Pension age family, orAGE >= 65for the 65-and-over family;groupby_variableofTOTAL_INCOMEandband_filter_dimensionoftotal_income. There is no band semantics, upper bound or period factor, so the build reads each band as[lower, upper)between the vendored lower edges.metric_namenames the row, and the UK local metric map keys the row's target by it (_uk_local_metric_targetsinuk_runtime/ledger_targets.py). Reference authoring falls back to it for an unnamed reference's measure (target_reference_authoring.py), and a counterfactual binding names its delta column with it (target_materialization.py:103). Neither of those reaches these rows: all 35 references take fan-out names as their measures, and the three bindings are plain. None of those reads changes whom these rows count.A binding that gains or changes any other key fails, unless the change only respells a value as Python compares it (65 as 65.0, 0 as false, true as 1).
_comparecasts numeric thresholds withfloat(), and a scratch run confirmed that each of those four respellings leaves both tests green and moves no count. The structural test also holds the keys whose effect the differential cannot see on these rows. The mutation test below pins each one as latent:band_filter_dimensionbeside a single band-like filter;measurement_periodon a frame without period metadata;require_matching_fact_period, since each spec's fact period, 2025, matches the year the build reads it at.The build and the check count the same people (
test_each_bound_cell_counts_whom_its_binding_counts).Frame, each in a household of their own. The specs are realTargetSpecs for the 35 bound references fromload_country_spec("uk"):_value_representative_factpicks the vendored fact that stands for each reference's calendar window, the latest (2025-26), and the compile's_ledger_metadatastamps the spec from it. That is where the band edges (ledger_filter_total_income_lower_bound) and the fact period come from. The test asserts that each spec's fact period equals its reference's period, 2025._reference_metadatathen adds the contract target and the geography predicate.materialize_target_bindingsthen runs as a UK calibration runs it:_band_boundsreads each band from the spec;_with_geography_predicatescopes each area;An edited binding turns exactly the cells it moves red (
test_an_edited_binding_turns_its_cells_red, 18 cases). Each edit changes a deep copy of the contract, and the build then disagrees with the check, or skips, on exactly the expected cells. The edits change:The £3m upper bound moves only the top band, and every other one of these edits moves its whole family. Three more cases pin the latent keys:
require_matching_fact_periodon the bands, ameasurement_periodof 2024 on the State Pension age family and aband_filter_dimensionofageon the bands each leave every cell green, so the structural test alone holds them.The tolerance comment's counts hold (
test_the_tolerance_note_cites_the_vendored_counts). The test reads the comment and computes every number from the rows:NATIONAL_TOLERANCE.The incumbent route needs a basis and a reason (
test_the_current_income_route_needs_a_basis).--diagnosticsor--references, or with an empty reason, the option exits 2 before the tool reads anything: no resolver call and no receipt.To confirm the tests read the real file, I edited
uk_population_targets.jsonon disk five ways, one at a time, then restored it from a copy (git diffempty afterwards). Each edit failed both the structural test and the differential test:equals: false;total_income;>=;Review
No Subfleet lane, Codex or Claude, could take the review, so an in-session Opus 5.5 agent reviewed 1f506fa. It approved items 2–6 and asked for changes: one should-fix and five nits.
band_upper_boundor aband_period_factorof 12 on the band binding therefore left every test green. Fixed: the differential now runsmaterialize_target_bindingsend to end, as described above. Both edits now fail on disk and in memory, and the whole-pence edit fails through the build's own reading.The same reviewer re-checked 39b8a93. It confirmed the round-1 fix and the follow-up list, and found that
band_filter_dimensionandmeasurement_periodstay invisible to the differential and that onlynotes,metric_nameand spellings Python compares as equal (65 as 65.0, 0 as false, true as 1) pass both tests, none of which moves a count. It asked for changes once more: one should-fix and one nit._value_representative_fact, which for a calendar-year window picks the latest fact (2025-26). The claim thatrequire_matching_fact_periodstays invisible therefore held for the compile but not for the harness. Fixed: the harness picks the fact with the compile's own_value_representative_factand asserts that each spec's fact period equals its reference's period. A new latent case addsrequire_matching_fact_periodto the bands and expects every cell green. A scratch run that stamps from the 2024 fact again turns exactly the eleven bands red on that case and leaves the base differential and the other two latent cases green. When the module's own registry picks the first fact, the new assertion fires.metric_namerationale named one reader, but three read it. Fixed in the test's comment and in the invariants above.The same reviewer re-checked 9d1e62c and approved it, with no blocker and no should-fix. It compiled all 35 bound references with the real
target_spec_from_ledger_referenceon the vendored facts and found that the harness's specs match the compile's on every keymaterialize_target_bindingsreads. It also confirmed that each latent case turns red once its reason goes away: a second band-like filter makes the band dimension count, a frame period of 2025 makes the declared measurement period refuse, and the 2024 stamp trips the fact-period guard. It left three wording nits, all fixed in 3913048:metric_name, which the reviewer itself had suggested, said reference authoring copies it into these rows' measures. It doesn't:_reference_rowusesname or binding["metric_name"], and these rows' references always carry names. Fixed in the test's comment and in the invariants above, after I checked all 35 references.notesandmetric_namepass both tests" left out values Python compares as equal. Fixed in the structural test's docstring, the invariants and the round-2 paragraph.The session stopped on 2026-11-09 at an account limit before these fixes, and resumed on 2026-11-11. Main had moved by then, so 3913048 also merges main. Main changed the vendored feed's metadata but not its 256 rows, and the contract and the references but not these three bindings or their 35 references. It also changed
ledger_targets.py. The tests below ran on the merged tree.The rounds-1-to-3 reviewer could not resume after the account switch, so a fresh in-session Opus 5.5 reviewer checked 3913048 end to end and approved it, with no blocker and no should-fix. It re-derived the load-bearing claims rather than trusting the earlier rounds: the merge needed no conflict resolution and left the 256 vendored rows, the three bindings and their 35 references identical, the estate_income text reads right against
_export_input_mass_gate, and the four respellings and ametric_nameedit leave both tests green. It left three nits, all fixed: the test in 305452e, the other two in this body.ITL_BANDS. A scratch run turns it red when the note moves 65,500 to the wrong band, raises the top band's edge, says 65,501, or swaps two bands' labels, and leaves it green on the real note.docs/evidence/uk-consumption-taxes-1113/scripts/m2_m3_desk.py:106. Fixed below.The reviewer also saw
test_the_verdict_is_the_toleranceexceed hypothesis's 200 ms default deadline twice at host load 166 to 221; the test came from #1417 and kept that default. 305452e gives it,test_no_residual_lands_on_a_toleranceandtest_the_window_weights_the_two_tax_yearsdeadline=None, as every other property test in the file already has. Its examples run the same code; only a slow host no longer fails them.CI passed every check at 3913048. The same reviewer re-checked 305452e and approved it, with no blocker and no should-fix. It probed the new pair pattern with ten wrong notes, and each turned the test red. It left two nits:
_BAND_COUNT_PHRASEdoesn't anchor a pair's ends, so characters glued onto either end ("£1mn", "£2m and overall", "~65,500") still pass. Nobody writes a band edge that way, and anchoring it now would cost another CI cycle for that alone. The reviewer's anchored pattern ((?<![\d,.~])before the count,(?=[,;)]| and )after the band) keeps the real note green and turns all four red, so a later edit to this test can take it. The test fails safe on a correct note in another spelling (£1,000,000, an en dash, £150k): its message shows the pairs it expects.Tests
All runs used
pytest -n 4or fewer workers.tests/engine_free/uk/test_uk_itl_differential.pyat 305452e, on the tree merged with main: 65 passed.target_materialization,itl_differential,check_uk_itl_projections,build_us_fiscal_refresh_release), at 1f506fa: 1,658 passed, 12 skipped, 3 xfailed.[us]extra, four the pinned Chronicle consumer-facts feed, one the pinned ASEC archives and onemicrodf.itl_differential.pyand the changelog fragment, and CI runs the whole engine-free suite on the merge ref. Outside the ITL file, only the engine testtests/engine/uk/test_uk_itl_differential_build.pyimports the module or the tool, and it skips unlessMICROCOSM_UK_ITL_DIFFERENTIAL_H5names a built file.python3 tools/ci_test_plan.py verify:verification=ok.ruff formatandruff checkpass on the touched files.No committed pin moves:
spec_engine/seeds.pyattests none of the touched modules, and #1416 editedtarget_materialization.pywithout a re-pin. The docstring edit still changes that module's bytes.microcosm.build.target_materializationis inUK_TARGET_COMPILE_MODULE_NAMES, whose sources the UK target and national graph kernels hash (graph_targets.py,graph_national.py, throughmicrocosm.graph.kernel.source_hash). The next UK build therefore recomputes those cached nodes instead of reusing them.Follow-up outside this PR
Five other
uk_runtimemodules and one tool repeatK02000001:energy_pricing.py:118(UNITED_KINGDOM_GEOGRAPHY_ID);hmrc_property_rental.py:120andhmrc_capital_gains.py:360(_UK_GEOGRAPHY_ID);ledger_targets.py:218(a mapping key) and:2322(UK_LEDGER_HOUSEHOLDS_TOTAL_GEOGRAPHY);lcfs_consumption.py;tools/generate_uk_target_references.py:48.So do two evidence scripts:
docs/evidence/uk-pensioner-taxpayers-1069/scripts/hmrc_implied_recipients.py:30anddocs/evidence/uk-consumption-taxes-1113/scripts/m2_m3_desk.py:106. Moving them onto one canonical constant is a separate cleanup.ledger_targets.pyandlcfs_consumption.pysit among the hashed compile modules, so that change re-keys graph nodes too.axiom: n/a: a diagnostics check, its tests and two docstrings; no policy rule changes
🤖 Generated with Claude Code