Skip to content

Allocate Virginia's 529 deduction to the owner of record and deduct it after VAGI - #9875

Open
MaxGhenis wants to merge 4 commits into
mainfrom
fix-va-529-owner-taxable-income
Open

MaxGhenis wants to merge 4 commits into
mainfrom
fix-va-529-owner-taxable-income

Conversation

@MaxGhenis

@MaxGhenis MaxGhenis commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Virginia's Commonwealth Savers (529) deduction previously allocated contributions between spouses by federal AGI share and reduced Virginia adjusted gross income (VAGI). This PR assigns the deduction to the contributing owner and takes it after VAGI, on Form 760 line 13. Owners aged 70 or older can deduct their full current-year contribution under the model's existing input conventions.

Virginia Code § 58.1-322.03(7)(a)–(b) specifies the owner-of-record deduction, the $4,000 per-account cap, and its exemption at age 70. The 2024 Form 760 instructions, code 104, PDF page 28 confirm those rules. Form 760 computes VAGI on line 9, takes Schedule ADJ deductions on line 13, and computes taxable income on line 15. Subtraction code 34 concerns certain plan distributions and refunds, rather than these contributions.

Correct line placement preserves each spouse's VAGI before the 529 deduction. It can affect the Spouse Tax Adjustment, the VAGI filing threshold, and low-income credit eligibility. When the deductible amount and other deductions and exemptions are unchanged, moving it between lines leaves taxable income unchanged.

Changes

  • Each tax-unit head or spouse claims their own modeled contribution, capped at $4,000 times their account count before age 70 and uncapped from exactly age 70. Dependent-owned contributions are excluded from the parents' return.
  • va_529_plan_deduction sums the per-person deductions. va_taxable_income subtracts that sum after VAGI; the deduction is removed from the VAGI subtraction list.
  • Variables live under deductions/plan_529. The existing subtractions.plan_529 parameter path is retained for reform compatibility, with an explanatory comment in cap.yaml.
  • The statutory age threshold and corrected references are included, together with the existing changelog fragment.

Tests

The existing variable-named YAML cases are preserved. They cover owner allocation, contribution and account caps, mixed-age spouses, dependent exclusion, age 69, age 72, and exactly 70, including the regression added at b4fb768. The integration cases independently calculate Form 760 taxable income, the Spouse Tax Adjustment, and the filing threshold.

Focused Python coverage adds relational invariants across paired synthetic Virginia returns for 2024 and 2026, using leaf inputs for wages, ages, contributions, and account counts. It compares calculated arrays across matched returns and tax-unit membership rather than reproducing a policy formula. These cross-entity relationships complement YAML's fixed external-source examples. The test lives with the existing Core simulation invariants in the Rest core group. Existing policy, property, and differential coverage is retained; no CI jobs or concurrency are added.

The relational regression fails in both years on the exact merge base, bab1a47c6046a5f74a51f250e2ffaee171dc90e0: the former income-share allocation gives a positive deduction to a filer whose own contribution is zero. Both cases pass at the follow-up head. The existing unit YAML file passes all 10 cases with exit code 0, and the integration YAML file passes all 3 cases with exit code 0. Each YAML run reports one existing pytest plugin-rewrite warning. make format also passes. These are 15 passing cases from targeted single-file checks, not full-suite or statewide impact results.

Local cost on macOS/Python 3.13: the passing test calls took 3.88 seconds for 2024 and 14.44 seconds for 2026. The wrapper's elapsed times differ from pytest's reported durations and are not a reliable base-versus-head performance comparison. Peak memory is unavailable: the sandbox denied the profiler's kern.clockrate system-statistics lookup after pytest completed. The profiler's nonzero exit is recorded separately from pytest's two passing cases.

