From b9cf0e62ce97ae62eb472b0ae066ff73b1edaf76 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Mon, 12 Oct 2026 05:00:26 -0400 Subject: [PATCH 1/3] Freeze snapshot values and watch node and scale metadata GPT-6.1 Sol's round-4 P3 notes on #577: - shared_state records values as JSON text, so a list-valued parameter changed in place no longer compares equal to its own snapshot. - Node and scale metadata, and each scale's band count, are compared too (a scale's type picks its tax-scale class). Co-Authored-By: Claude Opus 5.5 --- .../uk/budget_reform_contract.py | 42 ++++++++++++------- tests/test_uk_budget_reform_contract.py | 37 ++++++++++++++++ 2 files changed, 65 insertions(+), 14 deletions(-) diff --git a/src/policyengine/tax_benefit_models/uk/budget_reform_contract.py b/src/policyengine/tax_benefit_models/uk/budget_reform_contract.py index 4ca13fac..f0c15bfc 100644 --- a/src/policyengine/tax_benefit_models/uk/budget_reform_contract.py +++ b/src/policyengine/tax_benefit_models/uk/budget_reform_contract.py @@ -1348,28 +1348,40 @@ def _baseline_parameter_reader() -> Callable[[str, int], float]: return parameter_reader(reference_parameters()) -def _parameter_values(parameters) -> dict[str, list[tuple]]: - """Each parameter's dated values and metadata, by name. +def _frozen(value: Any) -> str: + """A value as text, so a list or dict changed in place still shows.""" + return json.dumps(value, sort_keys=True, default=str) + - Every entry of its timeline (date and value) and its metadata, so a - change on any date, or to how it is uprated, shows. A name can occur - more than once (``parameters.baseline`` repeats the tree), so every - occurrence is kept, in tree order. +def _parameter_values(parameters) -> dict[str, list[tuple]]: + """Each parameter's dated values and metadata, and each node's metadata. + + For a parameter, every entry of its timeline (date and value) and its + metadata, so a change on any date, or to how it is uprated, shows; for + a node or scale, its metadata and, for a scale, how many bands it has. + Values are recorded as text, so a list changed in place is caught. A + name can occur more than once (``parameters.baseline`` repeats the + tree), so every occurrence is kept, in tree order. """ - from policyengine_core.parameters import Parameter + from policyengine_core.parameters import Parameter, ParameterScale values: dict[str, list[tuple]] = {} - for parameter in parameters.get_descendants(): - if isinstance(parameter, Parameter): - values.setdefault(parameter.name, []).append( + for item in parameters.get_descendants(): + if isinstance(item, Parameter): + values.setdefault(item.name, []).append( ( tuple( - (entry.instant_str, entry.value) - for entry in parameter.values_list + (entry.instant_str, _frozen(entry.value)) + for entry in item.values_list ), - json.dumps(parameter.metadata, sort_keys=True, default=str), + _frozen(item.metadata), ) ) + else: + bands = len(item.brackets) if isinstance(item, ParameterScale) else None + values.setdefault(f"{item.name}#node", []).append( + (_frozen(getattr(item, "metadata", None)), bands) + ) return values @@ -1407,7 +1419,9 @@ def _state_values(tree) -> dict: if isinstance(tree, Mapping): # Timelines by name: each entry's date and value. return { - name: [(entry.instant_str, entry.value) for entry in timeline or []] + name: [ + (entry.instant_str, _frozen(entry.value)) for entry in timeline or [] + ] for name, timeline in tree.items() } return _parameter_values(tree) diff --git a/tests/test_uk_budget_reform_contract.py b/tests/test_uk_budget_reform_contract.py index d07e91c9..d0be7973 100644 --- a/tests/test_uk_budget_reform_contract.py +++ b/tests/test_uk_budget_reform_contract.py @@ -298,6 +298,43 @@ def test_module_parameters_unchanged_compares_the_shared_state(): assert row["status"] == "fail" and f"{tree}:{RATE}" in row["actual"] +def _fake_shared_tree(monkeypatch, data): + from policyengine_core.parameters import ParameterNode + + tree = ParameterNode("", data=data) + monkeypatch.setattr(contract, "_shared_trees", lambda: {"system": tree}) + return tree + + +def test_a_list_value_changed_in_place_is_caught(monkeypatch): + tree = _fake_shared_tree( + monkeypatch, {"sources": {"values": {"2000-01-01": ["earnings"]}}} + ) + before = contract.shared_state() + tree.sources.values_list[0].value.append("pensions") + row = contract.module_parameters_unchanged(before) + assert row["status"] == "fail" and "system:sources" in row["actual"] + + +def test_a_scale_metadata_or_band_change_is_caught(monkeypatch): + band = { + "threshold": {"values": {"2000-01-01": 0}}, + "amount": {"values": {"2000-01-01": 100}}, + } + tree = _fake_shared_tree( + monkeypatch, + {"scale": {"metadata": {"type": "single_amount"}, "brackets": [band]}}, + ) + before = contract.shared_state() + tree.scale.metadata["type"] = "marginal_amount" + row = contract.module_parameters_unchanged(before) + assert row["status"] == "fail" and "system:scale#node" in row["actual"] + tree.scale.metadata["type"] = "single_amount" + assert contract.module_parameters_unchanged(before)["status"] == "pass" + tree.scale.brackets.append(tree.scale.brackets[0].clone()) + assert contract.module_parameters_unchanged(before)["status"] == "fail" + + def test_module_parameters_unchanged_compares_projected_timelines(): before = contract.shared_state() if "projected" not in before: From 7226ded637dca25d30f74f0c2c0d3d6e41417f64 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Mon, 12 Oct 2026 05:06:48 -0400 Subject: [PATCH 2/3] Add a changelog fragment for the snapshot hardening Co-Authored-By: Claude Opus 5.5 --- changelog.d/582.fixed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/582.fixed.md diff --git a/changelog.d/582.fixed.md b/changelog.d/582.fixed.md new file mode 100644 index 00000000..5752b15c --- /dev/null +++ b/changelog.d/582.fixed.md @@ -0,0 +1 @@ +The UK Budget reform contract's shared-parameter check now catches list-valued parameters changed in place and changes to node or scale metadata or a scale's band count. From 57b3e2ab316b4cb488616b4951b9ae43d42a450f Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Mon, 12 Oct 2026 05:55:09 -0400 Subject: [PATCH 3/3] Watch band order and root metadata, and never let a value stop the run Reviews of #583 (GPT-6.1 Sol and Opus 5.5, both APPROVE with P3 notes): - each scale records its bands' names in order, so a reordered band (which changes what a positional reform targets) shows, as well as an added or removed one; - the root's metadata is compared too; - _frozen falls back to repr when a value cannot be JSON (non-string keys, a cycle), so the snapshot cannot crash the run; - the row's check text names what it compares. Co-Authored-By: Claude Opus 5.5 --- .../uk/budget_reform_contract.py | 32 +++++++++++++------ tests/test_uk_budget_reform_contract.py | 32 +++++++++++++++++++ 2 files changed, 54 insertions(+), 10 deletions(-) diff --git a/src/policyengine/tax_benefit_models/uk/budget_reform_contract.py b/src/policyengine/tax_benefit_models/uk/budget_reform_contract.py index f0c15bfc..996c36f6 100644 --- a/src/policyengine/tax_benefit_models/uk/budget_reform_contract.py +++ b/src/policyengine/tax_benefit_models/uk/budget_reform_contract.py @@ -1349,8 +1349,15 @@ def _baseline_parameter_reader() -> Callable[[str, int], float]: def _frozen(value: Any) -> str: - """A value as text, so a list or dict changed in place still shows.""" - return json.dumps(value, sort_keys=True, default=str) + """A value as text, so a list or dict changed in place still shows. + + JSON where it can be (key order ignored); otherwise ``repr``, so an + unusual value (non-string keys, a cycle) cannot stop the run. + """ + try: + return json.dumps(value, sort_keys=True, default=str) + except (TypeError, ValueError): + return repr(value) def _parameter_values(parameters) -> dict[str, list[tuple]]: @@ -1358,15 +1365,16 @@ def _parameter_values(parameters) -> dict[str, list[tuple]]: For a parameter, every entry of its timeline (date and value) and its metadata, so a change on any date, or to how it is uprated, shows; for - a node or scale, its metadata and, for a scale, how many bands it has. - Values are recorded as text, so a list changed in place is caught. A - name can occur more than once (``parameters.baseline`` repeats the - tree), so every occurrence is kept, in tree order. + the root, every node and every scale, its metadata and, for a scale, + its bands' names in order, so an added, removed or reordered band + shows. Values are recorded as text, so a list changed in place is + caught. A name can occur more than once (``parameters.baseline`` + repeats the tree), so every occurrence is kept, in tree order. """ from policyengine_core.parameters import Parameter, ParameterScale values: dict[str, list[tuple]] = {} - for item in parameters.get_descendants(): + for item in [parameters, *parameters.get_descendants()]: if isinstance(item, Parameter): values.setdefault(item.name, []).append( ( @@ -1378,7 +1386,11 @@ def _parameter_values(parameters) -> dict[str, list[tuple]]: ) ) else: - bands = len(item.brackets) if isinstance(item, ParameterScale) else None + bands = ( + tuple(band.name for band in item.brackets) + if isinstance(item, ParameterScale) + else None + ) values.setdefault(f"{item.name}#node", []).append( (_frozen(getattr(item, "metadata", None)), bands) ) @@ -1454,8 +1466,8 @@ def module_parameters_unchanged(before: dict) -> dict: "route": "all", "reform_digest": None, "check": ( - "every parameter's dated values and metadata in " - f"{', '.join(sorted(before))}" + "every parameter's dated values and metadata, and every node's " + f"metadata and band order, in {', '.join(sorted(before))}" ), "status": FAIL if changed else PASS, "expected": "policyengine-uk's shared parameters as they were before the run", diff --git a/tests/test_uk_budget_reform_contract.py b/tests/test_uk_budget_reform_contract.py index d0be7973..b70e2514 100644 --- a/tests/test_uk_budget_reform_contract.py +++ b/tests/test_uk_budget_reform_contract.py @@ -335,6 +335,38 @@ def test_a_scale_metadata_or_band_change_is_caught(monkeypatch): assert contract.module_parameters_unchanged(before)["status"] == "fail" +def test_reordered_bands_and_root_metadata_are_caught(monkeypatch): + def band(rate): + return { + "threshold": {"values": {"2000-01-01": 0}}, + "rate": {"values": {"2000-01-01": rate}}, + } + + tree = _fake_shared_tree( + monkeypatch, {"scale": {"brackets": [band(0.1), band(0.2)]}} + ) + before = contract.shared_state() + tree.scale.brackets.reverse() + row = contract.module_parameters_unchanged(before) + assert row["status"] == "fail" and "system:scale#node" in row["actual"] + tree.scale.brackets.reverse() + assert contract.module_parameters_unchanged(before)["status"] == "pass" + tree.metadata["uprating"] = "wages" + row = contract.module_parameters_unchanged(before) + assert row["status"] == "fail" and "system:#node" in row["actual"] + + +def test_an_unusual_value_is_compared_without_stopping_the_run(monkeypatch): + import datetime as dt + + tree = _fake_shared_tree(monkeypatch, {"p": {"values": {"2000-01-01": 1}}}) + tree.p.values_list[0].value = {dt.date(2020, 1, 1): 1} + before = contract.shared_state() + assert contract.module_parameters_unchanged(before)["status"] == "pass" + tree.p.values_list[0].value[dt.date(2020, 1, 1)] = 2 + assert contract.module_parameters_unchanged(before)["status"] == "fail" + + def test_module_parameters_unchanged_compares_projected_timelines(): before = contract.shared_state() if "projected" not in before: