Skip to content

Fold the review follow-ups from #1417, #1109 and #1416 - #1553

Merged
MaxGhenis merged 6 commits into
mainfrom
claude/review-followups-1417-1109-1416
Nov 11, 2026
Merged

MaxGhenis merged 6 commits into
mainfrom
claude/review-followups-1417-1109-1416

Conversation

@MaxGhenis

@MaxGhenis MaxGhenis commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

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)

  1. The check now reads what the contract binds, and the tests fail when a binding changes.

    • uk_runtime/itl_differential.py now names the four engine variables its cells read: TAX, TOTAL_INCOME, IS_SP_AGE and AGE. resolved_people reads the age columns through those names.

    • New engine-free tests in tests/engine_free/uk/test_uk_itl_differential.py hold the check to the three policyengine bindings in uk/uk_population_targets.json, in two ways:

      • a structural test pins every key of each binding except notes and metric_name;
      • a differential runs the build's own materialize_target_bindings on a UKFrameTargetAdapter and 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_edges still pins the masks by hand. Its docstring now points to the test that reads the bindings through the build.

  2. --current-income-for-annual-measures now needs --diagnostics or --references. I chose refusal over a help-text warning, for these reasons:

    • The option exists for one case: reading a file built before the annual income descriptors (the incumbent).
    • The default in-sample basis is this tree's target references. They describe what a build of this tree binds, which at 2025 is every band and every area. On that basis, every band and area cell of the incumbent printed in at the default --year. That is a claim about the incumbent's calibration that nothing supports.
    • A help line reaches only someone who reads --help. The receipt and the printed table would still carry the wrong flags.
    • Refusal costs one argument. Pass --diagnostics with the incumbent run's diagnostics, or --references with 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.error refuses 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_resolver treats 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 under current_income_for_annual_measures (null without the option).

  3. 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.json at the calendar-2025 window (0.25 × 2024-25 + 0.75 × 2025-26), and they match the review:

    • Five of the eleven bands hold under 400,000 taxpayers: 342,750 at £150k–200k, 372,500 at £200k–500k, 65,500 at £500k–1m, 20,750 at £1m–2m and 10,750 at £2m and over.
    • The top three hold fewer than the smallest area cell, Northern Ireland's State Pension age count (198,750).

    The comment now says geography, not size, decides which tolerance a cell takes. NATIONAL_TOLERANCE stays 5% and REGIONAL_TOLERANCE stays 10%.

    test_the_tolerance_note_cites_the_vendored_counts reads 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.

  4. This PR deletes UNITED_KINGDOM = "K02000001". The module and its test import UK_HMRC_ITL_UK_GEOGRAPHY_ID from uk_runtime/hmrc_uprating.py, following docs/shared-constants.md.

From #1109 (fiscal refresh)

  1. The reviewed estate_income exclusion in tools/build_us_fiscal_refresh_release.py now ends "…, or if the flag is absent and the raw base becomes the reference."
    • _export_input_mass_gate falls back to the base frame when the caller passes no reference frame.
    • gates._reviewed_exclusion_reasons still accepts the register: I loaded it and checked all eight reasons.