The existing CI run at the reviewed head reports the Rest core group at 9:37.22 elapsed and 6,373,348 KiB peak RSS (about 6.1 GiB). The new test follows the existing cross-entity simulation-invariant coverage in that group, with 24 synthetic tax units/72 people per year, one simulation per year, shared read-only policy, and no dataset or reform. No new full-group or Linux resource benchmark was run; the local single-file measurements do not establish the new group's total peak memory.

The CI run triggered by the follow-up commit was skipped. Full-suite validation at the new head is therefore not established by that run.

Invariants

  • Each tax unit's deduction equals the sum of its own members' per-person deductions. Dependent contributions cannot increase the parents' deduction.
  • For nonnegative contribution and account-count inputs, each modeled filing owner's deduction is nonnegative and no larger than their contribution. Before age 70 it is also bounded by the modeled aggregate cap, $4,000 times their account count; at age 70 or older the contribution is fully deductible under the disclosed input conventions.
  • Matched returns differing only in their contribution inputs have identical person-level and tax-unit VAGI. With the taxable-income floor inactive, their taxable-income difference equals the modeled deduction.
  • Moving the same deductible amount from a VAGI subtraction to a deduction after VAGI leaves taxable income unchanged when other deductions and exemptions are fixed. VAGI-tested results can change.

Hub microsimulation impact (current head)

A real microsimulation on the default dataset, main at the merge base bab1a47c60 against this PR's head 5324f5038f, for 2025 and 2026. The runs went one at a time under the shared heavy-job lock. Sums are weighted.

Output 2025 change 2026 change 2025 records changed (weighted) 2026 records changed (weighted)
va_income_tax +0 +0 0 (0) 0 (0)
va_agi +0 +0 0 (0) 0 (0)
va_529_plan_deduction +0 +0 0 (0) 0 (0)
state_income_tax +0 +0 0 (0) 0 (0)
income_tax +0 +0 0 (0) 0 (0)
household_net_income +0 +0 0 (0) 0 (0)
household_benefits +0 +0 0 (0) 0 (0)

Comparison files are in fixes/9875-impact/ on the hub host.

Impact

The hub table above shows zero change, and the zero is by construction: the default dataset has no 529 contribution inputs (investment_in_529_plan_indv is not among its columns), so va_529_plan_deduction is $0 on both sides. A population run can't measure this PR. Its effect is limited to household calculations with entered contributions: married filers' Spouse Tax Adjustment, owners aged 70 or older, dependents' contributions, and filing-threshold or low-income-credit eligibility. The targeted unit, integration and invariant tests in Tests cover those.

Review status

Earlier independent reviews approved the model repair. The delta review at b4fb768 found no P1 defect and one P2, missing microsimulation impact, which the hub table above closes. The round 2 delta review at 5324f5038f (static read, Opus via Subfleet): APPROVE. The accepted methodology disclosures below remain in force under the October 7 leaf-input principle.

Methodology

  • Ownership: the leaf input investment_in_529_plan_indv is interpreted as contributions to accounts owned by that person. A spouse paying into the other spouse's account is credited as the payer; that can affect the age test and deduction allocation.
  • Per-account cap: the statute caps each actual account. The model uses the owner's total contribution and count_529_contribution_beneficiaries, imposing $4,000 times that count. For $7,000 in one account and $1,000 in another, the statute allows $5,000 while this existing aggregate approximation allows $8,000.
  • Carryforward: excess contributions may carry forward, and the age-70 provision offsets amounts previously deducted. Neither carryforwards nor that offset is modeled, as before.
  • Dependents: dependent-owned contributions are excluded from the parents' return; separate dependent returns are not modeled by this tax unit.
  • Age: the model uses annual age; the instructions test whether age 70 was attained on or before December 31.

Partner contract coverage

No partner contract test files or expectations are edited. This round makes no additional formula change. A static search found no explicit Virginia or 529-contribution scenario in the partner fixtures. The overall PR can change Virginia API outputs for the affected ownership, age, VAGI, and eligibility cases; partner impact is not measured by this round's targeted tests.

axiom: Va. Code § 58.1-322.03(7) TheAxiomFoundation/rulespec-us#1510 queued

MaxGhenis and others added 2 commits October 6, 2026 07:08
…after VAGI

The Commonwealth Savers (529) deduction is a deduction from Virginia adjusted
gross income (Va. Code § 58.1-322.03(7); Form 760 instructions, deduction code
104, entered on Form 760 line 13), not a subtraction in computing VAGI. Code 34,
which the subtraction list cited, is for distributions and refunds. Only the
owner of record may claim the deduction, and an owner who has attained age 70
deducts the full contribution.

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

codecov Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (909176a) to head (5324f50).
⚠️ Report is 337 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #9875   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         3    -1     
  Lines           76        45   -31     
  Branches         2         0    -2     
=========================================
- Hits            76        45   -31     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@DTrim99

DTrim99 commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator

PR Review: Allocate Virginia's 529 deduction to the owner of record and deduct it after VAGI

PR #9875 (author: MaxGhenis, head 3fa0fd7063).

Summary

The PR makes three changes to Virginia's Commonwealth Savers (529) deduction:

  • Owner of record. The deduction now goes to the account owner of record. Before, it was prorated between spouses by federal AGI share.
  • Age 70. It adds the § 58.1-322.03(7)(b) rule: an owner aged 70 or older deducts the full contribution, with no per-account cap.
  • After VAGI. It moves the deduction out of the VAGI subtraction list (Form 760 lines 4–7) and takes it from VAGI on line 13, inside va_taxable_income.

The law, the values and the code are all correct, and every caller of the changed values behaves as Virginia's forms require. All 12 new YAML cases pass on the extracted head tree, and I checked each one by hand. The only gap is one boundary test: nothing tests an owner aged exactly 70, so the new >= comparison is not pinned. The 7 suggestions are optional polish or documented deferrals.

Critical (must fix)

None.

Should address

1. Nothing tests the new age-70 rule at exactly age 70.

  • Where: policyengine_us/variables/gov/states/va/tax/income/deductions/plan_529/va_529_plan_deduction_person.py:31 (age_exempt = person("age", period) >= p.age_threshold). The tests are at policyengine_us/tests/policy/baseline/gov/states/va/tax/income/deductions/plan_529/va_529_plan_deduction.yaml:71-133.

  • What's wrong: The age tests use 69 (capped), 71 and 72 (full deduction). A formula written with > instead of >= would pass all of them, so the boundary the law sets is not protected.

  • Source:

    • Va. Code § 58.1-322.03(7)(b): a contributor "who has attained age 70 shall not be subject to the limitation that the amount of the deduction not exceed $4,000 per ... college savings trust account in any taxable year" (statute).
    • 2024 Form 760 instructions, code 104: "if you are age 70 or older on or before December 31 of the taxable year, you may deduct the entire amount contributed during the taxable year" (PDF p. 28).
  • The code is already right. On the head tree, an owner aged exactly 70 who puts $10,000 into one account deducts $10,000. On main the same owner deducts $4,000. Only the test is missing.

  • Fix: Add a case next to the age-69 case:

    - name: An owner who turns 70 in the year deducts the full contribution
      period: 2024
      input:
        state_code: VA
        age: 70
        investment_in_529_plan_indv: 10_000
        count_529_contribution_beneficiaries: 1
      output:
        va_529_plan_deduction: 10_000

