fix(mcp): harden tool output quality and coverage - #337
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const safeError = new Error( | ||
| 'The Terminal49 upstream request could not be completed.', | ||
| ); | ||
| const name = error instanceof Error ? error.name : 'Error'; | ||
| safeError.name = /^[A-Za-z][A-Za-z0-9_.:-]{0,63}$/.test(name) | ||
| ? name | ||
| : 'Error'; | ||
| Sentry.captureException(safeError); |
There was a problem hiding this comment.
captureMcpException replaces every caught tool exception with a new generic Error, so Sentry loses the original call-site stack and cause chain and groups distinct upstream failures at this sanitizer, making production incidents harder to diagnose.
Knowledge Base Used: MCP Server Core (@terminal49/mcp)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/mcp/src/sentry.ts
Line: 97-104
Comment:
**Generic errors discard diagnostics**
`captureMcpException` replaces every caught tool exception with a new generic `Error`, so Sentry loses the original call-site stack and cause chain and groups distinct upstream failures at this sanitizer, making production incidents harder to diagnose.
**Knowledge Base Used:** [MCP Server Core (`@terminal49/mcp`)](https://app.greptile.com/terminal49/-/custom-context/knowledge-base/terminal49/api/-/docs/mcp-server-core.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
![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: 80da54d283
ℹ️ 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".
| carrier: z.string().optional().describe('Filter by shipping line SCAC'), | ||
| updated_after: z | ||
| .string() | ||
| number: z.string().optional().describe('Filter by shipment number'), |
There was a problem hiding this comment.
When a user asks for a shipment using a container number, this description can lead the model to pass that identifier here and report an empty result. The OpenAPI source explicitly defines number as the original tracking request number (typically a master bill of lading or booking number) and states that it does not match container numbers (docs/openapi.json:604-605); describe that limitation and direct container-number lookups to search_container so valid shipments are not missed.
AGENTS.md reference: AGENTS.md:L23-L28
Useful? React with 👍 / 👎.
![vorflux[bot]](https://avatars.githubusercontent.com/in/2483280?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Reviewed — found 5 issues in the MCP tool-output hardening changes. I reviewed the tool response contracts, tracking-request error paths, Sentry exception handling, list-tool schemas, and corresponding public documentation.
Findings
packages/mcp/src/tools/track-container.ts
- The linked-but-not-readable response is classified as pending and contradicts its own metadata.
- All upstream
404errors are converted into successful invalid-input responses.
packages/mcp/src/sentry.ts
- Replacing the captured exception removes the original failure location and useful diagnostic context.
packages/mcp/src/server.ts
- The changed list-tool schemas leave the public MCP documentation advertising removed filters and stale defaults.
- Container-list contracts still recommend filters even though that endpoint now declares no supported filters.
Verdict
| return { | ||
| tracking_request_created: true, | ||
| infer_result: infer, | ||
| tracking_request: { | ||
| request_number: number, | ||
| number_type: inferredNumberType, | ||
| scac: requestedScac || heuristicScac, | ||
| container_id: containerId, | ||
| }, |
There was a problem hiding this comment.
This response explicitly contains tracking_request.container_id, so the request is already linked. Because it has no top-level id, buildTrackContract() classifies it as pending and emits that container linking is not immediate, contradicting both this payload and _metadata.presentation_guidance, which says it was created and linked. Preserve an explicit linked-but-details-unavailable state, return the linked ID in the shape the contract checks, or update buildTrackContract() to recognize tracking_request.container_id.
| if (isNotFound(error)) { | ||
| logMcpEvent({ | ||
| event: 'tracking_request.not_found', | ||
| number, | ||
| numberType: inferredNumberType, | ||
| scac: requestedScac || heuristicScac, | ||
| duration_ms: duration, | ||
| timestamp: new Date().toISOString(), | ||
| }); | ||
| return { | ||
| error: 'NotFound', | ||
| message: | ||
| 'No tracked container matched this number, and Terminal49 could not create a tracking request for it. Verify the number and carrier SCAC, then retry.', | ||
| tracking_request_created: false, | ||
| _metadata: { | ||
| presentation_guidance: | ||
| 'Clearly state that no tracking request was created. Ask the user to verify the identifier and carrier; do not imply that tracking is pending.', | ||
| recommendations: ['get_supported_shipping_lines', 'search_container'], | ||
| }, | ||
| }; |
There was a problem hiding this comment.
This converts every NotFoundError from createTrackingRequestFromInfer() or the direct-create fallback into a successful MCP result claiming the number/carrier could not be resolved. However, the OpenAPI contract documents invalid tracking inputs as 422; a 404 can instead mean the infer/create route is missing or the configured upstream base URL is wrong. In that deployment/configuration scenario, callers receive isError: false, are told to correct valid input, and the operational failure bypasses the outer error handler. Only translate a specifically identified domain “number not found” response here; otherwise rethrow the 404 as an upstream/tool error.
| const safeError = new Error( | ||
| 'The Terminal49 upstream request could not be completed.', | ||
| ); | ||
| const name = error instanceof Error ? error.name : 'Error'; | ||
| safeError.name = /^[A-Za-z][A-Za-z0-9_.:-]{0,63}$/.test(name) | ||
| ? name | ||
| : 'Error'; | ||
| Sentry.captureException(safeError); |
There was a problem hiding this comment.
Replacing every exception with a newly constructed Error discards the original stack, cause, HTTP status, and sanitized SDK details. The new stack always points to captureMcpException(), so failures with the same error name become effectively indistinguishable in Sentry. Since stderr logging also redacts message, operators no longer have enough context to identify the failing request path or cause. Redact sensitive fields in a Sentry beforeSend hook, or construct a sanitized exception that preserves safe stack frames and diagnostic tags such as status/tool/operation.
| inputSchema: z.object({ | ||
| status: z.string().optional().describe('Filter by shipment status'), | ||
| port: z.string().optional().describe('Filter by POD port LOCODE'), | ||
| carrier: z.string().optional().describe('Filter by shipping line SCAC'), | ||
| updated_after: z | ||
| .string() | ||
| number: z.string().optional().describe('Filter by shipment number'), | ||
| tracking_stopped: z | ||
| .boolean() | ||
| .optional() | ||
| .describe('Filter by updated_at (ISO8601) >= value'), | ||
| .describe('Filter by whether shipping-line tracking has stopped'), | ||
| include_containers: z | ||
| .boolean() | ||
| .optional() | ||
| .default(false) | ||
| .describe( | ||
| 'Include containers relationship in response. Default: true.', | ||
| 'Include container relationships in each shipment. Default: false to keep list responses compact.', |
There was a problem hiding this comment.
These input changes are not reflected in the published MCP documentation. docs/mcp/home.mdx and docs/api-docs/in-depth-guides/mcp.mdx still advertise status, port, carrier, and updated_after for both list tools and describe include_containers using the previous default. Users following those docs will pass fields that Zod strips before the handler, causing an unfiltered request without a dropped_filters warning. Update both documentation surfaces alongside this schema change, including the new number/tracking_stopped shipment filters and compact defaults.
| const filterGuidance = | ||
| supportedFilters.length > 0 | ||
| ? `a filter to scope this list (${supportedVocab})` | ||
| : 'server-side filters are not available for this list endpoint; use pagination and inspect returned rows'; |
There was a problem hiding this comment.
For list_containers, supportedFilters is now empty, but later branches in this same contract still instruct the model to apply a filter before quoting a total and to try alternative filters or tighter date ranges when the page is empty. Those actions are impossible on this endpoint and contradict this new guidance. Condition those messages on supportedFilters.length > 0; for container lists, direct the model to paginate and qualify conclusions as page-local instead.
Co-authored-by: Akshay Dodeja <dodeja@gmail.com>
Co-authored-by: Akshay Dodeja <dodeja@gmail.com>
Co-authored-by: Akshay Dodeja <dodeja@gmail.com>
Co-authored-by: Akshay Dodeja <dodeja@gmail.com>
Co-authored-by: Akshay Dodeja <dodeja@gmail.com>
Co-authored-by: Akshay Dodeja <dodeja@gmail.com>
Co-authored-by: Akshay Dodeja <dodeja@gmail.com>
Co-authored-by: Akshay Dodeja <dodeja@gmail.com>
80da54d to
c285883
Compare
2 participants
Summary
m/macarrier completions and full tool/prompt/resource surfaces on protocols2026-07-28and2025-11-25track_containeroutcomes without leaking internals: no request created, request created but details pending, and existing match temporarily unavailablenumberandtracking_stopped; container lists now state that server-side operational filters are unavailableMeasurement
list_shipmentsno longer includes nested containers by default; callers can opt in withinclude_containers: trueformat: both, avoiding construction of a discarded mapped payload_response_contractVerification
npm run test --workspace @terminal49/mcp -- --run— 15 files, 212 tests passednpm run build --workspace @terminal49/mcp— passednpm run lint --workspace @terminal49/mcp— passed under pinned Node 24.4.1npx tsc --noEmit -p tsconfig.json— API gateway typecheck passedgit diff --check origin/cursor/fix-list-output-schema-0c41...HEAD— passedLeft unchanged
unsupportedFiltersschema correction forlist_containersandlist_shipments; this PR does not duplicate itRequirements verified against
readOnlyHint,destructiveHint, andopenWorldHint; five positive and three negative casesNeed help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Greptile Summary
The PR hardens the MCP tool surface by correcting advertised list filters, bounding default list sizes, reducing route and relationship payloads, clarifying tracking outcomes, sanitizing reported errors, and adding transport-level contract coverage.
Confidence Score: 4/5
The PR appears safe to merge, with a non-blocking observability issue in how sanitized exceptions are reported to Sentry.
Runtime contracts and list behavior are consistent with the SDK paths examined, but replacing each reported exception with a new generic Error removes the original stack and cause needed for effective incident diagnosis.
Files Needing Attention: packages/mcp/src/sentry.ts
Important Files Changed
Sequence Diagram
sequenceDiagram participant C as MCP Client participant S as MCP Server participant K as Terminal49 SDK participant A as Terminal49 API participant E as Sentry C->>S: Call public tool S->>K: Invoke SDK operation K->>A: API request alt Successful response A-->>K: Raw or mapped data K-->>S: Curated result S-->>C: Structured content and response contract else Tool exception A-->>K: Error K-->>S: Exception S->>E: Generic replacement Error S-->>C: Redacted tool error endPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(mcp): advertise only supported list ..." | Re-trigger Greptile
Context used:
@terminal49/mcp)