Skip to content

Require every S.770 and SSA contributed parameter node to be classified in the calculator reference test - #10572

Merged
MaxGhenis merged 1 commit into
mainfrom
ss-calculator-reference-contrib-node-guard
Nov 1, 2026
Merged

MaxGhenis merged 1 commit into
mainfrom
ss-calculator-reference-contrib-node-guard

Conversation

@MaxGhenis

@MaxGhenis MaxGhenis commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

This makes the calculator reference test fail when a parameter node under gov.contrib.congress.s770 or gov.contrib.ssa hasn't been classified as either calculator-only or kept in the reference.

The gap

policyengine_us/tests/unit/test_ss_calculator_differential.py builds WithoutHouseholdCalculator. That reference model removes the calculator-only variables (CALCULATOR_PREFIXES) and the parameter nodes in NEW_PARAMETER_NODES, and the test then checks that legacy outputs match. Two changes narrowed that tuple:

At main, NEW_PARAMETER_NODES holds only gov.ssa.social_security.pia.bend_points and gov.ssa.social_security.cola, so no node under either namespace is removed.

Failure scenario. A later PR adds a calculator-only node, say gov.contrib.congress.s770.section_3 or gov.contrib.ssa.<new>, without listing it. A legacy formula then reads it by accident. The reference and the real simulation both hold the node and compute the same values, so the differential still passes. Nothing makes the author decide whether the node is calculator-only.

Readers of each child at main

Node Readers outside the household calculator Classification
gov.contrib.congress.s770.section_2 tools/social_security/population.py and population_survivor.py, which serve the reported-benefit PIA-ratio variables (social_security_*_pia_ratio_*, *_inferred_aime*) kept
gov.contrib.congress.s770.section_8 reforms/congress/s770/section_8.py (create_s770_section_8_reform reads it at simulation start); the s770_nii_base, s770_nii_taxable_base, s770_nii_addon_tax, s770_active_taxable_base and s770_active_business_income_tax formulas kept
gov.contrib.ssa.benefit_levers social_security_*_payable_factor, social_security_across_the_board_factor, reforms/ssa/cola_means_test.py, tools/social_security_benefit_levers.py, tools/social_security_periods.py, population.py, means_test.py kept
gov.contrib.ssa.owners_draw reforms/ssa/owners_draw.py; s770_active_business_income; system.py writes current_law_cap into it at every system build kept
gov.contrib.ssa.pia tools/social_security/population.py (reported-benefit PIA ratios) kept

Calculator modules (tools/social_security/engine.py, parameters.py) also read section_2, benefit_levers and pia, but because of the readers above none of the five is calculator-only. #10483 had merged before this branch, so gov.contrib.ssa.owners_draw is on main and classified here.

The change

Only test_ss_calculator_differential.py changes, plus a changelog fragment:

  • A new module-level tuple, REFERENCE_PARAMETER_NODES, lists the five kept nodes, each with a one-line comment naming its readers. CLASSIFIED_CONTRIB_NAMESPACES names the two namespaces.
  • NEW_PARAMETER_NODES stays a flat tuple of dotted paths, unchanged.
  • The new test, test_every_contributed_calculator_namespace_child_is_classified, reads each namespace's children from the live parameter tree (Simulation.default_tax_benefit_system_instance.parameters, which the file already builds). It fails when any of these is true, and the failure message names the node and the choice to make:
    • a child appears in neither tuple;
    • a path appears more than once across the two tuples;
    • a kept entry names a node that doesn't exist.

When #10560 lands, it adds gov.contrib.ssa.history, read only by the history route. That PR will need to list it, probably in NEW_PARAMETER_NODES.

Mutant check (scratch copy, not committed)

I added an unlisted sibling, gov.contrib.congress.s770.section_3, with a single in_effect: false parameter:

  • Unlisted: the new test fails with "gov.contrib.congress.s770.section_3 is unclassified: add it to NEW_PARAMETER_NODES if only the household calculator reads it, or to REFERENCE_PARAMETER_NODES with a comment naming the legacy formula, reform start-up or system build that reads it".
  • Listed in REFERENCE_PARAMETER_NODES: passes.
  • Listed in NEW_PARAMETER_NODES: passes.
  • Listed in both: fails with "is listed 2 times".
  • Scratch node removed but still listed as kept: fails with "is listed in REFERENCE_PARAMETER_NODES but does not exist".

The scratch node and the edits were removed, and the file was compared byte for byte with the committed version.

Tests

  • The whole file at this head: 9 passed in 141 s of pytest time (190 s wall, peak RSS 2.59 GB), in one local foreground process on a 4-vCPU VM. These are local measurements, not CI's.
  • The new test itself takes under 0.5 s and builds nothing new.
  • CI ran the whole file at this head: 9 passed, in Pull request run 41342824694.

impact: none (one test file changes; no variable, parameter or reform file changes)
axiom: n/a (test only)

🤖 Generated with Claude Code

…nce test

No node under gov.contrib.congress.s770 or gov.contrib.ssa is removed from
the calculator reference any more, so a new calculator-only node left
unlisted would stay in both models and a legacy formula that read it by
accident would go undetected. The new test reads both namespaces' children
from the live tree and fails unless each appears exactly once, either in
NEW_PARAMETER_NODES or in REFERENCE_PARAMETER_NODES, whose entries name
their non-calculator readers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@MaxGhenis

Copy link
Copy Markdown
Contributor Author

Ready for hub review

Head: 42022d84f14aa67b22aa790c09cfdc3dffb6be86 (from main 469ad12cb9, which includes #10484 at fea0d26's merge and #10483).

Steps 1–5

  1. Gap confirmed at main. NEW_PARAMETER_NODES holds only gov.ssa.social_security.pia.bend_points and gov.ssa.social_security.cola; no node under either namespace is removed. Main hasn't changed the file beyond what the item describes.

  2. Children and readers. Main has five: s770.section_2, s770.section_8, ssa.benefit_levers, ssa.owners_draw (Add contributed owners draw SECA reform #10483 has merged; system.py writes current_law_cap into it at every system build) and ssa.pia. Each has a reader outside the household calculator, so all five are kept. The reader table is in the PR body. None is calculator-only.

  3. Test. REFERENCE_PARAMETER_NODES is at policyengine_us/tests/unit/test_ss_calculator_differential.py:39, with a one-line reader comment per entry. test_every_contributed_calculator_namespace_child_is_classified is at :117. It reads both namespaces' children from the live tree the file already builds. It fails on any of three conditions, and the message names the node and the choice:

    • an unclassified child;
    • a duplicate across the two tuples;
    • a kept entry that doesn't exist.

    NEW_PARAMETER_NODES is unchanged and flat.

  4. Scratch mutant. I added an unlisted gov.contrib.congress.s770.section_3/in_effect.yaml (false):

    • unlisted: fails with the message;
    • listed as kept: passes;
    • listed as calculator-only: passes;
    • listed in both: fails ("listed 2 times");
    • node removed but still listed: fails ("does not exist").

    Removed and restored; checked with cmp. Nothing committed.

  5. Whole file at head: 9 passed, 141 s pytest time, 190 s wall, peak RSS 2.59 GB. Local, one foreground process. The file took about 63 s at Add S.770 section 8 investment and active income tax options #10402's head, so main's other cases have grown since; the new test itself is under 0.5 s.

CI and body

  • CI: draft, not run.
  • Body: checked against my file; the only difference is that the GitHub proxy appends its attribution footer after the impact:/axiom: lines. I couldn't keep those two lines last.
  • Changelog: changelog.d/ss-calculator-reference-contrib-node-guard.changed.md, because pr.yaml requires a fragment.

Judgment calls

  • I added two checks beyond "exactly once": a duplicate across the tuples fails, and so does a stale kept entry. Without the stale-entry check, a renamed node could stay "classified" under its old name.
  • When Add annual Social Security earnings histories and per-year reforms #10560 lands, it adds gov.contrib.ssa.history. Only the history route reads it, so that PR must list it, most likely in NEW_PARAMETER_NODES. This test will force that choice.

Generated by Claude Code

@MaxGhenis
MaxGhenis marked this pull request as ready for review November 1, 2026 19:39
@MaxGhenis
MaxGhenis merged commit c8c61c9 into main Nov 1, 2026
41 checks passed
@MaxGhenis

Copy link
Copy Markdown
Contributor Author

US + core hub merge audit, us#10572 at 42022d84f14aa67b22aa790c09cfdc3dffb6be86 (squash): require every S.770 and SSA contributed parameter node to be classified in the calculator reference test.

  • Reviews: GPT-6.1 Sol on a Subfleet hard-tier lane approved this head in round 1. It found each node's readers itself, reproduced the gap at main, and confirmed all five mutants in the PR body. It raised only P3 notes.
  • Gates checked live at merge:
    • gh pr checks exits 0;
    • the latest Pull request run at the head (41342824694) concluded success;
    • MERGEABLE;
    • not a draft;
    • no reviewer's latest review requests changes;
    • head pinned.
  • Review notes applied: the hub corrected two stale body lines, on CI and on which nodes the calculator modules read. The test code didn't change.
  • Follow-ups the hub keeps: the test reads direct children only, so it misses a calculator-only leaf under a kept node. It checks stale paths in REFERENCE_PARAMETER_NODES but not in NEW_PARAMETER_NODES. Separately, Sol found that both existing NEW_PARAMETER_NODES entries also have readers on the reported-benefit PIA-ratio route. That behaviour is the same on main, so this PR doesn't change it.
  • Merge order: Add annual Social Security earnings histories and per-year reforms #10560 adds gov.contrib.ssa.history, so its CI will fail this test until it classifies that node. That's the test working as intended.
  • Impact: none. The diff changes one test file and a changelog fragment, so it can't move any output, and no pair ran.
  • Axiom: n/a, test only.

@MaxGhenis
MaxGhenis deleted the ss-calculator-reference-contrib-node-guard branch November 1, 2026 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants