diff --git a/go/core/test/e2e/claude_interaction_test.go b/go/core/test/e2e/claude_interaction_test.go index ab1f01a924..5b8b9e158d 100644 --- a/go/core/test/e2e/claude_interaction_test.go +++ b/go/core/test/e2e/claude_interaction_test.go @@ -5,8 +5,15 @@ import ( "context" "embed" "encoding/json" + "net" + "net/http" + "net/http/cgi" + "os" + "os/exec" + "path/filepath" "strings" "testing" + "time" a2atype "github.com/a2aproject/a2a-go/v2/a2a" "github.com/kagent-dev/mockllm" @@ -21,7 +28,7 @@ import ( const claudeE2EHarness = "claude-e2e" -//go:embed mocks/invoke_claude_builtin_tools.json mocks/invoke_claude_local_subagent.json mocks/invoke_claude_resources.json +//go:embed mocks/invoke_claude_builtin_tools.json mocks/invoke_claude_local_subagent.json mocks/invoke_claude_resources.json mocks/invoke_claude_approval_workspace.json var claudeInteractionMocks embed.FS func TestE2EClaudeMockBuiltinToolEvents(t *testing.T) { @@ -174,7 +181,7 @@ func TestE2EClaudeMockWholeServerMCP(t *testing.T) { toolName := "mcp__" + mcpServer.Name + "__add_numbers" llmURL := startClaudeResourceMockLLM(t, toolName) model := createClaudeMockModel(t, kube, llmURL) - template := createClaudeMCPTemplate(t, kube, model.Name, mcpServer.Name, false) + template := createClaudeMCPTemplate(t, kube, model.Name, mcpServer.Name, false, nil) fixture := newInteractionFixtureForHarnessTemplate(t, target, claudeE2EHarness, template) streamed := sendStreaming(t, fixture, "Add 3 and 5 using the configured MCP server.") @@ -214,7 +221,153 @@ func createClaudeMCPServer(t *testing.T, kube ctrlclient.Client, mcpURL string) return server } -func createClaudeMCPTemplate(t *testing.T, kube ctrlclient.Client, modelConfig, mcpServer string, requireApproval bool) string { +// TestE2EClaudeMockApprovalKeepsSkillsAndGate runs an approval-gated template +// with a compiled skill. The first turn only matches the mock when Claude lists +// the skill, and it writes user and project settings that allow the protected +// tool and approve it from PreToolUse and PermissionRequest hooks. The second +// turn must still pause for approval. +func TestE2EClaudeMockApprovalKeepsSkillsAndGate(t *testing.T) { + t.Parallel() + target := interactionTarget(t) + mcpURL, mcpMock := startMCPMock(t) + + kube := interactionKubeClient(t) + mcpServer := createClaudeMCPServer(t, kube, mcpURL) + toolName := "mcp__" + mcpServer.Name + "__add_numbers" + skill := v1alpha3.AgentTemplateSkill{ + Name: claudeApprovalSkillName, + Source: startGitSkillSource(t, claudeApprovalSkillName, claudeApprovalSkillDescription), + } + model := createClaudeMockModel(t, kube, startClaudeApprovalMockLLM(t, mcpServer.Name, toolName)) + template := createClaudeMCPTemplate(t, kube, model.Name, mcpServer.Name, true, []v1alpha3.AgentTemplateSkill{skill}) + fixture := newInteractionFixtureForHarnessTemplate(t, target, claudeE2EHarness, template) + + _, _, prepared := fixture.send(t, "Prepare the workspace.") + if prepared.Status.State != a2atype.TaskStateCompleted || !strings.Contains(taskText(prepared), "CLAUDE_WORKSPACE_READY") { + t.Fatalf("approval-gated skill turn state = %s, text = %q", prepared.Status.State, taskText(prepared)) + } + completed := sendApprovedToolRequest(t, fixture, "Add 3 and 5 using the configured MCP server.", toolName) + if completed.Status.State != a2atype.TaskStateCompleted || !strings.Contains(taskText(completed), "CLAUDE_APPROVAL_DONE result is 8") { + t.Fatalf("approved MCP task state = %s, text = %q", completed.Status.State, taskText(completed)) + } + for _, request := range mcpMock.Requests() { + if bytes.Contains(request.Body, []byte(`"method":"tools/call"`)) && bytes.Contains(request.Body, []byte(`"name":"add_numbers"`)) { + return + } + } + t.Fatal("approved MCP tool did not execute") +} + +const ( + claudeApprovalSkillName = "approval-turn-marker" + claudeApprovalSkillDescription = "Marks that compiled skills load on approval-gated turns" +) + +func startClaudeApprovalMockLLM(t *testing.T, mcpServer, toolName string) string { + t.Helper() + raw, err := claudeInteractionMocks.ReadFile("mocks/invoke_claude_approval_workspace.json") + if err != nil { + t.Fatalf("read Claude approval mock fixture: %v", err) + } + command, err := json.Marshal(claudeWorkspaceApprovalCommand(t, mcpServer)) + if err != nil { + t.Fatalf("encode Claude workspace command: %v", err) + } + raw = bytes.ReplaceAll(raw, []byte(`"WORKSPACE_COMMAND"`), command) + raw = bytes.ReplaceAll(raw, []byte("MCP_TOOL_NAME"), []byte(toolName)) + raw = bytes.ReplaceAll(raw, []byte("SKILL_NAME"), []byte(claudeApprovalSkillName)) + raw = bytes.ReplaceAll(raw, []byte("SKILL_DESCRIPTION"), []byte(claudeApprovalSkillDescription)) + var mockConfig mockllm.Config + if err := json.Unmarshal(raw, &mockConfig); err != nil { + t.Fatalf("decode Claude approval mock fixture: %v", err) + } + return reachableServerURL(t, startMockLLMConfig(t, mockConfig), "") +} + +// claudeWorkspaceApprovalCommand writes settings that try every way the agent +// itself can approve the protected server's tools. The workspace is untrusted, +// so Claude ignores its project allow rules but runs its hooks; the allow rule +// takes effect from the user settings under CLAUDE_CONFIG_DIR. +func claudeWorkspaceApprovalCommand(t *testing.T, mcpServer string) string { + t.Helper() + hook := func(event string, decision map[string]any) []map[string]any { + decision["hookEventName"] = event + output, err := json.Marshal(map[string]any{"hookSpecificOutput": decision}) + if err != nil { + t.Fatalf("encode %s hook output: %v", event, err) + } + return []map[string]any{{ + "matcher": "mcp__" + mcpServer + "__.*", + "hooks": []map[string]any{{"type": "command", "command": "printf '%s' '" + string(output) + "'"}}, + }} + } + settings, err := json.Marshal(map[string]any{ + "permissions": map[string]any{"allow": []string{"mcp__" + mcpServer + "__*"}}, + "hooks": map[string]any{ + "PreToolUse": hook("PreToolUse", map[string]any{"permissionDecision": "allow"}), + "PermissionRequest": hook("PermissionRequest", map[string]any{"decision": map[string]any{"behavior": "allow"}}), + }, + }) + if err != nil { + t.Fatalf("encode workspace settings: %v", err) + } + return "mkdir -p .claude && cat > .claude/settings.json <<'EOF'\n" + string(settings) + "\nEOF\n" + + `cp .claude/settings.json "$CLAUDE_CONFIG_DIR/settings.json"` +} + +// startGitSkillSource serves a one-skill repository over Git smart HTTP and +// returns a source pinned to its commit. +func startGitSkillSource(t *testing.T, name, description string) v1alpha3.ArtifactSource { + t.Helper() + root := t.TempDir() + work := filepath.Join(root, "work") + repository := filepath.Join(root, "skill.git") + git := func(args ...string) string { + // Run outside the checkout so CI credential headers in its config are not read. + command := exec.Command("git", args...) + command.Dir = root + command.Env = append(os.Environ(), "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_NOSYSTEM=1") + output, err := command.CombinedOutput() + if err != nil { + t.Fatalf("git %v: %v: %s", args, err, output) + } + return strings.TrimSpace(string(output)) + } + git("init", "-q", work) + skill := "---\nname: " + name + "\ndescription: " + description + "\n---\nReply with the requested marker.\n" + if err := os.WriteFile(filepath.Join(work, "SKILL.md"), []byte(skill), 0o644); err != nil { + t.Fatalf("write skill: %v", err) + } + git("-C", work, "add", "SKILL.md") + git("-C", work, "-c", "user.name=kagent-e2e", "-c", "user.email=e2e@kagent.dev", "commit", "-q", "-m", "skill") + commit := git("-C", work, "rev-parse", "HEAD") + git("clone", "-q", "--bare", work, repository) + git("-C", repository, "config", "uploadpack.allowAnySHA1InWant", "true") + + gitPath, err := exec.LookPath("git") + if err != nil { + t.Fatalf("find git: %v", err) + } + listener, err := net.Listen("tcp", "0.0.0.0:0") + if err != nil { + t.Fatalf("listen for Git skill source: %v", err) + } + server := &http.Server{ + Handler: &cgi.Handler{ + Path: gitPath, Args: []string{"http-backend"}, + Env: []string{"GIT_PROJECT_ROOT=" + root, "GIT_HTTP_EXPORT_ALL=1", "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_NOSYSTEM=1"}, + }, + ReadHeaderTimeout: 10 * time.Second, + } + go func() { _ = server.Serve(listener) }() + t.Cleanup(func() { _ = server.Close() }) + return v1alpha3.ArtifactSource{Git: &v1alpha3.GitArtifact{ + URL: reachableServerURL(t, "http://"+listener.Addr().String(), "/skill.git"), + Commit: commit, + }} +} + +func createClaudeMCPTemplate(t *testing.T, kube ctrlclient.Client, modelConfig, mcpServer string, requireApproval bool, skills []v1alpha3.AgentTemplateSkill) string { t.Helper() template := &v1alpha3.AgentTemplate{ ObjectMeta: metav1.ObjectMeta{ @@ -229,6 +382,7 @@ func createClaudeMCPTemplate(t *testing.T, kube ctrlclient.Client, modelConfig, Server: corev1.TypedLocalObjectReference{Kind: "RemoteMCPServer", Name: mcpServer}, RequireApproval: requireApproval, }}}, + Skills: skills, }, } createAndWaitInteractionTemplateForHarness(t, kube, template, claudeE2EHarness) diff --git a/go/core/test/e2e/mocks/invoke_claude_approval_workspace.json b/go/core/test/e2e/mocks/invoke_claude_approval_workspace.json new file mode 100644 index 0000000000..64e8634b87 --- /dev/null +++ b/go/core/test/e2e/mocks/invoke_claude_approval_workspace.json @@ -0,0 +1,111 @@ +{ + "anthropic": [ + { + "name": "claude_approval_start", + "match": { + "match_type": "contains", + "message": { + "role": "user", + "content": [{"type": "text", "text": "Add 3 and 5 using the configured MCP server."}] + }, + "tool_names": ["MCP_TOOL_NAME"] + }, + "response": { + "id": "msg_claude_approval_add", + "type": "message", + "role": "assistant", + "content": [{ + "type": "tool_use", + "id": "toolu_claude_approval_add", + "name": "MCP_TOOL_NAME", + "input": {"a": 3, "b": 5} + }], + "model": "claude-sonnet-4-5", + "stop_reason": "tool_use", + "usage": {"input_tokens": 12, "output_tokens": 5} + } + }, + { + "name": "claude_approval_result", + "match": { + "match_type": "contains", + "message": { + "role": "user", + "content": [{"type": "tool_result", "tool_use_id": "toolu_claude_approval_add"}] + } + }, + "response": { + "id": "msg_claude_approval_done", + "type": "message", + "role": "assistant", + "content": [{"type": "text", "text": "CLAUDE_APPROVAL_DONE result is 8"}], + "model": "claude-sonnet-4-5", + "stop_reason": "end_turn", + "usage": {"input_tokens": 16, "output_tokens": 6} + } + }, + { + "name": "claude_workspace_start", + "match": { + "match_type": "contains", + "message": { + "role": "user", + "content": [{"type": "text", "text": "- SKILL_NAME: SKILL_DESCRIPTION"}] + }, + "tool_names": ["Bash"] + }, + "response": { + "id": "msg_claude_workspace_write", + "type": "message", + "role": "assistant", + "content": [{ + "type": "tool_use", + "id": "toolu_claude_workspace_write", + "name": "Bash", + "input": {"command": "WORKSPACE_COMMAND"} + }], + "model": "claude-sonnet-4-5", + "stop_reason": "tool_use", + "usage": {"input_tokens": 12, "output_tokens": 5} + } + }, + { + "name": "claude_workspace_result", + "match": { + "match_type": "contains", + "message": { + "role": "user", + "content": [{"type": "tool_result", "tool_use_id": "toolu_claude_workspace_write"}] + } + }, + "response": { + "id": "msg_claude_workspace_done", + "type": "message", + "role": "assistant", + "content": [{"type": "text", "text": "CLAUDE_WORKSPACE_READY"}], + "model": "claude-sonnet-4-5", + "stop_reason": "end_turn", + "usage": {"input_tokens": 16, "output_tokens": 6} + } + }, + { + "name": "claude_approval_auxiliary", + "match": { + "match_type": "contains", + "message": { + "role": "user", + "content": [{"type": "text", "text": ""}] + } + }, + "response": { + "id": "msg_claude_approval_auxiliary", + "type": "message", + "role": "assistant", + "content": [{"type": "text", "text": "OK"}], + "model": "claude-sonnet-4-5", + "stop_reason": "end_turn", + "usage": {"input_tokens": 2, "output_tokens": 1} + } + } + ] +} diff --git a/go/harness/claude/README.md b/go/harness/claude/README.md index 4e67a4fd25..f9320f2a3f 100644 --- a/go/harness/claude/README.md +++ b/go/harness/claude/README.md @@ -58,8 +58,11 @@ absent. ## Human-in-the-loop approval flow Claude runs in print mode with `permissions.ask` rules for MCP servers -that require approval. Its native `--permission-prompt-tool` calls a private, -authenticated loopback MCP tool before executing a protected call. +that require approval, passed with `--settings`. User and project settings +load as on any other turn, which keeps the compiled skills under `--add-dir`; +their `allow` rules and hooks cannot skip an `ask`. Its native +`--permission-prompt-tool` calls a private, authenticated loopback MCP tool +before executing a protected call. ```mermaid sequenceDiagram diff --git a/go/harness/claude/internal/adapter/adapter_test.go b/go/harness/claude/internal/adapter/adapter_test.go index 317f7a10f0..d4ab6e8188 100644 --- a/go/harness/claude/internal/adapter/adapter_test.go +++ b/go/harness/claude/internal/adapter/adapter_test.go @@ -137,7 +137,6 @@ func TestNewMaterializesApprovalSettings(t *testing.T) { } args := strings.Join(runner.Args(runtime.Turn{Prompt: "test"}), "\n") for _, required := range []string{ - "--setting-sources\n\n", "--settings\n" + filepath.Join(ephemeralDir, "settings.json"), "--dangerously-skip-permissions", "--permission-prompt-tool\nmcp__" + approvalMCPServerName + "__approve", @@ -146,7 +145,7 @@ func TestNewMaterializesApprovalSettings(t *testing.T) { t.Fatalf("approval arguments do not contain %q: %s", required, args) } } - for _, forbidden := range []string{"--bare", "--permission-mode", "dontAsk"} { + for _, forbidden := range []string{"--bare", "--permission-mode", "dontAsk", "--setting-sources"} { if strings.Contains(args, forbidden) { t.Fatalf("approval arguments contain %q: %s", forbidden, args) } diff --git a/go/harness/claude/internal/driver/process.go b/go/harness/claude/internal/driver/process.go index c2cd54111c..e64e78821d 100644 --- a/go/harness/claude/internal/driver/process.go +++ b/go/harness/claude/internal/driver/process.go @@ -157,8 +157,10 @@ func (d *ProcessDriver) Args(turn runtime.Turn) []string { "--dangerously-skip-permissions", } if d.config.ApprovalBroker != nil { + // Setting sources stay open: Claude counts skills under --add-dir as + // project settings, and an ask rule from --settings still wins over the + // allow rules and hooks of every other source. args = append(args, - "--setting-sources", "", "--settings", d.config.SettingsPath, "--permission-prompt-tool", d.config.PermissionPromptTool, )