Suggestions

  1. The per-account cap is pooled across the beneficiary count. This was already true, and the PR body discloses it.

    • Where: va_529_plan_deduction_person.py:27-28 (min_(contributions, p.cap * accounts)).
    • Law: § 58.1-322.03(7)(a) says "in no event shall the amount deducted in any taxable year exceed $4,000 per contract or college savings trust account". Code 104 says "the lesser of $4,000 or the amount contributed during the taxable year to each Commonwealth Savers account" (PDF p. 28).
    • Impact: Uneven splits are over-allowed. $6,000 + $1,000 into two accounts is allowed $5,000 by law and $7,000 by the model.
    • The test name is misleading. The case at va_529_plan_deduction.yaml:19 is called "Two beneficiaries increases the cap" (7,000 → 7,000). It is right only if neither account gets more than $4,000, for example $4,000 + $3,000.
    • Fix: Rename it to "Two accounts, neither over $4,000" and add a comment giving the assumed split. Per-account inputs can wait for a follow-up.
  2. Neither the carryforward nor the age-70 "previously deducted" offset is modeled. This is a documented deferral.

    • Law: § 58.1-322.03(7)(a) says the excess "may be carried forward and subtracted in future taxable years until the purchase price or college savings trust contribution has been fully deducted". Code 104 says "you may carry forward any undeducted amounts" (PDF p. 28).
    • The model has no prior-year input, and the PR body's methodology note says so. No change is needed in this PR.
  3. The input convention changed. Document it in the formula comment.

    • Where: va_529_plan_deduction_person.py:20-27.
    • How it works now: The formula reads the contributions and the account count from the same person. Every other state's 529 formula uses the tax-unit sum, add(tax_unit, period, ["count_529_contribution_beneficiaries"]). I probed this on the head tree for TY2024:
      • A $3,000 contribution recorded on the spouse, with count_529_contribution_beneficiaries: 1 on the head, now gives $0. main gives $3,000.
      • A tax-unit-level investment_in_529_plan input is now ignored for Virginia. The MD and AL tests use that input style.
      • An owner under 70 with no account count gets $0. This was also true before the PR. An owner aged 70 or older with no account count gets the full amount.
    • The law supports this. The owner of record is a per-person fact: "the person shown as such on the records of the Commonwealth Savers Plan as of December 31" (§ 58.1-322.03(7)(a)). The PR body already describes the convention.
    • Fix (optional): Add one sentence to the formula comment: "The account count must be recorded on the same person as the contributions; tax-unit-level investment_in_529_plan inputs are not used."
  4. Optional tests for the VAGI consumers that have none yet. The integration tests already pin va_agi, va_agi_person/STA and va_must_file. va_low_income_tax_credit_agi_eligible reads va_agi directly, so it is covered indirectly. The PR body names low-income credits as a third affected place, and a direct test would document that. I computed these expectations by hand for TY2024 and confirmed them on the head tree:

    Case Inputs Expected (PR) main
    Low-income credit eligibility Single parent aged 30 with a child aged 5; wages $21,000; $2,000 into one account owned by the parent va_agi 21,000 > tax_unit_fpg 20,440, so va_low_income_tax_credit_agi_eligible false and va_low_income_tax_credit 0. va_taxable_income 8,640 (21,000 − 8,500 − 1,860 − 2,000) va_agi 19,000, eligible, credit 600
    Owner of record on separate returns Two tax units, both filing_status: SEPARATE. Spouse A is 45 with $50,000 wages. Spouse B is 72 with $30,000 wages and $9,000 into one account va_529_plan_deduction [0, 9,000]. va_taxable_income [40,570, 0]: B's VAGI is 18,000 after the $12,000 age deduction, and 18,000 − 8,500 − 1,730 − 9,000 is floored at 0 B deducts 4,000

    The low-income source is the 2024 instructions: "family's Virginia adjusted gross income (family VAGI) is equal to or less than the federal poverty guidelines" (PDF p. 30). In the first case, household tax is unchanged at −529.95 on both branches, because the refundable EITC is larger than the credit.

  5. Update the "Microsim impact: Pending" section of the PR body.

    • Neither Populace nor policyengine-us-data populates investment_in_529_plan_indv, which defaults to 0. The queued 2024 and 2026 runs will therefore show no change, and the "Invariants" check "on the microsim data" will pass trivially.
    • Fix: Replace "Pending" with a note that the microdata has no 529 contributions, so the change affects household calculations only.
  6. Some test comments call unrounded tax amounts "form lines".

    • Where: integration.yaml:36 ("Line 16 = ... = 4_178.05") and :73-74 (STA "Line 11 ... 2_568.05", "Line 12 ... 213.025").
    • Form: 2024 Form 760 line 16 says "(round to whole dollars)" (Form 760 PDF p. 2). The instructions' rate-schedule example rounds "$4,917.50 ... to $4,918" (PDF p. 41). PolicyEngine's Virginia tax has never rounded, so the expectations are fine.
    • The case name at integration.yaml:8 says "lines 1-15", but the case also asserts line 16.
    • Fix: Reword the comments to "rate-schedule tax before whole-dollar rounding", and say "lines 1-16" in the name.
  7. The parameter folder name no longer matches the item's legal type.

    • Where: policyengine_us/parameters/gov/states/va/tax/income/subtractions/plan_529/. It now holds the parameters for a deduction from VAGI, including the new age_threshold.yaml. The formula reads them from a deductions/plan_529/ variable (va_529_plan_deduction_person.py:19).
    • The PR body explains that the path was kept so saved reforms that point at subtractions.plan_529.cap keep working. No in-repo reform references it.
    • Fix (optional, could be a follow-up): Move the folder to deductions/plan_529/, which matches the variable folder and the Schedule ADJ heading "Deductions from Virginia Adjusted Gross Income".

Verified correct

  • The deduction comes after VAGI.
    • The § 58.1-322.03 lead-in says "there shall be deducted from Virginia adjusted gross income".
    • The 2024 Form 760 runs line 9 (VAGI), then line 13 ("Deductions from Schedule ADJ, Line 9"), then line 15 (taxable income) (p. 1).
    • va_taxable_income.py:29-40 subtracts the deduction next to the existing line-13 items (the CDCC expense and educator deductions).
    • The old "Certification Number 34" label was wrong. Code 34 covers income from Commonwealth Savers distributions or refunds (instructions PDF p. 26).
  • Owner of record and the age-70 rule. Each spouse's deduction is computed from that spouse's own contributions, account count and age. Dependent-owned accounts are excluded from the parents' return. Code 104 says "Only the owner of record for an account may claim a deduction for contributions made" (PDF p. 28).
  • Values. The $4,000 cap and age 70 match § 58.1-322.03(7)(a)-(b) and code 104. Stage A checked this for every year from 2021 to 2025. The new section citations, (7)(a) for the cap and (7)(b) for the age, are correct.
  • Code patterns.
    • No hard-coded values: the formula uses p.cap and p.age_threshold.
    • Entities are right: va_529_plan_deduction_person is a Person variable and va_529_plan_deduction is a TaxUnit variable built with adds.
    • Both use definition_period = YEAR, defined_for = StateCode.VA and unit = USD.
    • The formula is fully vectorized (min_, where, and a boolean mask for head or spouse).
    • Each variable reference is single and corroborating.
    • The parameter descriptions follow the house template ("... at this amount", "... this age").
    • The test folder mirrors the variable folder (deductions/plan_529/).
    • All test periods are plain (2024).
  • No dead code or orphans.
    • The old subtractions/va_529_plan_deduction{,_person}.py files are deleted.
    • va_529_plan_deduction_person is removed from subtractions.yaml.
    • At the head, nothing else in variables, parameters, reforms, docs or programs.yaml references va_529* or subtractions.plan_529.
  • Callers (grepped at the head).
    • va_529_plan_deduction is read only by va_taxable_income.
    • The VAGI change reaches these consumers, all of which should use pre-529 VAGI under the forms:
      • va_must_file: the line 9 threshold.
      • va_low_income_tax_credit_agi_eligible: family VAGI.
      • state_agis.yaml and the TAXSIM state_agi alias.
      • The contrib va_dependent_exemption phase-out.
      • Through va_agi_person: va_agi_less_exemptions_person → STA, and va_agi_share → va_eitc_person, which is used only for separate filers.
    • va_age_deduction_agi uses FAGI minus taxable Social Security, so it is unaffected.
    • The HB 979 reform reads va_taxable_income, so it picks up the deduction automatically.
  • Tests.
    • All 12 new cases (9 unit, 3 integration) pass on the extracted head tree.
    • The 4 cases from the deleted subtractions/va_529_plan_deduction.yaml are carried over verbatim.
    • No existing expectation was changed.
    • Changed branches covered: owner of record on a joint return; the per-account cap (below, above, and two accounts); age 69 and age 71/72; mixed ages on one return; dependent owners; VAGI ordering through va_agi, the separate-VAGI/STA worksheet and the filing threshold.
  • Hand-checked expectations (TY2024).
    1. Contribution above the cap: min(6,000, 4,000 × 1) = 4,000.
    2. Owner of record: the spouse's min(3,000, 4,000) = 3,000 and the head's 0 give [0, 3,000]. main gives [1,800, 1,200] by FAGI share.
    3. Mixed ages: the head at 45 gets min(6,000, 4,000) = 4,000 and the spouse at 71 gets the full 6,000, giving [4,000, 6,000], a total of 10,000.
    4. Integration case 1:
      • Taxable income: 100,000 − 17,000 − 2 × 930 − min(5,000, 4,000) = 77,140.
      • Tax: 720 + 5.75% × 60,140 = 4,178.05.
    5. STA case:
      • Taxable income: 72,000 − 17,000 − 1,860 − 4,000 = 49,140. The smaller spouse's VAGI less exemptions is 11,070.
      • Lines 8-10: tax(11,070) = 423.50 and tax(38,070) = 1,931.525, which sum to 2,355.025.
      • Line 11: tax(49,140) = 2,568.05.
      • STA: 2,568.05 − 2,355.025 = 213.025, under the $259 cap.
    6. Filing threshold: VAGI 13,000 ≥ 11,950, so the filer must file. Taxable income is 13,000 − 8,500 − 930 − 2,000 = 1,570, and tax is 2% × 1,570 = 31.40.
  • PR-body figures. These match the merge base: the main values [1,800, 1,200], VAGI of 96,000, separate VAGI of [56,666.67, 11,333.33], and VAGI of 11,000. Seven of the new cases fail on main, as the body says. The invariant "va_taxable_income is unchanged when the deduction amount is unchanged" holds, because both paths subtract the same amount before the zero floor.
  • Changelog. changelog.d/fix-va-529-owner-taxable-income.fixed.md is top-level, the fixed type is correct, and the text matches the behavior.

Axiom line assessment

The line reads axiom: Va. Code § 58.1-322.03(7) TheAxiomFoundation/rulespec-us#1510 queued, which is a valid format.

  • rulespec-us#1510 ("Encode Virginia's Commonwealth Savers deduction for the recorded owner and apply it after VAGI") is OPEN and labelled pe-parity.
  • It is dispatch-ready. It gives the module path us-va/statutes/58.1/58/1-322/03.yaml, the corpus citation us-va/statute/58.1/58.1-322.03, verbatim text for § 58.1-322.03(7)(a)-(b), code 104, Schedule ADJ and the Form 760 line structure, the required outputs, and companion tests derived from the statute and form arithmetic.
  • It matches the current behavior and notes that carryforward is not modeled.

No action is needed.

CI status

All 37 checks pass (gh pr checks 9875). Codecov reports that all modified, coverable lines are covered.

Branch status

  • The branch is 38 commits behind main and 2 ahead.
  • git merge-tree --write-tree PolicyEngine/main 3fa0fd7063 is clean, with no conflicts.
  • The only Virginia change on main since the merge base is a new CDCC-limit test file, which does not overlap this PR.
  • The PR touches no partner test files.

Verdict: REQUEST_CHANGES

@DTrim99 DTrim99 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The law, values and code are correct: the deduction comes off VAGI (Form 760 line 13), goes to the owner of record, and is uncapped for owners 70 or older, and every VAGI consumer correctly sees pre-529 VAGI. rulespec-us#1510 is dispatch-ready. One should item (see the comment above): no test pins the age-70 boundary (the >= at va_529_plan_deduction_person.py:31). Please add an age-exactly-70 owner case with $10,000 into one account, expecting 10,000.

@MaxGhenis
MaxGhenis marked this pull request as draft October 8, 2026 23:18
@MaxGhenis
MaxGhenis marked this pull request as ready for review October 9, 2026 05:37
@MaxGhenis

Copy link
Copy Markdown
Contributor Author

@DTrim99, your 10/7 should-item is addressed at head 5324f5038f. va_529_plan_deduction.yaml now has an owner aged exactly 70 who contributes $10,000 to one account, and expects a $10,000 deduction. A > comparison would fail that case. The invariant test test_va_529_invariants.py also includes an age-70 row.

The default dataset has no 529 contribution inputs, so the hub's population run shows zero change, as the body explains. Could you re-review?

@MaxGhenis
MaxGhenis requested a review from DTrim99 October 9, 2026 05:38
@DTrim99

DTrim99 commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator

PR Review (round 2): Allocate Virginia's 529 deduction to the owner of record and deduct it after VAGI

PR #9875 (author: MaxGhenis). Round 1 reviewed 3fa0fd7063; this round reviews head 5324f5038f, which adds two test commits and no formula change.

Summary

The round-1 should item is fixed. Commit b4fb768 adds the YAML case I asked for: an owner aged exactly 70 puts $10,000 into one account and deducts $10,000. Commit 5324f5038f adds a deterministic invariants test, policyengine_us/tests/core/test_va_529_invariants.py, that also covers age 70.

I ran a mutation check on an extracted copy of the head. With >= changed to > at va_529_plan_deduction_person.py:31:

  • the unit YAML fails only the new age-70 case (1 failed, 9 passed);
  • the invariants test fails in both 2024 and 2026.

With the original >=, both pass. One new finding is a suggestion: a CI paragraph in the PR body is out of date. There are no open critical or should items.

Round-1 items

# Round-1 item Status Notes
Should 1 No test at exactly age 70 for the >= comparison ADDRESSED va_529_plan_deduction.yaml now has "An owner aged exactly 70 deducts the full contribution" (age 70, $10,000, one account, expects 10,000). The invariants test has rows (70, 4,001, 1) and (70, 10,000, 2). Under the > mutation it fails with 4,000 against 4,001 and 8,000 against 10,000.
Sugg. 1 Per-account cap pooled across the account count; "Two beneficiaries increases the cap" name NOT ADDRESSED (optional) The test name is unchanged. The body's Methodology still discloses the approximation with a worked example ($7,000 + $1,000: the law allows $5,000 and the model $8,000).
Sugg. 2 Carryforward and the age-70 "previously deducted" offset not modeled ACKNOWLEDGED-DEFERRED Disclosed under Methodology → Carryforward.
Sugg. 3 Formula comment on the input convention NOT ADDRESSED (optional) The body's "Ownership" bullet covers the per-person reading. The formula comment still doesn't say the account count must be on the same person, or that a tax-unit investment_in_529_plan input is ignored for Virginia.
Sugg. 4 Optional tests for VAGI consumers (low-income credit, separate returns) PARTIALLY The invariants test asserts identical va_agi_person and va_agi for paired returns in 12 configurations in 2024 and 2026, and a dollar-for-dollar drop in va_taxable_income. It adds no direct low-income-credit or separate-return case.
Sugg. 5 "Microsim impact: Pending" in the body ADDRESSED Replaced with a 2025/2026 hub table at this head showing zero change. A new Impact section explains that the dataset has no 529 contribution column, so household calculations are the only place the change shows.
Sugg. 6 Integration comments call unrounded tax "form lines"; case name says "lines 1-15" NOT ADDRESSED (optional) integration.yaml is unchanged. The expectations themselves are correct.
Sugg. 7 Parameter folder subtractions/plan_529/ holds a deduction's parameters ACKNOWLEDGED-DEFERRED cap.yaml now opens with "# Retain the legacy subtractions parameter path for reform compatibility.", and the body explains this. The new age_threshold.yaml has no saved reforms to protect, so it could move to deductions/plan_529/ in a follow-up.

