From 03bc877e3128776ad2dcb2b44df6d27f4efce8a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Thu, 17 Sep 2026 10:41:34 +0100 Subject: [PATCH] fix: approve Claude ACP Kandev MCP permissions (#3729) (#3738) --- apps/backend/internal/agentctl/AGENTS.md | 25 + .../internal/agentctl/server/acp/client.go | 8 + .../acp/client_permission_identity_test.go | 80 +++ .../transport/acp/adapter_permissions.go | 2 + .../server/adapter/transport/acp/dialect.go | 15 + .../adapter/transport/acp/dialect_claude.go | 75 +++ .../transport/acp/dialect_claude_test.go | 148 ++++++ .../internal/agentctl/server/config/config.go | 5 + .../agentctl/server/config/config_test.go | 55 ++ .../agentctl/server/process/manager.go | 3 + .../process/manager_permission_acp_test.go | 498 ++++++++++++++++++ .../process/manager_permission_policy.go | 76 +++ .../process/manager_permission_policy_test.go | 255 +++++++++ .../agentctl/types/permission_identity.go | 49 ++ .../types/permission_identity_test.go | 36 ++ apps/backend/internal/agentctl/types/types.go | 8 + .../2026-09-16-injected-mcp-approval.md | 42 ++ .../claude-injected-mcp-approval/plan.md | 107 ++++ .../task-01-injected-server-approval.md | 151 ++++++ docs/public/agents-and-profiles.md | 2 + .../requirements/injected-mcp-approval.md | 47 ++ .../system-design/injected-mcp-approval.md | 123 +++++ 22 files changed, 1810 insertions(+) create mode 100644 apps/backend/internal/agentctl/server/acp/client_permission_identity_test.go create mode 100644 apps/backend/internal/agentctl/server/adapter/transport/acp/dialect_claude.go create mode 100644 apps/backend/internal/agentctl/server/adapter/transport/acp/dialect_claude_test.go create mode 100644 apps/backend/internal/agentctl/server/process/manager_permission_acp_test.go create mode 100644 apps/backend/internal/agentctl/server/process/manager_permission_policy.go create mode 100644 apps/backend/internal/agentctl/server/process/manager_permission_policy_test.go create mode 100644 apps/backend/internal/agentctl/types/permission_identity.go create mode 100644 apps/backend/internal/agentctl/types/permission_identity_test.go create mode 100644 docs/decisions/2026-09-16-injected-mcp-approval.md create mode 100644 docs/plans/claude-injected-mcp-approval/plan.md create mode 100644 docs/plans/claude-injected-mcp-approval/task-01-injected-server-approval.md create mode 100644 docs/specs/agents/requirements/injected-mcp-approval.md create mode 100644 docs/specs/agents/system-design/injected-mcp-approval.md diff --git a/apps/backend/internal/agentctl/AGENTS.md b/apps/backend/internal/agentctl/AGENTS.md index 81320e275a..485e299b8a 100644 --- a/apps/backend/internal/agentctl/AGENTS.md +++ b/apps/backend/internal/agentctl/AGENTS.md @@ -88,6 +88,31 @@ Grok ACP currently exposes neither per-turn cost nor subscription quota/reset va JSON-RPC 2.0 over stdin/stdout between agentctl and agent process. Requests: `initialize`, `session/new`, `session/load`, `session/prompt`, `session/cancel`, `session/close`. Notifications: `session/update` with types `message_chunk`, `tool_call`, `tool_update`, `complete`, `error`, `permission_request`, `context_window`. +### ACP permission identity and injected MCP approval + +The ACP client preserves `ToolCall.Name` and `ToolCall.Meta` on the internal +permission request. The Claude dialect reads `_meta.claudeCode.toolName` only +for Claude frames and uses an exact qualified title fallback only when both +identity fields are absent and the ACP kind is `other`. Malformed or +conflicting identity data stays in the normal permission path. Other provider +dialects must not inherit this title fallback without a tested wire contract. + +The process manager may automatically select an offered allow-once, then +allow-always option for any qualified tool on the host-injected Kandev MCP +server when blanket approval is off. It requires the internal construction +provenance marker and the exact current-port HTTP or SSE server entry. This +server-wide provider-layer rule includes destructive Kandev tools; MCP +authentication, task/session authorization, questions, and workflow gates stay +separate. Shell, file, third-party, ambiguous, malformed, and unqualified +requests keep the pending permission flow. Internal identity fields are not +part of permission snapshots or stream events. + +The flattened `mcp__server__tool` parser accepts only one unambiguous `__` +separator. It rejects delimiter collisions, adjacent underscores at the +boundary, and additional `__` sequences in the tool suffix. This prevents a +server such as `kandev__external` from being read as the reserved `kandev` +server. Tool names with these ambiguous forms keep the pending flow. + ### ACP frame debug logging (`adapter/transport/shared/acplog.go`) When `KANDEV_DEBUG_AGENT_MESSAGES=true` (on by default in the **dev** profile), the ACP adapter dumps every raw + normalized frame to **per-session** JSONL files: diff --git a/apps/backend/internal/agentctl/server/acp/client.go b/apps/backend/internal/agentctl/server/acp/client.go index bd70e07cbd..ae36e1343c 100644 --- a/apps/backend/internal/agentctl/server/acp/client.go +++ b/apps/backend/internal/agentctl/server/acp/client.go @@ -202,10 +202,18 @@ func (c *Client) forwardPermissionRequest(ctx context.Context, handler Permissio actionDetails["description"] = description } + var toolName *string + if p.ToolCall.Name != nil { + name := *p.ToolCall.Name + toolName = &name + } + req := &types.PermissionRequest{ SessionID: string(p.SessionId), ToolCallID: string(p.ToolCall.ToolCallId), Title: title, + ToolName: toolName, + ToolMeta: p.ToolCall.Meta, ActionType: actionType, ActionDetails: actionDetails, Options: options, diff --git a/apps/backend/internal/agentctl/server/acp/client_permission_identity_test.go b/apps/backend/internal/agentctl/server/acp/client_permission_identity_test.go new file mode 100644 index 0000000000..96acb01290 --- /dev/null +++ b/apps/backend/internal/agentctl/server/acp/client_permission_identity_test.go @@ -0,0 +1,80 @@ +package acp + +import ( + "context" + "encoding/json" + "strings" + "testing" + + acpsdk "github.com/coder/acp-go-sdk" + "github.com/kandev/kandev/internal/agentctl/types" +) + +func TestPermissionProgrammaticName(t *testing.T) { + programmaticName := "mcp__kandev__update_task_plan_kandev" + meta := map[string]any{ + "claudeCode": map[string]any{"toolName": "update_task_plan_kandev"}, + } + var captured *types.PermissionRequest + handler := func(_ context.Context, req *types.PermissionRequest) (*types.PermissionResponse, error) { + captured = req + return &types.PermissionResponse{OptionID: "allow-once"}, nil + } + + client := NewClient() + _, err := client.forwardPermissionRequest(context.Background(), handler, acpsdk.RequestPermissionRequest{ + SessionId: "session-1", + ToolCall: acpsdk.ToolCallUpdate{ + ToolCallId: "tool-1", + Title: stringPtr("Update task plan"), + Kind: toolKindPtr(acpsdk.ToolKindOther), + Name: &programmaticName, + Meta: meta, + }, + Options: []acpsdk.PermissionOption{{ + OptionId: "allow-once", + Kind: acpsdk.PermissionOptionKindAllowOnce, + }}, + }) + if err != nil { + t.Fatalf("forwardPermissionRequest returned error: %v", err) + } + if captured == nil { + t.Fatal("permission handler was not called") + } + if captured.ToolName == nil || *captured.ToolName != programmaticName { + t.Fatalf("ToolName = %v, want %q", captured.ToolName, programmaticName) + } + // claudeCode.toolName uses the short form intentionally. The client passes + // both fields through as-is, and dialect normalization happens later in the + // adapter. The mismatch proves that the client is not synthesizing a name. + if captured.ToolMeta["claudeCode"].(map[string]any)["toolName"] != "update_task_plan_kandev" { + t.Fatalf("ToolMeta = %#v, want Claude metadata", captured.ToolMeta) + } + + encoded, err := json.Marshal(captured) + if err != nil { + t.Fatalf("marshal permission request: %v", err) + } + if strings.Contains(string(encoded), programmaticName) || strings.Contains(string(encoded), "claudeCode") { + t.Fatalf("internal identity fields leaked into serialized request: %s", encoded) + } +} + +func TestRequestPermissionWithoutOptionsCancels(t *testing.T) { + client := NewClient() + response, err := client.RequestPermission(context.Background(), acpsdk.RequestPermissionRequest{ + SessionId: "session-1", + ToolCall: acpsdk.ToolCallUpdate{ToolCallId: "tool-1"}, + }) + if err != nil { + t.Fatalf("RequestPermission returned error: %v", err) + } + if response.Outcome.Cancelled == nil || response.Outcome.Selected != nil { + t.Fatalf("empty permission response = %+v, want cancelled", response.Outcome) + } +} + +func stringPtr(value string) *string { return &value } + +func toolKindPtr(value acpsdk.ToolKind) *acpsdk.ToolKind { return &value } diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_permissions.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_permissions.go index 0e9ac8da50..a6dfc5c29b 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_permissions.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_permissions.go @@ -11,6 +11,8 @@ import ( // Since both acpclient and adapter now use the shared types package, // no conversion is needed - we just forward to the handler. func (a *Adapter) handlePermissionRequest(ctx context.Context, req *PermissionRequest) (*PermissionResponse, error) { + req.ToolName = a.dialect.normalizePermissionToolName(req.ToolName, req.ToolMeta, req.Title, req.ActionType) + a.mu.RLock() handler := a.permissionHandler fallbackSessionID := a.sessionID diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/dialect.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/dialect.go index 0e340165a8..b7b84d41eb 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/dialect.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/dialect.go @@ -28,6 +28,7 @@ type acpDialect struct { mcpToolCall func(map[string]any, any) (mcpToolCallFrame, bool) mcpToolResult func(any) (any, bool) responseAttemptReset func(map[string]any) bool + permissionToolName func(*string, map[string]any, string, string) *string } type mcpToolCallFrame struct { @@ -59,6 +60,8 @@ func newACPDialect(agentID string) acpDialect { return newGrokACPDialect() case codexAgentID: return newCodexACPDialect() + case claudeAgentID: + return newClaudeACPDialect() case mockAgentID: return newMockACPDialect() } @@ -155,3 +158,15 @@ func (d acpDialect) promptUsage( func (d acpDialect) resetsResponseAttempt(meta map[string]any) bool { return d.responseAttemptReset != nil && d.responseAttemptReset(meta) } + +func (d acpDialect) normalizePermissionToolName( + name *string, + meta map[string]any, + title string, + actionType string, +) *string { + if d.permissionToolName == nil { + return name + } + return d.permissionToolName(name, meta, title, actionType) +} diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/dialect_claude.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/dialect_claude.go new file mode 100644 index 0000000000..2cf47496dc --- /dev/null +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/dialect_claude.go @@ -0,0 +1,75 @@ +package acp + +import ( + "strings" + + "github.com/kandev/kandev/internal/agentctl/types" +) + +const claudePermissionOtherActionType = "other" + +// newClaudeACPDialect enables only the compatibility translations verified +// against Claude ACP frames. Other providers retain their normal identity path. +func newClaudeACPDialect() acpDialect { + return acpDialect{permissionToolName: normalizeClaudePermissionToolName} +} + +func normalizeClaudePermissionToolName( + programmaticName *string, + meta map[string]any, + title string, + actionType string, +) *string { + metadataName, metadataPresent, metadataValid := claudePermissionMetadataName(meta) + if !metadataValid { + return emptyPermissionToolName() + } + if metadataPresent { + if programmaticName != nil && *programmaticName != metadataName { + return emptyPermissionToolName() + } + return &metadataName + } + + // A provider-supplied programmatic name is authoritative when no Claude + // metadata identity is present. In particular, a nonmatching name must not + // be replaced with a matching display title. + if programmaticName != nil { + return programmaticName + } + + if actionType != claudePermissionOtherActionType { + return nil + } + candidate := strings.TrimSpace(title) + server, _, ok := types.ParseQualifiedMCPToolName(candidate) + if !ok || server != kandevMCPServerName { + return nil + } + return &candidate +} + +func claudePermissionMetadataName(meta map[string]any) (name string, present, valid bool) { + claudeMeta, ok := meta["claudeCode"] + if !ok { + return "", false, true + } + fields, ok := claudeMeta.(map[string]any) + if !ok { + return "", false, false + } + rawName, ok := fields["toolName"] + if !ok { + return "", false, true + } + name, ok = rawName.(string) + if !ok { + return "", false, false + } + return name, true, true +} + +func emptyPermissionToolName() *string { + name := "" + return &name +} diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/dialect_claude_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/dialect_claude_test.go new file mode 100644 index 0000000000..f9046244b0 --- /dev/null +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/dialect_claude_test.go @@ -0,0 +1,148 @@ +package acp + +import ( + "context" + "testing" + + "github.com/kandev/kandev/internal/agentctl/types/streams" +) + +func TestClaudePermissionIdentity(t *testing.T) { + a := newTestAdapterForAgent(claudeAgentID) + a.activeToolCalls["tool-1"] = &streams.NormalizedPayload{} + var captured *PermissionRequest + a.permissionHandler = func(_ context.Context, req *PermissionRequest) (*PermissionResponse, error) { + captured = req + return &PermissionResponse{OptionID: "allow-once"}, nil + } + + _, err := a.handlePermissionRequest(t.Context(), &PermissionRequest{ + ToolCallID: "tool-1", + Title: "mcp__kandev__update_task_plan_kandev", + ActionType: string(streams.ActionTypeOther), + ToolMeta: map[string]any{ + "claudeCode": map[string]any{"toolName": "mcp__kandev__update_task_plan_kandev"}, + }, + Options: []PermissionOption{{ + OptionID: "allow-once", + Kind: streams.PermissionOptionKindAllowOnce, + }}, + }) + if err != nil { + t.Fatalf("handlePermissionRequest returned error: %v", err) + } + if captured == nil || captured.ToolName == nil || *captured.ToolName != "mcp__kandev__update_task_plan_kandev" { + t.Fatalf("normalized ToolName = %#v, want mcp__kandev__update_task_plan_kandev", captured) + } +} + +func TestClaudePermissionIdentityNormalization(t *testing.T) { + qualified := "mcp__kandev__update_task_plan_kandev" + other := "mcp__other__tool" + empty := "" + + tests := []struct { + name string + programmatic *string + meta map[string]any + title string + actionType string + want *string + }{ + { + name: "programmatic name is authoritative when metadata agrees", + programmatic: &qualified, + meta: map[string]any{"claudeCode": map[string]any{"toolName": qualified}}, + title: qualified, + actionType: claudePermissionOtherActionType, + want: &qualified, + }, + { + name: "conflicting metadata blocks approval identity", + programmatic: &qualified, + meta: map[string]any{"claudeCode": map[string]any{"toolName": other}}, + want: &empty, + }, + { + name: "Claude metadata supplies qualified identity", + meta: map[string]any{"claudeCode": map[string]any{"toolName": qualified}}, + want: &qualified, + }, + { + name: "legacy exact title is accepted only for other kind", + title: qualified, + actionType: claudePermissionOtherActionType, + want: &qualified, + }, + { + name: "legacy ambiguous title has no identity", + title: "mcp__kandev__external__execute", + actionType: claudePermissionOtherActionType, + }, + { + name: "programmatic ambiguous name remains authoritative", + programmatic: stringPtr("mcp__kandev__external__execute"), + want: stringPtr("mcp__kandev__external__execute"), + }, + { + name: "legacy title with execute kind is not an identity", + title: qualified, + actionType: "execute", + }, + { + name: "malformed Claude metadata blocks title fallback", + meta: map[string]any{"claudeCode": "invalid"}, + title: qualified, + actionType: claudePermissionOtherActionType, + want: &empty, + }, + { + name: "malformed Claude metadata blocks programmatic identity", + programmatic: &qualified, + meta: map[string]any{"claudeCode": "invalid"}, + want: &empty, + }, + { + name: "malformed tool name blocks title fallback", + meta: map[string]any{"claudeCode": map[string]any{"toolName": 42}}, + title: qualified, + actionType: claudePermissionOtherActionType, + want: &empty, + }, + { + name: "unqualified metadata remains ineligible", + meta: map[string]any{"claudeCode": map[string]any{"toolName": "update_task_plan_kandev"}}, + want: stringPtr("update_task_plan_kandev"), + }, + { + name: "explicit empty name remains authoritative", + programmatic: &empty, + title: qualified, + actionType: claudePermissionOtherActionType, + want: &empty, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := newACPDialect(claudeAgentID).normalizePermissionToolName( + test.programmatic, test.meta, test.title, test.actionType) + if (got == nil) != (test.want == nil) { + t.Fatalf("normalized name = %v, want %v", got, test.want) + } + if got != nil && *got != *test.want { + t.Fatalf("normalized name = %q, want %q", *got, *test.want) + } + }) + } +} + +func TestNonClaudePermissionIdentityDoesNotUseClaudeTitleFallback(t *testing.T) { + qualified := "mcp__kandev__update_task_plan_kandev" + got := newACPDialect("other-agent").normalizePermissionToolName(nil, nil, qualified, claudePermissionOtherActionType) + if got != nil { + t.Fatalf("non-Claude normalized name = %q, want nil", *got) + } +} + +func stringPtr(value string) *string { return &value } diff --git a/apps/backend/internal/agentctl/server/config/config.go b/apps/backend/internal/agentctl/server/config/config.go index b7af923b44..14475c4bdf 100644 --- a/apps/backend/internal/agentctl/server/config/config.go +++ b/apps/backend/internal/agentctl/server/config/config.go @@ -278,6 +278,10 @@ type InstanceConfig struct { // McpServers is a list of MCP servers to configure for the agent McpServers []McpServerConfig + // InjectedKandevMCP records that this instance configuration was created + // with the host-owned Kandev MCP server injected into McpServers. + InjectedKandevMCP bool `json:"-"` + // ProcessBufferMaxBytes caps per-process output buffer size ProcessBufferMaxBytes int64 @@ -618,6 +622,7 @@ func (c *Config) NewInstanceConfig(port int, overrides *InstanceOverrides) *Inst // to forward tool calls to the backend. if port > 0 { cfg.McpServers = injectKandevMcpServer(cfg.McpServers, port) + cfg.InjectedKandevMCP = true } // Parse agent command into args diff --git a/apps/backend/internal/agentctl/server/config/config_test.go b/apps/backend/internal/agentctl/server/config/config_test.go index 92362471c2..3bc22a05a5 100644 --- a/apps/backend/internal/agentctl/server/config/config_test.go +++ b/apps/backend/internal/agentctl/server/config/config_test.go @@ -1,6 +1,7 @@ package config import ( + "encoding/json" "os" "os/exec" "path/filepath" @@ -11,6 +12,7 @@ import ( "time" commonconfig "github.com/kandev/kandev/internal/common/config" + "github.com/kandev/kandev/pkg/agent" ) func TestLoadWithStartupUsesExplicitManagedValues(t *testing.T) { @@ -106,6 +108,59 @@ func TestNewInstanceConfig_PropagatesMCPToolNamePresentationCapability(t *testin } } +func TestInjectedKandevMCPProvenance(t *testing.T) { + workDir := t.TempDir() + base := &Config{Defaults: InstanceDefaults{ + Protocol: agent.ProtocolACP, + AgentCommand: "agent --acp", + WorkDir: workDir, + }} + input := []McpServerConfig{ + {Name: "kandev", Type: "stdio", Command: "spoofed-kandev"}, + {Name: "third-party", Type: "http", URL: "https://mcp.example.test/mcp"}, + } + + cfg := base.NewInstanceConfig(43210, &InstanceOverrides{ + Env: []string{}, + McpServers: input, + }) + if !cfg.InjectedKandevMCP { + t.Fatal("positive-port instance must retain injected Kandev provenance") + } + if len(cfg.McpServers) != 3 { + t.Fatalf("McpServers = %+v, want injected HTTP/SSE plus third-party", cfg.McpServers) + } + if cfg.McpServers[0].Name != "kandev" || cfg.McpServers[0].Type != "http" || cfg.McpServers[0].URL != "http://localhost:43210/mcp" { + t.Fatalf("HTTP injection = %+v", cfg.McpServers[0]) + } + if cfg.McpServers[1].Name != "kandev" || cfg.McpServers[1].Type != "sse" || cfg.McpServers[1].URL != "http://localhost:43210/sse" { + t.Fatalf("SSE injection = %+v", cfg.McpServers[1]) + } + if cfg.McpServers[2].Name != "third-party" { + t.Fatalf("unrelated MCP server was not preserved: %+v", cfg.McpServers) + } + + encoded, err := json.Marshal(cfg) + if err != nil { + t.Fatalf("marshal InstanceConfig: %v", err) + } + var serialized map[string]any + if err := json.Unmarshal(encoded, &serialized); err != nil { + t.Fatalf("unmarshal InstanceConfig: %v", err) + } + if _, present := serialized["InjectedKandevMCP"]; present { + t.Fatalf("provenance marker leaked into serialized config: %s", encoded) + } + + withoutPort := base.NewInstanceConfig(0, &InstanceOverrides{ + Env: []string{}, + McpServers: input, + }) + if withoutPort.InjectedKandevMCP { + t.Fatal("zero-port instance must not claim injected Kandev provenance") + } +} + func TestCollectAgentEnvKeepsGitHubCLIShimAheadOfProfilePath(t *testing.T) { t.Setenv("KANDEV_GITHUB_CREDENTIAL_BROKER_URL", "https://kandev.example/api/github/credentials/resolve") t.Setenv("KANDEV_GITHUB_CLI_SHIM_DIR", "/kandev/shims") diff --git a/apps/backend/internal/agentctl/server/process/manager.go b/apps/backend/internal/agentctl/server/process/manager.go index 488caee22f..ca0d0f8c56 100644 --- a/apps/backend/internal/agentctl/server/process/manager.go +++ b/apps/backend/internal/agentctl/server/process/manager.go @@ -2762,6 +2762,9 @@ func (m *Manager) handlePermissionRequest(ctx context.Context, req *adapter.Perm if m.cfg.AutoApprovePermissions { return m.autoApprovePermission(req) } + if response, approved := m.autoApproveInjectedKandevPermission(req); approved { + return response, nil + } // Create pending permission with response channel createdAt := time.Now().UTC() diff --git a/apps/backend/internal/agentctl/server/process/manager_permission_acp_test.go b/apps/backend/internal/agentctl/server/process/manager_permission_acp_test.go new file mode 100644 index 0000000000..e6e271025f --- /dev/null +++ b/apps/backend/internal/agentctl/server/process/manager_permission_acp_test.go @@ -0,0 +1,498 @@ +package process + +import ( + "context" + "errors" + "io" + "sync" + "testing" + "time" + + acpsdk "github.com/coder/acp-go-sdk" + "github.com/kandev/kandev/internal/agentctl/server/adapter" + "github.com/kandev/kandev/internal/agentctl/server/config" + "github.com/kandev/kandev/internal/agentctl/types" + "github.com/kandev/kandev/internal/agentctl/types/streams" + "github.com/kandev/kandev/pkg/agent" +) + +const ( + permissionACPPort = 43210 + injectedKandevToolName = "mcp__kandev__update_task_plan_kandev" + thirdPartyToolName = "mcp__other__search" + permissionFlowToolCall = "tool-1" + permissionFlowSessionID = "session-acp" +) + +type permissionACPCase struct { + name string + taskID string + resume bool + toolName string + title string + toolKind acpsdk.ToolKind + toolMeta map[string]any + autoSelect string + wantEvent bool +} + +func TestInjectedKandevPermissionACPFlow(t *testing.T) { + tests := []permissionACPCase{ + { + name: "fresh task auto approves injected tool", + taskID: "task-1", + toolName: injectedKandevToolName, + title: "Update task plan", + toolKind: acpsdk.ToolKindOther, + toolMeta: claudePermissionMeta(injectedKandevToolName), + autoSelect: "allow-once", + }, + { + name: "resumed quick chat auto approves injected tool", + resume: true, + toolName: injectedKandevToolName, + title: "Update task plan", + toolKind: acpsdk.ToolKindOther, + toolMeta: claudePermissionMeta(injectedKandevToolName), + autoSelect: "allow-once", + }, + { + name: "fresh task keeps Bash pending", + taskID: "task-2", + toolName: "bash", + title: "Run bash command", + toolKind: acpsdk.ToolKind("execute"), + wantEvent: true, + }, + { + name: "resumed quick chat keeps third party MCP pending", + resume: true, + toolName: thirdPartyToolName, + title: "Search third-party service", + toolKind: acpsdk.ToolKindOther, + wantEvent: true, + }, + { + name: "programmatic colliding server stays pending", + taskID: "task-3", + toolName: "mcp__kandev__external__execute", + title: "Execute external MCP tool", + toolKind: acpsdk.ToolKindOther, + wantEvent: true, + }, + { + name: "legacy Claude colliding server stays pending", + resume: true, + title: "mcp__kandev__external__execute", + toolKind: acpsdk.ToolKindOther, + wantEvent: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { runPermissionACPCase(t, test) }) + } +} + +func runPermissionACPCase(t *testing.T, test permissionACPCase) { + t.Helper() + manager, fake, cleanup := newPermissionACPFlow(t, test) + defer cleanup() + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + preparePermissionACPTestSession(t, manager, ctx, test.resume) + drainAdapterUpdates(manager.adapter) + + promptDone := make(chan error, 1) + go func() { + promptDone <- manager.adapter.Prompt(ctx, "continue", nil, 1) + }() + + if test.wantEvent { + resolvePendingPermissionACP(t, manager, test) + } else { + awaitAutoApprovedPermission(t, manager, fake, test.autoSelect) + } + waitForPermissionPrompt(t, ctx, promptDone) + + if test.wantEvent { + if got := fake.selectedOption(); got != "reject-once" { + t.Fatalf("explicitly resolved permission option = %q, want reject-once", got) + } + return + } + + events := collectPromptEvents(manager.adapter.Updates()) + assertPromptEvents(t, events) + select { + case event := <-manager.GetUpdates(): + t.Fatalf("injected Kandev request emitted delayed manager event %q", event.Type) + default: + } +} + +func preparePermissionACPTestSession(t *testing.T, manager *Manager, ctx context.Context, resume bool) { + t.Helper() + if resume { + if err := manager.adapter.LoadSession(ctx, permissionFlowSessionID, nil); err != nil { + t.Fatalf("LoadSession: %v", err) + } + return + } + if _, err := manager.adapter.NewSession(ctx, permissionFlowMCPServers()); err != nil { + t.Fatalf("NewSession: %v", err) + } +} + +func resolvePendingPermissionACP(t *testing.T, manager *Manager, test permissionACPCase) { + t.Helper() + event := waitForPermissionEvent(t, manager.GetUpdates()) + if event.Type != streams.EventTypePermissionRequest { + t.Fatalf("manager event type = %q, want permission_request", event.Type) + } + if event.PermissionTitle != test.title { + t.Fatalf("permission title = %q, want %q", event.PermissionTitle, test.title) + } + if _, err := manager.ResolvePermission(event.RequestID, event.PendingID, "reject-once"); err != nil { + t.Fatalf("ResolvePermission: %v", err) + } +} + +func awaitAutoApprovedPermission(t *testing.T, manager *Manager, fake *permissionFlowAgent, wantOption string) { + t.Helper() + deadline := time.NewTimer(2 * time.Second) + defer deadline.Stop() + select { + case event := <-manager.GetUpdates(): + t.Fatalf("injected Kandev request emitted manager event %q", event.Type) + case <-fake.responseReady: + if fake.selectedOption() != wantOption { + t.Fatalf("selected permission option = %q, want %q", fake.selectedOption(), wantOption) + } + case <-deadline.C: + t.Fatal("injected Kandev permission did not complete") + } +} + +func waitForPermissionPrompt(t *testing.T, ctx context.Context, promptDone <-chan error) { + t.Helper() + select { + case err := <-promptDone: + if err != nil { + t.Fatalf("Prompt: %v", err) + } + case <-ctx.Done(): + t.Fatalf("Prompt did not complete: %v", ctx.Err()) + } +} + +type permissionFlowAgent struct { + burstPermissionAgent + conn *acpsdk.AgentSideConnection + toolName string + title string + toolKind acpsdk.ToolKind + toolMeta map[string]any + responseReady chan struct{} + + selectedMu sync.Mutex + selected string +} + +// The ACP server sends requests to this fake agent only through the methods +// below. All agent-to-client traffic in the test goes through conn, matching +// the production JSON-RPC boundary. +type burstPermissionAgent struct{} + +func (burstPermissionAgent) Authenticate(context.Context, acpsdk.AuthenticateRequest) (acpsdk.AuthenticateResponse, error) { + return acpsdk.AuthenticateResponse{}, nil +} + +func (burstPermissionAgent) Initialize(_ context.Context, req acpsdk.InitializeRequest) (acpsdk.InitializeResponse, error) { + return acpsdk.InitializeResponse{ + ProtocolVersion: req.ProtocolVersion, + AgentCapabilities: acpsdk.AgentCapabilities{ + LoadSession: true, + McpCapabilities: acpsdk.McpCapabilities{ + Http: true, + Sse: true, + }, + }, + }, nil +} + +func (burstPermissionAgent) Logout(context.Context, acpsdk.LogoutRequest) (acpsdk.LogoutResponse, error) { + return acpsdk.LogoutResponse{}, nil +} + +func (burstPermissionAgent) Cancel(context.Context, acpsdk.CancelNotification) error { return nil } + +func (burstPermissionAgent) CloseSession(context.Context, acpsdk.CloseSessionRequest) (acpsdk.CloseSessionResponse, error) { + return acpsdk.CloseSessionResponse{}, nil +} + +func (burstPermissionAgent) DeleteSession(context.Context, acpsdk.DeleteSessionRequest) (acpsdk.DeleteSessionResponse, error) { + return acpsdk.DeleteSessionResponse{}, errors.New("not implemented") +} + +func (burstPermissionAgent) ListSessions(context.Context, acpsdk.ListSessionsRequest) (acpsdk.ListSessionsResponse, error) { + return acpsdk.ListSessionsResponse{}, nil +} + +func (burstPermissionAgent) NewSession(context.Context, acpsdk.NewSessionRequest) (acpsdk.NewSessionResponse, error) { + return acpsdk.NewSessionResponse{SessionId: permissionFlowSessionID}, nil +} + +func (burstPermissionAgent) ResumeSession(context.Context, acpsdk.ResumeSessionRequest) (acpsdk.ResumeSessionResponse, error) { + return acpsdk.ResumeSessionResponse{}, nil +} + +func (burstPermissionAgent) SetSessionConfigOption(context.Context, acpsdk.SetSessionConfigOptionRequest) (acpsdk.SetSessionConfigOptionResponse, error) { + return acpsdk.SetSessionConfigOptionResponse{}, nil +} + +func (burstPermissionAgent) SetSessionMode(context.Context, acpsdk.SetSessionModeRequest) (acpsdk.SetSessionModeResponse, error) { + return acpsdk.SetSessionModeResponse{}, nil +} + +func (a *permissionFlowAgent) LoadSession(context.Context, acpsdk.LoadSessionRequest) (acpsdk.LoadSessionResponse, error) { + return acpsdk.LoadSessionResponse{}, nil +} + +func (a *permissionFlowAgent) Prompt(ctx context.Context, req acpsdk.PromptRequest) (acpsdk.PromptResponse, error) { + title := a.title + kind := a.toolKind + status := acpsdk.ToolCallStatus("pending") + if err := a.conn.SessionUpdate(ctx, acpsdk.SessionNotification{ + SessionId: req.SessionId, + Update: acpsdk.SessionUpdate{ToolCall: &acpsdk.SessionUpdateToolCall{ + ToolCallId: permissionFlowToolCall, + Title: title, + Kind: kind, + Name: a.toolNamePtr(), + Meta: a.toolMeta, + Status: status, + }}, + }); err != nil { + return acpsdk.PromptResponse{}, err + } + + response, err := a.conn.RequestPermission(ctx, acpsdk.RequestPermissionRequest{ + SessionId: req.SessionId, + ToolCall: acpsdk.ToolCallUpdate{ + ToolCallId: permissionFlowToolCall, + Title: &title, + Kind: &kind, + Name: a.toolNamePtr(), + Meta: a.toolMeta, + }, + Options: []acpsdk.PermissionOption{ + {OptionId: "reject-once", Kind: acpsdk.PermissionOptionKindRejectOnce, Name: "Reject"}, + {OptionId: "allow-once", Kind: acpsdk.PermissionOptionKindAllowOnce, Name: "Allow once"}, + }, + }) + if err != nil { + return acpsdk.PromptResponse{}, err + } + if response.Outcome.Selected != nil { + a.selectedMu.Lock() + a.selected = string(response.Outcome.Selected.OptionId) + a.selectedMu.Unlock() + } + close(a.responseReady) + + if response.Outcome.Cancelled != nil { + return acpsdk.PromptResponse{StopReason: acpsdk.StopReasonEndTurn}, nil + } + + completed := acpsdk.ToolCallStatus("completed") + if err := a.conn.SessionUpdate(ctx, acpsdk.SessionNotification{ + SessionId: req.SessionId, + Update: acpsdk.SessionUpdate{ToolCallUpdate: &acpsdk.SessionToolCallUpdate{ + ToolCallId: permissionFlowToolCall, + Status: &completed, + RawOutput: "permission handled", + }}, + }); err != nil { + return acpsdk.PromptResponse{}, err + } + if err := a.conn.SessionUpdate(ctx, acpsdk.SessionNotification{ + SessionId: req.SessionId, + Update: acpsdk.SessionUpdate{AgentMessageChunk: &acpsdk.SessionUpdateAgentMessageChunk{ + Content: acpsdk.TextBlock("permission handled"), + }}, + }); err != nil { + return acpsdk.PromptResponse{}, err + } + return acpsdk.PromptResponse{StopReason: acpsdk.StopReasonEndTurn}, nil +} + +func (a *permissionFlowAgent) selectedOption() string { + a.selectedMu.Lock() + defer a.selectedMu.Unlock() + return a.selected +} + +func newPermissionACPFlow(t *testing.T, test permissionACPCase) (*Manager, *permissionFlowAgent, func()) { + t.Helper() + workDir := t.TempDir() + cfg := (&config.Config{ + Defaults: config.InstanceDefaults{ + Protocol: agent.ProtocolACP, + WorkDir: workDir, + }, + }).NewInstanceConfig(permissionACPPort, &config.InstanceOverrides{ + AgentType: "claude-acp", + WorkDir: workDir, + TaskID: test.taskID, + SessionID: permissionFlowSessionID, + McpServers: permissionFlowConfigMCPServers(), + }) + if !cfg.InjectedKandevMCP { + t.Fatal("NewInstanceConfig did not record injected Kandev MCP provenance") + } + if !injectedKandevMCPConfigured(cfg) { + t.Fatal("NewInstanceConfig did not retain the exact injected Kandev MCP entry") + } + var hasCollidingServer bool + for _, server := range cfg.McpServers { + if server.Name == "kandev__external" { + hasCollidingServer = true + break + } + } + if !hasCollidingServer { + t.Fatal("NewInstanceConfig did not retain the kandev__external test server") + } + manager := NewManager(cfg, newTestLogger(t)) + if err := manager.buildAdapterConfig(); err != nil { + t.Fatalf("buildAdapterConfig: %v", err) + } + + clientToAgentReader, clientToAgentWriter := io.Pipe() + agentToClientReader, agentToClientWriter := io.Pipe() + if err := manager.adapter.Connect(clientToAgentWriter, agentToClientReader); err != nil { + t.Fatalf("adapter Connect: %v", err) + } + fake := &permissionFlowAgent{ + toolName: test.toolName, + title: test.title, + toolKind: test.toolKind, + toolMeta: test.toolMeta, + responseReady: make(chan struct{}), + } + fake.conn = acpsdk.NewAgentSideConnection(fake, agentToClientWriter, clientToAgentReader) + + cleanup := func() { + _ = manager.adapter.Close() + _ = clientToAgentReader.Close() + _ = clientToAgentWriter.Close() + _ = agentToClientReader.Close() + _ = agentToClientWriter.Close() + manager.lifetimeCancel() + } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := manager.adapter.Initialize(ctx); err != nil { + cleanup() + t.Fatalf("adapter Initialize: %v", err) + } + return manager, fake, cleanup +} + +func (a *permissionFlowAgent) toolNamePtr() *string { + if a.toolName == "" { + return nil + } + return &a.toolName +} + +func claudePermissionMeta(toolName string) map[string]any { + return map[string]any{ + "claudeCode": map[string]any{"toolName": toolName}, + } +} + +func permissionFlowMCPServers() []types.McpServer { + return []types.McpServer{ + {Name: "kandev", Type: "http", URL: "http://localhost:43210/mcp"}, + {Name: "kandev__external", Type: "http", URL: "https://mcp.example.test/external"}, + } +} + +func permissionFlowConfigMCPServers() []config.McpServerConfig { + return []config.McpServerConfig{ + {Name: "kandev__external", Type: "http", URL: "https://mcp.example.test/external"}, + } +} + +func drainAdapterUpdates(agentAdapter adapter.AgentAdapter) { + for { + select { + case <-agentAdapter.Updates(): + default: + return + } + } +} + +func waitForPermissionEvent(t *testing.T, events <-chan adapter.AgentEvent) adapter.AgentEvent { + t.Helper() + deadline := time.NewTimer(2 * time.Second) + defer deadline.Stop() + for { + select { + case event := <-events: + if event.Type == streams.EventTypePermissionRequest { + return event + } + case <-deadline.C: + t.Fatal("timed out waiting for permission_request event") + } + } +} + +func collectPromptEvents(events <-chan adapter.AgentEvent) []adapter.AgentEvent { + var collected []adapter.AgentEvent + deadline := time.NewTimer(2 * time.Second) + defer deadline.Stop() + for { + select { + case event := <-events: + collected = append(collected, event) + if event.Type == streams.EventTypeComplete { + return collected + } + case <-deadline.C: + return collected + } + } +} + +func assertPromptEvents(t *testing.T, events []adapter.AgentEvent) { + t.Helper() + want := []string{ + streams.EventTypeToolCall, + streams.EventTypeToolUpdate, + streams.EventTypeMessageChunk, + streams.EventTypeComplete, + } + var got []string + for _, event := range events { + for _, wantType := range want { + if event.Type == wantType { + got = append(got, event.Type) + break + } + } + } + if len(got) != len(want) { + t.Fatalf("prompt event types = %v, want at least %v", got, want) + } + for i, wantType := range want { + if got[i] != wantType { + t.Fatalf("prompt event types = %v, want %v", got, want) + } + } +} diff --git a/apps/backend/internal/agentctl/server/process/manager_permission_policy.go b/apps/backend/internal/agentctl/server/process/manager_permission_policy.go new file mode 100644 index 0000000000..70f98f0657 --- /dev/null +++ b/apps/backend/internal/agentctl/server/process/manager_permission_policy.go @@ -0,0 +1,76 @@ +package process + +import ( + "fmt" + + "github.com/kandev/kandev/internal/agentctl/server/adapter" + "github.com/kandev/kandev/internal/agentctl/server/config" + "github.com/kandev/kandev/internal/agentctl/types" + "github.com/kandev/kandev/internal/agentctl/types/streams" + "go.uber.org/zap" +) + +// injectedKandevMCPServerName is the reserved server identifier used by the +// host-injected Kandev MCP entry. This must match kandevMCPServerName in +// adapter/transport/acp/adapter_session.go and kandevMcpServerName in +// server/config/config.go. +const injectedKandevMCPServerName = "kandev" + +func (m *Manager) autoApproveInjectedKandevPermission(req *adapter.PermissionRequest) (*adapter.PermissionResponse, bool) { + if req == nil || m.cfg == nil || !m.cfg.InjectedKandevMCP || m.cfg.Port <= 0 { + return nil, false + } + if !injectedKandevMCPConfigured(m.cfg) { + return nil, false + } + if req.ToolName == nil { + return nil, false + } + server, tool, ok := types.ParseQualifiedMCPToolName(*req.ToolName) + if !ok || server != injectedKandevMCPServerName { + return nil, false + } + + option, ok := injectedKandevPermissionOption(req.Options) + if !ok { + return nil, false + } + m.logger.Info("auto-approving injected Kandev MCP permission", + zap.String("reason", "injected_kandev_mcp"), + zap.String("tool", tool), + zap.String("option_kind", string(option.Kind))) + return &adapter.PermissionResponse{OptionID: option.OptionID}, true +} + +func injectedKandevMCPConfigured(cfg *config.InstanceConfig) bool { + for _, server := range cfg.McpServers { + if server.Name != injectedKandevMCPServerName || server.Command != "" || len(server.Args) != 0 || len(server.Env) != 0 || len(server.Headers) != 0 { + continue + } + switch server.Type { + case "http": + if server.URL == fmt.Sprintf("http://localhost:%d/mcp", cfg.Port) { + return true + } + case "sse": + if server.URL == fmt.Sprintf("http://localhost:%d/sse", cfg.Port) { + return true + } + } + } + return false +} + +func injectedKandevPermissionOption(options []adapter.PermissionOption) (adapter.PermissionOption, bool) { + for _, allowedKind := range []streams.PermissionOptionKind{ + streams.PermissionOptionKindAllowOnce, + streams.PermissionOptionKindAllowAlways, + } { + for _, option := range options { + if option.Kind == allowedKind { + return option, true + } + } + } + return adapter.PermissionOption{}, false +} diff --git a/apps/backend/internal/agentctl/server/process/manager_permission_policy_test.go b/apps/backend/internal/agentctl/server/process/manager_permission_policy_test.go new file mode 100644 index 0000000000..eaf69f109e --- /dev/null +++ b/apps/backend/internal/agentctl/server/process/manager_permission_policy_test.go @@ -0,0 +1,255 @@ +package process + +import ( + "context" + "strconv" + "testing" + "time" + + "github.com/kandev/kandev/internal/agentctl/server/adapter" + "github.com/kandev/kandev/internal/agentctl/server/config" + "github.com/kandev/kandev/internal/agentctl/types/streams" +) + +func TestInjectedKandevPermissionPolicy(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + toolName := "mcp__kandev__update_task_plan_kandev" + + m := &Manager{ + cfg: &config.InstanceConfig{ + Port: 43210, + InjectedKandevMCP: true, + McpServers: []config.McpServerConfig{ + {Name: "kandev", Type: "http", URL: "http://localhost:43210/mcp"}, + {Name: "kandev", Type: "sse", URL: "http://localhost:43210/sse"}, + {Name: "third-party", Type: "http", URL: "https://mcp.example.test/mcp"}, + }, + }, + logger: newTestLogger(t), + updatesCh: make(chan adapter.AgentEvent, 2), + pendingPermissions: make(map[string]*PendingPermission), + } + + resultCh := make(chan struct { + response *adapter.PermissionResponse + err error + }, 1) + go func() { + response, err := m.handlePermissionRequest(ctx, &adapter.PermissionRequest{ + SessionID: "session-1", + ToolCallID: "tool-1", + Title: "Update task plan", + ToolName: &toolName, + ActionType: string(streams.ActionTypeOther), + Options: []adapter.PermissionOption{ + {OptionID: "reject", Kind: streams.PermissionOptionKindRejectOnce}, + {OptionID: "allow-once", Kind: streams.PermissionOptionKindAllowOnce}, + }, + }) + resultCh <- struct { + response *adapter.PermissionResponse + err error + }{response: response, err: err} + }() + + select { + case result := <-resultCh: + if result.err != nil { + t.Fatalf("handlePermissionRequest returned error: %v", result.err) + } + if result.response == nil || result.response.Cancelled || result.response.OptionID != "allow-once" { + t.Fatalf("response = %+v, want allow-once without cancellation", result.response) + } + case event := <-m.updatesCh: + cancel() + select { + case <-resultCh: + case <-time.After(time.Second): + t.Fatal("permission request did not stop after cancellation") + } + t.Fatalf("injected Kandev request emitted %q instead of being approved", event.Type) + } +} + +func TestInjectedKandevPermissionOptions(t *testing.T) { + m := injectedKandevPermissionManager(t, injectedKandevMCPServers(43210)) + toolName := "mcp__kandev__update_task_plan_kandev" + + tests := []struct { + name string + options []adapter.PermissionOption + wantOption string + wantOK bool + }{ + { + name: "allow once wins regardless of order", + options: []adapter.PermissionOption{ + {OptionID: "reject", Kind: streams.PermissionOptionKindRejectOnce}, + {OptionID: "allow-always", Kind: streams.PermissionOptionKindAllowAlways}, + {OptionID: "allow-once", Kind: streams.PermissionOptionKindAllowOnce}, + }, + wantOption: "allow-once", + wantOK: true, + }, + { + name: "allow always is fallback", + options: []adapter.PermissionOption{ + {OptionID: "reject", Kind: streams.PermissionOptionKindRejectOnce}, + {OptionID: "allow-always", Kind: streams.PermissionOptionKindAllowAlways}, + }, + wantOption: "allow-always", + wantOK: true, + }, + { + name: "reject only retains normal flow", + options: []adapter.PermissionOption{ + {OptionID: "reject", Kind: streams.PermissionOptionKindRejectOnce}, + }, + wantOK: false, + }, + { + name: "unknown option kind retains normal flow", + options: []adapter.PermissionOption{ + {OptionID: "unknown", Kind: streams.PermissionOptionKind("maybe")}, + }, + wantOK: false, + }, + { + name: "empty options retain normal flow", + options: nil, + wantOK: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + response, ok := m.autoApproveInjectedKandevPermission(&adapter.PermissionRequest{ + ToolName: &toolName, + Options: test.options, + }) + if ok != test.wantOK { + t.Fatalf("approved = %v, want %v; response=%+v", ok, test.wantOK, response) + } + if !test.wantOK { + return + } + if response == nil || response.Cancelled || response.OptionID != test.wantOption { + t.Fatalf("response = %+v, want option %q", response, test.wantOption) + } + }) + } +} + +func TestInjectedKandevPermissionPolicyCoversServerTools(t *testing.T) { + m := injectedKandevPermissionManager(t, injectedKandevMCPServers(43210)) + for _, toolName := range []string{ + "mcp__kandev__create_task_kandev", + "mcp__kandev__update_task_plan_kandev", + "mcp__kandev__delete_task_kandev", + } { + t.Run(toolName, func(t *testing.T) { + response, approved := m.autoApproveInjectedKandevPermission(&adapter.PermissionRequest{ + ToolName: permissionStringPtr(toolName), + Options: []adapter.PermissionOption{{ + OptionID: "allow-once", + Kind: streams.PermissionOptionKindAllowOnce, + }}, + }) + if !approved || response == nil || response.OptionID != "allow-once" { + t.Fatalf("tool %q response = %+v, approved = %v", toolName, response, approved) + } + }) + } +} + +func TestInjectedKandevPolicyRunsAfterBlanketApproval(t *testing.T) { + m := injectedKandevPermissionManager(t, injectedKandevMCPServers(43210)) + m.cfg.AutoApprovePermissions = true + toolName := "mcp__kandev__update_task_plan_kandev" + response, err := m.handlePermissionRequest(context.Background(), &adapter.PermissionRequest{ + ToolName: &toolName, + Options: []adapter.PermissionOption{ + {OptionID: "allow-always", Kind: streams.PermissionOptionKindAllowAlways}, + {OptionID: "allow-once", Kind: streams.PermissionOptionKindAllowOnce}, + }, + }) + if err != nil { + t.Fatalf("handlePermissionRequest returned error: %v", err) + } + if response == nil || response.OptionID != "allow-always" { + t.Fatalf("blanket response = %+v, want first offered allow-always", response) + } +} + +func TestInjectedKandevPermissionPolicyRejectsUntrustedRequests(t *testing.T) { + matchingTitle := "mcp__kandev__update_task_plan_kandev" + + tests := []struct { + name string + toolName *string + title string + servers []config.McpServerConfig + injectedProvenance bool + wantApproved bool + }{ + {name: "every injected tool is eligible", toolName: stringPtr("mcp__kandev__delete_task_kandev"), servers: injectedKandevMCPServers(43210), injectedProvenance: true, wantApproved: true}, + {name: "matching title cannot establish identity", toolName: nil, servers: injectedKandevMCPServers(43210), injectedProvenance: true, wantApproved: false}, + {name: "nonmatching programmatic name wins over title", toolName: permissionStringPtr("mcp__other__tool"), servers: injectedKandevMCPServers(43210), injectedProvenance: true, wantApproved: false}, + {name: "Bash title resembling MCP name is not identity", toolName: permissionStringPtr("bash"), title: "Run mcp__kandev__update_task_plan_kandev", servers: injectedKandevMCPServers(43210), injectedProvenance: true, wantApproved: false}, + {name: "suffix alone is not qualified", toolName: permissionStringPtr("update_task_plan_kandev"), servers: injectedKandevMCPServers(43210), injectedProvenance: true, wantApproved: false}, + {name: "empty qualified suffix is rejected", toolName: permissionStringPtr("mcp__kandev__"), servers: injectedKandevMCPServers(43210), injectedProvenance: true, wantApproved: false}, + {name: "invalid characters are rejected", toolName: permissionStringPtr("mcp__kandev__delete/task"), servers: injectedKandevMCPServers(43210), injectedProvenance: true, wantApproved: false}, + {name: "missing provenance is rejected", toolName: permissionStringPtr("mcp__kandev__update_task_plan_kandev"), servers: injectedKandevMCPServers(43210), wantApproved: false}, + {name: "wrong port is rejected", toolName: permissionStringPtr("mcp__kandev__update_task_plan_kandev"), servers: injectedKandevMCPServers(43211), injectedProvenance: true, wantApproved: false}, + {name: "stdio server is rejected", toolName: permissionStringPtr("mcp__kandev__update_task_plan_kandev"), servers: []config.McpServerConfig{{Name: "kandev", Type: "stdio", Command: "agent"}}, injectedProvenance: true, wantApproved: false}, + {name: "streamable HTTP is not an injected transport", toolName: permissionStringPtr("mcp__kandev__update_task_plan_kandev"), servers: []config.McpServerConfig{{Name: "kandev", Type: "streamable_http", URL: "http://localhost:43210/mcp"}}, injectedProvenance: true, wantApproved: false}, + {name: "configured command invalidates HTTP entry", toolName: permissionStringPtr("mcp__kandev__update_task_plan_kandev"), servers: []config.McpServerConfig{{Name: "kandev", Type: "http", URL: "http://localhost:43210/mcp", Command: "spoof"}}, injectedProvenance: true, wantApproved: false}, + {name: "reserved-name spoof with headers is rejected", toolName: permissionStringPtr("mcp__kandev__update_task_plan_kandev"), servers: []config.McpServerConfig{{Name: "kandev", Type: "http", URL: "http://localhost:43210/mcp", Headers: map[string]string{"Authorization": "spoof"}}}, injectedProvenance: true, wantApproved: false}, + {name: "unrelated server does not become eligible", toolName: permissionStringPtr("mcp__third-party__tool"), servers: injectedKandevMCPServers(43210), injectedProvenance: true, wantApproved: false}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + m := injectedKandevPermissionManager(t, test.servers) + m.cfg.InjectedKandevMCP = test.injectedProvenance + title := test.title + if title == "" { + title = matchingTitle + } + response, approved := m.autoApproveInjectedKandevPermission(&adapter.PermissionRequest{ + Title: title, + ToolName: test.toolName, + Options: []adapter.PermissionOption{{ + OptionID: "allow-once", + Kind: streams.PermissionOptionKindAllowOnce, + }}, + }) + if approved != test.wantApproved { + t.Fatalf("approved = %v, response = %+v, want %v", approved, response, test.wantApproved) + } + }) + } +} + +func injectedKandevPermissionManager(t *testing.T, servers []config.McpServerConfig) *Manager { + t.Helper() + return &Manager{ + cfg: &config.InstanceConfig{ + Port: 43210, + InjectedKandevMCP: true, + McpServers: servers, + }, + logger: newTestLogger(t), + } +} + +func injectedKandevMCPServers(port int) []config.McpServerConfig { + return []config.McpServerConfig{ + {Name: "kandev", Type: "http", URL: "http://localhost:" + strconv.Itoa(port) + "/mcp"}, + {Name: "kandev", Type: "sse", URL: "http://localhost:" + strconv.Itoa(port) + "/sse"}, + {Name: "third-party", Type: "http", URL: "https://mcp.example.test/mcp"}, + } +} + +func permissionStringPtr(value string) *string { return &value } diff --git a/apps/backend/internal/agentctl/types/permission_identity.go b/apps/backend/internal/agentctl/types/permission_identity.go new file mode 100644 index 0000000000..98d7e72f71 --- /dev/null +++ b/apps/backend/internal/agentctl/types/permission_identity.go @@ -0,0 +1,49 @@ +package types + +import "strings" + +// ParseQualifiedMCPToolName parses an unambiguous protocol-qualified MCP tool +// name used by ACP clients, such as mcp__kandev__update_task_plan_kandev. +// The flattened format cannot identify a boundary when either side contains +// a delimiter-like underscore sequence, so those names fail closed. +func ParseQualifiedMCPToolName(name string) (server, tool string, ok bool) { + if !strings.HasPrefix(name, "mcp__") { + return "", "", false + } + + qualified := strings.TrimPrefix(name, "mcp__") + separator := strings.Index(qualified, "__") + if !unambiguousQualifiedMCPSeparator(qualified, separator) { + return "", "", false + } + server, tool = qualified[:separator], qualified[separator+2:] + if !permissionIdentifier(server) || !permissionIdentifier(tool) { + return "", "", false + } + return server, tool, true +} + +func unambiguousQualifiedMCPSeparator(qualified string, separator int) bool { + if separator <= 0 || separator+2 >= len(qualified) { + return false + } + if qualified[separator-1] == '_' || qualified[separator+2] == '_' { + return false + } + return !strings.Contains(qualified[separator+2:], "__") +} + +func permissionIdentifier(value string) bool { + if value == "" { + return false + } + for _, char := range value { + if (char < 'a' || char > 'z') && + (char < 'A' || char > 'Z') && + (char < '0' || char > '9') && + char != '_' && char != '-' { + return false + } + } + return true +} diff --git a/apps/backend/internal/agentctl/types/permission_identity_test.go b/apps/backend/internal/agentctl/types/permission_identity_test.go new file mode 100644 index 0000000000..8b8cc65957 --- /dev/null +++ b/apps/backend/internal/agentctl/types/permission_identity_test.go @@ -0,0 +1,36 @@ +package types + +import "testing" + +func TestParseQualifiedMCPToolName(t *testing.T) { + tests := []struct { + name string + input string + wantServer string + wantTool string + wantOK bool + }{ + {name: "qualified tool", input: "mcp__kandev__update_task_plan_kandev", wantServer: "kandev", wantTool: "update_task_plan_kandev", wantOK: true}, + {name: "hyphenated identifiers", input: "mcp__third-party__tool-name", wantServer: "third-party", wantTool: "tool-name", wantOK: true}, + {name: "single underscores in tool", input: "mcp__kandev__tool_with_underscores", wantServer: "kandev", wantTool: "tool_with_underscores", wantOK: true}, + {name: "delimiter in tool suffix is ambiguous", input: "mcp__kandev__tool__with__underscores"}, + {name: "delimiter in server name is ambiguous", input: "mcp__kandev__external__execute"}, + {name: "trailing underscore server is ambiguous", input: "mcp__kandev___execute"}, + {name: "missing prefix", input: "kandev__update_task_plan_kandev"}, + {name: "missing server", input: "mcp____update_task_plan_kandev"}, + {name: "missing tool", input: "mcp__kandev__"}, + {name: "whitespace", input: "mcp__kandev__tool name"}, + {name: "slash", input: "mcp__kandev__tool/name"}, + {name: "unicode identifier", input: "mcp__kandev__工具"}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + server, tool, ok := ParseQualifiedMCPToolName(test.input) + if server != test.wantServer || tool != test.wantTool || ok != test.wantOK { + t.Fatalf("ParseQualifiedMCPToolName(%q) = (%q, %q, %v), want (%q, %q, %v)", + test.input, server, tool, ok, test.wantServer, test.wantTool, test.wantOK) + } + }) + } +} diff --git a/apps/backend/internal/agentctl/types/types.go b/apps/backend/internal/agentctl/types/types.go index 46d5b584e1..d44e942f3e 100644 --- a/apps/backend/internal/agentctl/types/types.go +++ b/apps/backend/internal/agentctl/types/types.go @@ -121,6 +121,14 @@ type PermissionRequest struct { Title string `json:"title"` Options []PermissionOption `json:"options"` + // ToolName is the optional programmatic identity supplied by ACP. It is + // retained for internal permission policy decisions and is never serialized + // into public permission snapshots. + ToolName *string `json:"-"` + // ToolMeta contains provider-specific ACP metadata used by adapter dialects. + // It is internal and must not be exposed through permission snapshots. + ToolMeta map[string]any `json:"-"` + // PendingID is the unique identifier for this permission request. // If set by the adapter (e.g., OpenCode's "per_xxx" ID or Claude Code's requestID), // the process manager will use this ID instead of generating a new one. diff --git a/docs/decisions/2026-09-16-injected-mcp-approval.md b/docs/decisions/2026-09-16-injected-mcp-approval.md new file mode 100644 index 0000000000..1633b15e94 --- /dev/null +++ b/docs/decisions/2026-09-16-injected-mcp-approval.md @@ -0,0 +1,42 @@ +# ADR-2026-09-16-injected-mcp-approval: Approve the injected Kandev server through ACP + +**Status:** accepted +**Date:** 2026-09-16 +**Area:** backend, protocol + +## Context + +Issue #3729 reports repeated Claude ACP permission prompts for Kandev plan and task tools. +Codex already receives server-wide approval through its launch configuration. +The issue explicitly offers server-wide Claude parity as its minimum fix. + +## Decision + +The proposed repair adds a server-specific decision before ACP creates a pending permission request. +It requires a provider tool identity and host-owned injection provenance. +It approves every tool on the injected Kandev server, not only bookkeeping tools. +It does not enable blanket approval or change task authorization. + +Provider naming compatibility belongs in pure adapter dialect hooks. +The process manager owns policy and choice selection. +Unknown identities retain normal permission handling. +The flattened `mcp__server__tool` form is accepted only when it has one +unambiguous separator. Delimiter collisions and adjacent underscores remain +pending because the server/tool boundary cannot be trusted. + +The [requirements](../specs/agents/requirements/injected-mcp-approval.md) +and [design](../specs/agents/system-design/injected-mcp-approval.md) define the implemented boundary. + +## Consequences + +Claude can update plans and task titles without repeated prompts. +The same policy also approves task creation, deletion, and session-management calls on the injected server. +Existing server-side authorization and user-input barriers still apply. +No approval history, new profile field, or database migration is necessary. + +## Alternatives considered + +- Blanket profile approval also permits shell and third-party operations. It exceeds the requested boundary. +- A selective allowlist and a restore-prompting setting need a different policy across Claude and Codex. They are outside this minimum package. +- CLI-only allow flags depend on provider behavior and do not resolve the confirmed host-side pending-request path. +- A generic title-prefix match cannot distinguish provider presentation from programmatic identity. The design restricts legacy title interpretation to Claude. diff --git a/docs/plans/claude-injected-mcp-approval/plan.md b/docs/plans/claude-injected-mcp-approval/plan.md new file mode 100644 index 0000000000..65b9e766b3 --- /dev/null +++ b/docs/plans/claude-injected-mcp-approval/plan.md @@ -0,0 +1,107 @@ +--- +created: 2026-09-16 +status: complete +requirements: + - REQ-AGENTS-INJECTED-MCP-APPROVAL-001 +system_design: + - ../../specs/agents/system-design/injected-mcp-approval.md +legacy_specs: [] +--- + +# Fix plan: Claude injected MCP approval + +## Overview + +Resolve [issue #3729](https://github.com/kdlbs/kandev/issues/3729) through its minimum server-wide parity option. +The issue is assigned to `carlosflorencio`. +One sequential work order implements the complete permission path and its regression coverage. +Implementation is complete through the single work order below. + +## Evidence and root cause + +Investigated commit: `4c6e80091d82b4302e9dfc0a17b80965f1d4c62d`. +The canonical issue has no comments or image attachments at investigation time. + +The process manager checks only blanket `AutoApprovePermissions` before it stores a pending request. +There is no injected-server exception. The ACP client also discards the programmatic tool name. +Codex's separate passthrough strategy already emits a server-wide approval override. + +A temporary `TestIssue3729Repro` used `Config.NewInstanceConfig(43210, nil)` and the real manager permission handler. +It submitted `mcp__kandev__update_task_plan_kandev`, kind `other`, with allow-once and reject-once choices. +Blanket approval remained false. The handler emitted `permission_request` and waited until context cancellation. + +Command run: + +```bash +(cd apps/backend && go test ./internal/agentctl/server/process -run '^TestIssue3729Repro$' -count=1 -v) +``` + +Result: PASS, with the unwanted permission event observed. The temporary test was removed after execution. +This proves the host permission path, not a live Claude or desktop session. + +## Scope + +### In scope + +- Preserve ACP programmatic tool identity and normalize legacy Claude identity. +- Approve the host-injected server before pending-request creation. +- Preserve unrelated permissions and normal tool events. +- Document the server-wide policy and cover fresh and resumed configurations. + +### Out of scope + +- Per-tool allowlists, new settings, persistence, and Claude terminal passthrough. +- Frontend changes, provider upgrades, and unverified provider dialects. +- Changes to Codex policy or server-side authorization. + +## Technical approach + +Follow the [system design](../../specs/agents/system-design/injected-mcp-approval.md). +The implementation crosses the existing ACP client, adapter, instance configuration, and process manager. +Use new focused helper and test files instead of expanding large manager files. + +## Tests + +| Criteria | Permanent regression evidence | +| --- | --- | +| 001.1, 001.3, 001.8 | `client_permission_identity_test.go:TestPermissionProgrammaticName` and `dialect_claude_test.go:TestClaudePermissionIdentity` | +| 001.2, 001.4, 001.7 | `config_test.go:TestInjectedKandevMCPProvenance` and `manager_permission_policy_test.go:TestInjectedKandevPermissionPolicy` | +| 001.5, 001.6 | `manager_permission_policy_test.go:TestInjectedKandevPermissionOptions` | +| 001.1, 001.3, 001.7, 001.8 | `manager_permission_acp_test.go:TestInjectedKandevPermissionACPFlow` | + +The first policy regression must fail on current code because the manager emits a pending request. +Tests must include mixed server lists, spoofed names, a Bash title that resembles an MCP name, and reversed option ordering. + +## End-to-end evidence + +The Go protocol integration test connects the real ACP conversion, adapter, and process manager through an in-memory ACP peer. +It emits a Kandev permission request and verifies the selected wire response without a pending permission event. +It then emits Bash and third-party requests and verifies that both remain pending until explicit resolution. +Run the same sequence with fresh and resumed session identities and with task and Quick Chat instance contexts. +Verify normal tool-call/result events remain present. +This protocol boundary is the deterministic end-to-end evidence. No artificial browser test or UI redraw is required. + +## Work orders + +- [x] [Task 01: Implement injected server approval](task-01-injected-server-approval.md) + +## Verification results + +Investigation reproduction passed as described above. Implementation and verification passed. + +- `(cd apps/backend && go test -race ./internal/agentctl/server/acp ./internal/agentctl/server/adapter/transport/acp ./internal/agentctl/server/config ./internal/agentctl/server/process ./internal/agent/mcpconfig -count=1)`: passed. +- `node --test scripts/validate-public-docs.test.mjs`: passed. +- `node scripts/validate-public-docs.mjs`: passed. +- `python3 scripts/list-docs.py validate`: passed. +- `python3 scripts/lint-spec-files.test.py`: passed. +- `python3 scripts/lint-spec-files.py --all`: passed. +- `git diff --check`: passed. +- Catalog discovery found both new agent specification files. +- GitHub assignment readback confirmed `carlosflorencio`. + +## Risks + +- The minimum policy approves all injected Kandev tools, including destructive and work-spawning tools. +- Legacy Claude title interpretation requires exact provider and kind checks. It must not become a generic title heuristic. +- Installed provider formats can differ. Unsupported shapes retain prompts, and compatibility claims require fixtures. +- No live provider session was run during investigation. diff --git a/docs/plans/claude-injected-mcp-approval/task-01-injected-server-approval.md b/docs/plans/claude-injected-mcp-approval/task-01-injected-server-approval.md new file mode 100644 index 0000000000..ef8a243f6a --- /dev/null +++ b/docs/plans/claude-injected-mcp-approval/task-01-injected-server-approval.md @@ -0,0 +1,151 @@ +--- +id: "01-injected-server-approval" +title: "Implement injected server approval" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-AGENTS-INJECTED-MCP-APPROVAL-001 +acceptance_criteria: + - AC-AGENTS-INJECTED-MCP-APPROVAL-001.1 + - AC-AGENTS-INJECTED-MCP-APPROVAL-001.2 + - AC-AGENTS-INJECTED-MCP-APPROVAL-001.3 + - AC-AGENTS-INJECTED-MCP-APPROVAL-001.4 + - AC-AGENTS-INJECTED-MCP-APPROVAL-001.5 + - AC-AGENTS-INJECTED-MCP-APPROVAL-001.6 + - AC-AGENTS-INJECTED-MCP-APPROVAL-001.7 + - AC-AGENTS-INJECTED-MCP-APPROVAL-001.8 +system_design: + - ../../specs/agents/system-design/injected-mcp-approval.md +--- + +# Task 01: Implement injected server approval + +## Summary + +Add a permission decision for the host-injected Kandev server without enabling blanket approval. +Preserve identity through the ACP conversion and use strict provider normalization. + +## In scope + +- Start with a failing manager regression for an injected Kandev plan update with blanket approval disabled. +- Add identity fields, Claude dialect normalization, injection provenance, and offered-option selection through the documented path. +- Add the unit and protocol integration tests named in the plan. +- Update the permission sections in `docs/public/agents-and-profiles.md` and scoped agentctl guidance. +- Mark the ADR accepted only after explicit implementation authorization for this package. + +## Out of scope + +- New permission settings, storage changes, UI controls, and unrelated approval cleanup. +- Changes to blanket approval's existing first-option fallback. +- Removing Codex's existing launch override. + +## Acceptance + +1. All listed criteria pass through real production seams, including the protocol integration test. +2. Unknown identity, forged title, unrelated server, missing provenance, and missing allow choices never receive approval through the new policy. +3. Public reference text explains the server-wide exception, its destructive-tool scope, and its independence from blanket approval. + +## Test matrix + +- Names: programmatic qualified name, Claude metadata, legacy Claude title, empty name, malformed metadata, conflicting fields, wrong server, suffix-only name. +- Operations: plan creation/update, title update, task deletion/creation, Bash, file edit, third-party MCP. +- Injection: actual constructor, zero port, user-supplied reserved name, wrong port/path/transport, mixed server inventory, recreated instance. +- Options: reject first, allow-always first with allow-once later, allow-always only, reject only, empty, unknown kinds. +- Lifecycle: fresh/resumed session, task/Quick Chat context, cancellation of a remaining prompt, blanket approval on/off. +- Streams: normal tool-call/result events, no automatic-path permission card, normal explicit resolution for unrelated tools. + +Use channel synchronization and cleanup registration before assertions. +Do not use arbitrary sleeps or a real user instance. + +## Verification + +Run from the repository root: + +```bash +(cd apps/backend && go test -race ./internal/agentctl/server/acp ./internal/agentctl/server/adapter/transport/acp ./internal/agentctl/server/config ./internal/agentctl/server/process ./internal/agent/mcpconfig -count=1) +node --test scripts/validate-public-docs.test.mjs +node scripts/validate-public-docs.mjs +python3 scripts/list-docs.py validate +python3 scripts/lint-spec-files.test.py +python3 scripts/lint-spec-files.py --all +git diff --check +``` + +Record the initial failing regression and each final command result. + +## Files likely touched + +Paths below are repository-relative. New files are marked `(new)`. + +- `apps/backend/internal/agentctl/types/types.go` +- `apps/backend/internal/agentctl/types/permission_identity.go` (new) +- `apps/backend/internal/agentctl/types/permission_identity_test.go` (new) +- `apps/backend/internal/agentctl/server/acp/client.go` +- `apps/backend/internal/agentctl/server/acp/client_permission_identity_test.go` (new) +- `apps/backend/internal/agentctl/server/adapter/transport/acp/dialect.go` +- `apps/backend/internal/agentctl/server/adapter/transport/acp/dialect_claude.go` (new) +- `apps/backend/internal/agentctl/server/adapter/transport/acp/dialect_claude_test.go` (new) +- `apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_permissions.go` +- `apps/backend/internal/agentctl/server/config/config.go` +- `apps/backend/internal/agentctl/server/config/config_test.go` +- `apps/backend/internal/agentctl/server/process/manager.go` +- `apps/backend/internal/agentctl/server/process/manager_permission_policy.go` (new) +- `apps/backend/internal/agentctl/server/process/manager_permission_policy_test.go` (new) +- `apps/backend/internal/agentctl/server/process/manager_permission_acp_test.go` (new) +- `apps/backend/internal/agentctl/AGENTS.md` +- `docs/public/agents-and-profiles.md` +- `docs/decisions/2026-09-16-injected-mcp-approval.md` + +## Dependencies + +None. + +## Risks + +Server-wide approval includes task deletion and creation. +The implementation must not infer trust from raw input or a display title from an unknown provider. +Public permission snapshots must not expose the newly preserved provider metadata. + +## Parallelism + +`sequential` + +## Inputs + +- [Requirements](../../specs/agents/requirements/injected-mcp-approval.md) +- [System design](../../specs/agents/system-design/injected-mcp-approval.md) +- [Plan and evidence](plan.md) +- Existing `client_test.go` permission conversion tests. +- Existing `manager_permission_test.go` pending-request and cancellation tests. +- Existing `config_test.go` injected HTTP/SSE ordering tests. +- Repository `/tdd` skill and its backend test reference. + +## Results + +Implemented the Claude ACP injected-server permission path. ACP conversion now +preserves internal programmatic identity and metadata, the Claude dialect +normalizes only verified identity forms, and instance construction records +host-injection provenance. The process manager approves every qualified tool +on the exact injected Kandev HTTP or SSE server, preferring allow-once and then +allow-always, while blanket approval and all unrelated pending flows remain +unchanged. Flattened names with delimiter collisions fail closed to the +pending flow. Permanent unit and in-memory ACP protocol tests cover fresh and +resumed task and Quick Chat contexts, destructive and planning tools, spoofed +identities, malformed metadata, option ordering, normal tool/result events, and +explicit unrelated-tool resolution. + +The public permissions reference and scoped agentctl guidance document the +server-wide exception and its independent authorization boundaries. The ADR is +accepted, and the paired requirement and system design are active/current. + +Verification passed: + +- `(cd apps/backend && go test -race ./internal/agentctl/server/acp ./internal/agentctl/server/adapter/transport/acp ./internal/agentctl/server/config ./internal/agentctl/server/process ./internal/agent/mcpconfig -count=1)` +- `node --test scripts/validate-public-docs.test.mjs` +- `node scripts/validate-public-docs.mjs` +- `python3 scripts/list-docs.py validate` +- `python3 scripts/lint-spec-files.test.py` +- `python3 scripts/lint-spec-files.py --all` +- `git diff --check` diff --git a/docs/public/agents-and-profiles.md b/docs/public/agents-and-profiles.md index c93fc4be57..320e9259ba 100644 --- a/docs/public/agents-and-profiles.md +++ b/docs/public/agents-and-profiles.md @@ -383,6 +383,8 @@ Literal values remain in profile configuration. A secret reference avoids copyin In a structured ACP session, the agent can present a permission request and its available responses. With **Auto-approve all permissions** disabled, a person chooses a response in the session. With it enabled, the runtime selects the first allow-once or allow-always response without waiting. If the agent supplies no allow response, Kandev selects its first response even when that response is not approval; with no responses, it cancels. +Claude ACP also has a narrower built-in rule. When the agent uses the host-injected `kandev` MCP server, Kandev automatically selects an offered allow-once response, or allow-always when allow-once is not offered, even when **Auto-approve all permissions** is disabled. This covers every tool on that server, including task creation, task deletion, and plan changes. It is not a saved approval decision and is recreated for each task or Quick Chat session. Kandev still enforces MCP authentication, task and session authorization, user-question barriers, and workflow gates. Shell commands, file operations, third-party tools, and ambiguous requests keep the normal permission flow. + An external MCP client can also list live pending requests for an authorized task and submit one exact option originally offered by the agent. The request-generation ID prevents an old approval from targeting a replacement prompt that reused a provider pending ID. Kandev records who selected diff --git a/docs/specs/agents/requirements/injected-mcp-approval.md b/docs/specs/agents/requirements/injected-mcp-approval.md new file mode 100644 index 0000000000..147c7a56e4 --- /dev/null +++ b/docs/specs/agents/requirements/injected-mcp-approval.md @@ -0,0 +1,47 @@ +--- +status: active +system: agents +created: 2026-09-16 +owners: + - kandev +--- + +# Injected MCP approval requirements + +## Overview + +Claude ACP sessions require repeated approval for Kandev task and plan tools. +Codex already receives automatic approval for the injected Kandev server. +The agent system owns this provider permission contract. + +This package adopts the minimum server-wide parity option in +[issue #3729](https://github.com/kdlbs/kandev/issues/3729). +It does not adopt the issue's separate proposal for selective approval and a new setting. + +## Requirements + +### REQ-AGENTS-INJECTED-MCP-APPROVAL-001: Approval for the injected Kandev server + +**Intent:** Users can use Kandev tools through Claude ACP without enabling blanket approval for the agent. + +#### Acceptance criteria + +- **AC-AGENTS-INJECTED-MCP-APPROVAL-001.1:** With blanket approval disabled, identifiable calls to the injected Kandev server shall proceed without permission prompts in Claude ACP sessions. +- **AC-AGENTS-INJECTED-MCP-APPROVAL-001.2:** This policy shall apply to every tool on that server, including task creation and deletion. Tool authorization shall remain enforced. +- **AC-AGENTS-INJECTED-MCP-APPROVAL-001.3:** Shell commands, file operations, third-party tools, and ambiguous requests shall retain their existing permission behavior. +- **AC-AGENTS-INJECTED-MCP-APPROVAL-001.4:** A user-configured server with a similar or identical name shall not receive approval through this policy. +- **AC-AGENTS-INJECTED-MCP-APPROVAL-001.5:** Automatic approval shall select an offered allow-once choice. If none exists, it shall use an offered allow-always choice. +- **AC-AGENTS-INJECTED-MCP-APPROVAL-001.6:** Without an offered allow choice, the request shall retain its normal permission behavior. A request without choices shall cancel. +- **AC-AGENTS-INJECTED-MCP-APPROVAL-001.7:** Fresh and resumed task sessions, including Quick Chat, shall use the same policy without a saved approval decision. +- **AC-AGENTS-INJECTED-MCP-APPROVAL-001.8:** Existing Codex approval and blanket approval behavior shall remain unchanged. Unknown provider naming formats shall retain normal permission behavior. + +## Out of scope + +- Selective tool lists, persisted approval decisions, and new profile or workspace settings. +- Claude terminal passthrough, which does not use ACP permission frames. +- Automatic support claims for unverified provider naming formats. +- Changes to question barriers, workflow gates, or server-side task authorization. + +## Implementation plans + +- [Issue 3729 fix package](../../../plans/claude-injected-mcp-approval/plan.md) diff --git a/docs/specs/agents/system-design/injected-mcp-approval.md b/docs/specs/agents/system-design/injected-mcp-approval.md new file mode 100644 index 0000000000..ae4cc96618 --- /dev/null +++ b/docs/specs/agents/system-design/injected-mcp-approval.md @@ -0,0 +1,123 @@ +--- +status: current +system: agents +requirements: + - REQ-AGENTS-INJECTED-MCP-APPROVAL-001 +--- + +# Injected MCP approval design + +## Purpose and boundaries + +The process manager owns the automatic permission decision. +The ACP client preserves provider identity fields, and the adapter normalizes provider-specific names. +The MCP server continues to enforce task and session authorization. + +## Requirement mapping + +| Acceptance criteria | Design section | +| --- | --- | +| 001.1, 001.3, 001.8 | Identity normalization | +| 001.2, 001.4 | Injection provenance | +| 001.5, 001.6 | Permission decision | +| 001.7 | Lifecycle and observability | + +All references belong to `REQ-AGENTS-INJECTED-MCP-APPROVAL-001`. + +## Current path and defect + +`internal/agent/mcpconfig/passthrough.go:codexServerArgs` adds +`default_tools_approval_mode=approve` for the `kandev` server. +ACP uses a different path: + +1. `server/acp/client.go:forwardPermissionRequest` converts the permission frame. +2. `transport/acp/adapter_permissions.go:handlePermissionRequest` forwards it. +3. `server/process/manager.go:handlePermissionRequest` checks only `AutoApprovePermissions`. +4. Without blanket approval, the manager creates a pending request and waits. + +The ACP conversion currently drops the programmatic `ToolCall.Name` and its metadata. +The pinned Go SDK already supports both fields. No SDK upgrade is necessary. +The shared adapter's `AutoApprove` field does not implement the permission decision. + +## Identity normalization + +Extend the internal `types.PermissionRequest` with optional `ToolName` and `ToolMeta` fields. +Copy `ToolCall.Name` and `ToolCall.Meta` in `forwardPermissionRequest`. +These fields remain internal. Do not expose metadata in public permission snapshots or logs. + +Use a strict qualified-name parser for `mcp__kandev__`. +Because this flattened format uses `__` as its boundary, accept only one +unambiguous separator. Reject a separator next to another underscore and +reject another `__` in the remaining name. This keeps names such as +`mcp__kandev__external__execute` and `mcp__kandev___execute` out of the +automatic path, even when the tool identifier itself could contain `__`. +Require the exact server segment and a nonempty tool identifier with letters, +digits, underscores, or hyphens. +Do not match substrings, descriptions, raw arguments, or the `_kandev` suffix alone. + +A supplied programmatic name is authoritative, including a nonmatching name. +Never replace a nonmatching name with a matching display title. +For `claude-acp` frames without a name, use a pure compatibility hook in `dialect_claude.go`. +The hook first reads `_meta.claudeCode.toolName` when present. +Malformed or conflicting metadata produces no eligible identity. +Only when both identity fields are absent can the hook use an exact qualified title with kind `other`. +This fallback is specific to Claude's observed MCP presentation, not a generic ACP rule. +Other provider formats retain the normal prompt path until a tested dialect supports them. + +The upstream [Claude tool formatter](https://github.com/agentclientprotocol/claude-agent-acp/blob/v0.75.1/src/tools.ts) +uses the tool name as the default MCP title. +Its [permission presentation](https://github.com/agentclientprotocol/claude-agent-acp/blob/v0.75.1/src/permissions/presentation.ts) +also supplies a programmatic name. +Implementation fixtures must cover both shapes because installed versions can differ. + +## Injection provenance + +Add an internal `InjectedKandevMCP` boolean with `json:"-"` to `config.InstanceConfig`. +Only `Config.NewInstanceConfig` sets it, immediately after local server injection with a positive instance port. +Do not add it to `InstanceOverrides`, public DTOs, persisted profiles, or incoming JSON. +The marker is false for configurations without host injection. + +The permission predicate requires this marker and the exact injected server configuration. +Require a URL for the current instance port at `http://localhost:/mcp` or `/sse`. +Require no stdio command and the corresponding supported HTTP/SSE transport. +The current injector replaces user entries named `kandev`, so those entries cannot retain the reserved slot. +Cover a mixed list containing both the injected server and unrelated servers. +The existence of unrelated entries must not grant them approval or disable the injected entry. + +## Permission decision + +Keep the existing blanket approval branch first. +Before pending-request allocation, evaluate the injected-server predicate. +Select an offered `allow_once`, then an offered `allow_always`. +Return its exact option ID. Never invent a choice or fall back to a reject option. +If no allow choice exists, continue through the existing pending-request path. +The ACP client's existing empty-options cancellation remains intact. + +Use a small helper in `server/process/manager_permission_policy.go`. +Do not reuse `autoApprovePermission` for this branch: that helper falls back to the first option, including rejection. +Keep all existing pending-request identity, cancellation, and exactly-once resolution rules unchanged. + +## Lifecycle and observability + +The policy has no persistence or migration. Instance construction recreates provenance after restart or resume. +The adapter retains normal tool-call and result events on the automatic path. +Automatic approval does not create a pending permission card on desktop or phone. +Log the policy reason, tool identifier, and selected option kind without arguments, headers, or credentials. +No frontend layout or copy changes are required. + +## Security and compatibility + +This policy trusts the configured agent's protocol identity, as the existing permission flow does. +It does not defend against a malicious agent process that fabricates protocol frames. +Raw MCP arguments cannot establish identity or injection provenance. +Unknown and malformed names fail closed to the normal permission flow. + +Server-wide parity also approves destructive Kandev tools at the provider permission layer. +Task reachability, ownership, explicit user-question answers, and workflow completion rules remain separate enforcement points. +The external permission-resolution specification remains authoritative for requests that become pending. +Its unchanged-auto-approval statement describes that feature's scope, not this proposed policy addition. + +## Related decisions + +- [Injected server approval policy](../../../decisions/2026-09-16-injected-mcp-approval.md) +- [Live permission authority](../../../decisions/2026-08-11-live-agent-permission-authority.md)