diff --git a/AGENTS.md b/AGENTS.md index 9b6c43e888..0270caee12 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -23,6 +23,10 @@ apps/ - **Container image**: `ghcr.io/kdlbs/kandev` (GitHub Container Registry) - **Dev mock providers**: In `KANDEV_MOCK_AGENT=true` dev mode the base `mock-agent` is enabled alongside the real CLIs. To exercise the office provider-routing UI without installing real CLIs, set `KANDEV_MOCK_PROVIDERS=claude-acp,codex-acp,opencode-acp` (or any subset of `RoutableProviderIDs`) when launching `make dev` — those canonicals are then replaced with MockAgent aliases. +### Worktrees and commit hooks + +The commit-msg hook runs `cd apps && pnpm exec commitlint --edit "$1"`. A fresh git worktree shares `.git/` but **not** `apps/node_modules/`, so the first commit attempted in a worktree fails with `Command "commitlint" not found` / `ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL`. Run `pnpm install --frozen-lockfile` from `apps/` in the worktree once before the first commit; subsequent commits work normally. + --- ## Backend Architecture diff --git a/apps/backend/cmd/mock-agent/main.go b/apps/backend/cmd/mock-agent/main.go index 84310efb5a..f7cc8508aa 100644 --- a/apps/backend/cmd/mock-agent/main.go +++ b/apps/backend/cmd/mock-agent/main.go @@ -114,12 +114,12 @@ func (a *mockAgent) NewSession(_ context.Context, req acp.NewSessionRequest) (ac // mockSessionModels returns the mock agent's advertised model list for ACP // session responses. Two models are exposed so tests can verify both // selection and default behavior (mock-fast is the default). -func mockSessionModels() *acp.UnstableSessionModelState { +func mockSessionModels() *acp.SessionModelState { fastDesc := "Fast mock model for testing" smartDesc := "Smart mock model for testing" - return &acp.UnstableSessionModelState{ + return &acp.SessionModelState{ CurrentModelId: "mock-fast", - AvailableModels: []acp.UnstableModelInfo{ + AvailableModels: []acp.ModelInfo{ {ModelId: "mock-fast", Name: "Mock Fast", Description: &fastDesc}, {ModelId: "mock-smart", Name: "Mock Smart", Description: &smartDesc}, }, @@ -190,11 +190,35 @@ func (a *mockAgent) SetSessionConfigOption(_ context.Context, _ acp.SetSessionCo return acp.SetSessionConfigOptionResponse{}, nil } +// CloseSession releases any state for a session (no-op for mock). +func (a *mockAgent) CloseSession(_ context.Context, req acp.CloseSessionRequest) (acp.CloseSessionResponse, error) { + a.mu.Lock() + delete(a.sessions, req.SessionId) + delete(a.commandsEmitted, req.SessionId) + a.mu.Unlock() + return acp.CloseSessionResponse{}, nil +} + +// ListSessions is not supported by the mock and returns an empty list. +func (a *mockAgent) ListSessions(_ context.Context, _ acp.ListSessionsRequest) (acp.ListSessionsResponse, error) { + return acp.ListSessionsResponse{}, nil +} + +// ResumeSession is not supported by the mock; LoadSession is used instead. +func (a *mockAgent) ResumeSession(_ context.Context, _ acp.ResumeSessionRequest) (acp.ResumeSessionResponse, error) { + return acp.ResumeSessionResponse{}, nil +} + // emitAvailableCommandsOnce sends the available commands list once per session. // Idempotent — guarded by `commandsEmitted` so callers (NewSession, LoadSession, -// first Prompt) can call it freely. +// first Prompt) can call it freely. Skips emission if the session was closed +// while the post-handshake delay was sleeping. func (a *mockAgent) emitAvailableCommandsOnce(ctx context.Context, sid acp.SessionId) { a.mu.Lock() + if !a.sessions[sid] { + a.mu.Unlock() + return + } if a.commandsEmitted[sid] { a.mu.Unlock() return diff --git a/apps/backend/go.mod b/apps/backend/go.mod index e6a521de42..03b8eea687 100644 --- a/apps/backend/go.mod +++ b/apps/backend/go.mod @@ -4,9 +4,10 @@ go 1.26.0 require ( github.com/UserExistsError/conpty v0.1.4 - github.com/coder/acp-go-sdk v0.6.3 + github.com/coder/acp-go-sdk v0.13.0 github.com/creack/pty v1.1.21 github.com/docker/docker v28.5.2+incompatible + github.com/docker/go-connections v0.6.0 github.com/fsnotify/fsnotify v1.6.0 github.com/gin-gonic/gin v1.9.1 github.com/github/copilot-sdk/go v0.1.18 @@ -28,6 +29,7 @@ require ( go.uber.org/zap v1.26.0 golang.org/x/sync v0.20.0 golang.org/x/sys v0.42.0 + gopkg.in/natefinch/lumberjack.v2 v2.2.1 gopkg.in/yaml.v3 v3.0.1 ) @@ -45,7 +47,6 @@ require ( github.com/containerd/log v0.1.0 // indirect github.com/davecgh/go-spew v1.1.1 // indirect github.com/distribution/reference v0.6.0 // indirect - github.com/docker/go-connections v0.6.0 // indirect github.com/docker/go-units v0.5.0 // indirect github.com/felixge/httpsnoop v1.0.4 // indirect github.com/gabriel-vasile/mimetype v1.4.2 // indirect @@ -109,10 +110,5 @@ require ( google.golang.org/grpc v1.80.0 // indirect google.golang.org/protobuf v1.36.11 // indirect gopkg.in/ini.v1 v1.67.0 // indirect - gopkg.in/natefinch/lumberjack.v2 v2.2.1 // indirect gotest.tools/v3 v3.5.2 // indirect ) - -// Fix for ACP notification ordering race condition -// See: https://github.com/coder/acp-go-sdk/issues/20 -replace github.com/coder/acp-go-sdk => github.com/agentcooper/acp-go-sdk v0.0.0-20260227120930-f21f3950f56d diff --git a/apps/backend/go.sum b/apps/backend/go.sum index c8fac48f30..143b551178 100644 --- a/apps/backend/go.sum +++ b/apps/backend/go.sum @@ -48,8 +48,6 @@ github.com/Microsoft/go-winio v0.4.21 h1:+6mVbXh4wPzUrl1COX9A+ZCvEpYsOBZ6/+kwDnv github.com/Microsoft/go-winio v0.4.21/go.mod h1:JPGBdM1cNvN/6ISo+n8V5iA4v8pBzdOpzfwIujj1a84= github.com/UserExistsError/conpty v0.1.4 h1:+3FhJhiqhyEJa+K5qaK3/w6w+sN3Nh9O9VbJyBS02to= github.com/UserExistsError/conpty v0.1.4/go.mod h1:PDglKIkX3O/2xVk0MV9a6bCWxRmPVfxqZoTG/5sSd9I= -github.com/agentcooper/acp-go-sdk v0.0.0-20260227120930-f21f3950f56d h1:9VmeQddWE4m9UksAzVg0Me8V1KhXy8fOI1nLnWn94WM= -github.com/agentcooper/acp-go-sdk v0.0.0-20260227120930-f21f3950f56d/go.mod h1:yKzM/3R9uELp4+nBAwwtkS0aN1FOFjo11CNPy37yFko= github.com/bahlo/generic-list-go v0.2.0 h1:5sz/EEAK+ls5wF+NeqDpk5+iNdMDXrh3z3nPnH1Wvgk= github.com/bahlo/generic-list-go v0.2.0/go.mod h1:2KvAjgMlE5NNynlg/5iLrrCCZ2+5xWbdbCW3pNTGyYg= github.com/buger/jsonparser v1.1.2 h1:frqHqw7otoVbk5M8LlE/L7HTnIq2v9RX6EJ48i9AxJk= @@ -72,6 +70,8 @@ github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDk github.com/cncf/udpa/go v0.0.0-20191209042840-269d4d468f6f/go.mod h1:M8M6+tZqaGXZJjfX53e64911xZQV5JYwmTeXPW+k8Sc= github.com/cncf/udpa/go v0.0.0-20200629203442-efcf912fb354/go.mod h1:WmhPx2Nbnhtbo57+VJT5O0JRkEi1Wbu0z5j0R8u5Hbk= github.com/cncf/udpa/go v0.0.0-20201120205902-5459f2c99403/go.mod h1:WmhPx2Nbnhtbo57+VJT5O0JRkEi1Wbu0z5j0R8u5Hbk= +github.com/coder/acp-go-sdk v0.13.0 h1:IAKBDIbe/iBfKAGikeIndzb8fowt4ioD+gCtSU4HwMA= +github.com/coder/acp-go-sdk v0.13.0/go.mod h1:yKzM/3R9uELp4+nBAwwtkS0aN1FOFjo11CNPy37yFko= github.com/containerd/errdefs v1.0.0 h1:tg5yIfIlQIrxYtu9ajqY42W3lpS19XqdxRQeEwYG8PI= github.com/containerd/errdefs v1.0.0/go.mod h1:+YBYIdtsnF4Iw6nWZhJcqGSg/dwvV7tyJ/kCkyJ2k+M= github.com/containerd/errdefs/pkg v0.3.0 h1:9IKJ06FvyNlexW690DXuQNx2KA2cUJXx151Xdx3ZPPE= diff --git a/apps/backend/internal/agent/acpdbg/ops.go b/apps/backend/internal/agent/acpdbg/ops.go index 2d1e91fa71..b5bf11660a 100644 --- a/apps/backend/internal/agent/acpdbg/ops.go +++ b/apps/backend/internal/agent/acpdbg/ops.go @@ -163,8 +163,6 @@ func sendSessionNew(ctx context.Context, r *Runner) (Frame, error) { } func sendSetModel(ctx context.Context, r *Runner, sessionID, modelID string) error { - // Try the unstable form first (matches the acp-go-sdk fork we use). - // If that returns method-not-found, fall back to the stable name. req, _ := r.Framer().NewRequest("session/set_model", map[string]any{ "sessionId": sessionID, "modelId": modelID, diff --git a/apps/backend/internal/agentctl/server/acp/authmethod.go b/apps/backend/internal/agentctl/server/acp/authmethod.go new file mode 100644 index 0000000000..5446f651bd --- /dev/null +++ b/apps/backend/internal/agentctl/server/acp/authmethod.go @@ -0,0 +1,19 @@ +package acp + +import "github.com/coder/acp-go-sdk" + +// AuthMethodFields collapses upstream's tagged-union acp.AuthMethod +// ({Agent, Terminal, EnvVar}) into the (id, name, description, meta) tuple +// that callers want before storing or transmitting. Adding a new variant +// here is the single place that needs to learn about it. +func AuthMethodFields(m acp.AuthMethod) (id, name string, description *string, meta map[string]any) { + switch { + case m.Agent != nil: + return m.Agent.Id, m.Agent.Name, m.Agent.Description, m.Agent.Meta + case m.Terminal != nil: + return m.Terminal.Id, m.Terminal.Name, m.Terminal.Description, m.Terminal.Meta + case m.EnvVar != nil: + return m.EnvVar.Id, m.EnvVar.Name, m.EnvVar.Description, m.EnvVar.Meta + } + return "", "", nil, nil +} diff --git a/apps/backend/internal/agentctl/server/acp/authmethod_test.go b/apps/backend/internal/agentctl/server/acp/authmethod_test.go new file mode 100644 index 0000000000..d6be65e212 --- /dev/null +++ b/apps/backend/internal/agentctl/server/acp/authmethod_test.go @@ -0,0 +1,79 @@ +package acp + +import ( + "testing" + + "github.com/coder/acp-go-sdk" +) + +func TestAuthMethodFields(t *testing.T) { + desc := "with description" + cases := []struct { + name string + in acp.AuthMethod + wantID string + wantName string + wantDesc *string + wantMetaKey string + }{ + { + name: "Agent variant", + in: acp.AuthMethod{Agent: &acp.AuthMethodAgent{ + Id: "agent-id", + Name: "Agent", + Description: &desc, + Meta: map[string]any{"k": "agent"}, + }}, + wantID: "agent-id", wantName: "Agent", wantDesc: &desc, wantMetaKey: "agent", + }, + { + name: "Terminal variant", + in: acp.AuthMethod{Terminal: &acp.AuthMethodTerminalInline{ + Id: "term-id", + Name: "Terminal", + Meta: map[string]any{"k": "terminal"}, + }}, + wantID: "term-id", wantName: "Terminal", wantMetaKey: "terminal", + }, + { + name: "EnvVar variant", + in: acp.AuthMethod{EnvVar: &acp.AuthMethodEnvVarInline{ + Id: "env-id", + Name: "EnvVar", + Meta: map[string]any{"k": "envvar"}, + }}, + wantID: "env-id", wantName: "EnvVar", wantMetaKey: "envvar", + }, + { + name: "zero-value (no variant set)", + in: acp.AuthMethod{}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + id, name, desc, meta := AuthMethodFields(tc.in) + if id != tc.wantID { + t.Errorf("id = %q, want %q", id, tc.wantID) + } + if name != tc.wantName { + t.Errorf("name = %q, want %q", name, tc.wantName) + } + if (desc == nil) != (tc.wantDesc == nil) { + t.Errorf("desc nil mismatch: got=%v want=%v", desc, tc.wantDesc) + } + if desc != nil && tc.wantDesc != nil && *desc != *tc.wantDesc { + t.Errorf("desc = %q, want %q", *desc, *tc.wantDesc) + } + if tc.wantMetaKey == "" { + if meta != nil { + t.Errorf("meta = %v, want nil", meta) + } + return + } + if got, _ := meta["k"].(string); got != tc.wantMetaKey { + t.Errorf("meta[k] = %q, want %q", got, tc.wantMetaKey) + } + }) + } +} diff --git a/apps/backend/internal/agentctl/server/acp/client.go b/apps/backend/internal/agentctl/server/acp/client.go index 0caed5f91b..2d1f0e9f10 100644 --- a/apps/backend/internal/agentctl/server/acp/client.go +++ b/apps/backend/internal/agentctl/server/acp/client.go @@ -397,17 +397,17 @@ func (c *Client) CreateTerminal(ctx context.Context, p acp.CreateTerminalRequest return acp.CreateTerminalResponse{TerminalId: id}, nil } -// KillTerminalCommand sends SIGTERM to a terminal's process. -func (c *Client) KillTerminalCommand(ctx context.Context, p acp.KillTerminalCommandRequest) (acp.KillTerminalCommandResponse, error) { - _, span := shared.TraceProtocolRequest(ctx, shared.ProtocolACP, "", "request.kill_terminal_command") +// KillTerminal sends SIGTERM to a terminal's process. +func (c *Client) KillTerminal(ctx context.Context, p acp.KillTerminalRequest) (acp.KillTerminalResponse, error) { + _, span := shared.TraceProtocolRequest(ctx, shared.ProtocolACP, "", "request.kill_terminal") defer span.End() span.SetAttributes(attribute.String("terminal_id", p.TerminalId)) if err := c.terminals.Kill(p.TerminalId); err != nil { span.RecordError(err) - return acp.KillTerminalCommandResponse{}, err + return acp.KillTerminalResponse{}, err } - return acp.KillTerminalCommandResponse{}, nil + return acp.KillTerminalResponse{}, nil } // TerminalOutput returns the current output of a terminal. 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 eae9e44d22..fd67e68998 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go @@ -132,7 +132,7 @@ type Adapter struct { // Available models from the most recent session creation/load. // Used by SetModel to validate the requested model exists. - availableModels []acp.UnstableModelInfo + availableModels []acp.ModelInfo // usageDelta tracks the running cumulative `usage_update.used` and // the most recent USD cost reported per session. codex-acp emits no @@ -1407,7 +1407,7 @@ func (a *Adapter) emitInitialModeState(modes *acp.SessionModeState) { } // emitSessionModels emits a session_models event from the session response. -func (a *Adapter) emitSessionModels(sessionID string, models *acp.UnstableSessionModelState, meta map[string]any, acpConfigOptions []acp.SessionConfigOption) { +func (a *Adapter) emitSessionModels(sessionID string, models *acp.SessionModelState, meta map[string]any, acpConfigOptions []acp.SessionConfigOption) { currentModelID := string(models.CurrentModelId) // Prefer typed config options from the response; fall back to _meta extraction for older agents configOptions := convertACPConfigOptions(acpConfigOptions) @@ -1575,9 +1575,11 @@ func (a *Adapter) SetConfigOption(ctx context.Context, configID, value string) e } _, err := conn.SetSessionConfigOption(ctx, acp.SetSessionConfigOptionRequest{ - SessionId: acp.SessionId(sessionID), - ConfigId: acp.SessionConfigId(configID), - Value: acp.SessionConfigValueId(value), + ValueId: &acp.SetSessionConfigOptionValueId{ + SessionId: acp.SessionId(sessionID), + ConfigId: acp.SessionConfigId(configID), + Value: acp.SessionConfigValueId(value), + }, }) if err != nil { return fmt.Errorf("set session config option failed: %w", err) diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/meta_convert.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/meta_convert.go index 70c9e25f38..614d90abed 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/meta_convert.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/meta_convert.go @@ -5,6 +5,7 @@ import ( "github.com/coder/acp-go-sdk" + acpclient "github.com/kandev/kandev/internal/agentctl/server/acp" "github.com/kandev/kandev/internal/agentctl/types/streams" ) @@ -16,11 +17,15 @@ func convertAuthMethods(methods []acp.AuthMethod) []streams.AuthMethodInfo { } result := make([]streams.AuthMethodInfo, 0, len(methods)) for _, m := range methods { + id, name, desc, meta := acpclient.AuthMethodFields(m) + if id == "" && name == "" { + continue + } info := streams.AuthMethodInfo{ - ID: string(m.Id), - Name: m.Name, - Description: derefStr(m.Description), - Meta: toStringMap(m.Meta), + ID: id, + Name: name, + Description: derefStr(desc), + Meta: toStringMap(meta), } // Normalize _meta.terminal-auth → TerminalAuth info.TerminalAuth = extractTerminalAuth(info.Meta) @@ -65,7 +70,7 @@ func extractTerminalAuth(meta map[string]any) *streams.TerminalAuth { // convertSessionModels converts ACP model info to stream types, // normalizing known _meta patterns (copilotUsage) while preserving raw _meta. -func convertSessionModels(models []acp.UnstableModelInfo) []streams.SessionModelInfo { +func convertSessionModels(models []acp.ModelInfo) []streams.SessionModelInfo { if len(models) == 0 { return nil } diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/ordering_race_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/ordering_race_test.go index 0105b7eab0..0537eda7fd 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/ordering_race_test.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/ordering_race_test.go @@ -194,12 +194,9 @@ func TestACPUpdateHandlerOrdering(t *testing.T) { // Helper to verify AdapterEvent type (unused variable fix) var _ = streams.EventTypeMessageChunk -// TestNotificationOrderingFix verifies that the ACP SDK fork (github.com/kdlbs/acp-go-sdk) -// correctly preserves notification ordering by processing them synchronously. -// -// This test is the primary verification that the fix works. It should: -// - Pass 100% of the time with the patched SDK -// - Fail frequently (40-80% out of order) with the original SDK +// TestNotificationOrderingFix verifies that the ACP SDK preserves notification +// ordering by serializing notification processing. Upstream coder/acp-go-sdk#8 +// (merged 2026-02-24) is the fix this regression guards against. func TestNotificationOrderingFix(t *testing.T) { const numChunks = 200 // Use more chunks to increase confidence diff --git a/apps/backend/internal/agentctl/server/utility/acp_executor.go b/apps/backend/internal/agentctl/server/utility/acp_executor.go index 20511f4905..24df809aa4 100644 --- a/apps/backend/internal/agentctl/server/utility/acp_executor.go +++ b/apps/backend/internal/agentctl/server/utility/acp_executor.go @@ -215,14 +215,12 @@ func (e *ACPInferenceExecutor) executeACPSession( return result, nil } -// toACPMcpServers converts the cross-process DTO list into the ACP SDK -// shape. Uses the *Inline variants because the agentcooper fork (vendored -// via go.mod replace) names them that way; the upstream SDK would call them -// McpServerHttp / McpServerSse. Returns nil when there are no entries so -// callers can use the nil-as-empty convention upstream. The second return -// value carries the names of any DTOs we couldn't convert (unsupported -// transport, e.g. stdio) so the caller can surface them in logs rather than -// having them silently disappear from the agent's tool surface. +// toACPMcpServers converts the cross-process DTO list into the ACP SDK shape. +// Returns nil when there are no entries so callers can use the nil-as-empty +// convention. The second return value carries the names of any DTOs we +// couldn't convert (unsupported transport, e.g. stdio) so the caller can +// surface them in logs rather than having them silently disappear from the +// agent's tool surface. func toACPMcpServers(in []MCPServerDTO) ([]acp.McpServer, []string) { if len(in) == 0 { return nil, nil @@ -424,11 +422,15 @@ func buildInitProbeFields(initResp acp.InitializeResponse) *ProbeResponse { out.AgentVersion = initResp.AgentInfo.Version } for _, m := range initResp.AuthMethods { + id, name, desc, meta := acpclient.AuthMethodFields(m) + if id == "" && name == "" { + continue + } out.AuthMethods = append(out.AuthMethods, ProbeAuthMethod{ - ID: string(m.Id), //nolint:unconvert // AuthMethodId is a named string type; conversion required - Name: m.Name, - Description: derefString(m.Description), - Meta: m.Meta, + ID: id, + Name: name, + Description: derefString(desc), + Meta: meta, }) } return out