Fix UK /calculate crashing on the policyengine-uk wrapper Simulation#1632
Merged
Conversation
policyengine-uk 2.43 replaced its core Simulation subclass with a wrapper that builds its own tax-benefit system and no longer accepts one from outside, so every UK calculation has returned a 500 since the pin first crossed that boundary (2.65.0, December 2025). Detect which constructor shape each country package exposes, pass situations and reforms through the wrapper's own arguments (preserving the API's reform value casting), and serialize the wrapper's string-array enum results alongside core-style EnumArrays. Adds UK coverage to the unit and deployed test lanes, which previously never sent a UK household through /calculate — the gap that let this ship broken. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The integration_with_auth lane never sent a UK household through the local Flask endpoint, so a UK regression there would only surface at deploy time. Cover it with the most hand-verifiable case: a single unemployed adult aged 25+ with no housing costs, whose Universal Credit is exactly the 12-month standard allowance (including the Universal Credit Act 2025 uplift the model applies from April 2026), pinned at £5,079.13 for 2026. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…orm handling policyengine-us's Simulation.__init__ is (*args, **kwargs), so probing the signature for an explicit tax_benefit_system parameter misclassified it as wrapper-style: no-reform US requests worked by accident through core's default-system fallback, but any US reform request would have handed the policy dict to core's `reform` argument, which expects a Reform class. Treat a **kwargs-forwarding constructor as core-style and pin the US reform path with a unit test. Also consolidate reform handling found in review: one _cast_reform_values step now feeds both simulation paths (the injected-system path applies it to a cloned system; the wrapper path passes it through), calculate() checks the path flag once, emptiness checks use plain truthiness, and the dead create_policy_reform helper (zero call sites, third copy of the casting logic) is deleted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s; validate policy period keys Replace signature sniffing with explicit routing chosen once in PolicyEngineCountry.__init__: UK calculations go through a dedicated wrapper-style builder, all other countries through the standard core-style builder. The UK builder applies reforms through a Scenario with applied_before_data_load=True. The wrapper's own reform argument applies parameter changes only after it has created structural reforms from baseline parameter values, so reforms on structural-trigger parameters (gov.contrib marriage-tax, disable_simulated_benefits, Scottish child payment, household HITC) changed the parameter but never activated the reform, returning baseline numbers with a 200. Pinned by a marriage-allowance unit test; note the wrapper samples trigger parameters at its default_input_period, so reforms must cover that instant. /calculate now validates policy period keys up front: every key must be two dot-separated instants. This closes a silent divergence where the UK wrapper path accepted a bare instant and applied the change to a single day, returning near-baseline numbers with a 200 while the core path 500'd. The check returns a descriptive 500 to match the endpoint's long-standing status for bad policy input; issue #1628 tracks moving it to a 400 and documenting the grammar. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Enum outputs requested together with axes returned silent nulls on the
UK path: the wrapper decodes enums to string arrays, the shared
astype(float) reshaping raised, and the axes error handler swallowed it.
Axes reshaping and scalar enum decoding are now country strategy
functions bound alongside the simulation builder; the UK variants return
decoded names, the standard variants are unchanged (US enum-under-axes
still returns numeric indices, tracked upstream).
_cast_reform_value now takes the parameter's type from its most recent
non-null value (values_list is newest-first, so the old [-1] read the
oldest — a null placeholder there crashed valid reforms) and casts
booleans explicitly: bool("false") is True in Python, so string "false"
used to enable the flag it was meant to disable. Unrecognized boolean
strings now raise instead of silently coercing.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Move the policy period-key check after all household validation in get_calculate, restoring main's error precedence (mixed-invalid payloads returned 400 "Invalid household variables." before the policy 500; the first placement flipped that). The grammar now lives in country.py next to the code that consumes it — validate_policy_periods runs inside _cast_reform_values on every calculate() regardless of caller, so future endpoints can't reintroduce the silently-accepted bare-instant bug, and the endpoint calls the same helper for its explicit-500 mapping. Pin the deliberate _cast_bool narrowing with tests: string "2"/"1.0" and null — accepted by accident before the rewrite — now raise, alongside the intended "false" fix. Add a changelog fragment for the casting rewrite, fix the stale _cast_reform_values docstring that still described the reform= argument, document the policy grammar in the OpenAPI spec, and pin the Scenario interface the UK builder relies on so a policyengine-uk rename fails in CI rather than per-request in prod. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
_cast_bool accepted any number via bool() while rejecting the same digits as strings — JSON 2 silently enabled a flag that "2" 500'd on. Only 0 and 1 now coerce; other numbers raise like ambiguous strings do. Pinned by rejection tests (2, -3, 2.5) and acceptance tests (0.0, 1.0), with the changelog fragment and OpenAPI policy description updated to match. Also fix the stale "up front" wording (the policy check deliberately runs after household validation) in the changelog fragment and the validate_policy_periods docstring, and tighten the endpoint 500 test to startswith so dropping the endpoint's own validation call — which exists solely for the unwrapped message — fails a test instead of silently degrading the contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
anth-volk
marked this pull request as ready for review
July 16, 2026 14:38
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
1 participant
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.
Fixes #1631
What broke
policyengine-uk 2.43.0 replaced its core
Simulationsubclass with a wrapper whose constructor is(scenario, situation, dataset, trace, reform)and which builds its own tax-benefit system internally.PolicyEngineCountry.calculatestill passedtax_benefit_system=, so every UK/calculaterequest has returned a 500 since the UK pin crossed that boundary (2.65.0, December 2025). No test lane sent a UK household through the endpoint, so nothing caught it.What this PR does
PolicyEngineCountry.__init__: UK calculations go through a wrapper-style builder; US/CA/NG/IL keep the injected-system path byte-for-byte. Enum serialization (scalar and under axes) is routed the same way, since the wrapper decodes enums to string arrays while core returnsEnumArrays.Scenariowithapplied_before_data_load=True. The wrapper's ownreform=argument applies parameter changes only after it creates structural reforms from baseline parameters, so reforms on structural-trigger parameters (gov.contrib.*) changed the parameter but never activated the reform — HTTP 200 with baseline numbers. Upstream: Structural-trigger parameters are sampled at a fixed instant, so reforms gating structural changes can silently fail to activate policyengine-uk#1800."start.stop"instant pairs) as a single grammar owned bycountry.pyand enforced on everycalculate(); the endpoint maps failures to a descriptive 500 after all household validation, preserving historical error precedence. The UK wrapper otherwise silently applies bare-instant keys to a single day. Moving to 400 + docs: Malformed policy period keys should return 400 Bad Request, not 500, and be documented #1628."false"previously enabled boolean parameters (bool("false")isTrue); accepted boolean forms are now true/false,"true"/"false","1"/"0", and the numbers 0/1, with ambiguous forms raising. The parameter's type is read from its most recent non-null value instead of its oldest.create_policy_reform, updates the OpenAPIpolicydescription, and adds three changelog fragments.Verification
make test(676+ tests),make test-with-auth(7), andmake format-checkall green locally through development; the final suite run at the branch tip covers the same slices that ran green per-commit.Related
#1628 (400 migration), #1629 (UK app extrication), #1630 (drop CA/NG/IL)
🤖 Generated with Claude Code