fix(mcp): honest list filters/contract + meta.total guard + page-size cap + stop leaking errors + polish - #276
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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: 6d32bbda48
ℹ️ 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".
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…arden status derivation Address PR #276 review: - isProvided() now treats an empty array/plain object as not-provided, so a raw `{ filters: {} }` pass-through no longer marks list_tracking_requests as filtered (which falsely trusted the firehose meta.total). - buildListContract presentation guidance no longer claims "single result" when the list is empty; it points agents at the empty_state hint instead. - determineContainerStatus is defensive against missing/oddly-typed `attrs` from the lightweight /search payload (an unverified shape) and documents that `unknown` is the honest fallback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Triaged the automated review (Codex + Greptile) and pushed Addressed:
Intentionally skipped: none — the two distinct bot findings (Greptile P1 and Codex P2 are the same empty-object issue) and the residual are all addressed. Gate green from the worktree: SDK build/type-check/test (51 pass, 2 skip), MCP build/type-check/test (88 pass). No SDK public-surface change, so no docs regeneration. oxfmt run only on the three changed files. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c46eed1579
ℹ️ 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".
…s filters The list_tracking_requests tool exposes a raw `filters` pass-through that the SDK copies verbatim into the query string. A caller could smuggle `page[size]`/`page[number]` through it and bypass the MAX_LIST_PAGE_SIZE cap that the dedicated `page`/`page_size` schema enforces. Strip those keys before building the SDK filter object so pagination is owned exclusively by the capped schema. Adds a regression test. Addresses Codex review comment on PR #276. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Triaged the automated review comments (Codex + Greptile) against the current tip ( Addressed (1):
Skipped — already fixed in
Known residual (already resolved at tip):
Green gate from a fresh worktree off the PR tip: SDK build/type-check/test (51 pass, 2 skip) and MCP build/type-check/test (89 pass) all green. No SDK public surface changed, so no docs regen. Formatting kept to the changed logical lines only (oxfmt is not clean on this file on main, so I avoided mass reformat churn). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4c5096afd
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d3f902b05
ℹ️ 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".
| > = { | ||
| container: ['status', 'port', 'carrier', 'updated_after'], | ||
| shipment: ['status', 'port', 'carrier', 'updated_after'], | ||
| tracking_request: ['status', 'request_type', 'filters'], |
There was a problem hiding this comment.
In a list_tracking_requests({ request_type: 'container' }) call, this new supported-filter vocabulary makes buildListContract report applied filters and trust meta.total. The OpenAPI source of truth for GET /tracking_requests does not define filter[request_type], while executeListTrackingRequests just forwards that unsupported key, so a successful response can still be unscoped and agents may present an account-wide list as request-type filtered. Either map this to a real API filter or report it as dropped.
Useful? React with 👍 / 👎.
… cap + stop leaking errors + polish Make the list_* tools honest about what they can answer and stop leaking internal error detail to MCP clients. List contract (DEV-10658, DEV-10665): - buildListContract is now filter-aware. An unfiltered firehose no longer claims can_answer:["which records match filters"]; instead it tells the agent it needs a scoping filter. A filtered call reports "which records match the applied filters". - Echo dropped/unsupported filters: prefers the SDK's unsupportedFilters when present, otherwise derives them from the per-entity supported vocabulary (status/port/carrier/updated_after; tracking_request: status/request_type/ filters) so the agent is never told a phantom filter applied. - meta.total honesty: an unfiltered total above a plausibility threshold (admin-token firehose) is flagged total_is_reliable:false and the agent is warned not to quote it as the filtered worklist size. - Slim the repeated ~2KB column_catalog off every list response. It moved to a one-time MCP resource (terminal49://docs/list-display-columns); contracts now reference it via display.column_catalog_resource. - Enforce a page_size cap (clamp to 100) at the MCP Zod layer. Stop leaking errors (DEV-10663): - Tool error path returns a generic message and logs the real error to stderr. - api/mcp.ts 500 path no longer returns error.data = err.message. Polish (DEV-10665): - get_supported_shipping_lines hides the T49 Test Carrier (scac TEST). - search_container derives a real status instead of blindly returning "unknown", and flags duplicate container numbers (duplicate_number) so the agent can disambiguate same-number results. - query-guidance: removed phantom filters (status=discharged, has_hold) and the non-functional demurrage.pickup_lfd sort path; aligned to the real filter vocabulary and explained client-side derivation/sorting. - Removed dead *Tool export objects in search-container and get-supported-shipping-lines. Green gate: SDK 51 pass/2 skip; MCP 86 pass (77 baseline + 9 new TDD tests). Closes DEV-10658 Closes DEV-10663 Closes DEV-10665 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…arden status derivation Address PR #276 review: - isProvided() now treats an empty array/plain object as not-provided, so a raw `{ filters: {} }` pass-through no longer marks list_tracking_requests as filtered (which falsely trusted the firehose meta.total). - buildListContract presentation guidance no longer claims "single result" when the list is empty; it points agents at the empty_state hint instead. - determineContainerStatus is defensive against missing/oddly-typed `attrs` from the lightweight /search payload (an unverified shape) and documents that `unknown` is the honest fallback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s filters The list_tracking_requests tool exposes a raw `filters` pass-through that the SDK copies verbatim into the query string. A caller could smuggle `page[size]`/`page[number]` through it and bypass the MAX_LIST_PAGE_SIZE cap that the dedicated `page`/`page_size` schema enforces. Strip those keys before building the SDK filter object so pagination is owned exclusively by the capped schema. Adds a regression test. Addresses Codex review comment on PR #276. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
executeListTrackingRequests strips raw page[size]/page[number] from the
nested `filters` bag before the SDK call, but buildListContract was still
fed the original args. A `list_tracking_requests({ filters: { 'page[size]':
'10000' } })` request is unfiltered after sanitization, yet the contract saw
a non-empty `filters` arg, reported applied filters, and flagged the
account-wide meta.total as reliable — letting agents present an unscoped
firehose as a scoped worklist.
Sanitize the filters context (same page-key stripping) before building the
contract so it reflects the request the API actually saw. An emptied
`filters` bag is treated as unprovided by isProvided; genuine filters remain
scoped. Adds contract tests for both cases.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two follow-up Greptile P2 findings on the list-contract-honesty work:
- request_type has no filter[request_type] in the GET /tracking_requests
OpenAPI source of truth, so buildListContract no longer treats it as a
scoping filter (it now falls through to dropped/unsupported filters
instead of falsely marking the list as filtered and meta.total reliable).
- The raw `filters` pass-through bag is only treated as applied when it
contains a real filter[...] key; a bag of only non-filter knobs (e.g.
`{ include: 'tracked_object' }`) no longer counts as scoping the list.
Adds contract tests covering both cases.
1d3f902 to
509f820
Compare
|
Rebased onto current Fixed
Already resolved on this branch (verified, no action needed): Green gate (after
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 509f820d09
ℹ️ 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".
| container: ['status', 'port', 'carrier', 'updated_after'], | ||
| shipment: ['status', 'port', 'carrier', 'updated_after'], |
There was a problem hiding this comment.
When list_containers or list_shipments is called with status, port, carrier, or updated_after, these entries make appliedFilterKeys treat the response as filtered and total_is_reliable as true. I checked the SDK query builder, and it explicitly reports status/port/carrier/updatedAfter as unsupported for /containers and /shipments instead of forwarding them, so this scenario returns an unfiltered list while the contract tells agents they can answer “which records match the applied filters” and safely quote meta.total.
Useful? React with 👍 / 👎.
1 participant
Summary
Makes the MCP
list_*tools honest about what they can answer, stops leaking internal error detail to clients, and polishessearch_container/get_supported_shipping_lines/ the query-guidance resource. AI-drafted PR for human review.List contract honesty
buildListContractis now filter-aware. An unfiltered firehose no longer claimscan_answer:["which records match filters"]; it tells the agent it needs a scoping filter. A filtered call reports"which records match the applied filters".unsupportedFilterswhen present, otherwise derives them from the per-entity supported vocabulary (status/port/carrier/updated_after; tracking_request:status/request_type/filters) so the agent is never told a phantom filter applied.meta.totalhonesty: an unfiltered total above a plausibility threshold (admin-token firehose) is flaggedtotal_is_reliable:falseand the agent is warned not to quote it as the filtered worklist size.column_catalogoff every list response — it moved to a one-time MCP resource (terminal49://docs/list-display-columns); contracts reference it viadisplay.column_catalog_resource.page_sizecap (clamp to 100) at the MCP Zod layer.Stop leaking errors
api/mcp.ts500 path no longer returnserror.data = err.message.Polish
get_supported_shipping_lineshides the T49 Test Carrier (scac: TEST).search_containerderives a real status instead of blindly returning"unknown", and flags duplicate container numbers (duplicate_number) so the agent can disambiguate same-number results.status=discharged,has_hold) and the non-functionaldemurrage.pickup_lfdsort path; aligned to the real filter vocabulary and explained client-side derivation/sorting.*Toolexport objects insearch-containerandget-supported-shipping-lines.Issues
Closes DEV-10658
Closes DEV-10663
Closes DEV-10665
Green gate
npm run build --workspace @terminal49/sdk— passnpm run build --workspace @terminal49/mcp— passnpm run type-check --workspace @terminal49/sdk— passnpm run type-check --workspace @terminal49/mcp— passnpm test --workspace @terminal49/sdk -- --run— 51 pass / 2 skipnpm run test --workspace @terminal49/mcp -- --run— 86 pass (77 baseline + 9 new TDD tests)TDD: the 9 new tests were written failing first (in
contracts.test.ts), then implemented to green.🤖 Generated with Claude Code
Greptile Summary
This PR makes the MCP
list_*tool contracts honest about filter scope, stops error detail from leaking to clients, and polishes several tools (search_container,get_supported_shipping_lines) and the query-guidance resource.buildListContractnow derivesapplied/droppedfilter keys, flags an implausibly large unfilteredmeta.totalas unreliable, enforces a 100-rowpage_sizecap via Zod transform, and moves the ~2KB column catalog to a one-time MCP resource.wrapToolWithContractnow catches all tool errors and returns a generic message;api/mcp.ts500 handler dropsdata: err.messagefrom the JSON-RPC response body.search_containerderives a real container status from timestamp/availability signals and flags duplicate container numbers;get_supported_shipping_lineshides the T49 Test Carrier; query-guidance removes phantom filters and corrects field names.Confidence Score: 3/5
The changes are broadly correct and well-tested, but a gap in
isProvidedcauseslist_tracking_requests({ filters: {} })to report an incorrectly filtered contract, the opposite of what this PR aims to guarantee.The
isProvidedhelper treats an empty plain object as a provided value. Forlist_tracking_requests,'filters'is in the supported-filter vocabulary, so passingfilters: {}causesisFiltered = true, the contract claims 'which records match the applied filters', andtotalIsReliableis forced true even for a firehose total. This directly undermines the PR's core goal. All other changes (error masking, column-catalog extraction, status derivation, duplicate-number flagging, TEST carrier hiding) look correct and are covered by the new TDD tests.packages/mcp/src/server.ts (isProvided and buildListContract logic)
Important Files Changed
buildListContractwith filter-honesty signals, page-size cap, and generic tool-error wrapping.isProvideddoesn't handle empty objects, causing a false filtered signal whenlist_tracking_requestsis called withfilters: {}; also minorpresentationGuidancetext issue for zero-result pages.determineContainerStatusandflagDuplicateContainerNumbers; both are correct. Minor:destinationfield reusesport_of_discharge_nameinstead of an inland-destination field, which misrepresents containers on rail.data: err.messagefrom 500 response to stop leaking internal error detail. Correct and straightforward.filters: {}false-positive case is not covered.isHiddenCarrierto filter out the T49 Test Carrier by SCAC and name with correct case-insensitive comparison.list-display-columnsresource and non-leaking error message. No logic concerns.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Agent calls list_* tool] --> B[Zod validates & clamps page_size <= 100] B --> C[executeList* fetches from Terminal49 API] C --> D{Error?} D -- Yes --> E[wrapToolWithContract catches\nLogs real error to stderr\nReturns generic message to agent] D -- No --> F[buildListContract] F --> G{appliedFilterKeys} G -- filters provided --> H[isFiltered = true] G -- no filters --> I[isFiltered = false\nrequires_more_data: needs filter] F --> J{meta.total check} J -- unfiltered & total > 1000 --> K[total_is_reliable = false] J -- filtered OR total <= 1000 --> L[total_is_reliable = true] F --> M{dropped filters?} M -- unsupported --> N[dropped_filters echoed] F --> O[ResponseContract attached] O --> P[Agent receives honest contract]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[Agent calls list_* tool] --> B[Zod validates & clamps page_size <= 100] B --> C[executeList* fetches from Terminal49 API] C --> D{Error?} D -- Yes --> E[wrapToolWithContract catches\nLogs real error to stderr\nReturns generic message to agent] D -- No --> F[buildListContract] F --> G{appliedFilterKeys} G -- filters provided --> H[isFiltered = true] G -- no filters --> I[isFiltered = false\nrequires_more_data: needs filter] F --> J{meta.total check} J -- unfiltered & total > 1000 --> K[total_is_reliable = false] J -- filtered OR total <= 1000 --> L[total_is_reliable = true] F --> M{dropped filters?} M -- unsupported --> N[dropped_filters echoed] F --> O[ResponseContract attached] O --> P[Agent receives honest contract]Comments Outside Diff (1)
packages/mcp/src/tools/search-container.ts, line 212-214 (link)pod_terminalanddestinationare set toattrs.port_of_discharge_name. For sea-only containers this is a reasonable proxy, but for inland/rail containers the POD is a transshipment point, not the final destination. The fielddestinationis silently wrong for those records — agents reading it will report the sea-port, not the inland ramp. Consider usingattrs.destination_nameorattrs.final_destination_namefordestination, falling back to POD only when no inland destination is available.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(mcp): honest list filters/contract +..." | Re-trigger Greptile