New findings

Critical: none. Should: none.

Suggestion 1. The CI paragraph in the PR body is out of date.

  • What it says: "The CI run triggered by the follow-up commit was skipped. Full-suite validation at the new head is therefore not established by that run."
  • What happened since: A later run at the same head (5324f5038f, Actions run 37889457522) finished, and every check passed. That includes the Rest (Python + variables) job, which ran the new test.
  • Fix: Point the paragraph at the passing run.

The new invariants test

  • Deterministic and never skipped. The test uses a fixed tuple of 12 owner cases, each built as two paired returns of three people, and pytest.mark.parametrize over 2024 and 2026. It uses no Hypothesis, no randomness, no importorskip and no skip markers.
  • Runs in CI.
    • In the Rest (Python + variables) job, both cases show PASSED, taking 0.36 s and 0.37 s.
    • The Quick Feedback selective job also ran it (2 passed), along with the unit YAML (10 cases) and the integration YAML (3 cases).
    • The test's warnings are existing divide-by-zero RuntimeWarnings from other variables, such as va_national_guard_subtraction_person.
  • Catches the old bug. Run against the merge base bab1a47c60, it fails in both years at deduction <= amounts. The old AGI-share allocation gave a deduction to a spouse who contributed nothing, so the body's claim is right.
  • Independent oracle. The test uses the literal amounts from the statute (70 and $4,000) and compares results across returns and tax-unit membership. It never repeats the formula.
    • The aggregation check depends on Core keeping people in the order the situation lists them, and a comment documents that.
    • If Core reordered people, the test would fail rather than pass by accident.
  • The taxable-income check holds.
    • Wages total $100,000 per return, so the zero floor cannot apply.
    • The zero-contribution twin zeroes all three members' contributions.
    • The asserted difference is the tax-unit total, which already leaves out the dependent.
  • Hand-checked rows (TY2024).
    • (69, 4,001, 1) → 4,000.
    • (69, 4,001, 0) → 0, because zero accounts give a zero cap.
    • (70, 4,001, 1) → 4,001.
    • The spouse (40, $6,000, one account) → 4,000. The dependent → 0.
    • The (70, 10,000, 2) return totals 10,000 + 4,000 + 0 = 14,000, and its taxable income is 14,000 lower than its twin's.
  • Local runs on the extracted head. The invariants test gives 2 passed. Both 529 YAML files give 13 passed.
  • The cap.yaml change is a YAML comment only. It doesn't change how the parameter loads.

Axiom line assessment

Unchanged: axiom: Va. Code § 58.1-322.03(7) TheAxiomFoundation/rulespec-us#1510 queued. rulespec-us#1510 is still OPEN and labelled pe-parity. This round changes no formula, so the issue still matches the PR. No action is needed.

CI status

All 28 checks in the latest run at 5324f5038f pass, including codecov patch and project. Three wrapper checks (Baseline, Contrib and Python tests) from an earlier run at the same head show as skipped. A later run at that head replaced them.

Branch status

Verdict: APPROVE

@DTrim99 DTrim99 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 2: the age-70 boundary is now pinned deterministically (a YAML case and invariants-test rows). Flipping >= to > fails exactly those cases, the invariants test ran and passed in CI, and the merge-tree against main is clean. The remaining items are optional suggestions.

This branch has not been deployed

No deployments
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