From #1416 (measurement periods)

  1. The materialize_target_bindings docstring 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 in BOUND_BY, every key of the binding except notes and metric_name equals exactly the following:

    • person_count at person grain, mapped to the household;
    • filters of TAX > 0, plus IS_SP_AGE equal to true for the State Pension age family, or AGE >= 65 for the 65-and-over family;
    • for the band family only, groupby_variable of TOTAL_INCOME and band_filter_dimension of total_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_name names the row, and the UK local metric map keys the row's target by it (_uk_local_metric_targets in uk_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). _compare casts numeric thresholds with float(), 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_dimension beside a single band-like filter;
    • measurement_period on 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).

    • The test builds 23,920 people at every edge the bindings read:
      • tax at −1, 0, 0.01 and 1;
      • total income at 0, at £5m, and at each band's lower edge (at it, a penny either side, and 0.4p below, where a reading in whole pence moves a person across the edge);
      • both State Pension age flags;
      • ages 64, 64.99, 65, 65.01 and 90;
      • each of the twelve region-tier areas, and an unknown region.
    • The people sit on a real Frame, each in a household of their own. The specs are real TargetSpecs for the 35 bound references from load_country_spec("uk"):
      • The Ledger compile's own _value_representative_fact picks the vendored fact that stands for each reference's calendar window, the latest (2025-26), and the compile's _ledger_metadata stamps 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.
      • The compile's _reference_metadata then adds the contract target and the geography predicate.
    • materialize_target_bindings then runs as a UK calibration runs it:
      • _band_bounds reads each band from the spec;
      • _with_geography_predicate scopes each area;
      • the fact-period and measurement-period guards and provider dispatch run. On this frame, which carries no period metadata, the measurement-period guard refuses nothing; the latent cases show it.
    • For every one of the 35 cells (eleven bands, plus two age measures across twelve areas), the column the build writes equals the check's mask, person by person. The build skips none of them.
    • Each cell counts someone and leaves someone out, so the comparison is never vacuous.
  • 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:

    • a filter's operator, variable or value;
    • the set of filters, by dropping or adding one;
    • the band variable, by swapping or removing it;
    • the band reading: whole pence, a period factor of 12, or an upper bound of £3m;
    • the binding's kind, to a provider.

    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_period on the bands, a measurement_period of 2024 on the State Pension age family and a band_filter_dimension of age on 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:

    • how many bands sit under 400,000, and each one's count, paired with the band that holds it, in band order;
    • how many of the top bands sit below the smallest area cell;
    • that cell's area, measure and count;
    • that every band takes NATIONAL_TOLERANCE.
  • The incumbent route needs a basis and a reason (test_the_current_income_route_needs_a_basis).

    • Without --diagnostics or --references, or with an empty reason, the option exits 2 before the tool reads anything: no resolver call and no receipt.
    • With a basis, the tool runs, passes the reason on and records it in the receipt, and an empty basis marks every cell out of sample.
    • Without the option, the default basis is still this tree's references, and the receipt records no reason.

To confirm the tests read the real file, I edited uk_population_targets.json on disk five ways, one at a time, then restored it from a copy (git diff empty afterwards). Each edit failed both the structural test and the differential test:

  • the State Pension age filter set to equals: false;
  • the band variable set to total_income;
  • the 65-and-over taxpayer filter set to >=;
  • a £3m upper bound on the bands;
  • a period factor of 12 on the bands.

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.

  • Should-fix: the first differential handed the build the check's own band edges. A £3m band_upper_bound or a band_period_factor of 12 on the band binding therefore left every test green. Fixed: the differential now runs materialize_target_bindings end 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.
  • Nit: the pin claim. Fixed in the pin note under Tests.
  • Nit: the tolerance test did not read the comment. Fixed: it reads it now.
  • Nit: the follow-up list miscounted. Fixed in the follow-up section.
  • Nit: passive voice ("is above zero" and three passives in this body). Fixed.
  • Nit: an empty reason left the route silently unused. Fixed: the tool refuses it and the receipt records the reason.

The same reviewer re-checked 39b8a93. It confirmed the round-1 fix and the follow-up list, and found that band_filter_dimension and measurement_period stay invisible to the differential and that only notes, metric_name and 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.

  • Should-fix: the harness stamped each spec from the window's first fact (2024-25). The compile stamps it from _value_representative_fact, which for a calendar-year window picks the latest fact (2025-26). The claim that require_matching_fact_period stays invisible therefore held for the compile but not for the harness. Fixed: the harness picks the fact with the compile's own _value_representative_fact and asserts that each spec's fact period equals its reference's period. A new latent case adds require_matching_fact_period to 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.
  • Nit: the metric_name rationale 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_reference on the vendored facts and found that the harness's specs match the compile's on every key materialize_target_bindings reads. 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:

  • Nit: the round-2 reader list for metric_name, which the reviewer itself had suggested, said reference authoring copies it into these rows' measures. It doesn't: _reference_row uses name 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.
  • Nit: "only notes and metric_name pass both tests" left out values Python compares as equal. Fixed in the structural test's docstring, the invariants and the round-2 paragraph.
  • Nit: "the period guard and provider dispatch apply" overstated the measurement-period guard, which refuses nothing on a frame without period metadata. Fixed above. The reviewer measured that the build's answer to a declared period depends on the frame's period and what the frame stores, so the edge frame keeps no period, and the structural test holds the key.

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 a metric_name edit leave both tests green. It left three nits, all fixed: the test in 305452e, the other two in this body.

  • Nit: the tolerance-note test read each count but not the band it named, so a note that put 65,500 on £1m to £5m stayed green. Fixed: the test now pairs each count with its band's label, spelled from 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.
  • Nit: one passive in this body. Fixed.
  • Nit: the follow-up list missed docs/evidence/uk-consumption-taxes-1113/scripts/m2_m3_desk.py:106. Fixed below.

The reviewer also saw test_the_verdict_is_the_tolerance exceed 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_tolerance and test_the_window_weights_the_two_tax_years deadline=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:

  • Nit, left as it stands: _BAND_COUNT_PHRASE doesn'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.
  • Nit: "all fixed in 305452e" claimed the two body fixes for that commit. Fixed above.

Tests

All runs used pytest -n 4 or fewer workers.

  • tests/engine_free/uk/test_uk_itl_differential.py at 305452e, on the tree merged with main: 65 passed.
  • The 36 engine-free files that import the changed modules (target_materialization, itl_differential, check_uk_itl_projections, build_us_fiscal_refresh_release), at 1f506fa: 1,658 passed, 12 skipped, 3 xfailed.
    • Every skip comes from something this venv lacks: six need the policyengine-us [us] extra, four the pinned Chronicle consumer-facts feed, one the pinned ASEC archives and one microdf.
    • All 21 tests of the reviewed exclusions ran.
    • Since then this PR has changed only the ITL test file, the check tool, a comment in itl_differential.py and the changelog fragment, and CI runs the whole engine-free suite on the merge ref. Outside the ITL file, only the engine test tests/engine/uk/test_uk_itl_differential_build.py imports the module or the tool, and it skips unless MICROCOSM_UK_ITL_DIFFERENTIAL_H5 names a built file.
  • python3 tools/ci_test_plan.py verify: verification=ok.
  • ruff format and ruff check pass on the touched files.

No committed pin moves: spec_engine/seeds.py attests none of the touched modules, and #1416 edited target_materialization.py without a re-pin. The docstring edit still changes that module's bytes. microcosm.build.target_materialization is in UK_TARGET_COMPILE_MODULE_NAMES, whose sources the UK target and national graph kernels hash (graph_targets.py, graph_national.py, through microcosm.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_runtime modules and one tool repeat K02000001:

  • energy_pricing.py:118 (UNITED_KINGDOM_GEOGRAPHY_ID);
  • hmrc_property_rental.py:120 and hmrc_capital_gains.py:360 (_UK_GEOGRAPHY_ID);
  • ledger_targets.py:218 (a mapping key) and :2322 (UK_LEDGER_HOUSEHOLDS_TOTAL_GEOGRAPHY);
  • three literals in 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:30 and docs/evidence/uk-consumption-taxes-1113/scripts/m2_m3_desk.py:106. Moving them onto one canonical constant is a separate cleanup. ledger_targets.py and lcfs_consumption.py sit 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

MaxGhenis and others added 6 commits November 9, 2026 11:51
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
MaxGhenis marked this pull request as ready for review November 11, 2026 22:06
@MaxGhenis
MaxGhenis merged commit b427c0a into main Nov 11, 2026
12 checks passed
@MaxGhenis

Copy link
Copy Markdown
Contributor Author

Merged at 305452e as b427c0a (merge commit).

Gates at that head:

  • CI: all 12 checks pass (gh pr checks exit 0; no pending, failing or cancelled check), on the tree merged with main at 27e305d. Main's later commit 83b1a34 (NZ walk: pin four rules the review of 04c8379fd found unpinned (#1578) #1579, NZ) touches none of this PR's files.
  • GitHub reported MERGEABLE, the PR was out of draft, and no review requested changes.
  • Independent review: no Subfleet lane could take it (Codex and Claude lanes held), so in-session Opus 5.5 agents reviewed it in five rounds. Rounds 1 to 3 (2026-11-09) reviewed 1f506fa, 39b8a93 and 9d1e62c. Round 4 (a fresh reviewer, 2026-11-11) approved 3913048. Round 5 approved 305452e with no blocker and no should-fix. The Review section of this PR's description records every finding and its fix. It also records the one nit left as it stands: the pair pattern in the tolerance-note test doesn't anchor its ends.
  • The PR changes a diagnostics check, its tests and two docstrings, and moves no published number.

🤖 Generated with Claude Code

@MaxGhenis
MaxGhenis deleted the claude/review-followups-1417-1109-1416 branch November 11, 2026 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant