From 083b2b5bb0e06b8aacc55d3edc0b073193bc68d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Sat, 14 Mar 2026 21:58:31 +0000 Subject: [PATCH] fix(acp): improve claude acp tool messages and model selector flow (#442) --- .../server/adapter/transport/acp/adapter.go | 71 ++++++- .../server/adapter/transport/acp/normalize.go | 12 ++ .../adapter/transport/acp/normalize_test.go | 195 ++++++++++++++++++ .../internal/agentctl/server/config/config.go | 16 +- apps/web/components/task/model-selector.tsx | 51 ++++- apps/web/lib/diff/adapter.ts | 30 ++- .../session-runtime/session-runtime-slice.ts | 8 + apps/web/lib/ws/handlers/session-models.ts | 14 +- 8 files changed, 386 insertions(+), 11 deletions(-) diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go index e7ef5516ef..98817b8957 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go @@ -813,10 +813,18 @@ func (a *Adapter) convertNotification(n acp.SessionNotification) *AgentEvent { case u.ConfigOptionUpdate != nil: configOptions := convertACPConfigOptions(u.ConfigOptionUpdate.ConfigOptions) if len(configOptions) > 0 { + // Include cached available models so this event doesn't overwrite + // the model list that was set during session initialization. + a.mu.RLock() + cachedModels := a.availableModels + a.mu.RUnlock() + currentModelID := resolveCurrentModelFromConfig(configOptions) return &AgentEvent{ - Type: streams.EventTypeSessionModels, - SessionID: sessionID, - ConfigOptions: configOptions, + Type: streams.EventTypeSessionModels, + SessionID: sessionID, + CurrentModelID: currentModelID, + SessionModels: convertSessionModels(cachedModels), + ConfigOptions: configOptions, } } } @@ -1273,6 +1281,21 @@ func (a *Adapter) convertToolCallResultUpdate(sessionID string, tcu *acp.Session a.normalizer.NormalizeToolResult(payload, tcu.RawOutput) } + // Enrich modify_file payload from tool_call_contents. + // Claude ACP sends path and content in tool_call_update, not in the initial tool_call. + if payload != nil && payload.Kind() == streams.ToolKindModifyFile { + if mf := payload.ModifyFile(); mf != nil { + enrichModifyFileFromContents(mf, tcu.Content) + } + } + + // Enrich read_file payload path from title if still empty. + if payload != nil && payload.Kind() == streams.ToolKindReadFile { + if rf := payload.ReadFile(); rf != nil && rf.FilePath == "" && tcu.Title != nil { + rf.FilePath = extractPathFromTitle(*tcu.Title) + } + } + if isTerminal { delete(a.activeToolCalls, toolCallID) } @@ -1309,6 +1332,48 @@ func (a *Adapter) convertToolCallResultUpdate(sessionID string, tcu *acp.Session } } +// enrichModifyFileFromContents updates a ModifyFilePayload with data from +// tool_call_contents. Claude ACP sends file path and content in tool_call_update +// events rather than in the initial tool_call rawInput. +func enrichModifyFileFromContents(mf *streams.ModifyFilePayload, contents []acp.ToolCallContent) { + for _, c := range contents { + if c.Diff == nil { + continue + } + if mf.FilePath == "" && c.Diff.Path != "" { + mf.FilePath = c.Diff.Path + } + if len(mf.Mutations) == 0 { + continue + } + mut := &mf.Mutations[0] + if mut.Diff != "" { + continue // Already has diff, don't overwrite + } + if c.Diff.OldText != nil { + diffPath := c.Diff.Path + if diffPath == "" { + diffPath = mf.FilePath + } + mut.Diff = shared.GenerateUnifiedDiff(*c.Diff.OldText, c.Diff.NewText, diffPath, mut.StartLine) + } else if c.Diff.NewText != "" { + mut.Type = streams.MutationCreate + mut.Content = c.Diff.NewText + } + break + } +} + +// extractPathFromTitle extracts a file path from tool titles like "Read /path/to/file". +func extractPathFromTitle(title string) string { + for _, prefix := range []string{"Read ", "Write ", "Edit "} { + if strings.HasPrefix(title, prefix) { + return strings.TrimPrefix(title, prefix) + } + } + return "" +} + // handlePermissionRequest handles permission requests from the agent. // Since both acpclient and adapter now use the shared types package, // no conversion is needed - we just forward to the handler. diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/normalize.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/normalize.go index 7245abfc69..4ca6e808a1 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/normalize.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/normalize.go @@ -251,6 +251,18 @@ func (n *Normalizer) UpdatePayloadInput(payload *streams.NormalizedPayload, rawI shellExec.Description = desc } } + + // Claude ACP sends file_path in incremental rawInput updates + if mf := payload.ModifyFile(); mf != nil { + if path := shared.GetString(inputMap, "file_path"); path != "" && mf.FilePath == "" { + mf.FilePath = path + } + } + if rf := payload.ReadFile(); rf != nil { + if path := shared.GetString(inputMap, "file_path"); path != "" && rf.FilePath == "" { + rf.FilePath = path + } + } } // normalizeEdit converts ACP edit tool data. diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/normalize_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/normalize_test.go index ef4682ec5c..f1d203d18f 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/normalize_test.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/normalize_test.go @@ -5,6 +5,7 @@ import ( "strings" "testing" + "github.com/coder/acp-go-sdk" "github.com/kandev/kandev/internal/agentctl/server/adapter/transport/shared" "github.com/kandev/kandev/internal/agentctl/types/streams" ) @@ -654,6 +655,200 @@ func TestUpdatePayloadInput(t *testing.T) { }) } +// TestEnrichModifyFileFromContents tests enriching modify_file payloads from tool_call_contents. +func TestEnrichModifyFileFromContents(t *testing.T) { + oldText := "old content" + + t.Run("enriches file path from diff content", func(t *testing.T) { + mf := &streams.ModifyFilePayload{ + FilePath: "", + Mutations: []streams.FileMutation{{Type: streams.MutationPatch}}, + } + contents := []acp.ToolCallContent{ + {Diff: &acp.ToolCallContentDiff{Path: "/workspace/README.md", NewText: "new content"}}, + } + enrichModifyFileFromContents(mf, contents) + if mf.FilePath != "/workspace/README.md" { + t.Errorf("expected FilePath '/workspace/README.md', got %q", mf.FilePath) + } + }) + + t.Run("generates unified diff when oldText is provided", func(t *testing.T) { + mf := &streams.ModifyFilePayload{ + FilePath: "", + Mutations: []streams.FileMutation{{Type: streams.MutationPatch}}, + } + contents := []acp.ToolCallContent{ + {Diff: &acp.ToolCallContentDiff{Path: "file.ts", OldText: &oldText, NewText: "new content"}}, + } + enrichModifyFileFromContents(mf, contents) + if mf.Mutations[0].Diff == "" { + t.Error("expected Diff to be generated") + } + if mf.Mutations[0].Type != streams.MutationPatch { + t.Errorf("expected type to remain patch, got %q", mf.Mutations[0].Type) + } + if !strings.Contains(mf.Mutations[0].Diff, "-old content") { + t.Error("expected diff to contain removed old content") + } + if !strings.Contains(mf.Mutations[0].Diff, "+new content") { + t.Error("expected diff to contain added new content") + } + }) + + t.Run("sets create mutation when only newText", func(t *testing.T) { + mf := &streams.ModifyFilePayload{ + FilePath: "", + Mutations: []streams.FileMutation{{Type: streams.MutationPatch}}, + } + contents := []acp.ToolCallContent{ + {Diff: &acp.ToolCallContentDiff{Path: "file.ts", NewText: "new file content"}}, + } + enrichModifyFileFromContents(mf, contents) + if mf.Mutations[0].Type != streams.MutationCreate { + t.Errorf("expected type 'create', got %q", mf.Mutations[0].Type) + } + if mf.Mutations[0].Content != "new file content" { + t.Errorf("expected content 'new file content', got %q", mf.Mutations[0].Content) + } + }) + + t.Run("does not overwrite existing file path", func(t *testing.T) { + mf := &streams.ModifyFilePayload{ + FilePath: "existing.ts", + Mutations: []streams.FileMutation{{Type: streams.MutationPatch}}, + } + contents := []acp.ToolCallContent{ + {Diff: &acp.ToolCallContentDiff{Path: "other.ts", NewText: "content"}}, + } + enrichModifyFileFromContents(mf, contents) + if mf.FilePath != "existing.ts" { + t.Errorf("expected FilePath to remain 'existing.ts', got %q", mf.FilePath) + } + }) + + t.Run("does not overwrite existing diff", func(t *testing.T) { + mf := &streams.ModifyFilePayload{ + FilePath: "file.ts", + Mutations: []streams.FileMutation{{Type: streams.MutationPatch, Diff: "existing diff"}}, + } + contents := []acp.ToolCallContent{ + {Diff: &acp.ToolCallContentDiff{Path: "file.ts", OldText: &oldText, NewText: "new"}}, + } + enrichModifyFileFromContents(mf, contents) + if mf.Mutations[0].Diff != "existing diff" { + t.Errorf("expected Diff to remain 'existing diff', got %q", mf.Mutations[0].Diff) + } + }) + + t.Run("handles empty content gracefully", func(t *testing.T) { + mf := &streams.ModifyFilePayload{ + FilePath: "", + Mutations: []streams.FileMutation{{Type: streams.MutationPatch}}, + } + enrichModifyFileFromContents(mf, nil) + enrichModifyFileFromContents(mf, []acp.ToolCallContent{}) + if mf.FilePath != "" { + t.Errorf("expected FilePath to remain empty, got %q", mf.FilePath) + } + }) +} + +// TestExtractPathFromTitle tests file path extraction from tool titles. +func TestExtractPathFromTitle(t *testing.T) { + tests := []struct { + title string + want string + }{ + {"Read /path/to/file.ts", "/path/to/file.ts"}, + {"Write /path/to/file.ts", "/path/to/file.ts"}, + {"Edit /path/to/file.ts", "/path/to/file.ts"}, + {"Unknown /path/to/file.ts", ""}, + {"", ""}, + {"Read", ""}, + } + for _, tt := range tests { + t.Run(tt.title, func(t *testing.T) { + got := extractPathFromTitle(tt.title) + if got != tt.want { + t.Errorf("extractPathFromTitle(%q) = %q, want %q", tt.title, got, tt.want) + } + }) + } +} + +// TestUpdatePayloadInput_ModifyFile tests incremental rawInput updates for modify_file payloads. +func TestUpdatePayloadInput_ModifyFile(t *testing.T) { + normalizer := NewNormalizer() + + t.Run("updates empty file path from rawInput", func(t *testing.T) { + payload := normalizer.NormalizeToolCall("edit", map[string]any{ + "kind": "edit", + "raw_input": map[string]any{}, + }) + if payload.ModifyFile().FilePath != "" { + t.Errorf("expected empty initial FilePath, got %q", payload.ModifyFile().FilePath) + } + normalizer.UpdatePayloadInput(payload, map[string]any{ + "file_path": "/workspace/file.ts", + }) + if payload.ModifyFile().FilePath != "/workspace/file.ts" { + t.Errorf("expected FilePath '/workspace/file.ts', got %q", payload.ModifyFile().FilePath) + } + }) + + t.Run("does not overwrite existing file path", func(t *testing.T) { + payload := normalizer.NormalizeToolCall("edit", map[string]any{ + "kind": "edit", + "raw_input": map[string]any{ + "path": "existing.ts", + }, + }) + normalizer.UpdatePayloadInput(payload, map[string]any{ + "file_path": "other.ts", + }) + if payload.ModifyFile().FilePath != "existing.ts" { + t.Errorf("expected FilePath to remain 'existing.ts', got %q", payload.ModifyFile().FilePath) + } + }) +} + +// TestUpdatePayloadInput_ReadFile tests incremental rawInput updates for read_file payloads. +func TestUpdatePayloadInput_ReadFile(t *testing.T) { + normalizer := NewNormalizer() + + t.Run("updates empty file path from rawInput", func(t *testing.T) { + payload := normalizer.NormalizeToolCall("read", map[string]any{ + "kind": "read", + "raw_input": map[string]any{}, + }) + if payload.ReadFile().FilePath != "" { + t.Errorf("expected empty initial FilePath, got %q", payload.ReadFile().FilePath) + } + normalizer.UpdatePayloadInput(payload, map[string]any{ + "file_path": "/workspace/README.md", + }) + if payload.ReadFile().FilePath != "/workspace/README.md" { + t.Errorf("expected FilePath '/workspace/README.md', got %q", payload.ReadFile().FilePath) + } + }) + + t.Run("does not overwrite existing file path", func(t *testing.T) { + payload := normalizer.NormalizeToolCall("read", map[string]any{ + "kind": "read", + "raw_input": map[string]any{ + "path": "existing.md", + }, + }) + normalizer.UpdatePayloadInput(payload, map[string]any{ + "file_path": "other.md", + }) + if payload.ReadFile().FilePath != "existing.md" { + t.Errorf("expected FilePath to remain 'existing.md', got %q", payload.ReadFile().FilePath) + } + }) +} + // TestSplitLines tests the line splitting utility. func TestSplitLines(t *testing.T) { tests := []struct { diff --git a/apps/backend/internal/agentctl/server/config/config.go b/apps/backend/internal/agentctl/server/config/config.go index c5f3aeeae2..6be45819d0 100644 --- a/apps/backend/internal/agentctl/server/config/config.go +++ b/apps/backend/internal/agentctl/server/config/config.go @@ -304,11 +304,13 @@ func ParseCommand(cmd string) []string { func CollectAgentEnv(additional map[string]string) []string { envMap := make(map[string]string) - // Start with current environment, excluding AGENTCTL_* vars + // Start with current environment, excluding AGENTCTL_* and npm_config_* vars. + // npm_config_* vars from the parent pnpm process cause "Unknown env config" + // warnings when the agent is launched via npx. for _, e := range os.Environ() { if idx := strings.Index(e, "="); idx > 0 { key := e[:idx] - if !strings.HasPrefix(key, "AGENTCTL_") { + if !strings.HasPrefix(key, "AGENTCTL_") && !isNpmEnvVar(key) { envMap[key] = e[idx+1:] } } @@ -327,6 +329,16 @@ func CollectAgentEnv(additional map[string]string) []string { return result } +// isNpmEnvVar returns true if the key is an npm-related environment variable +// that should be filtered out to prevent warnings in npx commands. +func isNpmEnvVar(key string) bool { + return strings.HasPrefix(key, "npm_config_") || + strings.HasPrefix(key, "npm_package_") || + strings.HasPrefix(key, "npm_lifecycle_") || + strings.HasPrefix(key, "npm_execpath") || + strings.HasPrefix(key, "npm_node_execpath") +} + func getEnv(key, defaultValue string) string { if value := os.Getenv(key); value != "" { return value diff --git a/apps/web/components/task/model-selector.tsx b/apps/web/components/task/model-selector.tsx index 4088b98599..2be76868d6 100644 --- a/apps/web/components/task/model-selector.tsx +++ b/apps/web/components/task/model-selector.tsx @@ -96,6 +96,43 @@ function resolveCurrentModel( return activeModel || acpCurrentModel || snapshotModel || profileModel; } +function logModelSelectorState( + sessionId: string | null, + state: { + usingAcpModels: boolean; + sessionModelsData: + | { models?: { modelId: string }[]; currentModelId?: string; configOptions?: unknown[] } + | undefined; + activeModel: string | null; + acpCurrentModel: string | null; + snapshotModel: string | null; + profileModel: string | null; + currentModel: string | null; + modelOptions: ModelOption[]; + }, +) { + if (!sessionId) return; + console.log("[model-selector] state resolved", { + sessionId, + usingAcpModels: state.usingAcpModels, + sessionModelsData: state.sessionModelsData + ? { + modelsCount: state.sessionModelsData.models?.length ?? 0, + models: state.sessionModelsData.models?.map((m) => m.modelId), + currentModelId: state.sessionModelsData.currentModelId, + configOptionsCount: state.sessionModelsData.configOptions?.length ?? 0, + } + : null, + activeModel: state.activeModel, + acpCurrentModel: state.acpCurrentModel, + snapshotModel: state.snapshotModel, + profileModel: state.profileModel, + resolvedCurrentModel: state.currentModel, + modelOptionsCount: state.modelOptions.length, + modelOptions: state.modelOptions.map((m) => ({ id: m.id, name: m.name })), + }); +} + /** Resolves available models and current model from store state. */ function useModelSelectorState(sessionId: string | null) { useSettingsData(true); @@ -116,7 +153,8 @@ function useModelSelectorState(sessionId: string | null) { [session?.agent_profile_id, settingsAgents], ); - const availableModels = sessionModelsData?.models?.length + const usingAcpModels = !!sessionModelsData?.models?.length; + const availableModels = usingAcpModels ? sessionModelsToOptions(sessionModelsData.models) : resolveStaticModels(settingsAgents as Agent[], session?.agent_profile_id, availableAgents); @@ -130,6 +168,17 @@ function useModelSelectorState(sessionId: string | null) { ); const modelOptions = buildModelOptions(availableModels, currentModel); + logModelSelectorState(sessionId, { + usingAcpModels, + sessionModelsData, + activeModel, + acpCurrentModel, + snapshotModel, + profileModel, + currentModel, + modelOptions, + }); + const handleModelChange = useCallback( (sid: string, modelId: string) => { setActiveModel(sid, modelId); diff --git a/apps/web/lib/diff/adapter.ts b/apps/web/lib/diff/adapter.ts index ecd97cbd03..0affb7c0fe 100644 --- a/apps/web/lib/diff/adapter.ts +++ b/apps/web/lib/diff/adapter.ts @@ -62,16 +62,42 @@ export function normalizeDiffString(diff: string, filePath: string): string { * The DiffViewer component handles diff generation from content using the library. */ export function transformFileMutation(filePath: string, mutation: FileMutation): FileDiffData { + const resolvedPath = mutation.new_path || filePath; return { - filePath: mutation.new_path || filePath, + filePath: resolvedPath, oldContent: mutation.old_content || "", newContent: mutation.new_content || mutation.content || "", - diff: mutation.diff ? normalizeDiffString(mutation.diff, filePath) : undefined, + diff: resolveMutationDiff(mutation, resolvedPath), additions: 0, deletions: 0, }; } +/** Resolve the diff string for a mutation: explicit diff > create diff > undefined. */ +function resolveMutationDiff(mutation: FileMutation, filePath: string): string | undefined { + if (mutation.diff) return normalizeDiffString(mutation.diff, filePath); + if (mutation.type === "create" && mutation.content) + return generateCreateDiff(mutation.content, filePath); + return undefined; +} + +/** + * Generate an all-additions diff for new file creation. + * Shows the entire content as green (added) lines in the diff viewer. + */ +function generateCreateDiff(content: string, filePath: string): string { + const lines = content.split("\n"); + const header = [ + `diff --git a/${filePath} b/${filePath}`, + `new file mode 100644`, + `--- /dev/null`, + `+++ b/${filePath}`, + `@@ -0,0 +1,${lines.length} @@`, + ]; + const addedLines = lines.map((l) => `+${l}`); + return [...header, ...addedLines].join("\n"); +} + /** * Transform a git status diff string to FileDiffData. * Language detection is handled automatically by the library. diff --git a/apps/web/lib/state/slices/session-runtime/session-runtime-slice.ts b/apps/web/lib/state/slices/session-runtime/session-runtime-slice.ts index 5e4ba6ef6c..6a036947ae 100644 --- a/apps/web/lib/state/slices/session-runtime/session-runtime-slice.ts +++ b/apps/web/lib/state/slices/session-runtime/session-runtime-slice.ts @@ -250,6 +250,14 @@ export const createSessionRuntimeSlice: StateCreator< }), setSessionModels: (sessionId, data) => set((draft) => { + console.log("[store] setSessionModels", { + sessionId, + modelsCount: data.models?.length ?? 0, + models: data.models?.map((m) => m.modelId), + currentModelId: data.currentModelId, + configOptionsCount: data.configOptions?.length ?? 0, + configOptions: data.configOptions?.map((o) => ({ id: o.id, category: o.category })), + }); draft.sessionModels.bySessionId[sessionId] = data; }), setPromptUsage: (sessionId, usage) => diff --git a/apps/web/lib/ws/handlers/session-models.ts b/apps/web/lib/ws/handlers/session-models.ts index f248824657..2531335d82 100644 --- a/apps/web/lib/ws/handlers/session-models.ts +++ b/apps/web/lib/ws/handlers/session-models.ts @@ -22,11 +22,19 @@ export function registerSessionModelsHandlers(store: StoreApi): WsHand currentModelId = modelOpt.current_value; } } - console.debug("[session-models] received session.models_updated", { + console.log("[session-models] received session.models_updated", { sessionId: payload.session_id, rawCurrentModelId: payload.current_model_id, resolvedCurrentModelId: currentModelId, - models: acpModels.map((m) => m.model_id), + modelsCount: acpModels.length, + models: acpModels.map((m) => ({ id: m.model_id, name: m.name })), + configOptions: (payload.config_options ?? []).map((o) => ({ + id: o.id, + category: o.category, + currentValue: o.current_value, + options: o.options?.map((opt: { value: string; name: string }) => opt.value), + })), + rawPayload: payload, }); store.getState().setSessionModels(payload.session_id, { currentModelId, @@ -53,7 +61,7 @@ export function registerSessionModelsHandlers(store: StoreApi): WsHand const state = store.getState(); const currentActive = state.activeModel.bySessionId[payload.session_id]; if (currentActive && !acpModels.some((m) => m.model_id === currentActive)) { - console.debug("[session-models] clearing stale activeModel", { + console.log("[session-models] clearing stale activeModel", { sessionId: payload.session_id, staleModel: currentActive, });