Repository navigation
Conversation
… test From policyengine-core 3.34.8, applying a reform that neutralises a variable gives the variable's existing holder the neutralised definition. Holder.set_input then applies the rule that TaxBenefitSystem.neutralize_variable documents: setting an input for a neutralised variable has no effect except a warning. Up to 3.34.1, a holder that core created before the neutralisation kept the old definition and stored such an input. On the neutralize_variable route, test_neutralized_supplied_gains_report_default_without_unused_warning set 555 after the neutralising reform and read it back after the restoring reform. Core now drops that 555, so the test expects the situation's 120,000. Until core publishes 3.34.9, uv.lock installs core from #653's head (14837f78), so CI tests that fix before its release. Core's dependencies at 14837f78 match 3.34.1's, so no other locked package moves. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
juaristi22
left a comment
juaristi22
left a comment
There was a problem hiding this comment.
Review status: COMPLETE — no findings or material evidence gaps
Reviewed head: 428d8ce5ac7ee25c489b993d8bf6f482ab84bfd4.
The current draft intentionally tests Core commit 14837f784b52d731101dd2e322c7727895d08753, whose package metadata is 3.34.8. The Git pin agrees between pyproject.toml and uv.lock. I verified the complete upstream archive and executed that exact source in an isolated overlay: all 163 installed Core Python files matched the archive and all 18 checked dependency versions matched the lock.
The changed expectation agrees with the documented holder contract: writing an input while its variable is neutralised is ignored, so restoration retains the supplied £120,000. Synthetic abolition leaves the definition intact and retains the £555 write. The holder rebinding and input-list identity changes are supported by the upstream source and the cited historical failures.
156 targeted tests passed (140 Python and 16 YAML), followed by two explicit-2026 households across six contexts each. Final capital-gains tax and household net income return to their exact baselines after restoration; restore-and-write and synthetic-abolition controls also pass. No private dataset or population run was performed.
This is a COMMENT, with approval withheld while the PR remains a draft and applicable current-head CI is incomplete. The planned transition to a published registry release, a Core floor of at least 3.34.9, removal of the Git override, and validation of the resulting head must be checked before approval. The current declared floor is still >=3.33.0; this review does not certify that the future release transition has happened. axiom: n/a is appropriate for this dependency change.
Aggregate impact on certified data: passBase Checks: 0 broken by this change, 1 failing on both sides, 0 fixed. Moves flagged for review: 0 (thresholds in Notes
Moves flagged for reviewNone. Spending by benefit and service
Caseloads
Household net income by decileNo household's net income changes in any year. No change in any year: receipts by tax head; government balance, each component counted once; model totals; household income totals; population; poverty rates; people in poverty; inequality; mean household net income in each side's own deciles; standard reform costings (change in gov_balance); standard reforms: change in each reform's target. Checks failing on both base and head (1)
Counted-once checkThe check raised each of 92 components by £1 per unit in 2026-27 and read every head of its totals household by household: its own head must rise by exactly £1, no other tax head may move, no other benefit may rise, and the model's own total must move by what its heads moved. A response that is not finite breaks every rule. The check measures a head that breaks a rule twice more: once holding every other head at its baseline value, and once holding every other head at its value after the first £1 while the component rises again. The head fails where either measurement still shows the response. The check cannot hold a head at a value that is not finite. In each unit where another head is not finite after the first £1, it holds that head at its baseline value, and the second measurement judges no household the unit belongs to. There a head fails wherever it first broke a rule, or wherever the check could not confirm whether its response contains the component. 0 failed and the check skipped 0.
Provenance
Dataset: Runtime differences:
Receipts by tax head: head levels
Spending by benefit and service: head levels
Caseloads: head levels
Government balance, each component counted once: head levels
Model totals: head levels
Household income totals: head levels
Population: head levels
Poverty rates: head levels
People in poverty: head levels
Inequality: head levels
Mean household net income in each side's own deciles: head levels
Standard reform costings (change in gov_balance): head levels
Standard reforms: change in each reform's target: head levels
|
Core published policyengine-core#653 as 3.34.9. Lock 3.34.9's wheel and sdist from PyPI in place of #653's head (14837f78) from GitHub. Drop that git source from pyproject.toml and require policyengine-core>=3.34.9. Core's release commit changes only the version and the changelog, and the 3.34.9 wheel's 194 package files match 14837f78 byte for byte. The release's dependencies match 3.34.1's, so no other locked package moves. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
juaristi22
left a comment
juaristi22
left a comment
There was a problem hiding this comment.
Review status: COMPLETE — no findings or material evidence gaps
Reviewed head: 58b342e553cc5d4bc260d89fc4c08746d91b2192.
The release transition is verified: Core 3.34.9 is published on PyPI, the wheel and source archive match the lockfile hashes, the declared floor is >=3.34.9, and the Git override is removed. All 194 package files in both release artifacts match the previously reviewed Core commit 14837f784b52d731101dd2e322c7727895d08753. Other locked packages are unchanged, dependency metadata agrees with the reviewed environment, and all 18 checked runtime dependency versions match the lock. Released Core 3.34.9 and this UK snapshot imported successfully from an isolated installation without changing the borrowed environment.
The prior 156 targeted test passes (140 Python and 16 YAML) and 12 household-context evaluations are retained from 428d8ce5, supported by that package-byte, dependency and unchanged UK source/test proof. They are not new executions at this head; this round ran packaging/import validation without repeating pytest or model calculations. The prior final capital-gains-tax and net-income results remain applicable to the tested paths.
The technical scope remains the inspected Simulation.set_input/get_holder/apply_reform paths with compatible Person/YEAR gains, ordinary clone and reset. This does not certify raw held-Holder writes, serialization restore, neutralized-subsample ordering, schema/storage migrations, arbitrary system reassignment, or future #2265 integration. The historical aggregate report is dated evidence with its documented suppression/alignment and unchanged VAT-sign qualification; no private population run was performed.
This is a COMMENT, with approval withheld until applicable required CI passes at this exact head. The latest captured CI has no observed failures but is still running. The earlier Draft and release-transition conditions have been satisfied; pending CI should be checked later without repeating this completed code/source review. axiom: n/a remains appropriate for this dependency change.
|
Thanks, María. CI passed at Once this lands, #2670 will conflict in |
juaristi22
left a comment
juaristi22
left a comment
There was a problem hiding this comment.
Review status: COMPLETE — no findings or material evidence gaps
Approved head: 58b342e553cc5d4bc260d89fc4c08746d91b2192.
The completed code/source review remains valid. Core 3.34.9's published artifacts, registry lock, minimum version and removal of the Git override were verified; the 194 package files match the reviewed payload and the released-package import check passed.
Applicable exact-head CI has now passed. The updated description's current full-suite and aggregate citations were checked against their primary run records and public report, retaining the dataset/year, DSA suppression/alignment and unchanged VAT-sign qualifications.
This approval reuses the existing COMPLETE zero-finding code review and its API/storage scope. No model calculations or tests were repeated. Downstream #2670 merge coordination and future #2265 integration remain separate work.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
From 3.34.9, Population.get_holder gives an existing holder its variable's current definition whenever the holder's storage fits that definition. After a reform restores a neutralised variable, a write to that variable now replaces the value its holder kept through the neutralisation. Drop the strict expected failures from both variants of test_neutralized_inputs_and_model_values_survive_subsampling and from the subsampled variants of test_restoring_reform_can_replace_a_neutralized_input, with their two allowlist entries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
juaristi22
left a comment
juaristi22
left a comment
There was a problem hiding this comment.
Completed incremental review at 7e7ad197e24765da7c3ce181b0d9fecb0f9d5595: no open findings or material evidence gaps; all 25 targeted tests passed. Approval is deferred until pending exact-head CI completes.
Program review
Base repository: PolicyEngine/policyengine-uk
PR number: 2897
Reviewed head SHA: 7e7ad19
Merge base SHA: b8b383c
Mode: incremental from 58b342e
Scope: Main merge and removal of strict expected failures from neutralized-input/restoration tests and associated allowlist; verify released Core3.34.9 compatibility, reused dependency custody and affected final consumers.
Source manifest: /privateretained review artifacts/pr-2897-sweep20261109-1359-review-sources.json
Review status: COMPLETE
Source Documents
- 000-retained-core-archive.tar.gz — cached original (retained review artifact).
- 001-retained-core652.json — cached original (retained review artifact).
- 002-current-core653.json — cached original (retained review artifact).
- 003-retained-core652-diff.diff — cached original (retained review artifact).
- 004-retained-core653-diff.diff — cached original (retained review artifact).
- 005-retained-core-commit.json — cached original (retained review artifact).
- 006-retained-core-tree.json — cached original (retained review artifact).
- 007-retained-core-pypi.json — cached original (retained review artifact).
- 008-retained-uk2265-run.json — cached original (retained review artifact).
- 009-retained-uk2265-logs.zip — cached original (retained review artifact).
- 010-pypi-3.34.9.json — cached original (retained review artifact).
- 011-policyengine_core-3.34.9-py3-none-any.whl — cached original (retained review artifact).
- 012-policyengine_core-3.34.9.tar.gz — cached original (retained review artifact).
- 013-core-release-compare.json — cached original (retained review artifact).
- 014-uk2897-historical-aggregate-run.json — cached original (retained review artifact).
- 015-uk2897-historical-aggregate-logs.zip — cached original (retained review artifact).
- 016-uk2897-historical-suite-run.json — cached original (retained review artifact).
- 017-core-release-commit.json — cached original (retained review artifact).
- 018-uk2897-historical-aggregate-report.zip — cached original (retained review artifact).
Critical
None.
Should Address
None.
Suggestions
None.
Evidence Gaps
No material gaps reported.
Notes
- Exact captured head and merge-base are recorded. Imported main behavior outside affected dependencies reuses prior evidence.
- Approval deferred while exact-head hosted Python CI remains pending; the completed code/source review can be reused for a later CI-only approval gate.
- Pure infrastructure/dependency feature; imported main policy is not re-audited.
- 25 current repository passes; all newly unmarked cases actually run without xfail/skip.
- Core3.34.9/package lock unchanged from prior;194 package byte identity and17 current pins verified.
- Code-only dependency/infrastructure scope; no changed statutory rule or policy/source reviewer required. A failed reviewer-only path-normalization preflight ran no repository tests and is excluded from passing counts; original campaign deadline retained.
Validation Summary
25 current targeted tests passed; no failures, skips or expected failures in the focused campaign. 19 retained originals and 19 extracts validated; no new source acquisition or rendering. 717 immutable technical receipt members SHA-256 checked before cleanup. All four newly unmarked restoration/subsampling variants passed; neutralized-versus-synthetic gains, stored-input provenance and final income-tax/household-net-income parity covered. Released Core3.34.9 matches all194 cached package files and17 current runtime pins; no dependency installation.
Timing
setup seconds: 160.74s; consolidation cleanup seconds: 59.40s; elapsed seconds: 828.82s
Review Severity
APPROVE. Open findings: 0 critical, 0 should address, 0 suggestions.
There was a problem hiding this comment.
Approve at 7e7ad197e24765da7c3ce181b0d9fecb0f9d5595. The pending-CI gate from the completed incremental review has cleared: 38 latest exact-head contexts passed, including required Lint and Test / Result; the nonrequired Publish job was skipped. The completed review has no open findings or material gaps; its 25 targeted passes are retained, with no repeated code review or model execution.
The current head, description, human activity, source dependencies, readiness and applicable required checks are refreshed before this approval.
axiom: n/a: dependency update, no policy rule change
What changes
policyengine-uk now requires policyengine-core 3.34.9, and
uv.lockpins the release from PyPI.In policyengine-core 3.34.8 (policyengine-core#652),
Simulation.apply_reformbinds each existing holder to the reformed system. In 3.34.9, policyengine-core#653 also updates a holder's definition whenever a simulation looks the holder up. With both changes, a holder takes the neutralised definition once a reform neutralises its variable.Holder.set_inputthen applies the rule thatTaxBenefitSystem.neutralize_variabledocuments: setting an input for a neutralised variable has no effect except a warning. Up to 3.34.1, a holder that core created before the neutralisation kept the old definition and stored such an input.test_neutralized_supplied_gains_report_default_without_unused_warningrelied on the old behaviour. Its situation supplies £120,000 of capital gains for 2026, and on theneutralize_variableroute the test neutralisescapital_gains_before_response, sets £555 and restores the variable. With core 3.34.8, a CI run on #2265 read £120,000 where the test expected £555, so the test now expects £120,000 on that route. Thesynthetic_abolitionroute keeps £555: the abolition parameter leaves the variable's definition in place, and core stores the input.The same run failed both routes of
test_a_reform_names_nothing_it_stores_on_a_clone_or_branch, becauseapply_reformreplacedinput_variableswith a copy after every reform. 3.34.9 replaces the list only when a reform drops a name, so that test needs no change.#2265 needs 3.34.9 for its subsample restoration tests and lands after this pull request.
Tests
At
58b342e5, with core 3.34.9 from PyPI, the full suite passed. The aggregate impact check found no change in any year in tax receipts, poverty, inequality or any household's net income.Core's release commit changes only the version and the changelog, and the 3.34.9 wheel's 194 package files match #653's head (
14837f78) byte for byte. The release's dependencies match 3.34.1's, so no other locked package moves.🤖 Generated with Claude Code