Skip to content

Actionable reform-input validation for the MCP/CLI tools#38

Merged
vahid-ahmadi merged 2 commits into
mainfrom
reform-key-dx
Jul 18, 2026
Merged

Actionable reform-input validation for the MCP/CLI tools#38
vahid-ahmadi merged 2 commits into
mainfrom
reform-key-dx

Conversation

@vahid-ahmadi

Copy link
Copy Markdown
Contributor

Fixes two developer-experience defects found in live end-to-end testing of the hosted MCP server.

1. Date-range reform keys returned a raw pydantic dump

{"gov.hmrc.income_tax.rates.uk[0].rate": {"2026-01-01.2029-12-31": 0.21}} returned:

Error executing tool score_reform: 1 validation error for Simulation — Value error, unconverted data remains: .2029-12-31

Root cause (upstream, confirmed in the installed package). policyengine/tax_benefit_models/common/reform.py compiles each reform entry to ParameterValue(start_date=datetime.strptime(effective_date, "%Y-%m-%d"), end_date=None, ...). Every reform value is applied from an effective date and stays in force indefinitelyend_date is hardcoded to None, and the reform-dict API exposes no field for it. strptime("2026-01-01.2029-12-31", "%Y-%m-%d") reproduces the reported error exactly.

Decision: reject explicitly, do not fake. Accepting the range and silently using only the start date would score a permanent reform while the caller asked for a temporary one — a wrong number returned confidently, the worst failure mode for a costing tool. The error now names the end date it cannot express, shows the correct single-date form, and gives a workaround:

reform['gov.hmrc.income_tax.rates.uk[0].rate'] uses the date-range key
'2026-01-01.2029-12-31', which the PolicyEngine reform-dict API does not
support: a reform value is applied from an effective date and stays in force
indefinitely (end_date is always None), so the end date 2029-12-31 cannot be
expressed.
Use the start date alone to score the reform as permanent: {"2026-01-01": 0.21}.
To approximate a time-limited reform, score the affected years individually
(e.g. one population_reform_impact call per year in the window) and treat later
years as baseline.
Supported reform shapes (per parameter path):
  - flat value:      {"gov.hmrc.income_tax.rates.uk[0].rate": 0.21}
  - effective date:  {"gov.hmrc.income_tax.rates.uk[0].rate": {"2026-01-01": 0.21}}

If upstream ever exposes end_date through the reform dict, validate_reform is the single place to change.

2. calculate_household returned a raw pydantic dump on missing arguments

Omitting country produced 2 validation errors for calculate_householdArguments: country Field required... people Field required. The MCP arguments are now optional at the schema level and validated in core, so callers get a sentence:

country is required and must be 'uk' or 'us' — there is no default, because the
two are different tax-benefit models and guessing would silently score the wrong
country. Example: country="uk".

No default for country. pe_population_impact keeps its historical country="uk" default for compatibility, but defaulting the household tools would silently run a US household through UK rules. The error message is made good instead of guessing a default.

Changes

  • core.validate_reform() — one validator behind every tool that accepts a reform (pe_household, pe_household_impact, pe_population_impact, score_reform, and the OG/OBR bridges). Accepts flat scalars and single-date keys, returns the normalised dict, and raises ValueError naming the supported shapes with a worked example. _validate_reform kept as a thin alias.
  • _validate_country / _validate_people — actionable messages for the other required arguments. Validation runs before the heavy PolicyEngine import, so bad input fails in milliseconds.
  • _og_build_policy — now honours dated values (previously assumed scalars), emitting one ParameterValue per effective date.
  • score_reform — a missing/unknown model names the three valid options and what each is for.
  • Docstrings / CLI help — MCP tool docs and --reform help document the two supported shapes and state that ranges are unsupported, so agents get it right on the first call.

Tests

New integration/tests/test_reform_input.py (28 tests): flat value, integer/bool values, single-date key, multiple dates on one parameter, mixed flat+dated; date-range rejection (both . and : separators) asserting the message names the end date, shows the correct form, and leaks no upstream text; and each malformed input's message (empty, non-dict, bad date key, non-numeric value, non-string path, missing country/people, unknown model). Also asserts PolicyEngine is never imported when input is invalid.

87 passed, 45 skipped, 1 failed

The single failure is test_core.py::test_summary_parses, pre-existing and environmental — boe_var is not installed locally and MACROMOD_BOE_VAR_REPO is unset. Untouched by this change; passes in CI where the model extras install.

ruff check src tests is clean (also fixed one pre-existing F541 in cli.py).

Not deployed to Modal — CI handles that on merge.

🤖 Generated with Claude Code

vahid-ahmadi and others added 2 commits July 18, 2026 18:54
Two defects found in live end-to-end testing of the hosted server:

1. Date-range reform keys ("2026-01-01.2029-12-31") surfaced a raw pydantic
   dump: "unconverted data remains: .2029-12-31". Root cause is upstream:
   policyengine's compile_reform parses each key with
   strptime(key, "%Y-%m-%d") and builds ParameterValue(end_date=None), so a
   reform value applies from its effective date and never expires. The
   reform-dict API has no way to express an end date, and silently dropping
   it would score a permanent reform when the caller asked for a temporary
   one. Ranges are therefore rejected up front with an explanation, the
   correct single-date form, and a per-year workaround -- not faked.

2. calculate_household returned a raw "2 validation errors ... Field
   required" dump when country/people were omitted. The MCP arguments are
   now optional at the schema level and validated in core, so callers get a
   sentence instead of a pydantic trace. country deliberately keeps NO
   default: uk and us are different tax-benefit models and guessing would
   silently score the wrong country.

Adds core.validate_reform() as the single validator behind every tool that
accepts a reform (household, household impact, population, score_reform, the
OG and OBR bridges). It accepts flat values and single-date keys, returns the
normalised dict, and raises ValueError naming the supported shapes with a
worked example. _og_build_policy now honours dated values too.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The previous commit was made from a stale checkout and unintentionally
reverted the workflow changes from #35. This restores all four workflow
files to their state on main, leaving this branch's diff scoped to the
reform-input validation in integration/.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@vercel

vercel Bot commented Jul 18, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
macromod Ready Ready Preview, Comment Jul 18, 2026 6:04pm

Request Review

@vahid-ahmadi
vahid-ahmadi merged commit aa048fb into main Jul 18, 2026
3 checks passed
@vahid-ahmadi
vahid-ahmadi deleted the reform-key-dx branch July 18, 2026 18:07
vahid-ahmadi added a commit that referenced this pull request Jul 19, 2026
#42)

The hosted tool surface was only ever checked as a SUBSET
(`EXPECTED_TOOLS <= names`), and the in-process check lived in a
slow-marked test that PR CI skips entirely. Adding, removing or renaming
a tool could therefore ship to clients with a fully green pipeline.

- tests/tool_surface.py: single golden list of the 10 published tool
  names, shared by the in-process, stdio and live-deployment tests, with
  an assert_surface() helper that reports added/removed names.
- tests/test_tool_surface.py: fast, PolicyEngine-free contract on exact
  names, exact count, no duplicates, per-tool description/schema, plus a
  regression guard that country/people stay schema-optional (#38).
- test_remote_mcp.py / test_mcp_server.py: subset checks upgraded to
  exact-surface equality against the same golden list.
- test_remote_mcp.py: reform-input validation (#38) now asserted AS
  SERVED — date-range rejection, missing country, empty reform, and a
  valid dated reform that must be accepted and score correctly.
- test-integration.yml: the tool-surface and reform-input suites run as
  named hard gates before the general suite.
- validate-deployment.yml: weekly Sunday deep run so the slow
  investment-closure and OBR bridge legs are exercised on a schedule
  instead of only on a manual dispatch.

Also fixes a genuinely broken job: contract.yml pinned
astral-sh/setup-uv@v8, but that action publishes no moving `v8` tag, so
the nightly contract run had been failing in ~5s at "Set up job" since
the action bump in #36. Pinned to v8.3.2.

Verified: fast suite and the full live remote suite (14 passed) run
locally against the deployed server.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.

1 participant