Isolate shared policy from later reforms and repair clone routing

PROGRESS.md

@@ -0,0 +1,134 @@
+# Simulation isolation repairs (Astra review of country 2.0.1)
+
+Branch `max/spm-runtime-isolation-20260914` off `upstream/main` (2.0.1).
+Source: `astra-review-country-201.md`. PR A of two.
+
+## State
+- [x] Read review; read `policyengine_us/spm.py`, core `simulations/simulation.py`,
+      `taxbenefitsystems/tax_benefit_system.py`, `reforms/reform.py`, `parameters/*`.
+- [x] Measured `system.parameters.clone()`: **1.5-2.0 s** against a 0.03 s warm
+      household build and a 0.16 s warm calculate. Detaching on every simulation
+      (the structural reform is re-applied to *every* one) was therefore not an
+      option; copy-on-write had to be genuinely lazy.
+- [x] Reproduced findings 1-5 on this runtime and turned each into a regression
+      test. Every new test fails on the parent commit `e24e655a04`.
+- [x] Fix findings 1-4 in `policyengine_us/spm.py`.
+- [x] Fix finding 5 (county input type) and the two documentation defects.
+- [x] Full core/unit + microsimulation + representative YAML runs.
+- [x] Draft PR #9463 opened against `main`.
+- [x] Adversarial review (4 lenses, 27 raised, 18 confirmed after per-finding
+      verification) and every confirmed finding repaired - see below.
+- [ ] Final re-run after the review repairs; update the PR body.
+
+## Design decisions
+1. **Copy-on-write parameter tree.** `share_spm_policy` returns an instance of a
+   per-base `SharedParameter<Base>` subclass whose `parameters` is a property.
+   `SPMSimulationMixin.apply_reform` and the shared system's own
+   `apply_reform_set` arm a read barrier for the duration of the reform; the
+   first read inside that window clones the tree through core and gives this
+   system a cold private at-instant cache, leaving the lender's tree and warm
+   caches untouched. `modify_parameters`, `load_extension` and
+   `add_abolition_parameters` - the three core methods that edit the live tree
+   in place - detach outright. A variable-only reform never reads `parameters`,
+   so the structural reform re-applied at each simulation's start instant still
+   pays nothing. Measured: household build stays 0.05 s; a parameter reform pays
+   1.24 s once. Branches created with `clone_system=False` are moved onto the
+   detached tree, because they share their parent's policy deliberately.
+2. **Clone alias routing.** Core keeps `self.calc = self.calculate` and
+   `self.df = self.calculate_dataframe` as bound methods in the instance dict and
+   copies them verbatim when cloning. `clone()` now rebinds every copied bound
+   method by method name, so an alias core adds later is repaired too.
+3. **Private entities.** `share_spm_policy` shallow-copies the entities and binds
+   them to the private system, keeping one object per key across `entities`,
+   `person_entity` and `group_entities` (`clone_spm_system` now does the same).
+   An entity resolves variable names through the system it is bound to, so this
+   is what makes `set_input` find a reform-added variable.
+4. **Traced parameter receipts.** Only the *root* node carries core's
+   request-specific `trace`/`tracer`/`branch_name` and caches the resulting
+   `TracingParameterNodeAtInstant`; children are wrapped on the fly. So a traced
+   simulation gets a shallow private root with its own at-instant cache
+   (microseconds, not a 1.5 s tree clone), primed as traced - core marks the tree
+   in `_run_formula`, but `_calculate` reads `gov.abolitions` first, so on
+   core 3.30.2 nothing was ever recorded. Two traced simulations now each record
+   their own four `spm_unit_fpg` parameter accesses, and the shared tree is left
+   untraced.
+5. **County input type.** Tightening `is_county_fips` alone would have been
+   inert: `value_type = str` maps to the numpy `object` dtype, so the model
+   stores an integer column as integers, and both readers stringify before
+   asking the provider. The provider now records which counties arrived as
+   something other than text - at the one point that still sees the type - and
+   rejects them when a county measurement needs them, before its own memo. A
+   caller who never asks for a county is still never asked for one.
+
+## Hardening carried from the blast-radius audit
+- A shared-policy system clones through an ordinary copy: core's clone writes
+  the cloned tree into the instance dictionary, where the property would shadow
+  it and hand back a "clone" still sharing the lender's tree.
+- The shared-policy class is published under its own name, so systems built on
+  it still pickle.
+- `tools/pinned_tbs.py` keys its cache on the parameter tree as well as the
+  system, because a shared system keeps its identity while detaching a tree.
+
+## Notes
+- Installed core here is **3.30.2**, not the 3.32.5 Astra reviewed. `calc`/`df`
+  sit at `simulation.py:223-224` and the clone copy loop at 1381-1416, matching
+  Astra's citations.
+- Astra's finding 1 sub-case "an existing unrelated simulation changes after
+  cache invalidation" needs a real invalidation: `delete_arrays("income_tax")`
+  alone leaves the cached standard deduction, so the contaminated tree is not
+  reached. The regression test uses `_invalidate_all_caches()`, the same purge
+  `apply_reform` performs.
+- Astra's finding 4 count ("four accesses, then zero") does not reproduce
+  literally on core 3.30.2, where a traced simulation recorded *no* parameter
+  accesses at all: `_calculate` reads `gov.abolitions` and caches a non-tracing
+  at-instant node before `_run_formula` marks the tree. Priming the private root
+  as traced repairs that as well, and the test asserts the four accesses per
+  simulation that Astra described.
+
+## Review repairs (adversarial review of the change itself)
+
+Confirmed findings and what each cost, all now fixed and pinned by tests that
+fail when the fix is reverted:
+
+1. **Ownership was tracked on the borrower, not on the tree.** A system that
+   detached a private copy and then lent it to a second simulation went on
+   writing to it in place. Sharing is now a one-way mark on the tree, and every
+   simulation's own system carries the barrier - not only the ones borrowing
+   the shipped policy - because a tree a simulation starts out owning acquires
+   readers later. Core's `ParameterNode.clone` copies the instance dictionary,
+   so a private clone is explicitly unmarked.
+2. **Traced simulations stranded their shared-policy branches.** Branches were
+   moved onto a detached tree by root identity, which a traced branch can never
+   match, because tracing hands it a shallow copy of the root. Since the model's
+   own formulas branch this way (itemizing vs not, state EITC refundability,
+   marginal tax rates), a traced `calculate -> apply_reform -> calculate`
+   reported pre-reform numbers - up to a $2,860 error in the verifier's
+   constructed case. Branches are matched on the children of their root now.
+3. **An adopting branch had its copy-on-write switched off**, so its own reform
+   rewrote its parent's tree. It keeps sharing instead.
+4. **A shallow traced root orphaned its children's parent chain**, so an
+   in-place edit cleared the original root's at-instant cache and left the
+   traced copy serving pre-reform values. The root is only copied when the tree
+   is shared; otherwise it is marked traced where it stands.
+5. **A tuple of variable-only reforms paid for a full tree clone**, because
+   core recurses into `apply_reform` per member and the nested call's read of
+   `parameters` fell inside the outer call's armed window. 1.29 s -> 0.001 s.
+6. **The county input-type record was a monotone, unkeyed blacklist.** Keyed on
+   `(year, county)` and replaced per year now, so correcting an input clears
+   it; carried only by `copy_receipts=True` snapshots, so a new simulation on a
+   lender's system is not tainted; recorded on a reform simulation's baseline
+   arm as well; skipped entirely for non-county selections; and each write
+   scans only the period it wrote.
+7. **Test-efficacy gaps**: the clone override, the entity de-duplication, the
+   `load_extension` detach and the pinned-system re-key had no test that would
+   fail if reverted. All four now do (verified by reverting each).
+
+Residual, documented rather than fixed:
+
+- A county rejection is scoped to a county code and year, not to a row, because
+  the county reaching a provider has already been stringified by the variable
+  that read the column. A simulation holding a mistyped code fails every
+  request for that code in that year.
+- `holder.set_input` bypasses the country's `set_input`, so a county written
+  straight through core's holder API after construction is not re-recorded.
+  That channel behaves exactly as it did before this change.

