fix: USITC HTS-filtered query payloads#8
Conversation
Port of Redliana/cmm-trade-mcp@b85ebcd. Fixes the known "step 4" validation failure when commoditySelectType was set to "entered" with non-empty commodity or country filters. DataWeb expects: - commoditySelectType / countriesSelectType = "list" (not "entered") - aggregation labels stay at "Aggregate Commodities" / "Aggregate countries" - commoditiesExpanded / countriesExpanded must carry TextValuePair objects mirroring the commodities/countries arrays - commoditiesManual must be the comma-joined code string when any HTS filter is set - groupGranularity stays at "2" Live probe confirms HTS-filtered imports now return data (HTS 2836.91.00 lithium carbonate 2023 imports = \$293.1M). Adds unit tests asserting the envelope shape so the validated payload doesn't regress silently. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Ports a validated USITC DataWeb “SavedQuery” envelope shape into packages/usitc-mcp, fixing HTS/country-filtered get_trade_data requests that previously failed server-side validation.
Changes:
- Fixes USITC
/report2/runReportpayload construction for HTS/country filters (list-mode select types, expanded objects, manual string, pinned group granularity). - Adds USITC unit tests asserting the corrected envelope shape for HTS and country filtering.
- Includes several small typing/robustness adjustments across other packages (casts for
model_dump()/JSON access, World Bank year parsing, loader signature alignment).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/usitc-mcp/src/usitc_mcp/client.py | Adjusts SavedQuery payload fields to match validated DataWeb templates for filtered queries. |
| packages/usitc-mcp/tests/test_client.py | Adds assertions for the corrected filtered-query envelope shape. |
| packages/worldbank-mcp/src/worldbank_mcp/client.py | Makes year parsing tolerant of non-integer date values. |
| packages/fred-mcp/src/fred_mcp/server.py | Adds a cast around model_dump() to satisfy typing expectations of tool return values. |
| packages/fred-mcp/src/fred_mcp/client.py | Adds casts for response list shapes (categories, seriess). |
| packages/cmm-fine-tune/src/cmm_fine_tune/training/config.py | Casts model_dump() output to a typed dict for downstream flattening. |
| packages/cmm-embedding/src/cmm_embedding/training/paired_data_loader.py | Tightens typing for tokenizer outputs and sampler variable. |
| packages/cmm-embedding/src/cmm_embedding/training/alignment_training.py | Tightens typing for scheduler and encoder outputs / lookup. |
| packages/cmm-data/src/cmm_data/utils/parsing.py | Switches NaN sentinel creation to float("nan"). |
| packages/cmm-data/src/cmm_data/loaders/osti_docs.py | Aligns loader signature with BaseLoader (load(**kwargs)) and hardens full_text extraction typing. |
| packages/arxiv-mcp/src/arxiv_mcp/server.py | Adds a cast around response.text to satisfy typing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if value is None or pd.isna(value): | ||
| return np.nan | ||
| return float("nan") | ||
|
|
There was a problem hiding this comment.
parse_numeric_value is annotated as returning float | None and its docstring says it returns None when parsing fails, but these branches now return float('nan'). This also makes the numpy as np import unused in this module. Consider either returning None on missing/unparseable values, or updating the return annotation/docstring to match the intended NaN-based behavior and remove the unused numpy import.
| # Commodity and country filtering are validated against the | ||
| # /savedQuery/getAllSystemSavedQueries payloads. The API expects | ||
| # list-based selection semantics, not "entered", and the aggregation | ||
| # labels are case-sensitive. | ||
| commodities_agg = "Aggregate Commodities" | ||
| commodities_select = "list" if hts_codes else "all" | ||
| countries_agg = "Aggregate countries" | ||
| countries_select = "list" if country_codes else "all" |
There was a problem hiding this comment.
This change fixes the SavedQuery payload to use list-based selection, but the module docstring and get_trade_data docstring still describe the previous 'step 4' validation failure with commoditySelectType='entered' and advise callers to drop HTS filters. Those docs/TODOs should be updated to reflect the new validated payload behavior so consumers aren't misled about expected failures or workarounds.
| try: | ||
| year = int(raw_date) if raw_date is not None else 0 | ||
| except (TypeError, ValueError): | ||
| year = 0 |
There was a problem hiding this comment.
The new year parsing now falls back to 0 when item['date'] is non-numeric. Previously, a non-numeric but truthy date would raise during int(...) and the whole record would be skipped by the outer except. Returning observations with year=0 can leak invalid data downstream; consider continue on invalid dates (or make year optional) to preserve prior semantics.
| year = 0 | |
| continue |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - uses: actions/checkout@v6 | ||
|
|
||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v7 | ||
| with: | ||
| version: "latest" |
There was a problem hiding this comment.
The workflow bumps actions/checkout and astral-sh/setup-uv to new major tags (v6/v7). Please confirm these tags exist and are intended; otherwise CI will fail at runtime. If the goal is supply-chain hardening, consider pinning actions to a commit SHA (or at least a known-good major version already used in this repo).
| - uses: actions/checkout@v6 | |
| - name: Install uv | |
| uses: astral-sh/setup-uv@v7 | |
| with: | |
| version: "latest" | |
| - uses: actions/checkout@v4 | |
| - name: Install uv | |
| uses: astral-sh/setup-uv@v4 | |
| with: | |
| version: "0.5.29" |
| - uses: actions/checkout@v6 | ||
|
|
||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v7 | ||
| with: | ||
| version: "latest" |
There was a problem hiding this comment.
PR title/description focus on the USITC HTS-filter payload fix, but this PR also changes CI action versions (and multiple other packages). Either update the PR description to cover these extra changes or split the unrelated changes into separate PRs to keep review/rollback risk contained.
Summary
packages/usitc-mcp/copyget_trade_dataraiseUSITCAPIErrorwhenever an HTS code or country filter was suppliedKey payload corrections (vs. initial implementation)
commoditySelectType/countriesSelectType="list"(not"entered")aggregationlabels stay at"Aggregate Commodities"/"Aggregate countries"commoditiesExpanded/countriesExpandednow carry TextValuePair objects mirroring the code arrayscommoditiesManualnow set to the comma-joined code string when any HTS filter is presentgroupGranularitypinned to"2"Verification
ruff format --check packages/passesruff check packages/passes (All checks passed!)pytest packages/usitc-mcp/tests— 5 passed (including 2 new envelope-shape assertions)USITC_API_TOKEN:$3,076,796,376,327(unchanged)$293,083,540— previously failed with step-4🤖 Generated with Claude Code