Skip to content

fix: preserve ignored-filter diagnostics and reject unscoped MCP results - #484

Merged
alosec merged 3 commits into
mainfrom
fix/container-ignored-filters-mcp
Oct 22, 2026
Merged

alosec merged 3 commits into
mainfrom
fix/container-ignored-filters-mcp

Conversation

@alosec

@alosec alosec commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Declare optional container summary meta.ignored_filters in OpenAPI and regenerate SDK types.
  • Preserve diagnostics in mapped SDK summary responses; raw responses and list metadata retain them.
  • Make list_containers and summarize_containers return an actionable error before presenting results when the API reports ignored filters. Do not silently drop requested constraints.
  • Continue accepting responses with absent or empty diagnostics for compatibility. Public API status behavior is unchanged.

Validation

  • SDK: 427 tests pass (one test file skipped).
  • MCP: 747 tests pass, including real-SDK preservation/refusal regressions.
  • SDK and MCP builds pass.
  • Both package lint/format checks pass with existing warnings, zero errors.

No deployment performed. Backend diagnostics must be available for the guard to detect ignored filters.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

RetriggerView in GreptileConfidence Score: 4/5

Fix the lost correction message before merging so MCP callers can recover from ignored filters.

Fix All in CodexFindings

  1. P1 Filter errors lose their explanation ▶
Fix with agent prompt
### Issue 1
packages/mcp/src/tools/ignored-filters.ts:13-14
When the API returns non-empty `ignored_filters`, both container tools throw a plain `Error`. The shared `formatToolError` replaces its message with “The Terminal49 request could not be completed. Contact support if the problem persists.” Results are withheld, but MCP callers never learn which filters failed or how to correct them.

Use a dedicated error that the shared boundary can safely turn into an actionable response. Add a test through the MCP server; the current tests call the handlers directly and miss the replaced message.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

This PR preserves ignored_filters in SDK summary responses and makes the two MCP container tools withhold results when filters were ignored.

  • Mapped container summaries carry the API’s skipped-filter diagnostics.
  • MCP container tools stop when the API skips requested filters.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  API[API response] --> SDK[SDK preserves ignored_filters]
  SDK --> Guard{Ignored filters reported?}
  Guard -->|No| Result[Return rows or counts]
  Guard -->|Yes| Error[Throw plain Error]
  Error --> Boundary[Shared MCP error code]
  Boundary --> Generic[Generic failure replaces correction message]
Loading

Reviews (1) · Last reviewed commit: "fix(mcp): withhold container results whe..." · Reviewed by Greptile

@vercel

vercel Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
api Ready Ready Preview Oct 22, 2026 5:39pm UTC

Request Review

@mintlify

mintlify Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated
terminal49 🟢 Ready View Preview Oct 22, 2026, 4:14 PM

@github-actions github-actions Bot added risk:high Advisory path risk; current-head human review needed risk-shadow:high Shadow-only risk signal: high labels Oct 22, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Shadow risk calibration

Signal Result Authority
Path classifier risk:high Deterministic advisory policy
heuristic fallback risk-shadow:high Shadow only

Calibration result: agreement.

Shadow risk never blocks or enables merge. Keep it in observation mode until roughly 50 pull requests have human-reviewed labels, then calibrate before considering any policy use.

Comment thread packages/mcp/src/tools/ignored-filters.ts Outdated
@alosec

alosec commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

CI update: SDK/MCP suites, monorepo checks and all six local protocol versions pass. The 2026-07-28 preview evaluation failed during fixture discovery: list_shipments received an upstream HTTP 401 before the container-result tests ran. The other preview protocol check passed. Retrying the failed job once to distinguish a transient auth/availability issue; no credential or environment changes made.

@maurycy

maurycy commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

One follow-up on the MCP error response: assertNoIgnoredFilters correctly withholds results, but it throws a plain Error. The shared formatToolError then replaces the useful filter names and correction guidance with the generic "The Terminal49 request could not be completed. Contact support if the problem persists." The agent therefore cannot tell which constraint needs correcting.

Please use a controlled error type that the server can safely expose, such as the existing ToolInputError convention, or explicitly handle an ignored-filter error in formatToolError. Keep the generic handling for unexpected errors.

Add a test through the MCP server for both list_containers and summarize_containers, asserting isError: true, no result payload, and a message identifying the ignored filters with correction guidance. The current tests call the handlers directly, so they miss the message replacement at the server boundary.

Verified against 8a8d6caea74ecd442638ccfffd4b838313f80efb: guard, server error formatting.

@alosec

alosec commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the error-boundary gap in the latest commit. Nonempty diagnostics now raise ToolInputError with controlled correction guidance. Only bounded filter identifiers are accepted for that message; malformed diagnostics remain generic and unexpected-error sanitization is unchanged.

Added real MCP client/server transport tests for both list_containers and summarize_containers: isError true, no structured results, ignored names/correction guidance retained, and no item/group payload or applied_filters leaked. Added a malformed-diagnostics boundary regression too. All 750 MCP tests pass; build and package lint/format pass (existing warnings, zero errors). This complements the existing real-SDK mapping tests.

@github-actions github-actions Bot added risk:unknown Advisory evidence unavailable or changed; no merge permission risk:high Advisory path risk; current-head human review needed and removed risk:high Advisory path risk; current-head human review needed risk:unknown Advisory evidence unavailable or changed; no merge permission labels Oct 22, 2026

@SylvainGravejat SylvainGravejat left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Analysis done with Claude)

LGTM. One optional improvement: malformed ignored_filters diagnostics fall back to "contact support". Since the keys echo the caller's own request, a custom field slug with a space would get a generic error. A message like "some requested filters were not applied", without echoing the keys, would still tell the caller what to fix.

@alosec
alosec merged commit 352b9e6 into main Oct 22, 2026
30 of 31 checks passed
@alosec
alosec deleted the fix/container-ignored-filters-mcp branch October 22, 2026 20:05
@github-actions github-actions Bot mentioned this pull request Oct 19, 2026

This branch was successfully deployed

2 active (1 outdated) deployments
Preview — 1ddb7158 Deployed Oct 22, 2026 by vercel[bot]
staging - docs — 8a8d6cae Deployed Oct 22, 2026 by mintlify[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:high Advisory path risk; current-head human review needed risk-shadow:high Shadow-only risk signal: high

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants