Phase 21: New Brunswick — 22 tools, and three guarantees that weren't

[mariomeyer]

Ships the new_brunswick module — the 8th province — and, more usefully, three cases where a claimed guarantee and the actual code had drifted apart.

The module

22 tools, 6 prompts, 7 resources. Repo now at 295 tools / 21 modules.

Surface	Tools	Notes
Federal CKAN discovery	5	open.canada.ca filtered to organization:nb — NB has no provincial CKAN (221 datasets)
gnb.socrata.com discovery	2	NB's provincial Socrata portal, keyless, 312 datasets — added mid-phase, see below
GeoNB service discovery	3	The ArcGIS Hub returns 401, so the REST directory is enumerated instead
Curated geospatial	9	Flood/water, Crown land, parcels/civic address, health/education
511 transport	3	Key-gated stubs returning a structured NOT_CONFIGURED envelope (Manitoba precedent)

Two additive functions land in shared/arcgis_hub.py (list_arcgis_server_services, get_arcgis_server_layers) — a service-directory enumerator standing in for the unavailable Hub Search API. Purely additive; York Region, Alberta, Manitoba and Saskatchewan callers are untouched.

Every curated layer id was resolved live against geonb.snb.ca into 21-SPIKE.md (62 services enumerated, 11/11 ids confirmed) rather than assumed. That discipline is not decorative: Crown Land's only layer is 3, Wetlands is 2, and the mineral-occurrences service publishes its layers out of order (0, 1, 7, 2, 3, 4, 5, 8, 6). Positional reasoning about "the first layer" is wrong twice over.

Three drifts between claim and code

1. A load-bearing planning claim was false

21-CONTEXT.md asserts New Brunswick has no provincial catalogue and no Socrata instance, and instructs the phase to remove NB from CLAUDE.md's Socrata note. Live probing at plan time found the opposite:

GET gnb.socrata.com/api/catalog/v1?domains=gnb.socrata.com&only=datasets  -> 200, resultSetSize 312
GET gnb.socrata.com/resource/4zbh-z2ij.json?$limit=2                      -> 200, real bilingual rows

Because MCP tool names are a published API surface, this went to a blocking checkpoint rather than a planner's judgement call. Resolution: the two Socrata tools ship (reusing shared/socrata.py, zero new client code), and nb_get_provincial_parks / nb_get_mineral_occurrences drop to the long tail to hold the tool budget — both still reachable via nb_query_geonb_layer.

What CONTEXT.md got right and this does not overturn: data.gnb.ca, opendata.gnb.ca and nbopendata.ca genuinely do not resolve, and the GeoNB Hub genuinely 401s.

2. An invariant that could not fail

Every plan was reporting "tool manifest set-equal ✓" against:

ALL_NB_TOOLS: tuple[str, ...] = ALL_NB_TOOL_NAMES   # "cross-checked in tests so they can never drift"

An alias. The assertion was unfalsifiable, and the real manifest tests only checked count, membership and nb_ prefix — nothing verified that a manifest name resolved to a registered tool. TestManifestMatchesShippedSurface now proves genuine bidirectional set-equality between the declared manifest and the shipped @tool objects, and was validated by decoy injection.

3. A guard that existed but did not guard

The sharpest one. FILTER_REQUIRED_TOOLS is the mitigation for threat T-21-03 — refuse an unfiltered query against the 604,520-row parcel layer before any network I/O, because the harm is an agent's context window destroyed by a well-formed request. The guard was present and greppable. It was also defeated by one character:

if any(filters): return                    # "%" and " " are both truthy
return value.replace("'", "''")            # escapes quotes, not LIKE metacharacters

So county="%" built UPPER(COUNTY) LIKE '%%%' and matched the entire layer. Fixed in 84192e3; the clause now escapes %, _ and \ and declares ESCAPE '\', and vacuous filters are rejected at both tool and client layers. Verified live against GeoNB_SNB_Parcels:

Input	Clause	Rows
county="%" (pre-fix)	LIKE '%%%'	604,520
county="%" (post-fix)	LIKE '%\%%' ESCAPE '\'	0
county="YORK"	LIKE '%YORK%' ESCAPE '\'	64,208
county=" " / ""	—	INVALID_INPUT, no network call

The generalisable point: a grep-depth check would have marked T-21-03 CLOSED both before and after this fix. Presence of a named guard is not evidence of a working one. The five remaining review findings (unparenthesised fq conjunction, unclamped limit echo, unvalidated negative limit, weak naming guard, dead schemas.py) were fixed in the same pass.

Security

21-SECURITY.md: 21 threats, 21 closed, threats_open: 0. The workflow's L1 short-circuit was deliberately overridden — its premise ("grep depth is sufficient") is exactly what #3 above disproves — so T-21-01/02/03/04/16 were traced at L2/L3 depth.

The audit surfaced one register gap worth recording: the threat register covered the planned surface, not the shipped one. All seven <threat_model> blocks were authored before the checkpoint, so nothing covered nb_query_gnb_socrata_dataset's raw SoQL where/select passthrough. Registered as T-21-20 (Tampering / medium / accept — read-only keyless public server, SoQL parser is the trust boundary), matching the already-accepted T-21-12 GeoNB escape hatch, with the boundary now named in the docstring where agents actually read it.

One caveat left open honestly: T-21-08's disposition claims the cache-key prefix is "asserted in unit tests". It isn't — the autouse fixture ignores key. The code mitigation is present at all 24 sites, so the threat is closed on code evidence, but the coverage claim is overstated and is flagged rather than quietly accepted.

Documentation

CLAUDE.md now records the verified gnb.socrata.com facts — neither CONTEXT.md's false claim nor the original forward-looking guess — and marks PEI's portal as still requiring independent verification. README/TOOLS.md regenerated (scripts/generate_catalog.py --check passes). ROADMAP's Phase 21 goal was restated so its sub-counts reconcile to 22 after the checkpoint, and two stale STATE.md entries were cleared (a resolved checkpoint still reading as open, and a stale Current focus).

Gates

3550 passed, 2 skipped; coverage 97.37% (≥95% required); ruff clean; pyright 0 errors; catalogue fresh. Live NB integration 29/29 through the MCP Client layer, asserting field presence and non-null values — not just response shape. Zero new dependencies (pyproject.toml / uv.lock diff empty across all 45 commits).

Phase verification: 30/30 must-haves, no gaps, no human-verification items.

One pre-existing live failure elsewhere in the repo (Quebec MTQ WFS) is unrelated — no commit in this phase touches any Quebec file.

Requirements: NB-01..NB-25. (ERR-01..ERR-07 belong to Phases 20.2–20.4; this module complies with them — no bare ValueError, no JSON decoded outside shared/http.py, catch-all coverage on every tool.)

[cursor]

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]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5ffea247ea

ℹ️ 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".

[chatgpt-codex-connector]

Prevent extra_fq from escaping the NB scope

Because extra_fq is inserted as raw Lucene syntax, parentheses do not guarantee the organization restriction: an input such as *:* ) OR (*:* produces (organization:nb) AND (*:* ) OR (*:*), allowing the final OR branch to match non-NB datasets. This contradicts the tool's advertised non-overridable scope, so reject delimiter-breaking fragments or construct the additional filters from structured parameters.

Useful? React with 👍 / 👎.

[mariomeyer]

Confirmed and fixed in 264449c.

Verified the exact input: extra_fq = "*:* ) OR (*:*" composed to (organization:nb) AND (*:* ) OR (*:*) — the caller's parenthesis broke out of the wrapper rather than being reinterpreted inside it. extra_fq is a live @tool parameter on nb_search_datasets, so this was reachable.

_validate_extra_fq now rejects delimiter-breaking fragments with InvalidInput. One subtlety worth recording: the attack string has equal counts of ( and ), so a count comparison would have passed it — the validator tracks nesting depth left-to-right and rejects when depth goes negative or ends non-zero, plus an unbalanced-quote check.

This also reopened threat T-21-04 in 21-SECURITY.md, which I had closed citing a test that asserted "boolean semantics, not string shape". That test only ever fed a balanced hostile fragment, so its premise was exactly what your input violates. Register corrected.

[chatgpt-codex-connector]

Enforce the GeoNB record cap

Every curated GeoNB tool forwards its caller-controlled limit here without validating or clamping it, and this assignment replaces query_feature_service's 5,000-record default rather than preserving the cap. A call such as nb_get_crown_land(limit=10000000) can therefore paginate through the entire matching layer, while zero or negative limits silently return an empty success; enforce 1 <= limit <= MAX_RECORDS before fetching.

Useful? React with 👍 / 👎.

[mariomeyer]

Confirmed and fixed in 3ecf46f.

Verified empirically before fixing:

fetch_crown_land(limit=10_000_000)  -> max_records forwarded = 10000000
fetch_crown_land(limit=0)           -> max_records forwarded = 0

MAX_RECORDS was only the default value of each tool's limit parameter — never a clamp — and _geonb_query passed max_records=limit straight through, replacing query_feature_service's own default.

Guard now enforces 1 <= limit <= MAX_RECORDS centrally in _geonb_query, so every curated GeoNB tool inherits it rather than each repeating the check.

Worth noting the asymmetry this exposed: the raw escape hatch fetch_geonb_layer_features had been clamping correctly all along — only the curated tools were unbounded. This reopened T-21-03, whose mitigation text claimed "MAX_RECORDS=5000 plus the truncated flag bound every other path". Register corrected.

[chatgpt-codex-connector]

Reject nonpositive Socrata query limits

The validation only rejects values above the cap, so limit=0 or a negative value is sent to Socrata as $limit; this either becomes an upstream error for caller-invalid input or yields a misleading payload whose truncated calculation is true even with no rows. Validate the lower bound before the network call, as fetch_query_dataset already does.

Useful? React with 👍 / 👎.

[mariomeyer]

Confirmed and fixed in 193c091. fetch_gnb_socrata_query now rejects limit <= 0 before the network call, matching fetch_query_dataset's existing lower-bound check. Verified live: nb_query_gnb_socrata_dataset(dataset_id="4zbh-z2ij", limit=0) now returns INVALID_INPUT instead of reaching Socrata.

[chatgpt-codex-connector]

Return coordinates for contaminated-site locations

When callers use nb_get_contaminated_sites to retrieve the mapped site locations advertised by the tool, this projection omits both Latitude and Longitude while geometry is also disabled. Those fields are present in the live layer and in NBContaminatedSite, so successful results currently contain status metadata but no way to locate the sites.

Useful? React with 👍 / 👎.

[mariomeyer]

Confirmed and fixed in 6e81ebe. Verified Latitude/Longitude exist on the live GeoNB_ELG_Contaminated_Sites/0 layer before widening the projection, since a non-existent outFields name returns HTTP 400. Live result now returns [FileOpenDate, Latitude, Longitude, PidType_E, PidType_F, Status_E, Status_F].

[chatgpt-codex-connector]

Include location data in civic-address results

When an agent follows nb_get_civic_addresses' documented geocoding workflow, the result cannot resolve an address to a point or supply the county for the subsequent parcel lookup: the projection omits the layer's LATITUDE, LONGITUDE, and COUNTY fields, and geometry is disabled. Include the available location fields or stop advertising that workflow.

Useful? React with 👍 / 👎.

[mariomeyer]

Confirmed and fixed in 4964e77, plus PID beyond what you flagged.

Verified the live GeoNB_DPS_Civic_Address/0 field list first. Added LATITUDE, LONGITUDE and PID alongside the existing COUNTY. PID matters because nb_get_parcels takes a pid filter — so the advertised address→parcel chain now resolves directly rather than only via county. Live result:

{"CIVIC_NUM": 10, "STREET": "GILLIS", "COMMUNITY": "Fredericton",
 "COUNTY": "York", "PID": "75282285",
 "LATITUDE": 45.91593028, "LONGITUDE": -66.62866143}

NBCivicAddress schema and the tool docstring updated to match; 5edda91 adds live integration assertions for both this and the contaminated-sites projection.

[mariomeyer]

All five Codex findings verified against the code and the live GeoNB layers, then fixed — each with a failing reproduction test first, one atomic commit each.

#	Sev	Finding	Commit
F1	P1	extra_fq delimiter-breaking escaped the NB scope	264449c
F2	P1	GeoNB limit never clamped (MAX_RECORDS was a default, not a bound)	3ecf46f
F3	P2	Socrata limit lower bound unvalidated	193c091
F4	P2	Contaminated sites returned no coordinates	6e81ebe
F5	P2	Civic addresses could not complete the documented geocoding chain	4964e77

Plus 5edda91 (live integration assertions for the F4/F5 projections) and 499a76c — one further defect my own adversarial probing of the F1 validator turned up: a whitespace-only extra_fq composed to AND ( ), which Solr answers with HTTP 409, surfacing caller-invalid input to the agent as UPSTREAM_ERROR. That is the misclassification ERR-01..ERR-07 exist to prevent, so a vacuous fragment is now treated as absent.

Two of these reopened threats that 21-SECURITY.md had recorded as closed (5660496 corrects the register):

• T-21-04 was closed citing a test that asserted "boolean semantics, not string shape" — but it only ever fed a balanced hostile fragment, so its premise was exactly what F1 violates.
• T-21-03 was closed partly on "MAX_RECORDS=5000 bounds every other path", which was simply false.

Both are the same failure mode as CR-01 earlier in this phase: a test that appears to prove a guarantee while testing the wrong thing. The security audit explicitly overrode its own grep-level short-circuit because of CR-01, and then reproduced the same class of error one level up. Three internal passes — code review, goal verification, security audit — missed what an external reviewer with no stake in the prior conclusions caught. That is recorded in the phase security doc rather than quietly patched.

Gates after all fixes: 3564 passed, 2 skipped, coverage 97.38%, ruff clean, pyright 0 errors, catalogue up to date. F1/F2 re-verified empirically and F3/F4/F5 re-verified against live endpoints.

[mariomeyer]

@codex please review again after changes

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5660496c88

ℹ️ 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".

[chatgpt-codex-connector]

Reject non-NB package IDs before shaping details

The earlier extra_fq fix protects only package_search; when a caller supplies the ID of any other open.canada.ca package, this unscoped package_show call returns it without checking raw["organization"]["name"] == "nb". Consequently both nb_get_dataset_details and the downstream nb_query_dataset escape the advertised non-overridable New Brunswick scope. Validate the returned organization before exposing metadata or resources.

Useful? React with 👍 / 👎.

[mariomeyer]

Confirmed and fixed in b5120aa. This one was the important find.

Verified live before fixing — a genuine scope escape, not theoretical:

nb_get_dataset_details(dataset_id="6059da1d-e1da-4f2b-a420-b5c2a130eeaa")
  -> SUCCESS, organization = "ec", title = "Weather Radar - DPQPE"

Environment Canada data returned by a tool whose own docstring says the NB filter "CANNOT be widened".

Root cause is worth stating: the earlier extra_fq hardening (264449c) protected package_search, and the WR-01 parenthesization before it protected fq precedence — but package_show never used fq at all. Three fixes to one door while a second stood open.

fetch_dataset_details now asserts the package belongs to the NB organization (via a shared NB_ORG_NAME constant, not a second literal) and rejects otherwise with NotFound — a well-formed request for something outside this tool's universe. Missing/None organization fails closed. nb_query_dataset inherits the guard through its fetch_dataset_details call.

Verified live in both directions, since a fix that rejects everything would pass a naive test:

non-NB id via nb_get_dataset_details -> NOT_FOUND
non-NB id via nb_query_dataset       -> NOT_FOUND
real NB id                            -> resolves, org=nb

I also swept all four CKAN _api_get call sites: lines 511/641/705 all route through _build_fq; package_show at 538 was the only unscoped one. No further doors of this class.

[chatgpt-codex-connector]

Enforce the record cap on parsed CKAN resources

When a caller supplies a very large positive limit, this check accepts it and rows[:limit] can place every row from a large CSV/XLSX/JSON resource into one MCP response. Unlike the GeoNB and Socrata paths, there is no MAX_RECORDS upper bound here, so an input such as limit=10000000 can produce an oversized response and exhaust an agent's context or server memory. Reject or clamp values above the module cap.

Useful? React with 👍 / 👎.

[mariomeyer]

Confirmed and fixed in 820855a. fetch_query_dataset checked limit <= 0 (from an earlier round) but nothing capped it, so rows[:limit] could slice an entire parsed CSV/XLSX into one response. Now rejects limit > MAX_RECORDS before fetching, mirroring fetch_gnb_socrata_query and _geonb_query.

Verified live: limit=10_000_000 and limit=5001 both return INVALID_INPUT; limit=10 still works.

Swept every limit-taking client function for the same class — 14 total. Twelve already reject or clamp correctly (fetch_search_datasets and fetch_gnb_socrata_search clamp 0->1 and 10M->100, which I left alone as legitimate). fetch_query_dataset was the only real gap.

[chatgpt-codex-connector]

Provide a usable spatial path for the flood workflow

When this prompt is invoked with a location such as Fredericton, it tells the agent to pass that value as a tool argument, but nb_get_flood_hazard_areas accepts only a map-sheet identifier, nb_get_historical_floods has no location filter, and nb_get_wetlands accepts only class/status; those clients also omit geometry. The workflow therefore cannot associate the returned flood or wetland polygons with the requested location. Direct the agent through a geometry-enabled spatial query or stop presenting these calls as a location-specific assessment.

Useful? React with 👍 / 👎.

[mariomeyer]

Confirmed and fixed in 9c24fae. Verified the signatures — no flood tool accepts a place name:

nb_get_flood_hazard_areas : [sheet, limit, lang]
nb_get_historical_floods  : [event, limit, lang]
nb_get_wetlands           : [wetland_class, status, limit, lang]

The prompt now opens by stating plainly that no flood-layer tool accepts a place name, and routes location resolution through nb_get_civic_addresses first — which is only viable because an earlier fix in this PR (4964e77) added LATITUDE/LONGITUDE/COUNTY/PID to that projection. Corrected in both language variants; no location parameter was invented on the flood tools.

[chatgpt-codex-connector]

Include Socrata in the module discovery description

The module now has four upstream surfaces, including two tools for gnb.socrata.com, but this description still claims three and omits Socrata entirely. meta/list_modules.py returns MODULE_DESCRIPTION verbatim for source discovery, and the generated TOOLS catalogue repeats it, so agents consulting module metadata can miss the provincial Socrata catalogue even though its tools are registered. Update both the English and French descriptions.

Useful? React with 👍 / 👎.

[mariomeyer]

Confirmed and fixed in af307ec — and it was stale in a second way you did not flag.

Beyond omitting gnb.socrata.com, the description still advertised "minerals, parks" as curated GeoNB coverage. Both nb_get_mineral_occurrences and nb_get_provincial_parks were dropped to the long tail by the same Wave 0 checkpoint that added Socrata, so that half of the sentence was stale for exactly the same reason.

Now names four surfaces and states explicitly that minerals and provincial parks are reachable only through nb_query_geonb_layer, not a dedicated curated tool. MODULE_DESCRIPTION_FR updated to match and TOOLS.md regenerated.

[mariomeyer]

@codex one more pass please

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 90434c169b

ℹ️ 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".

[chatgpt-codex-connector]

Make the county parameter affect the report

When this prompt is called with a county such as York, the value is only echoed in the user message; the workflow then calls nb_get_crown_land, which accepts only holder and returns no geometry, followed by province-wide mineral/forestry layer queries with no county or spatial filter. The resulting report therefore cannot be scoped to the requested county, so add an executable county-to-spatial-filter path or remove the county-specific framing.

Useful? React with 👍 / 👎.

[mariomeyer]

Confirmed and fixed in 533c69f.

Checked the live layer schemas first, because your comment offered two remedies and the data rules one of them out:

Crown Land layer 3 : OBJECTID, Shape, HOLDER, Shape_Length, Shape_Area
MineralOccurrences : Shape, URN, LAT, LON, NAME, COMMODITIE, MIN_OCCR_U, OBJECTID

Neither layer has a county attribute, so an "executable county-to-spatial-filter path" would mean adding spatial-query support — a feature, not a fix. But nb_get_parcels does take county, so rather than deleting the parameter I made it do real work.

The prompt now opens by stating plainly which layers can and cannot be scoped by county, adds a Step 1 that calls nb_get_parcels(county=) as the actual county-scoped step, and labels the Crown-land and mineral steps province-wide so the report cannot imply a scoping it did not perform. Both language variants updated; steps renumbered.

Worth noting: this is the same defect class as the flood-prompt finding you raised last round, and I fixed that one instance without sweeping for siblings — which is why this one survived. So this time I swept all six prompts for parameters that are echoed but not consumable by any named tool. Three surfaced: this one (real), the flood prompt (already fixed last round), and nb_property_lookup's civic_address — which turned out to be a false positive, since Step 2 already instructs decomposing it into nb_get_civic_addresses(community=, street=). All three now verified to route to a consuming tool in both en and fr.

Gates: 3578 passed, coverage 97.39%, ruff clean, pyright 0 errors, catalogue up to date.