[juaristi22]

Reform search included labelled compatibility aliases and fixed bracket sentinels that the parameter tree already hides. This could show duplicate-looking choices and expose parameters that metadata marks unavailable for editing. Search now uses the tree's existing economy || household visibility rule. Parameters with distinct IDs remain separate even when their labels match.

NI compatibility aliases leave search only when the metadata consumed by the app marks them hidden. The fixture uses candidate UK metadata; this PR does not change deployed metadata.

Refs PolicyEngine/policyengine-uk#2490

Regression tests cover true, false and omitted visibility flags, sorting, equal labels with different IDs, and UK National Insurance aliases and fixed sentinels. The modal test searches for and selects the canonical employee rate, enters 10%, and verifies the submitted 2026 value is 0.1 for gov.hmrc.national_insurance.class_1.employee_scale[1].rate.

Review cleanup at 710e10748d1876820057796486a1c51df543359f removes the local file-writing harness, provenance export and duplicate payload assertion from the tests. The production selector and canonical search, setter and submission assertions remain unchanged. The 70 focused tests, format check, lint and type checking passed.

The following full-suite and build results describe parent 260dd810df7f1e5ae03a91241e840a7223560201; they were not rerun for the cleanup commit:

• Full make test on Node 26: 5,889 passed and 10 existing skipped across 484 files, using one worker. The 70 focused tests also passed independently; these overlap the full suite and are not additional passing cases.
• Lint and type checking passed in all three workspaces.
• bun run build --concurrency=1: native exit 0; website and calculator completed, two tasks successful and zero cached. This uses the supported serial Turbo scheduling option rather than literal make build.

The original default-worker test startup hit the local CPU budget. Earlier build attempts stopped on generated-cache module resolution and then the parallel CPU budget; neither counted as a pass. The completed test and serial build runs preserve the same source, dependencies and assertions.

The modal submission is verified at a mocked fetch boundary; the earlier 13 real HTTP API checks are separate evidence. Browser and deployed-preview validation, consumer landing, deployed API adoption, owner acceptance and encoded parity remain open. The UK #2490 Axiom-needed follow-up remains open. This PR covers the app consumer change; GitHub CI must pass for the updated head.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
policyengine-calculator-next	Ready	Preview	Oct 22, 2026 9:12pm UTC
policyengine-website	Ready	Preview	Oct 22, 2026 9:12pm UTC

[vahid-ahmadi]

@juaristi22 thanks, the one-line change in selectSearchableParameters is right and brings the report builder search into line with the tree's visibility rule. One should item on the new integration test, one nit, and two notes. Reviewed at 260dd810.

What "visibility" means here. Each parameter carries two booleans from the API, economy and household. The tree (app/src/libs/buildParameterTree.ts:62) and the flagship search (app/src/libs/parameterSearch.ts:549) already show a parameter only if either is true. Live metadata always sends both flags explicitly: UK 2.130.1 has 1,267 true/true, 272 false/true and 11 false/false parameters; US 2.2.1 has 94,199, 3,510 and 2,567. So the "absent" cases in the unit tests are defensive, not live.

Findings

1. should: remove the local evidence harness from the integration test. app/src/tests/integration/pages/reportBuilder/PolicyCreationModal.parameterVisibility.test.tsx:1,26-35 imports writeFileSync and writes JSON and HTML files when POLICYENGINE_TEST_EVIDENCE_DIR is set, and the afterEach at :94 dumps document.body.innerHTML for every test. Related residue: the qualification string at :170, the double assertion at :174-175 (PRIOR_ACTUAL_PERSISTENCE_PAYLOAD is identical to EXPECTED_PAYLOAD, fixture :644), the unused CANDIDATE_METADATA_PROVENANCE export with its sha256 values (fixture :3-10), and test names such as "entering10percent for2026" and "genuine policy component" (:104, :137). No other test in app/src/tests writes files. Suggest dropping the file writes, the provenance export and the duplicate payload, and renaming to the repo's plain Given-When-Then style (docs/engineering/skills/testing.md). The assertions themselves are good and should stay.

2. nit: the modal search still differs from the tree on path exclusions. app/src/libs/metadataUtils.ts:203 now matches the tree on flags, but the tree and the flagship search also drop taxsim and gov.abolitions paths; this selector only drops pycache. Against live US metadata that is 5,002 entries (for example "Abolish QBI for the primary taxpayer (TAXSIM)") that appear in the modal search but not in the tree. This predates the PR and abolitions may well be useful in search, so a question rather than a request: is that intended? If not, one shared predicate for all three would stop them drifting again; happy for that to be a follow-up.

Notes

• The NI alias change is not visible yet. Searching "NI class 1 main rate" in the running app at this head still returns gov.hmrc.national_insurance.class_1.rates.employee.main, because live UK metadata (2.130.1) marks it economy: true, household: true. The fixture (2.141.11, after policyengine-uk#2490) marks it false/false. The code is correctly forward compatible; it may be worth a line in the PR body so nobody files "still shows duplicates" after merge. The visible UK effect today is that the 11 minimum wage parameters (gov.hmrc.minimum_wage.*) leave the modal search, consistent with the tree hiding them.
• Other entry points. Tree clicks go through the already filtered tree. Clicking a modified parameter in the policy overview (PolicyCreationModal.tsx:513 to handleSearchSelect) looks up metadata directly and so still opens a hidden parameter that is already in a saved policy. I think that is right, since it keeps saved changes editable and removable. The flagship search already filtered on the same flags.

What I checked

• New tests fail on the base source and pass at head: with the old metadataUtils.ts swapped in, 7 of 42 fail (4 visibility cases, 2 alias searches, the sentinel search).
• Full app suite on Node 24.22.1: 443 files, 5,532 passed, 10 skipped. tsc --noEmit clean; ESLint and Prettier clean on the five touched files. CI green at head.
• Ran the calculator app locally against the live API and searched the report builder modal, base versus head. UK: "minimum wage" returned the apprentice rate on base and nothing at head; "NI class 1 main rate" and "basic rate" unchanged. US: "slcsp" lost gov.aca.slcsp.* (hidden) at head while the abolition entries remain; "EITC maximum" returned the same 8 visible results on both. No layout change, as expected for a filter change.

🤖 Generated with Claude Code

[juaristi22]

Removed the file-writing harness, provenance export and duplicate payload assertion, and renamed the tests. The canonical search, 10-percent-to-0.1 setter, 2026 payload, name and single-submission assertions remain. The 70 focused tests, format check, lint and type checking passed at 710e1074; the production selector is unchanged.

The PR body now clarifies that NI aliases disappear only when the metadata consumed by the app marks them hidden. Full-suite and build evidence remains dated to parent 260dd810. The pre-existing taxsim/gov.abolitions exclusion difference remains a follow-up.

[vahid-ahmadi]

@juaristi22 thanks for the quick cleanup. Approving at 710e1074. The should item is fixed, the nit is a follow-up as agreed, and I have one small optional nit.

Earlier findings

1. should, test evidence harness: resolved. 710e1074 drops the writeFileSync/join imports, the evidence() helper and every call to it, the afterEach HTML dump, the qualification string, CANDIDATE_METADATA_PROVENANCE with its sha256 values, and PRIOR_ACTUAL_PERSISTENCE_PAYLOAD with its duplicate assertion. The describe block is now "PolicyCreationModal parameter visibility", and the setter test reads as plain Given-When-Then (PolicyCreationModal.parameterVisibility.test.tsx:87,115). The search, sentinel and submission assertions are unchanged, and nothing in app/src/tests writes files any more.
2. nit, taxsim/gov.abolitions exclusions: open, follow-up as agreed. metadataUtils.ts is unchanged in this commit, which is right for this PR. I found no follow-up issue yet. Worth filing one so the three filters don't drift further.

The PR body now says the NI aliases only leave search once the metadata the app reads marks them hidden. That covers the first note from round 1.

New findings

3. nit (optional): a few names from the evidence run are still there. They are harmless, but they read oddly to someone who wasn't part of that run: CANDIDATE_METADATA (fixture :3), the 'candidate2490-metadata' request id (test :52, :56), 'Local2490 component boundary' (fixture :617) and 'component-boundary-only-2' (test :76). Names like UK_METADATA, 'test-metadata', 'Test policy' and 'test-policy-id' would match the other fixtures. Fine to leave.

What I checked

• Delta 260dd810..710e1074 touches only the two test files (3 added, 51 removed lines). Production code is identical to what I reviewed in round 1.
• Ran the reportBuilder unit and integration tests plus metadataUtils.test.ts on Node 22.14 with bun 1.3.14: 29 files, 315 passed. tsc --noEmit is clean, and ESLint and Prettier are clean on the five touched files.
• Merges cleanly onto current main (71 commits ahead, none touching metadataUtils.ts or the new tests). CI is green at head (Tests, Type checking, Lint and format, Build, Visual regression).

🤖 Generated with Claude Code