feat(mcp-store): agent request permission for tool calling

[cvolzer3]

Problem

When the agent uses a blocked or approval-required MCP tool, Claude incorrectly tells users to check "Claude Code settings." PostHog Code owns MCP installations, and needs_approval tools should show an interactive approval dialog instead of a static denial.

Changes

• Intercept MCP tool calls in canUseTool(): do_not_use are denied and the user is directed to "Settings > MCP Servers"; needs_approval tools are shown permission dialog
• Fetch tool approval states from PostHog API at session start and thread them through _meta into the tool metadata cache
• When user approves a needs_approval tool, PATCH the backend to set approved before execution, otherwise the server-side proxy still rejects the call
• Sanitize server names to match the SDK's convention (non-alphanumeric → _)
• Skip read-only auto-approval for needs_approval tools
• Fix system prompt to stop Claude from suggesting Settings for generic MCP errors

How did you test this?

• New unit tests in tool-metadata.test.ts (10 tests) and permission-handlers.test.ts (8 tests) covering all approval states, bypass mode, read-only override, and dialog title format
• Updated auth-adapter.test.ts and service.test.ts for new return types
• pnpm typecheck, pnpm --filter agent test (287 pass), pnpm --filter code test (770 pass)
• Manual: needs_approval shows dialog, do_not_use shows correct denial message

[cvolzer3]

fyi, this builds on the other mcp store pr: #1747

[greptile-apps]

Comments Outside Diff (3)

1. apps/code/src/main/services/agent/service.ts, line 1437-1466 (link)

   needs_approval dialog re-fires on every call within the same session

   After a user approves a needs_approval tool with "Yes" (allow_once), session.mcpToolApprovals[toolName] is updated to "approved" and the backend is patched. But mcpToolMetadataCache (the module-level cache read by getMcpToolApprovalState in canUseTool) is never updated. So on the very next call to the same tool in the same session, canUseTool still sees "needs_approval" and re-invokes handleMcpApprovalFlow, showing the dialog again.

   The fix is to call setMcpToolApprovalStates({ [toolName]: "approved" }) immediately after the successful backend patch:

   await service.agentAuthAdapter.updateMcpToolApproval(...);
   session.mcpToolApprovals[toolName] = "approved";
   setMcpToolApprovalStates({ [toolName]: "approved" }); // sync the global cache

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: apps/code/src/main/services/agent/service.ts
   Line: 1437-1466
   
   Comment:
   **`needs_approval` dialog re-fires on every call within the same session**
   
   After a user approves a `needs_approval` tool with "Yes" (allow_once), `session.mcpToolApprovals[toolName]` is updated to `"approved"` and the backend is patched. But `mcpToolMetadataCache` (the module-level cache read by `getMcpToolApprovalState` in `canUseTool`) is never updated. So on the very next call to the same tool in the same session, `canUseTool` still sees `"needs_approval"` and re-invokes `handleMcpApprovalFlow`, showing the dialog again.
   
   The fix is to call `setMcpToolApprovalStates({ [toolName]: "approved" })` immediately after the successful backend patch:
   
   ```typescript
   await service.agentAuthAdapter.updateMcpToolApproval(...);
   session.mcpToolApprovals[toolName] = "approved";
   setMcpToolApprovalStates({ [toolName]: "approved" }); // sync the global cache
   ```
   
   How can I resolve this? If you propose a fix, please make it concise.

2. packages/agent/src/adapters/claude/mcp/tool-metadata.test.ts, line 604-620 (link)

   Prefer parameterised tests for sanitizeMcpServerName

   These four cases share identical structure — only the input and expected output differ. Per the team's convention, they should use it.each:

   it.each([
     ["passes through simple alphanumeric names", "HubSpot", "HubSpot"],
     ["replaces spaces with underscores", "My Server", "My_Server"],
     ["replaces special characters with underscores", "server@v2.0!", "server_v2_0_"],
     ["preserves hyphens and underscores", "my-server_v2", "my-server_v2"],
   ])("%s", (_desc, input, expected) => {
     expect(sanitizeMcpServerName(input)).toBe(expected);
   });

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: packages/agent/src/adapters/claude/mcp/tool-metadata.test.ts
   Line: 604-620
   
   Comment:
   **Prefer parameterised tests for `sanitizeMcpServerName`**
   
   These four cases share identical structure — only the input and expected output differ. Per the team's convention, they should use `it.each`:
   
   ```typescript
   it.each([
     ["passes through simple alphanumeric names", "HubSpot", "HubSpot"],
     ["replaces spaces with underscores", "My Server", "My_Server"],
     ["replaces special characters with underscores", "server@v2.0!", "server_v2_0_"],
     ["preserves hyphens and underscores", "my-server_v2", "my-server_v2"],
   ])("%s", (_desc, input, expected) => {
     expect(sanitizeMcpServerName(input)).toBe(expected);
   });
   ```
   
   How can I resolve this? If you propose a fix, please make it concise.

   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!

3. packages/agent/src/adapters/claude/mcp/tool-metadata.test.ts, line 559-567 (link)

   Test name is misleading — readOnly preservation is not actually verified

   The test is titled "merges with existing entries preserving readOnly", but it starts from an empty cache, so there is no existing entry to preserve. It only asserts that newly-created entries default to readOnly: false. A proper coverage of the invariant would pre-populate the cache with readOnly: true, call setMcpToolApprovalStates, and then assert readOnly is still true. Without this, the claim that readOnly is preserved is untested.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: packages/agent/src/adapters/claude/mcp/tool-metadata.test.ts
   Line: 559-567
   
   Comment:
   **Test name is misleading — `readOnly` preservation is not actually verified**
   
   The test is titled "merges with existing entries preserving readOnly", but it starts from an empty cache, so there is no existing entry to preserve. It only asserts that newly-created entries default to `readOnly: false`. A proper coverage of the invariant would pre-populate the cache with `readOnly: true`, call `setMcpToolApprovalStates`, and then assert `readOnly` is still `true`. Without this, the claim that readOnly is preserved is untested.
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: packages/agent/src/adapters/claude/mcp/tool-metadata.ts
Line: 16

