Catch in-place list edits and scale metadata changes in the Budget contract's shared-parameter check

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.

src/policyengine/tax_benefit_models/uk/budget_reform_contract.py

@@ -1348,28 +1348,52 @@ 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.
+
+    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)
+
 
-    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
+    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
+    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, *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 = (
+                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)
+            )
     return values
 
 
@@ -1407,7 +1431,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)
@@ -1440,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",

tests/test_uk_budget_reform_contract.py

@@ -298,6 +298,75 @@ 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_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: