fix(mcp): advertise only supported list filters - #338
Conversation
Co-authored-by: Akshay Dodeja <dodeja@gmail.com>
Co-authored-by: Akshay Dodeja <dodeja@gmail.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: Akshay Dodeja <dodeja@gmail.com>
![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: fd25778d09
ℹ️ 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".
| request_type: z | ||
| .string() | ||
| .enum(['bill_of_lading', 'booking_number', 'container']) | ||
| .optional() | ||
| .describe('Filter by request type (mapped to filter[request_type])'), | ||
| .describe( | ||
| 'Identifier type: bill_of_lading, booking_number, or container. Maps to filter[request_type].', |
There was a problem hiding this comment.
When callers pass request_type, executeListTrackingRequests forwards filter[request_type], but the /tracking_requests operation in docs/openapi.json does not define that query parameter. The API can therefore ignore it while SUPPORTED_LIST_FILTERS_BY_ENTITY marks the result as filtered and buildListContract reports its total as reliable, causing an account-wide page to be presented as matching the requested identifier type. Remove this advertised input or add actual API support before treating it as applied.
AGENTS.md reference: AGENTS.md:L27-L27
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| unknown: ['status', 'port', 'carrier', 'updated_after'], | ||
| container: [], | ||
| shipment: ['number', 'tracking_stopped'], | ||
| tracking_request: ['status', 'request_type', 'filters'], |
There was a problem hiding this comment.
When list_tracking_requests receives request_type, the changed support list classifies it as an applied filter even though GET /tracking_requests does not define filter[request_type]. The response contract consequently presents an account-wide page as matching the requested type and marks its total as reliable.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/mcp/src/server.ts
Line: 161
Comment:
**Unsupported request type scoping**
When `list_tracking_requests` receives `request_type`, the changed support list classifies it as an applied filter even though `GET /tracking_requests` does not define `filter[request_type]`. The response contract consequently presents an account-wide page as matching the requested type and marks its total as reliable.
**Knowledge Base Used:**
- [MCP Server Core (`@terminal49/mcp`)](https://app.greptile.com/terminal49/-/custom-context/knowledge-base/terminal49/api/-/docs/mcp-server-core.md)
- [MCP Tools](https://app.greptile.com/terminal49/-/custom-context/knowledge-base/terminal49/api/-/docs/mcp-tools.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| .describe( | ||
| 'Filter by tracking state: true means Terminal49 stopped polling the shipping line; false means tracking is active. Maps to filter[tracking_stopped].', | ||
| ), | ||
| include: z | ||
| .string() | ||
| .optional() | ||
| .describe('Filter by updated_at (ISO8601) >= value'), | ||
| .describe( | ||
| 'Comma-separated relationships to side-load: containers, pod_terminal, port_of_lading, port_of_discharge, destination, destination_terminal. POL is port of lading (origin); POD is port of discharge (destination). Include changes row shape, not result scope.', | ||
| ), |
There was a problem hiding this comment.
When callers provide include, the SDK lets it override include_containers: omitting containers from the string removes them despite the documented true default, while explicitly including them defeats include_containers: false. Clients therefore receive shipment rows whose related records contradict the requested option.
Knowledge Base Used: MCP Tools
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/mcp/src/server.ts
Line: 1624-1632
Comment:
**Conflicting shipment include controls**
When callers provide `include`, the SDK lets it override `include_containers`: omitting `containers` from the string removes them despite the documented true default, while explicitly including them defeats `include_containers: false`. Clients therefore receive shipment rows whose related records contradict the requested option.
**Knowledge Base Used:** [MCP Tools](https://app.greptile.com/terminal49/-/custom-context/knowledge-base/terminal49/api/-/docs/mcp-tools.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.![vorflux[bot]](https://avatars.githubusercontent.com/in/2483280?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Reviewed — found 2 issues in the MCP list-filter schema and response-contract changes. The review focused on whether every advertised filter is supported by the API and whether removed arguments fail safely for existing MCP clients.
Findings
packages/mcp/src/server.ts
request_typeis advertised as a real tracking-request filter even thoughGET /tracking_requestsdoes not definefilter[request_type].- Removed list-filter arguments are silently stripped by permissive
z.object()schemas, allowing obsolete calls to return unscoped data without a warning.
Verdict
| unknown: ['status', 'port', 'carrier', 'updated_after'], | ||
| container: [], | ||
| shipment: ['number', 'tracking_stopped'], | ||
| tracking_request: ['status', 'request_type', 'filters'], |
There was a problem hiding this comment.
Issue: request_type is not a supported GET /tracking_requests filter in the OpenAPI source of truth, but adding it here causes buildListContract() to classify the request as scoped. executeListTrackingRequests() also forwards filter[request_type]; if the API ignores that parameter, mixed request types can be returned while the contract says the applied filter matched and treats meta.total as reliable. Please remove request_type from the typed schema, this supported-filter map, and REAL_TRACKING_REQUEST_FILTER_KEYS, or first add and verify backend/OpenAPI support with an integration test.
| destructiveHint: false, | ||
| openWorldHint: false, | ||
| }, | ||
| inputSchema: z.object({ |
There was a problem hiding this comment.
Issue: This permissive z.object() silently strips the removed status, port, carrier, and updated_after properties; the same applies to list_containers at line 1674. Existing or schema-cached clients can therefore send an obsolete filter and receive an unscoped page with an empty unsupportedFilters array instead of an error or warning. Please make these input objects strict, or temporarily retain deprecated fields and report them as unsupported; the public MCP references that still advertise these arguments should be updated in the same change.
2 participants
Closed as a duplicate of #337, which already advertises only API-supported list filters plus the broader quality pass.
Do not merge this branch. Greptile flagged request_type scoping and conflicting shipment include controls (3/5). Keep the query-guidance honesty that landed here only if #337 is missing the glossary (SCAC / UN/LOCODE / unscoped list); otherwise #337 is the one cut.