fix(agent): escalate mcp-store tool approvals to the user#2954
fix(agent): escalate mcp-store tool approvals to the user#2954cvolzer3 wants to merge 2 commits into
Conversation
Cloud sessions surfaced the MCP Store proxy's "requires approval" (-32001) instead of prompting the user. Two root causes: - bypassPermissions (the cloud Claude default) skips canUseTool, where the MCP approval gate lives, so needs_approval tools ran unchecked. Add a PreToolUse hook that forces ask/deny for needs_approval / do_not_use MCP Store tools, which runs in every permission mode. - approval state was stored on the volatile MCP metadata cache, which clearMcpToolMetadataCache() wipes on every server refresh/reconnect. Move it to a dedicated cache that survives refreshes so the gate keeps seeing the right state mid-session. Also hardens mcp-store tool-key resolution and wires codex approval support.
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return sanitizeMcpServerName(name) | ||
| .replace(/_+/g, "_") | ||
| .replace(/^_+|_+$/g, "") |
| serverName: string; | ||
| toolName: string; | ||
| } | null { | ||
| const match = title.match(/^The agent wants to call\s+(.+?)\s+\((.+)\)$/); |
|
Reviews (1): Last reviewed commit: "fix(agent): escalate mcp-store tool appr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ...(this.config.mcpToolApprovals && { | ||
| mcpToolApprovals: this.config.mcpToolApprovals, | ||
| }), |
There was a problem hiding this comment.
mcpToolInstallations is omitted from sessionMeta even though mcpToolApprovals is included. When the Codex adapter runs in cloud mode via agent-server.ts, sessionState.mcpToolInstallations will be undefined. In codex-client.ts, getMcpToolNameFromPermissionRequest uses sessionState.mcpToolInstallations as a disambiguation fallback — without it, bare tool names that collide across multiple MCP servers will resolve to null, bypassing the in-client approval short-circuit and falling through to the upstream (agent-server) unchanged. The server-side approveMcpStoreToolIfNeeded still has this.config.mcpToolInstallations so the relay and persist path remains correct, but the Codex client's in-memory sessionState.mcpToolApprovals update will be skipped for those cases.
| ...(this.config.mcpToolApprovals && { | |
| mcpToolApprovals: this.config.mcpToolApprovals, | |
| }), | |
| ...(this.config.mcpToolApprovals && { | |
| mcpToolApprovals: this.config.mcpToolApprovals, | |
| }), | |
| ...(this.config.mcpToolInstallations && { | |
| mcpToolInstallations: this.config.mcpToolInstallations, | |
| }), |
| export function getMcpToolMetadataKey(toolName: string): string | undefined { | ||
| const resolvedKey = resolveMcpStoreToolKey(toolName, { | ||
| approvals: Object.fromEntries( | ||
| [...mcpToolMetadataCache.keys()].map((key) => [key, true]), | ||
| ), | ||
| }); | ||
| const normalizedToolName = normalizeMcpToolKey(toolName); | ||
| if (mcpToolMetadataCache.has(toolName)) return toolName; | ||
| if (mcpToolMetadataCache.has(normalizedToolName)) return normalizedToolName; | ||
| return resolvedKey ?? undefined; | ||
| } |
There was a problem hiding this comment.
resolvedKey is computed eagerly — iterating all metadata cache keys and calling resolveMcpStoreToolKey — before the cheaper direct and normalized-key checks at lines 125–126. For the common case where the tool name is already in the cache verbatim, this work is wasted. Moving the direct/normalized checks before the resolveMcpStoreToolKey call avoids the overhead without changing behavior.
| export function getMcpToolMetadataKey(toolName: string): string | undefined { | |
| const resolvedKey = resolveMcpStoreToolKey(toolName, { | |
| approvals: Object.fromEntries( | |
| [...mcpToolMetadataCache.keys()].map((key) => [key, true]), | |
| ), | |
| }); | |
| const normalizedToolName = normalizeMcpToolKey(toolName); | |
| if (mcpToolMetadataCache.has(toolName)) return toolName; | |
| if (mcpToolMetadataCache.has(normalizedToolName)) return normalizedToolName; | |
| return resolvedKey ?? undefined; | |
| } | |
| export function getMcpToolMetadataKey(toolName: string): string | undefined { | |
| if (mcpToolMetadataCache.has(toolName)) return toolName; | |
| const normalizedToolName = normalizeMcpToolKey(toolName); | |
| if (mcpToolMetadataCache.has(normalizedToolName)) return normalizedToolName; | |
| const resolvedKey = resolveMcpStoreToolKey(toolName, { | |
| approvals: Object.fromEntries( | |
| [...mcpToolMetadataCache.keys()].map((key) => [key, true]), | |
| ), | |
| }); | |
| return resolvedKey ?? undefined; | |
| } |
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!
2 participants
Problem
When an agent in a cloud session calls an MCP Store tool whose per-tool approval state is
needs_approval, the MCP Store proxy rejects the call withMCP error -32001: Tool '<name>' requires approval before it can be called. The run just fails — the user is never asked, so there is no way to approve and continue.The goal: escalate the approval to the user (approve/deny dialog in PostHog Code), and on approval let the agent keep working in the cloud.
Changes
The backend already passed per-tool approval metadata into the sandbox (
--mcpToolApprovals/--mcpToolInstallations), and the approval gate already lived in the Claude adapter'scanUseTool(handleMcpApprovalFlow→ relay dialog → persist approval → proxy then allows). Two defects kept that gate from ever firing:bypassPermissionsskipscanUseTool. Cloud Claude runs default tobypassPermissions, which the SDK documents as "bypass all permission checks" — socanUseTool(where the gate lives) is never invoked and the tool runs straight into the proxy →-32001.PreToolUsehook (hooks.ts), which runs in every permission mode. It forcespermissionDecision: "ask"forneeds_approvaltools (routing them back throughcanUseTool/the approval dialog) and"deny"fordo_not_use. This mirrors the existing destructive-PostHog-exec gate.Approval state was wiped on MCP server reconnect. Approval state was stored on the volatile MCP metadata cache, but
clearMcpToolMetadataCache()runs on every MCP server refresh/reconnect (which happens right after startup). After the wipe,getMcpToolApprovalState()returnedundefined, the gate was skipped, and since these tools are typicallyreadOnly,isToolAllowedForModeauto-allowed them indefaultmode →-32001.tool-metadata.ts) that survives metadata-cache clears. Approval state is session config, not per-connection metadata.This PR also includes the in-progress relay/plumbing work this fix builds on: robust MCP tool-key resolution (
mcp-store/tool-keys.ts), the cloud relay + persist path (agent-server.ts,posthog-api.ts,bin.ts,schemas.ts), and Codex-adapter approval support.No backend (
posthog/posthog) changes are needed — verified live that the sandbox already receives the correct--mcpToolApprovalsmap.How did you test this?
Automated (all run locally, passing):
vitestfortool-metadata,hooks,permission-handlerstest files — incl. new tests: approval state survives a metadata-cache clear;PreToolUsereturnsask/deny/respects-allow-rule for MCP-store tools.tsc --noEmitclean;biome checkclean.dist(tsup) and confirmed both fixes are bundled.Live investigation (not yet a full end-to-end dialog run):
--mcpToolApprovalscarriesneeds_approvalfor the affected tools, the runtime MCP server name matches the approval keys, the tools arereadOnly, and the metadata cache is cleared by a server reconnect at startup.Still to verify end-to-end (draft): trigger an actual tool call in a fresh sandbox and confirm the dialog renders and approval lets the run continue. If the relay reaches the desktop but no dialog appears, the next suspect is the desktop-side rendering of the relayed
_posthog/permission_requestfor cloud tasks.Automatic notifications