Skip to content

feat(discovery): surface quarantined tools in retrieve_tools (locked, name-only)#778

Merged
Dumbris merged 2 commits into
smart-mcp-proxy:mainfrom
HaloCollar:upstream-pr/discover-quarantined-tools
Jun 28, 2026
Merged

feat(discovery): surface quarantined tools in retrieve_tools (locked, name-only)#778
Dumbris merged 2 commits into
smart-mcp-proxy:mainfrom
HaloCollar:upstream-pr/discover-quarantined-tools

Conversation

@electrolobzik

Copy link
Copy Markdown
Contributor

Problem

Quarantined tools — both server-level quarantine and tool-level pending/changed approvals — are intentionally excluded from the BM25 search index so their untrusted descriptions can't expose a Tool Poisoning Attack (TPA) payload to the agent. That's a correct defense, but a side effect is that retrieve_tools can only answer "no such tool" for a capability a quarantined server provides — even when the right remediation is "ask the user to approve it." The agent never learns the capability exists.

Change

Adds an opt-in second pass to retrieve_tools(include_disabled=true) that enumerates quarantined tools from authoritative state and returns name-only locked entries (description and schema withheld) with a status + remediation:

  • server-level quarantine → new status server_quarantined
  • tool-level pending/changed → existing pending_approval

The pass is self-contained — it does not alter the shared callability/classification helpers, so visible-tool counts and other consumers are unchanged. It:

  • matches the query against the tool name only (quarantined tools aren't in the index, so they can't be BM25-ranked); short keywords (≥2 chars, e.g. ui/qa) are retained;
  • reuses the callable path's agent-scope + profile filtering via a shared serverDiscoverable helper (collapses what were duplicated inline copies for the discovery surface);
  • dedups against tools the index loop already handled (a seen set), so the brief post-quarantine reindex window can't list a tool as both callable and locked, nor double-count it in the zero-result nudge;
  • skips tools also denied by operator config (enabled_tools/disabled_tools), which approval cannot unlock — so the agent isn't sent down a dead-end remediation;
  • prepends its matches so the shared min(limit,10) cap can't truncate them in favor of index hits.

Quarantined tools remain non-callable: the call path already blocks server-quarantine and pending/changed before execution, so discovery only makes them visible, never callable.

Tests

internal/server/mcp_quarantine_discovery_test.go: pending tool surfaced by name with withheld description + remediation; server-level branch (tool-names source injected for unit testing); query-scoped exclusion; config-denied skipped; dedup against seen; short-keyword tokens; zero-result nudge; quarantined tool still blocked at the call path; remediation present.

go build ./..., go vet, and golangci-lint (repo .github/.golangci.yml) are clean; internal/{server,runtime,index,storage,contracts} suites pass (the binary-prereq E2E tier needs a pre-built binary and was not exercised).

🤖 Generated with Claude Code

… name-only)

Quarantined tools — both server-level quarantine and tool-level
pending/changed approvals — are intentionally absent from the BM25 search
index so their untrusted descriptions cannot expose a Tool Poisoning Attack
(TPA) payload to the agent. As a side effect, retrieve_tools could only answer
"no such tool" for a capability a quarantined server provides, even though the
correct remediation is "ask the user to approve it".

Add an opt-in second pass to retrieve_tools(include_disabled=true) that
enumerates quarantined tools from authoritative state and returns NAME-ONLY
locked entries (description and schema withheld) with a status + remediation:
  - server-level quarantine   -> new status `server_quarantined`
  - tool-level pending/changed -> existing `pending_approval`

The pass is fully self-contained — it does NOT alter the shared callability or
classification helpers, so visible-tool counts and other consumers are
unchanged. Specifically it:
  - matches the query against the tool NAME only (quarantined tools aren't in
    the index, so they can't be BM25-ranked); short keywords (>=2 chars, e.g.
    "ui"/"qa") are retained;
  - applies the same agent-scope + profile filtering as the callable path, via
    a shared `serverDiscoverable` helper (de-duplicates what were three inline
    copies down to one for the discovery surface);
  - dedups against tools the index loop already handled (a `seen` set), so the
    brief post-quarantine reindex window can't list a tool as both callable and
    locked, nor double-count it in the zero-result nudge;
  - skips tools also denied by operator config (enabled_tools/disabled_tools),
    which approval cannot unlock — so the agent isn't sent down a dead-end
    remediation;
  - prepends its matches so the shared min(limit,10) cap can't truncate them in
    favor of index hits.

Quarantined tools remain non-callable: the call path already blocks
server-quarantine and pending/changed before execution (handleQuarantinedToolCall),
so no change to isToolCallable is needed — discovery only makes them VISIBLE,
never callable.

Tests: pending tool surfaced by name with withheld description + remediation;
server-level branch (tool-names source injected); query-scoped exclusion;
config-denied skipped; dedup against seen; short-keyword tokens; zero-result
nudge; quarantined tool still blocked at the call path; remediation present.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@codecov-commenter

codecov-commenter commented Jun 27, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 80.82192% with 14 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/server/mcp.go 80.82% 7 Missing and 7 partials ⚠️

📢 Thoughts on this report? Let us know!

Dumbris added a commit that referenced this pull request Jun 28, 2026
…value taxonomy) (#779)

Aligns Spec 049 with PR #778, which adds a sixth disabled-tool status,
server_quarantined, surfaced by a dedicated quarantined-tool discovery pass
(quarantined tools are deliberately excluded from the search index as a TPA
defense). Spec 049 pinned the taxonomy to exactly five values and assumed all
locked tools live in the index, so #778's behavior was correct but undocumented.

- FR-004: five -> six values; server_quarantined assigned by the discovery pass
  (not the classifier), name-only, description/schema withheld; config-denied
  tools skipped by the pass.
- FR-003: note the name-only exception for quarantined entries.
- Assumptions: quarantined tools are excluded from the index and enumerated from
  authoritative quarantine state.
- contracts/mcp-deltas.md: add server_quarantined to the status enum + example
  response shape and remediation.
- design doc taxonomy: five -> six, with the server_quarantined explanation.

Related #778

Co-authored-by: Algis Dumbris <gordon.greatests@gmail.com>
@Dumbris

Dumbris commented Jun 28, 2026

Copy link
Copy Markdown
Member

Thank you, @electrolobzik — this is a genuinely well-crafted contribution. 🙏

A few things I appreciated reviewing it:

  • Security-conscious by design: surfacing quarantined tools as name-only locked entries (description/schema withheld) is exactly right — it keeps a quarantined server's untrusted tool text out of the model's context, so discovery doesn't become a Tool Poisoning Attack vector. Prepending them ahead of the index-derived entries so the result cap can't silently drop them is a nice touch.
  • Faithful to the model: reusing pending_approval for tool-level pending/changed on trusted servers, skipping config-denied tools (approval couldn't make them callable), and deduping against seen all match how the rest of the discovery path thinks.
  • Well-tested and cleanly factored (the serverDiscoverable helper dedups the inline scope filters).

One thing surfaced in review: the new server_quarantined status was correct behavior but sat outside Spec 049 FR-004, which pinned the taxonomy to five values (and assumed all locked tools live in the search index — deliberately not true for quarantined ones). Rather than ask you to round-trip a spec edit, I amended Spec 049 myself in #779 (now merged): FR-004 → six values, the name-only exception in FR-003, the index-exclusion carve-out in Assumptions, and the contracts/mcp-deltas.md enum/example. So the spec now matches your code, and this is good to merge. Thanks again for the careful work!

@mcpproxy-gatekeeper mcpproxy-gatekeeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Gatekeeper approval — Codex review verdict: ACCEPT.

This approval is posted automatically by the MCPProxy Gatekeeper App on behalf of the Codex reviewer (verdict of record lives in the Paperclip review thread). Author≠approver satisfied; QA + CI gates enforced separately.

Auto-approved per Model B (MCP-1249).

@Dumbris Dumbris merged commit 9702260 into smart-mcp-proxy:main Jun 28, 2026
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants