Let Scenario structural reforms read the simulation, and add baseline regression tests

[vahid-ahmadi]

Fixes #2188.

What and why

#2305 (3edfb4b99, already on main) fixed the bug in #2188: a Scenario's parameter changes no longer leak into parameters.baseline, so baseline_vat and adjust_budgets now compare a reform with the unreformed tree. That fix exposed a follow-on failure, raised as C1 in review: once adjust_budgets sees a Scenario change to DfE, DHSC or DfT spending, it registers a structural reform that reads self.simulation from the tax-benefit system. The UK Simulation never binds that attribute, so on main any Scenario that changes one of those budgets fails to construct with AttributeError: 'CountryTaxBenefitSystem' object has no attribute 'simulation' (at adjust_budgets.py:60). That holds for both Simulation and Microsimulation, before or after the data load.

This PR fixes that failure and adds the regression tests for #2188.

Changes

• Simulation.apply_reform (policyengine_uk/simulation.py) binds tax_benefit_system.simulation to the simulation applying the reform before it calls core's apply_reform. policyengine-core's own Simulation.__init__ makes this binding, but the UK class does not call it. Binding on every call also keeps a cloned system pointing at the simulation that applies the reform, not the one it was cloned from.
• adjust_budgets reads every year's allocation before it writes any. Each write is an input that carries over to later years without their own input, so reading a later year after writing an earlier one would stack the earlier year's change on top of its own. It also skips a year with no allocation instead of dividing by zero.
• changelog.d/2188.fixed.md describes the construction fix.

The baseline tree handling itself (process_parameters, apply_parameter_changes) is main's, from #2305; this branch no longer changes it.

Tests

New policyengine_uk/tests/test_scenario_baseline_parameters.py (16 cases, plus 1 dataset test marked microsimulation):

• every reform route (Scenario scalar and bare-year changes, reform={...}, Reform.from_dict) gives the same VAT change and leaves the baseline standard rate at 20%; no reform gives no VAT change;
• after a Scenario that changes VAT, the personal allowance, a UC standard allowance, fuel duty and DfE spending, each baseline value matches an unreformed system, while the reformed tree still carries the change;
• adjust_budgets returns a reform after a Scenario DfE spending change and none for an unreformed system;
• a before-load Scenario DfE change constructs under Simulation and Microsimulation, and scales each year's allocation by that year's own budget change, leaving other budgets alone;
• a structural reform can read the simulation it is applied to, including on a clone;
• on the dataset, the weighted DfE allocation rises by the budget change.

On main (merge base 3381500f4), 11 of the 16 fail, all with the AttributeError above. The 5 that pass there are the four VAT reform routes and the no-reform control, which #2305 already fixed. On this branch, 16 pass and the dataset test is skipped without the dataset.

Reproducer: policyengine_uk/tests/test_scenario_baseline_parameters.py::test_before_load_spending_scenario_constructs

axiom: n/a: simulation and reform-route plumbing only; no policy, parameter or eligibility rule changes

🤖 Generated with Claude Code

[juaristi22]

Review of #2193 — Keep the baseline parameter tree unreformed under Scenario reforms

Base repository: PolicyEngine/policyengine-uk
PR number: 2193
Reviewed head SHA: 826ac3c
Merge base SHA: f1a9a3c
Mode: full
Scope: changed behavior and affected dependencies
Source evidence: This framework change introduces no new statutory values. Existing counterfactual definitions and local validation artifacts were examined.
Review status: COMPLETE

Source Documents

No source documents registered; see scope and validation.

Critical

C1 — Keep before-load spending Scenarios constructible (OPEN)

CRITICAL — confirmed new public-route failure. Location: policyengine_uk/simulation.py:301 at the captured head, which now supplies the preserved baseline; the changed attachment is policyengine_uk/tax_benefit_system.py:129. A supported Scenario(parameter_changes={"gov.dfe.education_spending": 150}, applied_before_data_load=True) now prevents both Simulation and Microsimulation from constructing. With a London adult aged 40 earning £30,000 in 2026 and a supplied nonzero dfe_education_spending allocation, both raise AttributeError: 'CountryTaxBenefitSystem' object has no attribute 'simulation' at reforms/policyengine/adjust_budgets.py:60.

The public flag is declared in utils/scenario.py:42 and explicitly handled by the constructor at simulation.py:165–170. The intended path must construct and apply the detected spending adjustment: adjust_budget calculates the difference between the current-law and reformed budget and scales the existing allocation at adjust_budgets.py:44–69. This is an internal API/routing contract, not a new statutory claim. Preserving the baseline correctly changes the 2026 comparison to £107.27934668071654bn versus £150bn, but now makes create_structural_reforms_from_parameters register the budget reform. Core applies it to the tax-benefit system at simulation.py:200–206, while the helper assumes that system has a .simulation binding. It does not. The result is an exception before any final tax, benefit or net-income output is available.

A fresh exact-base control with identical inputs constructs both classes successfully: its contaminated baseline and active budget both read 150, so no budget reform is registered. The fixture's £1bn household allocation is deliberately synthetic and only establishes routing; it is not a population estimate. The underlying helper problem also exists on older direct reform routes, but this scalar Scenario route becomes newly broken when this diff preserves the baseline. Handle the active budget reform through the appropriate simulation context before enabling this route, and add a constructor/final-output regression rather than only asserting that adjust_budgets returns a class.

Reproduction and complete inputs: budget diagnostic (pr-2193-code-budget.py; retained locally). Fresh base/head observations and full stack traces: base JSON (pr-2193-code-budget-base.json; retained locally), head JSON (pr-2193-code-budget-head.json; retained locally), base log (pr-2193-code-budget-base.log; retained locally), head log (pr-2193-code-budget-head.log; retained locally). The control replaced only the two changed production files with their exact base versions; overlay provenance (pr-2193-overlay-provenance.json; retained locally) records source identities and hashes. Both comparison commands completed successfully while asserting the observed difference.

Should Address

None.

Suggestions

None.

Evidence Gaps

No material gaps reported.

Notes

• Default post-load spending helper detection still does not reach final allocations; inherited timing limitation.
• Initial bare-year budget crash reproduced on both base and head and was rejected as a new exposure; scalar150 case alone establishes code-1.
• Captured Model the temporary 0% VAT on GB domestic electricity #2189 overlay resolves prior fixed-price denominator issue on the exercised Scenario paths, with final VAT/net-income parity.
• Re-average fuel duty and other blended rates after mid-year reforms #2185 captured overlay passes both fuel/VAT Reform composition orders.
• Captured CI Test still in progress; successful lint and Python3.12/3.13 smoke checks are separate evidence.
• No dependencies installed, tracked edits, external posting or helper agents.
• Optional broader coverage limitation: Hypothesis unavailable: test_reform_representations.py and test_fuel_duty_schedule.py did not collect; finite diagnostics do not replace the unavailable property sweeps. No specific required review question remains unresolved beyond the 132 tests and 93 targeted assertions.
• One confirmed critical runtime defect is owned by the code review: scalar DfE budget150, pre-load public Scenario route, exact base constructs but head crashes after corrected baseline activates budget adjustment. No duplicate policy selector.
• Preserving the processed unreformed baseline agrees with the counterfactual contract; dynamic isolation, untouched schedules, combined scenarios and final VAT/net-income parity pass.
• Default post-load budget allocation remains zero on both revisions; inherited structural-registration timing, not a second new finding. Earlier bare-year crash control was also inherited and discarded for new-defect attribution.
• External policy-source audit and policy axiom are inapplicable to this framework change; no changed statutory values. Empty source manifest validated.
• Two regression modules cannot collect without Hypothesis; code role retains failure logs and supplies targeted checks. Captured overlay evidence does not update prior standalone reports.
• Scope, policy-role start, and dispatch subspans were not individually measured; code-role duration uses its first measured worker clock.
• Coverage classification adjudication span uses the clock marker immediately after contradictory handoff review at 15:26:33 UTC through the first clock marker after the original reviewer clarified optional coverage.

Validation Summary

132 passed, 1 warning in 74.34s; runnable combined test bounded command 77.14s, configuration pr-2193-code-tests-config.json and log pr-2193-code-tests.log. 53 isolation/processing/final VAT assertions: pr-2193-code-diagnostic-results.json (bounded 9.18s). 32 energy and 8 midyear overlay assertions: pr-2193-code-interaction-energy/midyear.json and logs (bounded 16.98s and 4.94s). Exact scalar base/head constructor observations: pr-2193-code-budget-base/head.json and logs (bounded 7.02s and 5.22s); base constructs, head before-load raises missing simulation AttributeError. Source overlay hashes and identities: pr-2193-overlay-provenance.json. Hypothesis collection failure retained; no full model/microsimulation rerun.

Independent static trace of exact four-file diff, Scenario field/constructor phases, processing/uprating/fiscal-date contract, baseline_vat→vat_change→household_tax→net-income route and budget allocation semantics. Read code-owned artifacts: pr-2193-code-tests.log (132 passed), pr-2193-code-diagnostic-results.json (53 assertions), pr-2193-code-interaction-energy.json (32), pr-2193-code-interaction-midyear.json (8), pr-2193-code-budget-base/head.json plus script (base success/head AttributeError for scalar pre-load150). No duplicate calculations. Source checker command existingPython3.13 check_source_manifest.py --manifest pr-2193-review-sources.json --worktree-root actual capture-policyengine-uk --run-root canonical RUN_ROOT returned exit0 (receipt f4f7e6); validation output pr-2193-review-source-cache-check.json.

CI at 826ac3c: Install + smoke-import (py3.11)=QUEUED, Install + smoke-import (py3.12)=SUCCESS, Install + smoke-import (py3.13)=SUCCESS, Install + smoke-import (py3.14)=QUEUED, Lint=SUCCESS, Test=IN_PROGRESS.

Detailed code, policy, source and reproduction artifacts were retained locally.

Timing

setup seconds: 40.46s; code role seconds: 896.96s; consolidation cleanup seconds: 211.04s; elapsed seconds: 1188.00s; agent count: 2.00s; source count: 0.00s; render count: 0.00s; tests passed: 132.00s; focused assertions passed: 93.00s; bounded tests cli wall seconds: 77.14s; adjudication seconds: 152.00s

Review Severity

REQUEST_CHANGES. Open findings: 1 critical, 0 should address, 0 suggestions.

[vahid-ahmadi]

Thanks @juaristi22, C1 is fixed in 1d264ea.

Root cause. policyengine-core's Simulation.apply_reform calls reform.apply(self.tax_benefit_system), so inside a structural reform's apply, self is the tax-benefit system. Core's own Simulation.__init__ sets tax_benefit_system.simulation = self before it applies reforms. The UK Simulation doesn't call core's __init__, so that link was never made. adjust_budgets (and disable_simulated_benefits) read self.simulation, so they could never run. Before this PR the before-load route hid that: the baseline tree held the reform too, so no spending change was found and the budget reform was never registered. Once the baseline stayed unreformed, the reform was registered and construction failed.

What changed.

• Simulation.apply_reform (UK) now sets tax_benefit_system.simulation = self before calling core. It does this on every call, so a cloned simulation's system points at the clone, not the original. The PR's unreformed baseline tree is unchanged.
• Running the adjustment for the first time exposed two bugs in adjust_budget, both fixed:
  • It read each year's allocation after writing earlier years. A written year carries over to later years, so each later year stacked the earlier change on top of its own (2027 came out at £79.7bn instead of £36.9bn for a £1bn allocation). It now reads every year before writing any.
  • A year with no allocation divided by zero, and set_input then rejected the NaN. That year is now skipped, since there is nothing to scale.

Tests (test_scenario_baseline_parameters.py, 6 new):

• Your repro (London adult, 40, £30,000, £1bn dfe_education_spending, gov.dfe.education_spending: 150 before data load) now constructs both Simulation and Microsimulation, with finite reformed and baseline household_net_income.
• Expected DfE values for both classes: each year from 2026 to 2029 equals £1bn plus (£150bn minus the unreformed budget). For 2026 that is £43.72bn against £107.28bn. The baseline stays at £1bn, 2023–25 stay at zero, and NHS and DfT spending are unchanged.
• A structural reform applied through apply_reform can read its simulation, on the original and on a clone.
• A dataset Microsimulation test, marked microsimulation: weighted 2026 DfE spending rises by exactly the budget change. It passed locally on the cached enhanced FRS 2023-24.

The 5 constructor, value and binding tests fail on the previous head and pass now. Full YAML policy suite: 2,712 passed. pytest outside tests/policy and tests/microsimulation: 3,232 passed, 9 skipped (8 as before plus the new dataset test, which needs the dataset), 2 xfailed, 0 failed. ruff format and check are clean.

One thing not fixed here: with the link in place, disable_simulated_benefits now gets past self.simulation but fails on its own stale variable names (attendance_allowance_reported, dla_m_reported, dla_sc_reported, pip_dl_reported, pip_m_reported no longer exist). It has never been reachable, so I'll handle it separately.

🤖 Generated with Claude Code

[MaxGhenis]

Suggested Reproducer: line for the new Fix evidence check; add it to the description, or narrow it to the test that reproduces the bug. It lists the tests this PR adds or changes, and the check (.github/check_fix_pr.py) accepts it against this PR's diff.

Reproducer: `policyengine_uk/tests/test_scenario_baseline_parameters.py::test_every_reform_route_scores_vat_against_the_unreformed_baseline`, `policyengine_uk/tests/test_scenario_baseline_parameters.py::test_no_reform_has_no_vat_change`, `policyengine_uk/tests/test_scenario_baseline_parameters.py::test_scenario_leaves_the_baseline_tree_identical_to_an_unreformed_system`, `policyengine_uk/tests/test_scenario_baseline_parameters.py::test_adjust_budgets_sees_a_scenario_spending_change`, `policyengine_uk/tests/test_scenario_baseline_parameters.py::test_before_load_spending_scenario_constructs`, `policyengine_uk/tests/test_scenario_baseline_parameters.py::test_before_load_spending_scenario_scales_the_allocation`

The check isn't required yet (decision d1368); it fails fix PRs whose description names no reproducer.

🤖 Generated with Claude Code

[vahid-ahmadi]

Rebased onto main (7f43df9).
The conflicts were in simulation.py and tax_benefit_system.py. Main's dated-reforms rework (3edfb4b) already keeps parameters.baseline unreformed, so I took main's version there and dropped my baseline= hunks. The regression tests and the C1 fix stay, and the changelog now only describes C1.
Tests: ruff check and format clean. This PR's tests pass (16, plus the dataset Microsimulation test). 162 pass across the Scenario, dated-reform-route, reform and core tests, and all 3,058 YAML policy tests pass.
@juaristi22 could you re-review C1 when you have a moment? It was fixed in 7f43df9 (was 1d264ea).

🤖 Generated with Claude Code

[vahid-ahmadi]

@juaristi22 after the rebase, main already keeps the baseline parameter tree unreformed (3edfb4b, via #2305), so what is left here is your C1 fix, the regression tests for #2188 and the adjust_budgets fix. Are you happy for me to retitle it to "Let Scenario structural reforms read the simulation, and add baseline regression tests"? Or would you rather I move C1 into its own PR and close this one?

[juaristi22]

Re-reviewed 7f43df920ecb3e79dc581f4b66b2cd84c20d4072. My earlier C1 is resolved: the structural reform now reads the current simulation, and the before-load Scenario/baseline regression tests pass (16 passed, 1 skipped locally). The proposed title, “Let Scenario structural reforms read the simulation, and add baseline regression tests”, works; C1 belongs in this PR.

One required fix remains in the PR description. This is a Fix PR, and the current repository Fix evidence checker fails because the body has no named Reproducer: line. Please add Reproducer: policyengine_uk/tests/test_scenario_baseline_parameters.py::test_before_load_spending_scenario_constructs. I verified that exact line makes the checker pass. Please also refresh the description, which still attributes baseline code and tests already on main to this branch. Full hosted CI is still pending. I can approve once the description and check are corrected.

[vahid-ahmadi]

@juaristi22 retitled, refreshed the description and added the Reproducer line. Could you re-review?