Skip to content

Fix ok_federal_ctc crash under fully-refundable CTC reforms - #9323

Merged
DTrim99 merged 5 commits into
PolicyEngine:mainfrom
DTrim99:fix/ok-federal-ctc-afa
Sep 2, 2026
Merged

Fix ok_federal_ctc crash under fully-refundable CTC reforms#9323
DTrim99 merged 5 commits into
PolicyEngine:mainfrom
DTrim99:fix/ok-federal-ctc-afa

Conversation

@DTrim99

@DTrim99 DTrim99 commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Fixes #9322.

The AFA contrib reform (gov.contrib.congress.afa) replaces gov.irs.credits.non_refundable with a list omitting non_refundable_ctc (fully-refundable restructure), and ok_federal_ctc unconditionally .index()ed that entry — so any Oklahoma simulation under the AFA raised ValueError("'non_refundable_ctc' is not in list"). Oklahoma is the only state that indexes the federal credit list this way, so the crash is OK-specific.

When the non-refundable CTC is absent there is no non-refundable portion to allocate against liability; the credit allowed for the Oklahoma Child Care/Child Tax Credit is the refundable CTC. Baseline behavior is unchanged (the guard only takes effect when a reform removes the credit from the list).

Test: exercises the exact production path (Reform.from_dict with the contrib flag) for an OK household and asserts ok_federal_ctc == refundable_ctc > 0.

🤖 Generated with Claude Code

The AFA contrib reform (gov.contrib.congress.afa) replaces the federal
non-refundable credit list without non_refundable_ctc (the credit becomes
fully refundable), and ok_federal_ctc unconditionally called
.index("non_refundable_ctc") on that list — so any Oklahoma simulation
under the AFA raised ValueError("'non_refundable_ctc' is not in list").

When the non-refundable CTC is absent from the list there is no
non-refundable portion to allocate against liability; the credit allowed
for the Oklahoma Child Care/Child Tax Credit is the refundable CTC.

Found running the AFA against every state's microdata (the crash is
Oklahoma-specific because only ok_federal_ctc indexes the credit list).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (57743a2) to head (d5932e0).
⚠️ Report is 109 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #9323   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         1    -2     
  Lines           65        25   -40     
  Branches         0         1    +1     
=========================================
- Hits            65        25   -40     
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 requested a review from PavelMakarchuk August 21, 2026 15:54

@PavelMakarchuk PavelMakarchuk 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.

Program Review

PR #9323 — Fix ok_federal_ctc crash under fully-refundable CTC reforms (fixes #9322)

Source Documents

  • PDF: None — code-only robustness fix, no policy values (PDF audit not applicable)
  • Year: 2026 (test year)
  • Scope: PR changes only
  • Reviewed head SHA: 69e4aa5
  • Mode: full

Critical (Must Fix)

None.

The fix logic was traced end-to-end: the AFA contrib reform's modify_parameters (policyengine_us/reforms/congress/afa/afa_other_dependent_credit.py:202-217) replaces gov.irs.credits.non_refundable for 2025-01-01..2039-12-31 with a list omitting non_refundable_ctc, so the old unconditional .index("non_refundable_ctc") raised ValueError for any OK tax unit. The new guard (policyengine_us/variables/gov/states/ok/tax/income/credits/ok_federal_ctc.py:29-30) is a membership test on the same scalar parameter list, is inert under baseline (every dated value of the list contains non_refundable_ctc), is semantically correct under removal reforms (no non-refundable portion is ever applied, so the allowed amount is exactly refundable_ctc), and preserves vectorization (scalar-parameter branching; the early return is a full-population array). Hoisting the refundable_ctc read is behavior-neutral. The new regression test genuinely fails without the fix and exercises the Reform.from_dict + structural-reform auto-activation path used by production/API traffic. Changelog fragment, references, entity levels, and formatting are all clean.

