fix(agent): show server name and exec context in MCP permission dialog#2339
Open
MukundaKatta wants to merge 1 commit into
Open
fix(agent): show server name and exec context in MCP permission dialog#2339MukundaKatta wants to merge 1 commit into
MukundaKatta wants to merge 1 commit into
Conversation
The default permission flow sent MCP tool calls without the claudeCode._meta tag the renderer uses to route them through McpPermission. As a result, the dialog fell back to the bare tool name (e.g. `exec`) with no server context and no unwrapped exec dispatch args, leaving the user without enough information to decide. Tag MCP tools in the default permission flow so the dialog shows `posthog - exec (MCP)` plus the formatted exec body, matching the needs_approval and PostHog destructive-subtool flows. Fixes PostHog#2177
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/agent/src/adapters/claude/permissions/permission-handlers.test.ts:107-134
**Prefer a parameterised test here**
The two new tests vary only along one axis — whether the tool name starts with `mcp__` — and therefore test the same branching condition in `handleDefaultPermissionFlow`. The team's simplicity rules ask for `it.each` in exactly this pattern. This also frees up the second test from accessing `.mock.calls` directly while the first uses `toHaveBeenCalledWith`, keeping the assertion style consistent.
Reviews (1): Last reviewed commit: "fix(agent): show server name and exec co..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+107
to
+134
| it("tags MCP tools in the default permission flow with claudeCode.toolName so the renderer can show the server name and unwrap exec dispatch args", async () => { | ||
| setMcpToolApprovalStates({ mcp__posthog__exec: "approved" }); | ||
|
|
||
| const context = createContext("mcp__posthog__exec", { | ||
| toolInput: { command: "call experiment-get-all {}" }, | ||
| }); | ||
| const result = await canUseTool(context); | ||
|
|
||
| expect(result.behavior).toBe("allow"); | ||
| expect(context.client.requestPermission).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| toolCall: expect.objectContaining({ | ||
| _meta: { claudeCode: { toolName: "mcp__posthog__exec" } }, | ||
| }), | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| it("does not set claudeCode._meta on non-MCP tools in the default permission flow", async () => { | ||
| const context = createContext("WebFetch", { | ||
| toolInput: { url: "https://example.com" }, | ||
| }); | ||
| await canUseTool(context); | ||
|
|
||
| const call = (context.client.requestPermission as ReturnType<typeof vi.fn>) | ||
| .mock.calls[0]?.[0]; | ||
| expect(call?.toolCall?._meta).toBeUndefined(); | ||
| }); |
Contributor
There was a problem hiding this comment.
The two new tests vary only along one axis — whether the tool name starts with mcp__ — and therefore test the same branching condition in handleDefaultPermissionFlow. The team's simplicity rules ask for it.each in exactly this pattern. This also frees up the second test from accessing .mock.calls directly while the first uses toHaveBeenCalledWith, keeping the assertion style consistent.
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/agent/src/adapters/claude/permissions/permission-handlers.test.ts
Line: 107-134
Comment:
**Prefer a parameterised test here**
The two new tests vary only along one axis — whether the tool name starts with `mcp__` — and therefore test the same branching condition in `handleDefaultPermissionFlow`. The team's simplicity rules ask for `it.each` in exactly this pattern. This also frees up the second test from accessing `.mock.calls` directly while the first uses `toHaveBeenCalledWith`, keeping the assertion style consistent.
**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
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!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Closes #2177
The permission dialog for the PostHog
exectoolcall (and any other MCP tool that falls into the default permission flow) shows only the bare tool name — e.g. justexecwith no server prefix, no unwrapped sub-tool name, and no dispatch args. The user has no way to tell what the agent is about to do before approving.The
needs_approvalMCP flow and the PostHog destructive-subtool flow both already tag their requestPermission payload with_meta.claudeCode.toolName, which is the signalPermissionSelectoruses to route throughMcpPermission(rendersposthog - <subtool> (MCP)+ the formatted exec body). The default permission flow was missing that tag, so MCP tools fell through toDefaultPermissionand lost all that context.Changes
handleDefaultPermissionFlow, add_meta: { claudeCode: { toolName } }to the toolCall payload whentoolName.startsWith("mcp__"). Non-MCP tools are unchanged.claudeCode.toolName.Net change: 36 lines (7 production, 29 test).
How did you test this?
pnpm vitest run src/adapters/claude/permissions/inpackages/agent: 33/33 pass (including the 2 new tests).tsc --noEmitinpackages/agent: clean.Publish to changelog?
no