changelog.d/spm-runtime-isolation.fixed.md

@@ -0,0 +1 @@
+Isolate one simulation's reform, clone and trace from every other simulation: a parameter reform applied after construction no longer changes unrelated simulations, a clone's `calc` and `df` aliases calculate on the clone rather than the original, `set_input` finds a variable a reform added, each traced request records its own parameter accesses, and a `county_fips` input that is not a five-digit string is rejected however it is spelled.

docs/usage/microsimulation.md

@@ -110,12 +110,16 @@ formula-owned SPM outputs such as `spm_unit_spm_threshold`. Any observed Census
 measurement is retained under a separate report-only name.
 
 The legacy files under `hf://policyengine/policyengine-us-data/` predate that
-contract, so resource and poverty outputs are not available over them:
+contract, so SPM measurements are not available over them:
 
 - `cps_2023.h5` stores `spm_unit_spm_threshold`, so the loader rejects it.
 - `enhanced_cps_2024.h5` loads and computes tax variables, but carries no SPM
-  independence roles, so SPM units consisting of one 15-to-17-year-old classify
-  no measurement adult and every resource output over the file fails closed.
+  independence roles, so 18 of its SPM units - each a lone 15-to-17-year-old -
+  classify no measurement adult, and `spm_unit_spm_threshold` raises
+  `SPM_COMPOSITION_REQUIRED` over the file. That is what
+  `test_legacy_enhanced_cps_lacks_source_backed_spm_independence_roles`
+  checks. Outputs that no longer reach the threshold, household net income
+  and benefits among them, do compute over the file.
 
 A household simulation that has no county input can select an SPM area
 explicitly instead:
@@ -205,12 +209,13 @@ When running microsimulations, verify that weights produce sensible population t
 ```python
 sim = Microsimulation()
 
-# Check population
-person_weight = sim.calc("person_weight", map_to="person")
+# Check population. The weights are the values here, so read them
+# unweighted: a weighted sum would square them.
+person_weight = sim.calc("person_weight", map_to="person", use_weights=False)
 print(f"Total population: {person_weight.sum():,.0f}")
 
 # Check household count
-household_weight = sim.calc("household_weight")
+household_weight = sim.calc("household_weight", use_weights=False)
 print(f"Total households: {household_weight.sum():,.0f}")
 
 # Verify key aggregates against published statistics
@@ -221,6 +226,14 @@ total_snap = sim.calc("snap").sum()
 print(f"Total SNAP benefits: ${total_snap / 1e9:.1f}B")
 ```
 
+`calc` returns a weighted series whose `sum()` multiplies each value by its
+weight, which is what makes the aggregates above population totals. Summing a
+weight variable that way squares the weights: with household weights of 2 and
+3, `sim.calc("household_weight").sum()` is 13 rather than 5. Either opt out
+with `use_weights=False`, as above, or use `count()`, which sums the weights
+themselves - `sim.calc("age", map_to="person").count()` is the same population
+total.
+
 Compare these totals against official statistics to validate your analysis:
 - US population: ~330 million
 - US households: ~130 million