Comment:
**Module-level cache shared across all concurrent sessions**

`mcpToolMetadataCache` is a module-level singleton. Each new session calls `setMcpToolApprovalStates(meta.mcpToolApprovals)` in `claude-agent.ts`, which mutates this shared map. If two sessions run concurrently for the same tool key (e.g. `mcp__posthog__search`), the second session's `setMcpToolApprovalStates` call overwrites the state set by the first — and `canUseTool` reads from this same global cache. A session that had `"approved"` could see `"needs_approval"` (or `"do_not_use"`) after another session starts.

The per-session `mcpToolApprovals` field already exists on `ManagedSession` in `service.ts`, but the permission enforcement path in `permission-handlers.ts` bypasses it entirely. Approval states should either be keyed by session, or the cache should be keyed by `(sessionId, toolKey)` to prevent cross-session contamination.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: apps/code/src/main/services/agent/service.ts
Line: 1437-1466

Comment:
**`needs_approval` dialog re-fires on every call within the same session**

After a user approves a `needs_approval` tool with "Yes" (allow_once), `session.mcpToolApprovals[toolName]` is updated to `"approved"` and the backend is patched. But `mcpToolMetadataCache` (the module-level cache read by `getMcpToolApprovalState` in `canUseTool`) is never updated. So on the very next call to the same tool in the same session, `canUseTool` still sees `"needs_approval"` and re-invokes `handleMcpApprovalFlow`, showing the dialog again.

The fix is to call `setMcpToolApprovalStates({ [toolName]: "approved" })` immediately after the successful backend patch:

```typescript
await service.agentAuthAdapter.updateMcpToolApproval(...);
session.mcpToolApprovals[toolName] = "approved";
setMcpToolApprovalStates({ [toolName]: "approved" }); // sync the global cache
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: packages/agent/src/adapters/claude/mcp/tool-metadata.test.ts
Line: 604-620

Comment:
**Prefer parameterised tests for `sanitizeMcpServerName`**

These four cases share identical structure — only the input and expected output differ. Per the team's convention, they should use `it.each`:

```typescript
it.each([
  ["passes through simple alphanumeric names", "HubSpot", "HubSpot"],
  ["replaces spaces with underscores", "My Server", "My_Server"],
  ["replaces special characters with underscores", "server@v2.0!", "server_v2_0_"],
  ["preserves hyphens and underscores", "my-server_v2", "my-server_v2"],
])("%s", (_desc, input, expected) => {
  expect(sanitizeMcpServerName(input)).toBe(expected);
});
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: packages/agent/src/adapters/claude/mcp/tool-metadata.test.ts
Line: 559-567

Comment:
**Test name is misleading — `readOnly` preservation is not actually verified**

The test is titled "merges with existing entries preserving readOnly", but it starts from an empty cache, so there is no existing entry to preserve. It only asserts that newly-created entries default to `readOnly: false`. A proper coverage of the invariant would pre-populate the cache with `readOnly: true`, call `setMcpToolApprovalStates`, and then assert `readOnly` is still `true`. Without this, the claim that readOnly is preserved is untested.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "fix: types" | Re-trigger Greptile}

[greptile-apps]

Module-level cache shared across all concurrent sessions

mcpToolMetadataCache is a module-level singleton. Each new session calls setMcpToolApprovalStates(meta.mcpToolApprovals) in claude-agent.ts, which mutates this shared map. If two sessions run concurrently for the same tool key (e.g. mcp__posthog__search), the second session's setMcpToolApprovalStates call overwrites the state set by the first — and canUseTool reads from this same global cache. A session that had "approved" could see "needs_approval" (or "do_not_use") after another session starts.

The per-session mcpToolApprovals field already exists on ManagedSession in service.ts, but the permission enforcement path in permission-handlers.ts bypasses it entirely. Approval states should either be keyed by session, or the cache should be keyed by (sessionId, toolKey) to prevent cross-session contamination.

Prompt To Fix With AI

This is a comment left during a code review.
Path: packages/agent/src/adapters/claude/mcp/tool-metadata.ts
Line: 16

Comment:
**Module-level cache shared across all concurrent sessions**

`mcpToolMetadataCache` is a module-level singleton. Each new session calls `setMcpToolApprovalStates(meta.mcpToolApprovals)` in `claude-agent.ts`, which mutates this shared map. If two sessions run concurrently for the same tool key (e.g. `mcp__posthog__search`), the second session's `setMcpToolApprovalStates` call overwrites the state set by the first — and `canUseTool` reads from this same global cache. A session that had `"approved"` could see `"needs_approval"` (or `"do_not_use"`) after another session starts.

The per-session `mcpToolApprovals` field already exists on `ManagedSession` in `service.ts`, but the permission enforcement path in `permission-handlers.ts` bypasses it entirely. Approval states should either be keyed by session, or the cache should be keyed by `(sessionId, toolKey)` to prevent cross-session contamination.

How can I resolve this? If you propose a fix, please make it concise.