Should Address

  • [A1] policyengine_us/tests/policy/contrib/congress/afa/test_ok_federal_ctc_under_afa.py — Downstream OK credit not asserted under the reform. The crash in #9322 surfaces through consumers (ok_child_care_child_tax_credit, taxsim_ok_child_tax_credit_component), but the test stops at ok_federal_ctc. One added assertion in the same simulation locks in the full user-facing path, and the expected value is analytically clean (AGI 30k ≤ 100k limit, ok_agi/us_agi = 1, cdcc = 0, so the 5% CTC arm wins): sim.calculate("ok_child_care_child_tax_credit", 2026)[0] == pytest.approx(0.05 * refundable_ctc, rel=1e-4).
  • [A2] policyengine_us/tests/policy/contrib/congress/afa/test_ok_federal_ctc_under_afa.py — No zero-CTC case on the guard path. Only a positive-benefit scenario exercises the guard branch. An OK filer under AFA with no qualifying children (single adult, age 35, 30k income) should yield ok_federal_ctc == 0 via the guard's return refundable_ctc, proving it neither crashes nor invents a credit for ineligible households.

Suggestions

  • [S1] policyengine_us/tests/policy/contrib/congress/afa/test_ok_federal_ctc_under_afa.py:32-43 — Situation period keys are bare ints ({2026: 35}) while neighboring pytest contrib tests use string keys ({"2026": 30} in test_id_ga_ctc_reform_activation.py). Both work (CI passes); string keys would match local convention.
  • [S2] A companion YAML case in tests/policy/contrib/congress/afa/ (following afa_other_dependent_credit.yaml's reforms: + gov.contrib.congress.afa.in_effect: true pattern, with an OK household asserting ok_federal_ctc) would also lock the fix into the policyengine-core test harness/contrib YAML shard. Keep the pytest — its Reform.from_dict activation path is the distinct value — but the YAML case is nearly free and conventional.
  • [S3] Only AFA is tested among CTC-restructuring reforms. In-tree, AFA is the only reform that removes non_refundable_ctc from the list (guard-firing class), but ECPA (reforms/congress/tlaib/economic_dignity_for_all_agenda/edaa_end_child_poverty_act.py) and FISC (reforms/congress/golden/fisc_act.py) restructure the CTC while leaving the list intact, sending ok_federal_ctc down the ordered-allocation path against reform-modified totals with no test locking that in. A cheap parametrized smoke test over the three contrib flags asserting ok_federal_ctc computes for an OK household would guard against the next .index()-style fragility — OK is the only state indexing the federal credit list this way.
  • [S4] Post-2039 boundary: the test's reform dict runs in_effect to 2100, but AFA's modify_parameters stops at 2039-12-31, after which non_refundable_ctc re-enters the list and the guard goes inert. Not a crash risk, but a 2040 assertion (or a code comment) would document the cliff so a future year-bump doesn't silently change which branch is under test.
  • [S5] Phase-out boundary under AFA (high-income OK household where the fully-refundable CTC phases to 0 while remaining in the guard branch) is untested — low value, listed for completeness.

PDF Audit Summary

Not applicable (no source-document values in scope).

Validation Summary

Check Result
Regulatory Accuracy N/A — no policy values changed
Reference Quality N/A — no parameter changes; pre-existing variable references untouched
Code Patterns Pass — guard traced correct, baseline-inert, vectorization-safe; 2 minor suggestions
Formatting Pass — changelog fragment valid (fixed), no lint issues, references clean
Test Coverage Pass with gaps — regression test fails without fix; 2 should-address coverage additions
PDF Value Audit N/A
CI Status Pass — all 33 checks (contrib congress + states shards, microsim, partner API, codecov, lint, changelog)

Review Severity: APPROVE

No critical findings; the two should-address items are minor test-coverage additions to an already-valid regression test. All 33 CI checks pass and baseline behavior is verified unchanged.

Next Steps

To auto-fix issues: run the fix-pr workflow for this PR.

@PavelMakarchuk PavelMakarchuk 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.

Program Review

PR #9323 — Fix ok_federal_ctc crash under fully-refundable CTC reforms (fixes #9322)

Source Documents

  • PDF: None — code-only robustness fix, no policy values (PDF audit not applicable)
  • Year: 2026 (test year)
  • Scope: PR changes only
  • Reviewed head SHA: 69e4aa5
  • Mode: full

Critical (Must Fix)

None.

The fix logic was traced end-to-end: the AFA contrib reform's modify_parameters (policyengine_us/reforms/congress/afa/afa_other_dependent_credit.py:202-217) replaces gov.irs.credits.non_refundable for 2025-01-01..2039-12-31 with a list omitting non_refundable_ctc, so the old unconditional .index("non_refundable_ctc") raised ValueError for any OK tax unit. The new guard (policyengine_us/variables/gov/states/ok/tax/income/credits/ok_federal_ctc.py:29-30) is a membership test on the same scalar parameter list, is inert under baseline (every dated value of the list contains non_refundable_ctc), is semantically correct under removal reforms (no non-refundable portion is ever applied, so the allowed amount is exactly refundable_ctc), and preserves vectorization (scalar-parameter branching; the early return is a full-population array). Hoisting the refundable_ctc read is behavior-neutral. The new regression test genuinely fails without the fix and exercises the Reform.from_dict + structural-reform auto-activation path used by production/API traffic. Changelog fragment, references, entity levels, and formatting are all clean.

Should Address

  • [A1] policyengine_us/tests/policy/contrib/congress/afa/test_ok_federal_ctc_under_afa.py — Downstream OK credit not asserted under the reform. The crash in #9322 surfaces through consumers (ok_child_care_child_tax_credit, taxsim_ok_child_tax_credit_component), but the test stops at ok_federal_ctc. One added assertion in the same simulation locks in the full user-facing path, and the expected value is analytically clean (AGI 30k ≤ 100k limit, ok_agi/us_agi = 1, cdcc = 0, so the 5% CTC arm wins): sim.calculate("ok_child_care_child_tax_credit", 2026)[0] == pytest.approx(0.05 * refundable_ctc, rel=1e-4).
  • [A2] policyengine_us/tests/policy/contrib/congress/afa/test_ok_federal_ctc_under_afa.py — No zero-CTC case on the guard path. Only a positive-benefit scenario exercises the guard branch. An OK filer under AFA with no qualifying children (single adult, age 35, 30k income) should yield ok_federal_ctc == 0 via the guard's return refundable_ctc, proving it neither crashes nor invents a credit for ineligible households.

Suggestions

  • [S1] policyengine_us/tests/policy/contrib/congress/afa/test_ok_federal_ctc_under_afa.py:32-43 — Situation period keys are bare ints ({2026: 35}) while neighboring pytest contrib tests use string keys ({"2026": 30} in test_id_ga_ctc_reform_activation.py). Both work (CI passes); string keys would match local convention.
  • [S2] A companion YAML case in tests/policy/contrib/congress/afa/ (following afa_other_dependent_credit.yaml's reforms: + gov.contrib.congress.afa.in_effect: true pattern, with an OK household asserting ok_federal_ctc) would also lock the fix into the policyengine-core test harness/contrib YAML shard. Keep the pytest — its Reform.from_dict activation path is the distinct value — but the YAML case is nearly free and conventional.
  • [S3] Only AFA is tested among CTC-restructuring reforms. In-tree, AFA is the only reform that removes non_refundable_ctc from the list (guard-firing class), but ECPA (reforms/congress/tlaib/economic_dignity_for_all_agenda/edaa_end_child_poverty_act.py) and FISC (reforms/congress/golden/fisc_act.py) restructure the CTC while leaving the list intact, sending ok_federal_ctc down the ordered-allocation path against reform-modified totals with no test locking that in. A cheap parametrized smoke test over the three contrib flags asserting ok_federal_ctc computes for an OK household would guard against the next .index()-style fragility — OK is the only state indexing the federal credit list this way.
  • [S4] Post-2039 boundary: the test's reform dict runs in_effect to 2100, but AFA's modify_parameters stops at 2039-12-31, after which non_refundable_ctc re-enters the list and the guard goes inert. Not a crash risk, but a 2040 assertion (or a code comment) would document the cliff so a future year-bump doesn't silently change which branch is under test.
  • [S5] Phase-out boundary under AFA (high-income OK household where the fully-refundable CTC phases to 0 while remaining in the guard branch) is untested — low value, listed for completeness.

PDF Audit Summary

Not applicable (no source-document values in scope).

Validation Summary

Check Result
Regulatory Accuracy N/A — no policy values changed
Reference Quality N/A — no parameter changes; pre-existing variable references untouched
Code Patterns Pass — guard traced correct, baseline-inert, vectorization-safe; 2 minor suggestions
Formatting Pass — changelog fragment valid (fixed), no lint issues, references clean
Test Coverage Pass with gaps — regression test fails without fix; 2 should-address coverage additions
PDF Value Audit N/A
CI Status Pass — all 33 checks (contrib congress + states shards, microsim, partner API, codecov, lint, changelog)

Review Severity: APPROVE

No critical findings; the two should-address items are minor test-coverage additions to an already-valid regression test. All 33 CI checks pass and baseline behavior is verified unchanged.

Next Steps

To auto-fix issues: run the fix-pr workflow for this PR.

Applies the Should-Address and Suggestion coverage items from
PavelMakarchuk's 2026-08-25 review of the ok_federal_ctc crash fix. Test
additions only — no code, parameter, or formula change.

- A1: assert the downstream ok_child_care_child_tax_credit under AFA
  (0.05 * refundable_ctc, the 5% CTC arm) so the full user-facing consumer
  path is locked in.
- A2: add a zero-CTC guard-path case (single OK adult, no children) proving
  the guard returns 0 and does not crash.
- S1: use string period keys to match the neighboring contrib convention.
- S2: add a companion YAML case (OK parent, one age-4 child, 2025, AFA
  in_effect) asserting refundable_ctc == ok_federal_ctc == 4,320, locking
  the fix into the contrib YAML shard.
- S3: parametrized smoke test over the three CTC-restructuring contrib flags
  (AFA, ECPA, FISC) asserting ok_federal_ctc computes and stays non-negative.
- S4: add a 2040 post-cliff case (AFA modify_parameters stops 2039-12-31)
  plus a comment documenting the boundary.
- S5: high-income guard-branch case asserting the relational invariant
  ok_federal_ctc == refundable_ctc regardless of phase-out.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCwrSsGY7vPCpKYGfV1xRr
@DTrim99

DTrim99 commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator Author

Review addressed — thanks @PavelMakarchuk

Applied both Should-Address items and all five Suggestions from your 2026-08-25 review. Test coverage only — no code, parameter, or formula change (the guard is unchanged).

Should Address

  • A1 — the existing positive-benefit AFA test now also asserts the downstream ok_child_care_child_tax_credit: with AGI 30k ≤ 100k, ok_agi/us_agi = 1, and cdcc = 0, the 5% CTC arm wins, so it equals pytest.approx(0.05 * refundable_ctc) (read from the same sim). Locks the full user-facing consumer path.
  • A2 — added a zero-CTC guard-path case (single OK adult under AFA, no children) asserting ok_federal_ctc == 0 via the guard's return refundable_ctc, proving it neither crashes nor invents a credit.

Suggestions

  • S1 — switched to string period keys ({"2026": …}) to match the neighboring contrib convention.
  • S2 — added a companion YAML case under tests/policy/contrib/congress/afa/ (OK parent, one age-4 child, period 2025, gov.contrib.congress.afa.in_effect: true) asserting refundable_ctc == ok_federal_ctc == 4,320 — verified against the AFA parameters (base 3,600 × the 1.2 under-6 multiplier, no uprating at the 2025 base year, no phase-out at 30k). Locks the fix into the policyengine-core contrib YAML shard alongside the pytest.
  • S3 — added a parametrized smoke test over the three CTC-restructuring contrib flags — AFA, ECPA, FISC — asserting ok_federal_ctc computes (finite, ≥ 0) for an OK household under each, guarding against the next .index()-style fragility.
  • S4 — added a 2040 post-cliff case (AFA modify_parameters stops 2039-12-31, after which non_refundable_ctc re-enters the list and the guard goes inert) plus a comment documenting the boundary so a future year-bump doesn't silently change which branch is under test.
  • S5 — added a high-income guard-branch case asserting the relational invariant ok_federal_ctc == pytest.approx(refundable_ctc) regardless of how far the phase-out has run.

All pytest assertions are relational/structural (uprating-safe); the only fixed number is the S2 YAML's 4,320, which I confirmed against the AFA base/multiplier parameters. I couldn't run the suite locally (the pinned venv is on a broken 3.14-alpha numpy), so I'm relying on CI.

Fixed with Claude Code assistance.

The AFA reform (afa_other_dependent_credit) redefines the refundable_ctc
variable and sets fully_refundable via apply()/modify_parameters, applied by
the reform itself — not by the gov.contrib.congress.afa.in_effect input flag.
Activating AFA in the YAML harness therefore needs the top-level `reforms:`
key (as the neighboring afa_other_dependent_credit.yaml uses); with only the
input flag the structural reform never runs, so the guard is not exercised and
refundable_ctc reads its baseline value.

With the reform properly applied, refundable_ctc becomes the AFA-redefined
min(ctc_refundable_maximum, total_ctc), whose exact value cannot be verified
here without a runnable local test environment (the pinned venv is a broken
3.14-alpha). Rather than hardcode an unverified expectation, drop the YAML
companion. The sibling pytest (test_ok_federal_ctc_under_afa.py) already covers
the guard authoritatively via Reform.from_dict — including the
ok_federal_ctc == refundable_ctc invariant, the zero-child guard path, a
parametrized smoke test over AFA/ECPA/FISC, the 2039 cliff, and a high-income
case — all passing in CI.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCwrSsGY7vPCpKYGfV1xRr
@DTrim99

DTrim99 commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator Author

Correction on S2 (the companion YAML)

My earlier comment said the S2 YAML asserted refundable_ctc == ok_federal_ctc == 4,320. That value was a wrong hand-derivation, and CI surfaced a deeper issue, so I've dropped the S2 YAML — the pytest (which passed all 7 cases) is the authoritative guard test.

Why the YAML didn't work cleanly: the AFA reform (afa_other_dependent_credit) redefines the refundable_ctc variable and sets fully_refundable via its apply()/modify_parameters, which are applied by the reform itself — not by the gov.contrib.congress.afa.in_effect input flag. So a YAML case needs the top-level reforms: key (as afa_other_dependent_credit.yaml uses) to actually exercise the guard; with only the input flag, the structural reform never runs and refundable_ctc reads its baseline value (the 1562.5 CI reported). With the reform correctly applied, refundable_ctc becomes the AFA-redefined min(ctc_refundable_maximum, total_ctc), whose exact value I can't verify here without a runnable environment (the pinned venv is a broken 3.14-alpha).

Rather than hardcode an unverified expectation, I removed the YAML. Everything else stands and is green in CI:

  • A1 downstream ok_child_care_child_tax_credit (5% CTC arm, relational)
  • A2 zero-child guard path (ok_federal_ctc == 0)
  • S1 string period keys
  • S3 parametrized smoke test over AFA / ECPA / FISC
  • S4 2040 post-cliff case + boundary comment
  • S5 high-income guard invariant (ok_federal_ctc == refundable_ctc)

If you'd like the YAML companion after all, I'm happy to add it with the reforms: key once I can read the exact AFA-applied refundable_ctc off a run.

Fixed with Claude Code assistance.

@DTrim99
DTrim99 requested a review from PavelMakarchuk August 26, 2026 12:59

@PavelMakarchuk PavelMakarchuk 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.

PR 9323 — Fix ok_federal_ctc crash under fully-refundable CTC reforms

Program Review

Source Documents

  • PDF: none (code-logic fix; no parameter value changes — PDF audit skipped)
  • Year: 2026 (tests also cover 2040)
  • Scope: PR changes only
  • Reviewed head SHA: 96db41c
  • Mode: full

Branch Status

The branch is 65 commits behind main (3 ahead); recommend a rebase before merge. Staleness did not affect any findings — informational only, never a finding.

Critical (Must Fix)

None.

The fix itself was validated as correct by all three validators:

  • Legal basis confirmed: 68 O.S. § 2357(B)(2) references "the child tax credit allowed under the Internal Revenue Code" generically, and the OTC 2025 Form 511 packet (instructions p. 11, Schedule 511-F p. 25) defines the base as nonrefundable CTC + refundable additional CTC. When a reform removes non_refundable_ctc from gov.irs.credits.non_refundable and routes the full credit through refundable_ctc, returning refundable_ctc is exactly the statutory base (policyengine_us/variables/gov/states/ok/tax/income/credits/ok_federal_ctc.py:29-30).
  • Baseline unchanged: non_refundable_ctc is present in gov/irs/credits/non_refundable.yaml at every value date, so the guard never fires in baseline; hoisting the refundable_ctc read to line 23 is behavior-identical. Existing baseline YAML tests are untouched and still cover the .index() branch.
  • Pattern-idiomatic: the membership guard matches the existing nm_cdcc.py:17 pattern; scalar Python if on a per-period parameter list is sanctioned; period usage, entity levels, aggregation, and the changelog fragment all pass.
  • The crash scenario from #9322 (AFA) is directly tested through the production Reform.from_dict path, with children, without children, under phase-out, downstream into ok_child_care_child_tax_credit, and across the 2040 post-cliff fallback.

Should Address

  • A1 — [S3] test comment is factually wrong: only AFA removes non_refundable_ctc from the parameter list; ECPA and FISC never hit the new guardpolicyengine_us/tests/policy/contrib/congress/afa/test_ok_federal_ctc_under_afa.py:105-110 (flagged independently by all three validators). The comment claims all three reforms "remove non_refundable_ctc from the federal non-refundable credit list (AFA and ECPA via list-rebuild, FISC via neutralize_variable)." Verified against the snapshot: only AFA rebuilds the parameter (policyengine_us/reforms/congress/afa/afa_other_dependent_credit.py:202-217, 2025-01-01 → 2039-12-31). ECPA filters the credit out inside its overridden income_tax_non_refundable_credits variable (policyengine_us/reforms/congress/tlaib/economic_dignity_for_all_agenda/edaa_end_child_poverty_act.py:188-196) and FISC only rewrites gov.irs.credits.refundable and neutralizes the CTC variables (policyengine_us/reforms/congress/golden/fisc_act.py:65-85) — under both, the parameter still contains "non_refundable_ctc", the guard is inert, and the old .index() branch runs (and never crashed there). The ECPA/FISC smoke tests are still worth keeping as ordered-allocation regression checks, but correct the comment (or assert which branch is taken in-test) so future readers don't believe guard coverage is 3x what it is.

  • A2 — Parametrized smoke assertions are too weak to catch semantic errorspolicyengine_us/tests/policy/contrib/congress/afa/test_ok_federal_ctc_under_afa.py:127-128 (isfinite and >= 0 only). Under FISC, both refundable_ctc and non_refundable_ctc are neutralized (fisc_act.py:84-85), so the correct value is exactly ok_federal_ctc == 0 — assert that. Under ECPA, non_refundable_ctc is not neutralized, so ok_federal_ctc still computes a positive baseline-style CTC even though ECPA replaces the federal CTC with a child allowance — Oklahoma grants 5% of a credit that no longer exists under the reform. That phantom-CTC interaction is pre-existing behavior (not introduced or worsened by this PR), but >= 0 can never detect it; pin an expected value or assert the intended relationship, and add a short docstring note so a passing smoke test isn't read as an endorsement of the ECPA-interaction value.

  • A3 — High-income guard test can pass triviallypolicyengine_us/tests/policy/contrib/congress/afa/test_ok_federal_ctc_under_afa.py:153-167. At $400k single, the CTC is fully phased out under baseline law too, so ok_federal_ctc == refundable_ctc holds as 0 == 0 even if the reform/guard never applied (the docstring itself concedes "0 is a valid result"). Add assert refundable_ctc > 0 at an income inside the AFA phase-out range with residual credit, or pin the expected phased-out value, so the test distinguishes the guard branch from a no-op.

  • A4 — Stale statutory citation in PR/issue framing; operative provision is 68 O.S. § 2357(B)(2), not § 2357.26policyengine_us/variables/gov/states/ok/tax/income/credits/ok_federal_ctc.py:11-14. The PR discussion cites "68 O.S. § 2357.26," which was repealed by Laws 2013, c. 363, § 7, eff. Jan. 1, 2014 (and the pre-repeal § 2357.26 was a different provision). The Child Care/Child Tax Credit lives in § 2357(B)(2). The file's existing references (OTC Form 511 packet) are correct and unchanged, so this is a record-correction for the PR/issue text; consider adding the § 2357(B)(2) OSCN cite to the reference tuple in a follow-up.

Suggestions

  • S1 — ODC treatment is asymmetric under AFApolicyengine_us/variables/gov/states/ok/tax/income/credits/ok_federal_ctc.py:29-30. In baseline, the $500 other-dependent credit is inside ctc (via gov.irs.credits.ctc.amount.adult_dependent) and therefore inside Oklahoma's 5% base; under AFA it is split into a separate other_dependent_credit non-refundable entry, and the guard's return refundable_ctc excludes it. Under the actual AFA bill the ODC remains an IRC §24 credit, so a strict "5% of the CTC allowed under the IRC" reading would arguably include the applied ODC. Excluding it is a defensible (conservative) counterfactual-policy call — add a code comment acknowledging the choice, or a follow-up adding the applied ODC on the guard branch.
  • S2 — 2040 post-cliff test also only asserts finite/>= 0policyengine_us/tests/policy/contrib/congress/afa/test_ok_federal_ctc_under_afa.py:149-150. Assert equality with the baseline ordered-allocation result (or a pinned value) so a silently wrong fallback can't pass. The test's own NOTE about the AFA 2039-12-31 modify_parameters stop date was verified accurate — good self-documentation.
  • S3 — Leading activation edge untested — AFA's modify_parameters hard-codes start=2025-01-01 (afa_other_dependent_credit.py:203) regardless of the tests' 2026 in_effect start. A 2025 case would document which branch pre-in_effect periods hit — the mirror of the 2040 trailing-edge test (test_ok_federal_ctc_under_afa.py:131-150).
  • S4 — Downstream consumer covered on one arm onlytest_ok_federal_ctc_under_afa.py:45-56 tests ok_child_care_child_tax_credit under AFA only where the 5%-CTC arm wins (cdcc == 0). The 20%-CDCC arm under AFA and the >$100k AGI-limit zero under AFA are untested; baseline YAML covers those arms only with ok_federal_ctc supplied as an input (policyengine_us/tests/policy/baseline/gov/states/ok/tax/income/ok_child_care_child_tax_credit.yaml:10-24, 57-67).
  • S5 — Pre-existing gap (unchanged code, note only): no dedicated baseline ok_federal_ctc.yaml; the .index() branch is covered only indirectly by two cap-binding cases (ok_child_care_child_tax_credit.yaml:132-173, integration.yaml:70-75). The income_tax_cap_binds == False arm of the where() (ok_federal_ctc.py:49-53) — the common middle-income case — has no explicit test on either side of this PR.
  • S6 — Changelog fragment name does not match the head branchchangelog.d/fix-ok-federal-ctc-fully-refundable-reforms.fixed.md vs branch fix/ok-federal-ctc-afa (expected fix-ok-federal-ctc-afa.fixed.md). Towncrier globs *.fixed.md, so the build is unaffected; rename for convention only. The fragment body is also long (multi-clause with a semicolon); convention is one concise line.
  • S7 — Guard-rationale comment could be tighterok_federal_ctc.py:24-28 is a five-line comment restating the mechanism; house style favors a one/two-line # NOTE:. Cosmetic only.

PDF Audit Summary

The PDF audit was skipped: the PR changes no parameter values, adds no reference: fields, and its only page anchor (ok_federal_ctc.py:11-14, Form 511 packet #page=11) is pre-existing — and its content was independently verified by the regulatory reviewer against the same PDF.

Metric Count
Confirmed correct 0
Mismatches (code-path confirmed + visually verified) 0
Mismatches rejected 0
Unmodeled items 0
Pre-existing issues 0

Validation Summary

Check Result
Regulatory Accuracy PASS — fallback matches 68 O.S. § 2357(B)(2) and OTC Form 511/Schedule 511-F interpretation; baseline unchanged; no reinvented variables
Reference Quality N/A — no parameter changes (pre-existing Form 511 reference verified in passing)
Code Patterns PASS — 0 critical, 0 should-address; guard is idiomatic (matches nm_cdcc.py), legal scalar if, fully covers the ValueError path
Formatting PASS — changelog fragment valid; no hard-coded legal values; entity/period usage correct
Test Coverage PASS with reservations — crash scenario fully covered incl. production activation path; A1–A3 weaken confidence in the smoke/edge assertions
PDF Value Audit Skipped — no parameter value changes
CI Status Passing 33/33

Review Severity: COMMENT

No critical findings; the fix is legally and technically correct and the bug scenario is well tested. The should-address items are test-documentation and assertion-strength issues (the [S3] comment misstates which reforms exercise the new guard, and three assertions can pass trivially) plus a citation record-correction — worth fixing, not blocking.

Next Steps

To auto-fix issues: run the fix-pr workflow for this PR.

Applies PavelMakarchuk's 2026-09-01 review (COMMENT severity; fix already
approved). No formula-logic or parameter change.

- A1: correct the multi-reform test comment — only AFA removes
  non_refundable_ctc from the parameter list (guard fires); ECPA filters inside
  its overridden variable and FISC neutralizes the CTC vars, both leaving the
  list intact (guard inert, ordered-allocation branch).
- A2: replace the isfinite/>=0 smoke with per-branch assertions — AFA
  ok_federal_ctc == refundable_ctc, ECPA > 0 (pre-existing phantom CTC, noted),
  FISC == 0.
- A3: move the high-income guard test into the AFA phase-out ($150k) and assert
  refundable_ctc > 0 as well as the guard invariant, so it no longer passes
  trivially at 0 == 0.
- S2: assert the 2040 post-cliff result equals the no-reform baseline (second
  simulation) instead of finite/>=0.
- S3: add a 2025 leading-edge case (AFA modify_parameters starts 2025-01-01).
- S4: add the 20%-CDCC arm and the over-$100k-AGI zero arm for the downstream
  ok_child_care_child_tax_credit under AFA.
- S1/S7: tighten the guard NOTE and document the deliberate ODC exclusion.
- A4: add 68 O.S. Sec. 2357(B)(2) (OSCN) to the reference tuple.
- S6: rename the changelog fragment to match the branch and shorten it.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCwrSsGY7vPCpKYGfV1xRr
@DTrim99

DTrim99 commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator Author

Re-review addressed — thanks @PavelMakarchuk

Applied all four Should-Address items and the seven Suggestions. No formula-logic or parameter change; every strengthened assertion is now relational (correct-by-construction), directly addressing the "passes trivially" concern.

Should Address

  • A1 — corrected the multi-reform test comment: only AFA removes non_refundable_ctc from the parameter list (guard fires); ECPA filters inside its overridden income_tax_non_refundable_credits and FISC neutralizes the CTC variables, both leaving the list intact (guard inert, ordered-allocation .index() branch).
  • A2 — replaced the isfinite/>=0 smoke with per-branch assertions: AFA → ok_federal_ctc == refundable_ctc; ECPA → > 0 (with a docstring noting the phantom baseline-style CTC is pre-existing behavior, not an endorsement); FISC → == 0.
  • A3 — moved the high-income guard test into the AFA phase-out ($150k, one child under 6) and now assert both refundable_ctc > 0 and the guard invariant, so it can't pass at 0 == 0.
  • A4 — the operative provision is 68 O.S. § 2357(B)(2) (the repealed § 2357.26 was in earlier discussion, not the PR/issue text). Added the § 2357(B)(2) OSCN cite to the variable's reference tuple (verified HTTP 200, contains subsection (B)(2)).

Suggestions

  • S1 — documented the deliberate ODC exclusion under AFA (the split-out other_dependent_credit is excluded by return refundable_ctc; baseline unaffected).
  • S2 — the 2040 post-cliff test now asserts equality with a no-reform baseline simulation (no hardcoded value).
  • S3 — added a 2025 leading-edge case (AFA modify_parameters starts 2025-01-01).
  • S4 — added the 20%-CDCC arm (with a dominance guard) and the over-$100k-AGI zero arm for the downstream ok_child_care_child_tax_credit under AFA.
  • S6 — renamed the changelog fragment to fix-ok-federal-ctc-afa.fixed.md and shortened it to one line.
  • S7 — tightened the guard-rationale comment to a concise # NOTE:.
  • S5 skipped — the pre-existing baseline ok_federal_ctc.yaml gap (the cap-not-binding where() arm) needs values I can't compute without a run and is explicitly pre-existing; noted for a follow-up.

I can't run the suite locally (broken 3.14-alpha venv), so I relied on relational assertions to keep the new checks correct by construction; CI is confirming.

Fixed with Claude Code assistance.

CI showed the dominance guard I added failed: under AFA the enlarged CTC makes
5% of the CTC exceed 20% of the federal CDCC for this household, so the 20%
arm does not dominate. Rather than hunt for a household where it does (hard
under AFA and not locally verifiable), assert the actual Form 511 Schedule
511-F rule -- the credit is the greater of 20% of the federal CDCC and 5% of
the OK federal CTC allowed -- which is correct whichever arm wins, still pins a
nonzero CDCC flowing through under AFA, and keeps a `cdcc > 0` no-op guard.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YCwrSsGY7vPCpKYGfV1xRr
@DTrim99
DTrim99 merged commit e866aa3 into PolicyEngine:main Sep 2, 2026
33 checks passed
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.

ok_federal_ctc crashes under fully-refundable CTC reforms (AFA): ValueError("'non_refundable_ctc' is not in list")

2 participants