|
| 1 | +# Agent-Discoverable Disabled Tools — Design |
| 2 | + |
| 3 | +- **Date:** 2026-05-18 |
| 4 | +- **Status:** Approved (brainstorming), pending implementation plan |
| 5 | +- **Author:** Claude (brainstormed with Algis) |
| 6 | +- **Builds on:** PR #468 (`feat/config-tool-allowlist` — layered config tool filter, introduces `config_denied` and `Runtime.IsToolConfigDenied`) |
| 7 | +- **Delivery:** Standalone follow-up PR. The four UX fixes (#1–#4) from the #468 review land in #468 itself; this design is everything else. |
| 8 | + |
| 9 | +## 1. Problem |
| 10 | + |
| 11 | +`mcpproxy` exposes a curated, filtered tool surface to agents. Today `retrieve_tools` |
| 12 | +runs `isToolCallable()` as a hard post-search filter: any disabled or config-denied |
| 13 | +tool is dropped from results and treated as non-existent. `upstream_servers` only |
| 14 | +counts callable tools. |
| 15 | + |
| 16 | +Consequence: when a capability an agent needs to complete a task exists on an |
| 17 | +upstream but is locked (by operator config, by the user, by quarantine, or because |
| 18 | +the server is off), the agent gets **zero signal**. It cannot tell the user |
| 19 | +"the `delete_repo` tool exists but is disabled — enable it to proceed", and it |
| 20 | +cannot distinguish a lock the user can lift (UI toggle) from operator policy the |
| 21 | +user *cannot* lift (mcp_config.json), so any suggestion it makes is a guess. |
| 22 | + |
| 23 | +## 2. Goal & non-goals |
| 24 | + |
| 25 | +**Goal:** Let an agent, on demand, discover that a relevant capability exists but |
| 26 | +is locked, learn *why* in a machine-branchable way, and relay the *correct* |
| 27 | +remediation to the user/operator — at near-zero token cost when the feature is |
| 28 | +not exercised. |
| 29 | + |
| 30 | +**Non-goals:** |
| 31 | + |
| 32 | +- No change to enforcement. A discovered locked tool remains non-callable. |
| 33 | + `isToolCallable()` is untouched; this is a discovery/observability layer only. |
| 34 | +- No change to default `retrieve_tools` / `upstream_servers` output when the |
| 35 | + feature is not triggered (byte-for-byte backward compatible). |
| 36 | +- No new persistent storage. Classification is computed at query time from data |
| 37 | + already loaded (config + approval records + StateView snapshot). |
| 38 | + |
| 39 | +## 3. Key facts established during brainstorming |
| 40 | + |
| 41 | +- **Disabled tools are already in the Bleve index.** `DiscoverAndIndexToolsForServer` |
| 42 | + indexes everything `client.ListTools()` returns with no disabled/config filter; |
| 43 | + filtering is purely query-time in the `callableResults` loop of |
| 44 | + `handleRetrieveToolsWithMode` (`internal/server/mcp.go`). Surfacing locked tools |
| 45 | + is therefore a change to *that loop*, not a new data path. |
| 46 | +- **`isToolCallable` collapses several distinct reasons** (server off, config-denied, |
| 47 | + user-disabled, quarantine pending/changed, storage error). The discovery layer |
| 48 | + re-derives a precise reason; enforcement stays collapsed. |
| 49 | +- **`Runtime.IsToolConfigDenied(server, tool)`** (added in #468) is the |
| 50 | + config-vs-user discriminator and is reused as-is. |
| 51 | + |
| 52 | +## 4. Design |
| 53 | + |
| 54 | +### 4.1 `retrieve_tools` — opt-in `include_disabled` |
| 55 | + |
| 56 | +- New optional boolean parameter `include_disabled` (default `false`). Tool schema |
| 57 | + description gains exactly one sentence (the "static hint", §4.4). |
| 58 | +- In `handleRetrieveToolsWithMode`, the existing loop that builds `callableResults` |
| 59 | + changes: when a result is dropped by `isToolCallable`, **count it always**; and |
| 60 | + **if `include_disabled` is true**, classify it (§4.3) and append to a separate |
| 61 | + `disabledResults` slice. |
| 62 | +- Agent-scope filtering (`authCtx.CanAccessServer`) is applied **before** |
| 63 | + classification — an agent never sees locked tools on servers it cannot access. |
| 64 | +- Response ordering: callable results first with their **existing ranking |
| 65 | + unchanged**, then `disabledResults`, capped at `min(limit, 10)` entries to bound |
| 66 | + tokens against a pathologically restrictive config. |
| 67 | +- Per disabled entry (lean shape): |
| 68 | + - `name` |
| 69 | + - `server` |
| 70 | + - `description` (the existing one-line description; already short — not truncated) |
| 71 | + - `status` (enum, §4.3) |
| 72 | +- A single `remediation` map is emitted **once** at the top level of the response, |
| 73 | + containing only the keys for statuses actually present in `disabledResults` |
| 74 | + (§4.3). Per-tool remediation prose is **not** emitted. |
| 75 | +- Telemetry: increment an in-memory `include_disabled` usage counter (consistent |
| 76 | + with spec 042 — in-memory only, never persisted) so adoption is observable. |
| 77 | + |
| 78 | +### 4.2 `upstream_servers` — conditional counts |
| 79 | + |
| 80 | +- `operation="list"` and `operation="get"`: each server entry gains a `tools` |
| 81 | + block **only when at least one non-callable count is > 0**: |
| 82 | + |
| 83 | + ```json |
| 84 | + "tools": { "callable": N, "disabled_by_config": N, "disabled_by_user": N, "pending_approval": N } |
| 85 | + ``` |
| 86 | + |
| 87 | + Zero-valued sub-keys are omitted; the whole `tools` block is omitted when every |
| 88 | + tool is callable. Computed from the StateView snapshot the existing |
| 89 | + `getVisibleToolCount` path already walks — one extra classification pass, no new |
| 90 | + storage reads. |
| 91 | + |
| 92 | +### 4.3 Status taxonomy & classification |
| 93 | + |
| 94 | +`status` is one of five values. Classification order is **first match wins**: |
| 95 | + |
| 96 | +| Order | Condition | `status` | `remediation` text (emitted once if present) | |
| 97 | +|------|-----------|----------|----------------------------------------------| |
| 98 | +| 1 | Server not enabled | `server_disabled` | "Its server is disabled. Ask the user to enable the server first." | |
| 99 | +| 2 | `IsToolConfigDenied` true | `disabled_by_config` | "Locked by operator policy in mcp_config.json (enabled_tools/disabled_tools). The user cannot enable this from the UI; ask the operator to change the server config." | |
| 100 | +| 3 | Approval record `Disabled` | `disabled_by_user` | "Disabled by the user. Ask the user to re-enable it in the mcpproxy UI (Server detail → Tools) or via the API." | |
| 101 | +| 4 | Approval status pending/changed | `pending_approval` | "Awaiting security approval. Ask the user to review and approve it in the mcpproxy UI." | |
| 102 | +| 5 | Storage error / indeterminate | `disabled_unknown` | "Reason undetermined; check server logs." | |
| 103 | + |
| 104 | +`disabled_unknown` (5th bucket) exists so a transient storage error never causes a |
| 105 | +*wrong* remediation (e.g. telling the user to toggle a UI switch for a |
| 106 | +config-locked tool). The four happy-path states stay clean. |
| 107 | + |
| 108 | +### 4.4 Reactive triggers (discoverability) |
| 109 | + |
| 110 | +Option C's only weakness is the agent not knowing the flag exists. Closed two ways: |
| 111 | + |
| 112 | +1. **Static hint** (one-time, cheap): one sentence appended to the `retrieve_tools` |
| 113 | + parameter/tool description — *"Set `include_disabled:true` to also surface |
| 114 | + tools that exist but are currently locked by config, user, or quarantine, |
| 115 | + with remediation guidance."* |
| 116 | +2. **Reactive nudges:** |
| 117 | + - The status-aware `TOOL_BLOCKED` message (the #1 fix shipping in #468) gains, |
| 118 | + for the config/user/pending cases: *"Run retrieve_tools with |
| 119 | + include_disabled:true to see locked capabilities and remediation."* |
| 120 | + - When `retrieve_tools` returns **0 callable results** but the always-on |
| 121 | + drop-counter (§4.1) is > 0, append a one-line note to the result: |
| 122 | + *"N relevant tools exist but are locked; retry with include_disabled:true |
| 123 | + for details."* — count only, never the entries, so the nudge is a few |
| 124 | + tokens regardless of how many are locked. |
| 125 | + |
| 126 | +### 4.5 Error handling & edges |
| 127 | + |
| 128 | +- Classification storage error → `disabled_unknown` (never a misleading |
| 129 | + remediation). |
| 130 | +- `server_disabled` reliability caveat: a fully-disabled server does not re-list |
| 131 | + its tools, so its entries surface in `retrieve_tools` only if stale in the |
| 132 | + index. The authoritative signal for a fully-off server is the |
| 133 | + `upstream_servers` server `state`, not search. Documented as a known limitation; |
| 134 | + no code attempts to "freshen" a disabled server's tools. |
| 135 | +- Backward compatibility: default path (`include_disabled` absent/false, all |
| 136 | + tools callable) is byte-for-byte unchanged. New behavior is additive behind the |
| 137 | + flag and the conditional `tools` block. |
| 138 | + |
| 139 | +## 5. Components & boundaries |
| 140 | + |
| 141 | +| Unit | Responsibility | Depends on | |
| 142 | +|------|----------------|------------| |
| 143 | +| `classifyDisabledTool(server, tool) -> status` | Pure mapping from (config, approval record, server-enabled state) to one of 5 statuses. No I/O beyond reads already done. | `Runtime.IsToolConfigDenied`, storage approval lookup, server-enabled check | |
| 144 | +| `retrieve_tools` handler change | Split dropped results into `disabledResults`, cap, attach `remediation` map, emit 0-result nudge | `classifyDisabledTool` | |
| 145 | +| `upstream_servers` list/get change | Per-server count rollup, conditional emit | `classifyDisabledTool`, StateView snapshot | |
| 146 | +| `TOOL_BLOCKED` message (#468) | Status-aware text + flag pointer | `IsToolConfigDenied` (already in #468) | |
| 147 | + |
| 148 | +`classifyDisabledTool` is the single source of truth for "why is this tool not |
| 149 | +callable" used by both surfaces — it can be unit-tested in isolation against the |
| 150 | +five branches without standing up an MCP server. |
| 151 | + |
| 152 | +## 6. Testing |
| 153 | + |
| 154 | +- **Classifier (`internal/server` or `internal/runtime`):** table test, one case |
| 155 | + per status including first-match-wins ordering (e.g. a tool that is both |
| 156 | + config-denied *and* user-disabled resolves to `disabled_by_config`) and the |
| 157 | + `disabled_unknown` fallback on injected storage error. |
| 158 | +- **`retrieve_tools`:** |
| 159 | + - `include_disabled` absent/false → result identical to today (regression guard, |
| 160 | + reuse existing fixtures). |
| 161 | + - `include_disabled` true → callable-first ordering preserved; cap of |
| 162 | + `min(limit,10)` enforced; `remediation` map keyed only by present statuses; |
| 163 | + agent-scope filter still applied before classification. |
| 164 | + - 0 callable + locked matches → nudge note present with correct count; |
| 165 | + 0 callable + no locked matches → no nudge. |
| 166 | +- **`upstream_servers`:** `tools` block omitted when all callable; present and |
| 167 | + correct (zero sub-keys omitted) when mixed. |
| 168 | +- **OAS:** regenerate `oas/swagger.yaml` + `oas/docs.go` for the new |
| 169 | + `include_disabled` param and disabled-entry/`tools` shapes; run |
| 170 | + `./scripts/verify-oas-coverage.sh`. |
| 171 | +- Full suite: `go test ./internal/... -race`, `./scripts/test-api-e2e.sh`. |
| 172 | + |
| 173 | +## 7. Out of scope / future |
| 174 | + |
| 175 | +- Surfacing locked tools in the Web UI search (the UI already shows per-tool lock |
| 176 | + badges on the server detail page from #468). |
| 177 | +- A "request enable" workflow where the agent programmatically asks the user to |
| 178 | + approve — this design only *informs*; acting on it is the agent's/user's call. |
| 179 | +- Freshening a disabled server's tool list for more reliable `server_disabled` |
| 180 | + discovery. |
0 commit comments