Phase 20.1: finish integration de-masking; 9 shipped bugs found and fixed - #2
Merged
Merged
Conversation
STATE.md had never advanced past "Roadmap created for v1.1" (2026-04-07), so it
reported "Phase 7 of 10, 0%" after 71 plans had shipped. That staleness had
propagated: two phases carried open verification that the artifacts contradicted,
and the requirement/roadmap bookkeeping had drifted project-wide.
Every flip below is evidence-driven — a ROADMAP plan box is only ticked when its
SUMMARY.md exists on disk; a requirement is only Complete when its phase's plans
all have summaries.
Phase 17 (Alberta) — was human_needed, now passed:
- BM25 discovery re-run live through MCP Client: alberta_ tools rank top-5 on
all 5 probe queries, #1 on 4 of 5.
- French verified live: NOT_FOUND -> "Jeu de données introuvable: ...";
alberta_list_categories(lang="fr") -> _meta.lang='fr'.
- All three doc-tracking gaps closed.
Phase 11 (IRCC) — was mechanically stale, now passed:
The report was written at 15:14 on 2026-04-08, before plan 11-04 (a gap-closure
plan) committed its summary at 15:57, so it covered 3 of 4 plans. 11-UAT, run at
22:00 the same day, was already 10/10 against the post-11-04 build. Added three
truths covering 11-04's merged-header parser and re-stamped.
Bookkeeping:
- REQUIREMENTS.md: 117 traceability rows Planned -> Complete, 100 checklist
boxes -> [x]
- ROADMAP.md: 61 plan checkboxes -> [x]
- STATE.md: Phase 20.1, 43%, 15/35 phases, status planning
Phase 08 UAT — tests 3-6 had been left `result: [pending]` and never run. Ran them
live; they surface a real defect, so 08-UAT is now status: diagnosed:
- Gap 1 (major): FREQUENCY_CODES in statcan/constants.py:28 is shifted from code
6 onward — monthly CPI reports as "Bi-monthly". sc_get_code_sets, which proxies
StatCan live, correctly says 6 = Monthly, so the server contradicts itself.
The unit tests cannot catch it: they assert the map against itself.
- Gap 2 (minor): sc_get_series_info_by_vector returns uom_code with no decoded
label.
Filed as .planning/todos/pending/2026-07-25-fix-statcan-frequency-codes-map.md.
Also noted, not fixed: phases 15 (BC) and 16 (Quebec) shipped without any
REQUIREMENTS.md entries, so they are invisible to the traceability table.
No source changes. Suite at reconciliation: 3032 passed, 2 skipped, 97.05% coverage.
… published set
Both hand-written WDS decode maps were shifted relative to StatCan's published
code set, so every sc_ tool that decodes them reported wrong values.
FREQUENCY_CODES was shifted from code 6 onward and truncated at 13:
code shipped StatCan published
6 Bi-monthly Monthly
7 Quarterly Bimonthly
9 Annual Quarterly
11 Every 3 years Semi-annual
12 Irregular Annual
14-21 (missing) Every 3/4/5/10 years, Occasional, Occasional Q/M/D
It also invented codes 3, 5, 8 and 10, which StatCan does not publish.
SCALAR_FACTOR_CODES carried the same defect, found while fixing the first, and
is arguably worse — the scalar factor is a magnitude multiplier, so code N means
10^N. The shipped map read 1="thousands" where upstream means "tens": a 100x
misread of every scaled observation. It also invented an 888 entry.
Live reproduction before the fix (CPI 18100004, vector 41690973):
sc_get_data_by_vector(vector_id=41690973, n=5)
ref_per: 2026-06-01, 2026-05-01, 2026-04-01 <- one month apart
frequency: "Bi-monthly" <- every row
The server contradicted itself: sc_get_code_sets proxies the live endpoint and
returned the correct 6="Monthly", while every other sc_ tool decoded against the
stale local copy.
Why nothing caught it:
- The unit tests built their expected value with FREQUENCY_CODES.get(...),
asserting the map against itself. Tautological — they pass for any map.
- The fixtures were fabricated to match the wrong map: monthly CPI was given
frequencyCode 5, which is not a StatCan code at all.
- 08-UAT test 4 ("e.g., 6 = Monthly") was designed to catch exactly this and
was left `result: [pending]`, never run, for three phases.
Changes:
- constants.py: both maps transcribed from getCodeSets, with a comment naming
the source and warning that codes are non-contiguous.
- resources.py: data://statcan/frequency-codes and .../scalar-factor-codes
rebuilt, including corrected French labels (they repeated the same errors).
- __tests__/test_code_maps.py (new, 39 tests): literal expected labels, exact
set equality, "code 6 is Monthly not Bimonthly" regression, guards against
re-inventing absent codes, and catalog-matches-constant checks.
- __tests__/conftest.py: fixture codes corrected to real upstream values
(5->6 monthly, 7->9 quarterly, 9->12 annual, scalar 6->2 hundreds).
- __tests__/test_client.py: de-tautologised — expected labels are now literals.
- tests/integration: TestStatCanCodeSetDrift (4 tests) fails if the local maps
ever diverge from live getCodeSets again, including the CPI-is-monthly case.
Closes 08-UAT.md Gap 1. Gap 2 (sc_get_series_info_by_vector returns uom_code
with no decoded label) remains open and is tracked in .planning/todos/pending/.
Verified: 3071 passed, 2 skipped, 97.06% coverage (was 3032/97.05%).
Integration drift tests pass against live StatCan. ruff and pyright clean on all
touched files.
… sessions
08-UAT.md Gap 2: sc_get_series_info_by_vector returned `uom_code` with no
decoded label, even though `frequency` and `scalar_factor` are both decoded in
the same response. The stated UAT expectation ("unit of measure label") was
therefore unmet.
Investigating turned up a THIRD instance of the Gap 1 bug class: the
data://statcan/uom-codes catalog was entirely fabricated — all 15 entries wrong.
code catalog claimed StatCan publishes
0 Not applicable (null)
1 Number 1981=100
17 Canadian dollars 2002=100
20 Percentage 2007=100
301 Tonnes (thousands) Vehicle-kilometres
Code 17 is the CPI index base — it matches that series' own title, "Consumer
Price Index (CPI), 2002=100". An agent following the catalog would have read it
as a dollar amount.
UOM is decoded from the live getCodeSets payload rather than a hardcoded map:
upstream publishes 464 codes, many of them index bases that cannot be guessed
from a name, and the fabricated catalog is direct evidence that hand-maintenance
fails for this set. The lookup shares get_code_sets()'s existing 7-day cache
entry (new _raw_code_sets_cached seam), and degrades to None on any failure so a
getCodeSets outage cannot take down series-info lookups.
Verified live after the fix — both gaps closed on the same call:
sc_get_series_info_by_vector(vector_id=41690973)
frequency_code 6 frequency "Monthly" <- Gap 1
uom_code 17 uom "2002=100" <- Gap 2
Changes:
- schemas.py: SeriesInfo.uom (str | None, defaults None)
- client.py: _raw_code_sets_cached / _raw_code_sets / _uom_label;
_flatten_series_info_async wired into both series-info call paths
- resources.py: uom catalog replaced with a verified 31-code subset plus a
_note pointing at sc_get_code_sets for the remaining 433
- __tests__/test_uom_decode.py (new, 10 tests): schema field, lookup, null and
unknown codes, outage degradation, both call paths, catalog honesty
- __tests__/test_client.py: series-info now makes two cached_fetch calls, so
the cache-key/TTL assertions pin the specific call instead of reading
`.call_args` (last call), and the limiter assertions state the real contract
(both upstream requests rate-limited) instead of a stale count of one
- integration: 2 more drift tests (decoded uom, catalog subset matches live)
Also closed three debug sessions that were fixed long ago but left open:
- ircc-header-parsing.md -> resolved/. Recommendation "Option 4" shipped as
written in plan 11-04; verified the DATASET_PARSE_CONFIG table matches the
proposed skip_rows/header_rows/label_cols key-for-key.
- bc-api-get-dict-mismatch.md, bc-bilingual-error-messages.md: deleted as
stale duplicates — resolved/ already holds the newer completed versions
(resolved 2026-04-11, with resolved_by, verification and files_changed).
They had been copied into resolved/ rather than moved.
Verified: 3081 passed, 2 skipped, 97.06% coverage (was 3071). Integration drift
suite 6/6 against live StatCan. ruff clean on touched files; pyright errors on
the statcan module went 86 -> 78.
…times todo
Phases 15 (British Columbia) and 16 (Quebec) shipped 20 and 18 tools with zero
REQUIREMENTS.md entries — both were planned with "Requirements: TBD (no explicit
REQ IDs yet)", so neither appeared anywhere in the traceability table. Two whole
provinces of delivered work were invisible to the project's own coverage record.
Backfilled BC-01..BC-22 and QC-01..QC-19 from shipped code, each verified against
the module at write time:
- BC: 5 discovery + 15 BCGW WFS-backed tools, with the actual BCGW layer names
and filter parameters; BC-21 records shared/ogc.py as a reusable third portal
technology (not BC-specific); BC-22 covers conventions + 6 prompts/7 resources.
- QC: 5 Données Québec CKAN discovery + 13 curated MSSS/MTQ/MELCCFP/MAMH/
Hydro-Québec tools, marking the three discovery-only ones (forest fire archive,
water quality, protected areas) as such; QC-19 covers conventions.
Both section headers state they were backfilled and why, so the provenance is not
mistaken for up-front requirements. ROADMAP "Requirements: TBD" lines for phases 15
and 16 replaced with the ID lists.
Every executed phase now has traceability coverage:
7(9) 8(15) 9(4) 10(4) 11(9) 12(8) 13(12) 14(14) 15(22) 16(19)
17(27) 18(18) 19(15) 20(18) 40(20)
The 20 remaining "Requirements: TBD" lines are all unplanned phases (20.1, 21-39),
which is correct — requirements get written at planning time.
Also refreshed the cross-Canada ER wait-times research todo (filed 2026-04-12,
before phases 17-20). Its research plan pointed at hosts those phases proved dead:
data.manitoba.ca is unreachable, data.saskatchewan.ca does not exist, and
data.novascotia.ca is Socrata rather than CKAN. Added what each phase already
settled — Alberta has AHS facility layers but no occupancy data (AB-16, Pitfall 9),
Saskatchewan health is deferred because SHA publishes no public FeatureServer, and
Nova Scotia shipped ns_get_health_facilities with no wait-time dataset found. The
item stays open as backlog; only its stale premises changed.
No source changes. Suite unchanged: 3081 passed, 2 skipped.
… tests
De-masking `test_weather_current_scenarios.py` immediately surfaced a real bug
that the masks had been hiding: every named-location weather lookup was broken.
wx_get_current_conditions(location="Toronto") -> NOT_FOUND
wx_get_forecast(location="Vancouver") -> NOT_FOUND
wx_get_current_conditions(lat=45.4, lon=-75.7) -> works fine
Root cause: with only `location` given, `bbox` stayed None, so ogc_fetch pulled
an unordered first-50 page out of the 844-city citypage collection and filtered
THAT by name. Toronto and Vancouver are not in the arbitrary first 50, so they
were never found. Both fetch_current_conditions and fetch_forecast had the
defect independently.
Why nothing caught it:
- The integration tests wrapped their assertions in `if "_meta" in data:`, so
a NOT_FOUND response skipped the body and the test passed green.
- The unit tests mock ogc_fetch to return the matching city, which assumes
away the very filtering under test.
Fix: pass a server-side `name.{lang}` property filter so all 844 cities are
searched upstream (confirmed: name.en=Toronto matches 2, name.fr=Montréal
matches 1; the wrong param name returns 0 rather than being ignored).
Two subtleties handled:
- The upstream filter is a case-insensitive TOKEN match, so name.en=Toronto
returns ["Toronto Island", "Toronto"] in that order. _pick_city prefers an
exact name match, so "weather in Toronto" no longer answers with Toronto
Island. "Ottawa" still resolves to "Ottawa (Kanata - Orleans)" via the
containing-match fallback.
- The containing-match check doubles as a client-side verification: if the
upstream ever ignores the name parameter and returns an unrelated city, we
return None rather than confidently answering with the wrong place. This
preserves the safety the old client-side filter provided.
Tests: 7 new in TestLocationNameLookupIsServerSide asserting the filter reaches
ogc_fetch (rather than mocking it away), French uses name.fr, exact beats
partial, and lat/lon still uses bbox with no name filter.
De-masking, per the Phase 20.1 CONTEXT decisions:
- tests/integration/conftest.py gains assert_live_or_transient() and
assert_rows(). The first implements the D-08 hardened pattern: an error
response is tolerated only if the code is UPSTREAM_ERROR or RATE_LIMITED,
so a real outage stays green while NOT_FOUND on a call that should succeed
fails loudly — which is exactly how this bug was caught. The second refuses
an empty payload unless the caller documents why empty is valid.
- All 9 tests in test_weather_current_scenarios.py rewritten onto them. The
`assert "_meta" in data or "error" in data` lines are gone; they passed for
both outcomes and asserted nothing.
- pyproject registers the tolerates_upstream_error marker (D-14).
Verified: 3081 unit tests pass; weather module 294 pass; the 9 live
weather/current scenarios pass against MSC GeoMet.
…te tests
De-masking `test_weather_climate_scenarios.py` surfaced that
`wx_get_climate_trends` has never returned a filtered record.
The ahccd-trends collection names its properties with bilingual
double-underscore suffixes, unlike the SCREAMING_CASE used by climate-daily and
climate-normals. The client used the latter convention throughout:
sent CLIMATE_IDENTIFIER=... real station_id__id_station
sent MEASUREMENT_TYPE=... real measurement_type__type_mesure
read TREND / YEAR_BEGIN / ... real trend_value__valeur_tendance, ...
So every filtered call matched zero records, and because _flatten_trend read the
same wrong keys, even an unfiltered call produced rows of all-None.
The collection also publishes PRECIPITATION only — "rain", "snow",
"total_precip". The tool docstring advertised "temperature", which matches
nothing; corrected, along with a note explaining the absence despite the AHCCD
name. The flattener now also surfaces station_name, period, year_range,
province and elevation_m, which the real payload carries and the old key set
discarded.
Why nothing caught it: the fixture `sample_trend_feature` was written with the
same invented SCREAMING_CASE keys as the client, so the unit tests agreed with
the code rather than with the API — the same self-referential pattern found in
StatCan on 2026-07-25. The integration test asserted shape only `if
data["data"]:`, so an empty list skipped the body and passed. Fixture replaced
with a real feature captured from the live collection.
Also fixed in the integration file, triaged as test-drift rather than tool bugs:
- test_climate_normals asked for normals with station 6158731, which exists in
climate-daily but not climate-normals. Ottawa CDA is indexed there as
6105976 (966 records). Split into OTTAWA_STATION / OTTAWA_NORMALS_STATION
with a comment, since the two collections genuinely disagree.
- test_climate_trends asked for "temperature"; now asks for "total_precip".
De-masked all three `if data["data"]:` guards via assert_rows(), which refuses
an empty payload unless the caller documents why empty is valid. Ottawa CDA
January 2024 daily data, its normals, and national precipitation trends are all
closed/published series — empty means broken, so none carry an exemption.
Verified: 3098 unit tests pass, 97% coverage; the 8 live weather/climate
scenarios pass; test_quality docstring checks still pass.
…AQHI ids
Completes the weather de-masking (D-10, D-12). All four weather files plus
test_live_apis.py now assert in every branch; the only masks left in the suite
are the 29 in test_tool_scenarios.py.
Files: aqhi_hydro (9 guards), marine_severe_snow (7), collections_summary (7),
live_apis (1). Every `assert "_meta" in data or "error" in data` line is gone —
it was true for both outcomes and asserted nothing.
One more class of invented test data surfaced, triaged as test-drift (D-04):
AQHI location_ids are opaque 5-letter MSC codes, not province-prefixed
strings. The tests used "ON106" and "ON-01", which match nothing, and the
masks hid the resulting NOT_FOUND. Resolved from the live collections by
filtering location_name_en: Ottawa is FEVNT. Worth noting for future work —
the id differs per collection for the same city (Toronto observations FDQBU,
Toronto forecasts FCWYG), so they are not interchangeable.
Error-path tests kept, now explicitly two-armed rather than one-armed:
- test_flood_risk_invalid_station / test_collection_not_found: NOT_FOUND is
the CORRECT answer for a nonexistent station or collection, so these assert
the error rather than tolerating it. They are error-path tests, not masks,
and need no exemption.
- test_hurricane_tracks_off_season and test_thunderstorm_outlook: the dead
`pass` branches now assert that a list payload is genuinely empty, so a
populated-but-wrong-shaped response fails instead of slipping through.
- test_bill_keyword_search_limitation asserted a known upstream limitation
only `if data["objects"]:`. Session 42-1 always has bills, so an empty list
means the request broke rather than the limitation ending; now asserted.
Payload emptiness is now explicit everywhere: assert_rows() refuses an empty
result unless the caller documents why empty is valid. Three tests legitimately
do (no active weather alerts in Ontario, none Canada-wide, no radar echoes near
Ottawa) and say so inline. The rest — NS marine areas, Ontario hydrometric
stations, Ottawa AQHI, climate-stations, MSC collection list — are published
series where empty means broken.
Verified: 3098 unit tests pass; 38 live weather scenarios, 18 live_apis and 10
marine/severe/snow scenarios all pass.
…asets
BC was never in quick task 1's five-province scope. It carried 4 _meta guards,
1 data-key guard, and 5 pytest.skip calls.
The skips were the worse half (D-11). Both bc_query_features routing tests
discovered a dataset by searching BCDC and skipped when the search came back
empty or matched nothing WFS-queryable. A skip reports neither pass nor fail, so
"BCDC search returned no results" silently meant the routing path under test was
never exercised — and nothing distinguished that from the tests passing.
Replaced the discovery dance with two pinned, canonical BCDC package ids:
22c7cb44 BC Wildfire Fire Perimeters - Historical (queryable_via_wfs=True)
7ec1a555 BC Greenhouse Gas Emissions (queryable_via_wfs=False, 11 resources)
Both routing tests now always run. If either dataset disappears the test fails
loudly, which is the intent — a vanished canonical dataset is news, not a reason
to skip. Added test_dataset_details_exposes_wfs_routing_metadata to assert the
two-step CKAN→WFS workflow's contract directly: details must carry
queryable_via_wfs and object_name, which is what bc_query_features routes on.
The file-parser test previously only asserted on a locally-collected dict and
never called bc_query_features at all; it now asserts the non-WFS branch's
preconditions against a known file-resource dataset.
De-masked the 4 _meta guards onto assert_live_or_transient, and tightened the
empty cases that were silently acceptable:
- 2023 fire perimeters (a record BC fire season) must return features
- PROVINCIAL PARK must return parks — BC has hundreds
- mineral tenure must return claims — BC has thousands, so empty means the
tenure_type filter is broken rather than the province being quiet
Verified: 9 live BC scenarios pass against BCDC + BCGW WFS.
…-mask YR+AB
De-masking York Region and Alberta surfaced two more real bugs, one of them in
shared code affecting four modules.
1. shared/arcgis_hub.py sent `q=` on no-query listings.
Every ArcGIS Hub portal rejects an empty q with HTTP 400 — verified against
aurora, newmarket, york_region, markham and manitoba: all five return 400 for
q='' and 200 when q is omitted. So every "list everything" call failed:
aurora_list_categories and newmarket_search_datasets both returned
UPSTREAM_ERROR, which read as an upstream outage. Now omitted when the query is
empty or whitespace, mirroring the startindex=0 handling on the next line.
This is the shared Hub client, so the fix reaches York Region, Alberta, Manitoba
and Saskatchewan — the same blast radius as the Phase 19 startindex fix.
2. York Region text filters were case-sensitive against uppercase data.
ArcGIS stores the attributes uppercase ("WATERBRIDGE", "MAIN"), but the WHERE
clauses compared the caller's raw string. Live counts:
STREET LIKE '%Main%' 0 rows
UPPER(STREET) LIKE '%MAIN%' 420 rows
NAME LIKE '%Main%' 40 rows (mixed-case data)
UPPER(NAME) LIKE '%MAIN%' 42 rows
An agent asking for "Main Street in Markham" got an empty result indistinguish-
able from a legitimate no-match. Fixed all three LIKE filters (transit stops,
Markham addresses, Markham roads) to uppercase both sides. Three existing unit
tests asserted the old case-sensitive clause and were updated — they encoded the
defect.
3. Shape drift resolved in favour of the tools (D-05, case by case).
The three York Region failures in the baseline were tests asserting
`isinstance(data["data"], list)` against tools that return
`{"features": [...], "count": N, "truncated": bool}`. That dict is the
deliberate, consistent convention for geospatial feature queries across BC WFS
and all four ArcGIS modules — the count and truncation flag are meaningful to an
agent. The tools are right; the tests drifted. Added assert_feature_payload() to
conftest so the contract is asserted in one place.
Alberta's three unhardened guards de-masked the same way. Its hospital test
previously wrapped the count-range assertion in `if isinstance(count, int)`, so
a missing count skipped the check entirely; the count is now required. Active
fires deliberately does NOT assert a feature count — zero is a legitimate winter
reading — but the shape is still checked.
Masking idioms remaining in the suite: 9, down from 62 at the start of the phase.
Verified: 3105 unit tests pass; York Region 140 unit / 8 live, Alberta 7 live,
shared hub 22 unit — all green. The 3 baseline York Region failures are fixed.
… shape change D-06 asked whether the BOC dict shape was intentional before deciding. It was: commit 485afdb (2026-04-09) "refactor: extract shared reshape utilities and apply to BOC tools" deliberately moved the observation tools to a series-keyed format so the label and description are stated once instead of repeated on every row: {"FXUSDCAD": {"label": "USD/CAD", "description": "...", "observations": {"2026-07-23": 1.39}}} That commit updated the BOC unit tests but not the integration tests or the module docs, so 5 live tests (4 BOC + 1 cross-module) have been failing since April with KeyError: 0 — indexing a dict with [0]. Verdict: tests drifted, tool is right. No code change. Rewrote the 5 tests against the real contract via a new assert_series_payload() helper, which requires each named series to be present AND to carry a non-empty observations map. The assertions got stronger in the process: - USD/CAD checks every observation is in a plausible range, not just the first - the EUR date-range test asserts a full month of banking days (>=15) and that a single-currency query returns exactly one series - the multi-series test names all three series explicitly - the policy-rate test asserts observations exist before range-checking them, rather than indexing blindly Also updated docs/modules/bank-of-canada.md, whose response example still showed the April flat-list shape — the same drift one layer out. The JSON block is now valid and matches what the tool actually returns. Verified: 14 live BOC + cross-module scenarios pass; 3105 unit tests pass. Baseline failures remaining: 10 of 18 (BOC 5 and York Region 3 now fixed).
Six live failures in the StatCan/SDMX cluster, three real defects among them.
1. A coordinate with no series crashed on Pydantic validation.
StatCan answers getSeriesInfoFromCubePidCoord for an unpopulated coordinate with
status=SUCCESS, responseStatusCode=2 ("Invalid cube and series combination") and
every field null. _unwrap only special-cased responseStatusCode 2 when the OUTER
status was not SUCCESS, so this fell through to SeriesInfo and the tool surfaced
"UPSTREAM_ERROR: 6 validation errors for SeriesInfo" — blaming the service for
what is really an empty lookup. Added _require_series(), which raises a clean
ValueError for the no-series statuses (2, 4, 5 per getCodeSets wdsResponseStatus)
on both the by-vector and by-coordinate paths, and the tool now maps it to
NOT_FOUND.
2. Empty SDMX results are malformed JSON, reported as the caller's fault.
Asking for a key with no observations returns HTTP 200 and a body with two
surplus closing braces:
"dataSets": [{ "action": "Information","series":{ }}}}],"structure":...
^^^^ should be }}
resp.json() raises JSONDecodeError, which subclasses ValueError and was caught
by the handler meant for the last_n/date-range conflict — so StatCan's broken
output surfaced as "INVALID_INPUT: Expecting ',' delimiter: line 1 column 200".
_parse_sdmx_body() now recognises the empty-series marker and returns an empty
result, and any other unparseable body raises as UPSTREAM_ERROR. The genuine
INVALID_INPUT case (lastN combined with a date range) keeps its code, asserted
by a test.
3. UPSTREAM_UNAVAILABLE added to the integration TRANSIENT_CODES set.
StatCan WDS has a documented nightly maintenance window (00:00-08:30 EST) and
deliberately reports UPSTREAM_UNAVAILABLE during it. That is scheduled downtime,
not a defect — treating it as transient is what lets the suite run overnight. It
stays distinct from NOT_FOUND and INVALID_INPUT, which remain hard failures.
Test-side fixes (drift, not defects):
- test_get_series_info_by_coord and test_get_data_by_coord asked for
coordinate 1.1.0.0..., which identifies no published series. They now use
2.2.0.0... (CPI all-items, vector 41690973) and assert the vector id and
Monthly frequency. Added test_coordinate_with_no_series_is_not_found to
cover the empty case deliberately rather than by accident.
- test_get_bulk_vector_data expected a list; the tool returns a dict keyed by
vector id. Per D-05, decided case by case: the key IS the answer to "which
vector is this row from", so a flat list would repeat it on every row —
same rationale as the BOC series-keyed shape. Tool is right, test updated.
- Four SDMX unit-test mocks set .json() but the client now reads .text; they
supply both.
Verified: 3111 unit tests pass, 97% coverage; 27 live StatCan + SDMX scenarios
pass. Baseline failures remaining: 4 of 18 (Toronto TTC 2, IRCC 2).
…es as JSON
The last four baseline failures. Two were real defects, both of the same
species: something pinned that upstream later moved.
1. Both TTC tools have been dead, not flaky.
D-09 required checking reachability before tolerating an error as transient —
and this is exactly why. constants pinned a dataset uuid, resource uuid and
filename:
.../7795b45e-...-c5b0dc4b531e/resource/f17e0649-.../download/
ttc-routes-and-schedules.zip
Toronto has since republished the feed under different uuids and a different
filename (opendata_ttc_schedules.zip). Both the pinned download URL AND the
pinned dataset uuid now 404, so toronto_get_ttc_stops and toronto_get_ttc_routes
returned "UPSTREAM_ERROR: Failed to fetch TTC GTFS stop data" — indistinguish-
able from an outage. Had the hardened pattern been applied without checking, a
permanently dead tool would have been marked green forever.
Fixed by removing GTFS_ZIP_URL entirely and resolving the ZIP resource from CKAN
package_show at call time, keyed by the dataset SLUG rather than a uuid — the
slug survives republishes, the uuids do not. constants.py carries a note saying
why the URL must not be pinned again, and a unit test asserts GTFS_ZIP_URL stays
absent. Live check after the fix: 9,361 TTC stops.
2. The datastore refused nested values.
Storing IRCC output produced "DATASTORE_ERROR: Error binding parameter 2: type
'dict' is not supported" — sqlite3's message, not an answer. IRCC reshapes
observations into {"years": {"2023": {"q1": {...}}}} by design, and the
datastore exists precisely so an agent can combine any module's output in one
SQL query, so rejecting a whole class of module output defeats its purpose.
insert_rows now serialises dict and list values to JSON text via _bind().
Scalars pass through untouched — a test asserts ints stay ints. The data stays
queryable through SQLite's JSON1 functions, which another test demonstrates:
SELECT json_extract(years, '$."2023".total') FROM ircc_pr;
3. test_ircc_invalid_breakdown asserted an unreachable branch.
`breakdown` is Literal-typed, so Pydantic rejects an unknown value at the MCP
boundary before the tool body runs; the tool's own INVALID_INPUT branch is dead
code for that parameter. The test expected a dict and died on the ToolError. It
now asserts what actually protects the agent — that the rejection names the
parameter and lists the valid options, which a bare INVALID_INPUT string would
not. (Same phenomenon recorded in 17-VERIFICATION.md during the reconciliation.)
All 18 baseline failures are now resolved:
BOC 5, StatCan 5, York Region 3, Toronto 2, IRCC 2, SDMX 1.
Verified: 3123 unit tests pass, 97% coverage; Toronto 116 unit / 10 live,
datastore 109 unit, IRCC 6 live.
…stream errors
The guard that makes this phase stick, plus the last of the failures.
1. tests/test_integration_test_quality.py — the anti-reintroduction guard (D-13).
Written first and used as the worklist for the whole phase; committed last so
main was never red. It parses tests/integration/ and enforces one rule: every
path through an integration test must reach an assertion. Three structural
violations are detected — a one-armed `if` on the response wrapping the
assertions, a bare `return` abandoning them, and pytest.skip. It runs in the
DEFAULT unit suite, like test_quality.py, so reintroducing a mask fails on every
commit rather than only in a live run.
The detector is itself tested (6 parametrized cases): the hardened pattern must
NOT flag, a one-armed guard must, and a conditional over local bookkeeping must
not. A guard nobody trusts gets deleted.
Two meta-tests keep the exemption honest: every exemption must state a
non-trivial reason, and exemptions may not exceed 10% of the suite.
Progress: 62 masking idioms at the start of the phase, 0 now, with exactly one
declared exemption.
2. PRESERVE audit (D-14) — the list shrank from ~10 to 1.
Re-examining each supposedly-tolerant test showed most did not need tolerance:
- StatCan invalid-product and the BC/weather not-found tests are error-PATH
tests where the error IS the expected answer; now asserted in both arms.
- Quebec road works and winter road conditions genuinely can be empty, so they
state that inline via allow_empty_reason rather than skipping the check.
- The Quebec bridges test guarded a nullable route_name; `or ""` keeps the
Route-204-leak assertion running on every row.
- test_store_pr_data_to_datastore skipped when 2023 PR data was missing —
which is exactly how it hid the datastore dict-binding bug. Now asserts.
- The NS AMI test mixed a zone-normalization PROOF with a sex-filter probe, so
whenever the filter matched nothing the proof silently did not run. Split:
the proof now queries bare "ami" and asserts strictly; the sex-filter
behaviour is a separate test carrying the sole exemption, since AMI has no
sex column upstream and both a 400 and zero rows are correct answers.
3. drug_database and nutrient_file had ZERO exception handling.
The full live suite surfaced 9 failures that pass in isolation: under load the
Health Canada Drug API is slow enough to time out, and with no try/except in
either module the timeout escaped as a raw fastmcp ToolError rather than an
error envelope. That violates the project rule that a tool returns make_error()
and never raises, and it defeats the hardened pattern — a test cannot classify
an exception it never receives.
Added shared/envelope.py:upstream_guard(), applied under @tool to all 16 tools.
functools.wraps keeps __wrapped__ so inspect.signature still sees the real
parameters and @tool registration is unaffected — asserted by a test, since a
broken signature would silently unregister the tools.
Verified: 3124 unit tests pass, 97.07% coverage; the masking guard passes; 17
live Drug/Nutrient scenarios pass; ruff clean on all touched files.
- datastore/client.py used Any in _bind() without importing it (masked at runtime by `from __future__ import annotations`, but wrong) - tests/test_envelope.py had an import above the module docstring - dropped two more extraneous f-string prefixes Repo ruff count is now 94, below the 96 baseline this phase started from.
…tfalls
Phase 20.1's ROADMAP title described work quick task 1 had already finished, and
its goal was still the placeholder "[Urgent work - to be planned]" (D-03).
Renamed to what the phase actually delivered, with its six plans listed.
Backfilled TEST-01..TEST-05 describing the guarantees the phase now enforces —
every path reaches an assertion, tolerance requires asserting a transient code,
reintroduction fails the default unit suite, exemptions carry a mandatory
reason, and tools return envelopes rather than raising. Added to the
traceability table.
Two cross-module pitfalls added to CLAUDE.md, both discovered the hard way:
- Every ArcGIS Hub portal 400s on an empty `q`. Affects four modules.
- Toronto republishes the TTC GTFS feed under fresh uuids, so the download URL
must never be pinned — resolve it from CKAN by slug.
CLAUDE.md and .claude/rules/tests.md now state the integration-test rule with
the banned idioms spelled out, the helper usage, and the reachability caveat
that caught the dead TTC tools. Written so the next module author copies the
right pattern from the nearest example, which is how this defect spread.
Also added the six PLAN/SUMMARY pairs so the phase reads correctly in GSD state.
Phase 20.1 verified PASSED. 5/5 requirements, 18/18 baseline failures fixed, 62 masking idioms reduced to 0 with one declared exemption. Nine real defects were found and fixed, all of which had been shipping behind green tests: 1. Every named-location weather lookup returned NOT_FOUND (Toronto, Vancouver) 2. wx_get_climate_trends never returned a filtered record — wrong field names 3. Every ArcGIS Hub "list everything" call 400'd on an empty q — 4 modules 4. York Region text filters matched 0 rows instead of 420 for mixed case 5. StatCan empty coordinates crashed on Pydantic validation 6. Malformed empty SDMX bodies were reported as the caller's INVALID_INPUT 7. Both Toronto TTC tools were permanently dead behind an UPSTREAM_ERROR 8. The datastore refused nested values, defeating its cross-module purpose 9. 16 tools had no exception handling, so timeouts escaped as raw ToolErrors Three share the root cause found in StatCan during the preceding reconciliation: tests and fixtures written to agree with the code rather than the API. Verified: 3124 unit tests, 97.07% coverage, guard green, ruff 94 (baseline 96).
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e1c533dbe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+112
to
+113
| except ValueError as exc: | ||
| return make_error("INVALID_INPUT", str(exc), lang=lang) |
There was a problem hiding this comment.
When either Health Canada endpoint returns HTTP 200 with an HTML or malformed JSON body, httpx.Response.json() in the drug and nutrient clients raises json.JSONDecodeError, which subclasses ValueError; this handler therefore labels an upstream response failure as INVALID_INPUT. Valid tool calls will misleadingly blame the caller, and integration tests that correctly tolerate only transient UPSTREAM_ERROR responses will fail during this upstream condition. Handle JSON decoding failures as UPSTREAM_ERROR before treating genuine argument-validation ValueErrors as invalid input.
Useful? React with 👍 / 👎.
mariomeyer
force-pushed
the
gsd/phase-20.1-integration-de-masking
branch
from
July 26, 2026 01:43
3e1c533 to
b88dcd1
Compare
Two of the four CI gates, both failing on main before this branch existed. Catalog freshness: TOOLS.md claimed "84 tools across 8 modules" while the repo ships 273 across 20 — stale since roughly phase 13. Regenerated. Only two lines of the 1966-line diff come from this branch (the wx_get_climate_trends docstring correction); the rest is twelve phases of accumulated drift. Lint: tests/integration/test_prompts_resources_scenarios.py imported the mcp_server fixture at module level, and every one of its 87 test parameters then shadowed that name — 87 F811s from a single redundant import. pytest discovers conftest fixtures automatically, so the import only ever created the collision. Removed; all 83 tests still collect. ruff: 96 on main -> 7 here. Remaining: 1 E402, 4 F841, 2 F541, all pre-existing. pyright (834) is untouched and still fails the gate.
…eter query_feature_service and get_count typed `where: str = "1=1"`, but four Alberta call sites passed None. httpx drops None-valued params, so the request reached ArcGIS with no `where` at all — and ArcGIS answers that with HTTP 200 carrying an error 400 "Unable to perform query operation". Agents saw a bogus UPSTREAM_ERROR that read as an upstream outage. Verified live against Saskatchewan Public_Fire_Ban on 2026-07-26: omitting `where` errors, `where=1=1` returns features. Fixed in the shared client rather than at the four call sites so no future caller can reintroduce it. Same class as the documented empty-`q` Hub pitfall. Reproduction test asserts the outgoing param.
- prompts.py (alberta, manitoba, nova_scotia, saskatchewan): import Message from fastmcp.prompts like the other 16 modules do, not from the deeper fastmcp.prompts.prompt path. Pyright could not resolve the deep path, so it treated every symbol in those four modules as Unknown — which silently suppressed 99 downstream errors in their test files. - british_columbia/client.py: _api_get returns the unwrapped CKAN `result`, which is a list for organization_list/tag_list. Its dict[str, Any] annotation was simply wrong; callers already isinstance-narrow. - shared/parsers.py: pandas is an optional extra with an openpyxl fallback, so the three lazy imports are legitimately unresolvable. Marked explicitly. Source code now type-checks clean.
Lint (7 -> 0): - ontario/tools.py: move `import re` to the top of the module (E402) - statcan/test_prompts_resources.py, shared/test_ogc.py: drop four dead assignments (F841) - integration/test_tool_scenarios.py: drop f-prefixes with no placeholders Type check (921 -> 0): the remaining errors were all in test files and all noise — runtime assertions narrow prompt Message.content and Resource.read() unions in ways pyright cannot follow, and several tests deliberately pass invalid values to exercise error handling. Each affected test file now declares a file-level pragma listing only the rules it actually needs. Source code stays strictly checked, which is where every real finding came from: the where=None bug and the four unresolved prompt imports. All four CI gates now pass locally: ruff, pyright, 3125 tests at 97.07% coverage, and catalog freshness.
httpx drops None-valued params, so where=None silently became "no where clause" and ArcGIS answered with a 200-wrapped error 400 that read as an outage. Documented alongside the empty-`q` pitfall it mirrors.
STATE.md still said "context exhaustion at 75%" from the session that was handed off; Phase 20.1 is complete and PR #2 now has all four CI gates green. Also records a defect found while probing ArcGIS endpoints for the where=None fix: Alberta's whole WMB wildfire FeatureServer group returns 499 Token Required, killing three fire tools. Its one integration test tolerates the error via assert_live_or_transient, so it passes silently — the masking pattern tests.md warns about. Deferred to its own phase rather than widened into this PR. .claude/.gitignore keeps the handoff scratch file out of git.
Addresses Codex review on PR #2 (P2, shared/envelope.py:113). json.JSONDecodeError subclasses ValueError, so when Health Canada answers HTTP 200 with an HTML error page, httpx's .json() raised inside a guarded tool and fell through to the ValueError arm — reporting INVALID_INPUT and blaming the caller for an upstream outage. It also broke live tests in a confusing way: assert_live_or_transient tolerates only UPSTREAM_ERROR, RATE_LIMITED and UPSTREAM_UNAVAILABLE, so a real outage failed as if the arguments were wrong. Handle JSONDecodeError ahead of the ValueError arm. A second test pins that genuine argument-validation ValueErrors still return INVALID_INPUT, so the fix cannot swallow real caller errors. Verified the mechanism rather than assuming it: JSONDecodeError does subclass ValueError, httpx .json() raises it on an HTML body, and both the drug and nutrient clients call .json() inside the guarded path. Also records in STATE.md that the same masking is latent in ~40 ValueError arms downstream of shared/http.py:api_get, which has no decode guard. That is pre-existing and best fixed once inside api_get; left out of this PR.
Sequenced between 20.1 and 21 rather than at the end of the milestone. Phases 21-39 add ~19 more modules that would each copy the current inconsistent handler shape, so normalizing across 24 modules now is far cheaper than across 43 later, and this is the root cause of the same masking class 20.1 just addressed. Scope, from the Codex review on PR #2 and the follow-up investigation: shared/http.py:api_get returns response.json() with no decode guard, so an HTTP 200 carrying HTML raises json.JSONDecodeError — a ValueError subclass — straight into the `except ValueError -> INVALID_INPUT` arms of seven modules. The guard cannot simply be added at the source: httpx.DecodingError is an HTTPError but NOT an HTTPStatusError, and 5 of 24 modules (bank_of_canada, ckan, ircc, ontario, recalls) catch only HTTPStatusError, so a naive central fix converts a mislabelled error into an unhandled one. Handler shape must be normalized first, then the guard added. Corrects the earlier STATE.md note that called this a one-line api_get fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Finishes the integration-test de-masking quick task 1 started, fixes every live failure it deferred, and adds a guard so the pattern cannot come back. Also includes a
.planning/reconciliation that preceded it.De-masking worked as intended: it surfaced nine real bugs that were shipping behind green tests.
The bugs
wx_get_climate_trendsnever returned a filtered recordqINVALID_INPUTToolErrorBug 7 is why the phase required a reachability check before tolerating any endpoint: applying the "tolerate transient errors" pattern without it would have marked a dead tool green forever. It now returns 9,361 stops.
Bugs 2 and 4, and the fixtures behind 2, share a root cause with the StatCan defects fixed in the earlier commits here: tests and fixtures written to agree with the code rather than the API.
test_client.pybuilt its expected values from the very constant under test, so the assertions passed for any map — including one that reported monthly CPI as "Bi-monthly" for three phases.The guard
tests/test_integration_test_quality.pyenforces one rule by AST: every path through an integration test must reach an assertion. It catches one-armed response guards, barereturn, and data-dependentpytest.skip— including idioms nobody has written yet.It runs in the default unit suite, so reintroducing a mask fails on every commit rather than only in a live run. The detector is itself tested (6 cases, including that the hardened pattern must not flag). Exemptions require a mandatory reason and are capped at 10% of the suite.
Numbers
Known caveat
No clean full-suite live run. Every cluster passes in isolation (weather 38, BC 9, YR 8, AB 7, BOC 14, StatCan/SDMX 27, Toronto 10, IRCC 6, NS/QC 58, Drug/Nutrient 17), but running all 339 live tests back-to-back times out on Drug/Nutrient under Health Canada rate limiting. That is upstream load, not a defect — and bug 9's fix means those timeouts now arrive as structured envelopes the assertions classify correctly instead of dying unhandled. Recorded in
20.1-VERIFICATION.mdrather than glossed over.Note on scope
6 of the 19 commits are
.planning/-only (state reconciliation, phase artifacts). Happy to strip them onto a source-only branch if you'd prefer a tighter diff.