From cda044316e5d7db021cb2b87951c4cdd345ca5bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Sat, 18 Apr 2026 17:01:53 +0100 Subject: [PATCH] fix(docker): route user shell to container instead of host (#638) --- .../agent/lifecycle/manager_interaction.go | 36 ++++++ .../lifecycle/manager_interaction_test.go | 108 ++++++++++++++++++ .../gateway/websocket/terminal_handler.go | 22 +--- apps/backend/internal/task/models/models.go | 12 ++ 4 files changed, 159 insertions(+), 19 deletions(-) diff --git a/apps/backend/internal/agent/lifecycle/manager_interaction.go b/apps/backend/internal/agent/lifecycle/manager_interaction.go index 57954f4235..3a3cb74daa 100644 --- a/apps/backend/internal/agent/lifecycle/manager_interaction.go +++ b/apps/backend/internal/agent/lifecycle/manager_interaction.go @@ -572,6 +572,42 @@ func (m *Manager) IsRemoteSession(ctx context.Context, sessionID string) bool { return info.RuntimeName == string(executor.NameSprites) || info.RuntimeName == string(executor.NameRemoteDocker) } +// ShouldUseContainerShell checks whether a session's shell should run inside a container/sandbox +// (via agentctl) rather than on the host. This is true for Docker, Sprites, and remote executors. +// It first checks the in-memory execution store, then falls back to the database. +func (m *Manager) ShouldUseContainerShell(ctx context.Context, sessionID string) bool { + // Check in-memory execution first (fast path). + if execution, exists := m.executionStore.GetBySessionID(sessionID); exists { + // Docker and Sprites executors run shells inside the container/sandbox + if execution.RuntimeName == string(executor.NameDocker) || + execution.RuntimeName == string(executor.NameSprites) { + return true + } + if execution.Metadata != nil { + if isRemote, ok := execution.Metadata[MetadataKeyIsRemote].(bool); ok && isRemote { + return true + } + } + return false + } + + // Fall back to database records (post-restart, execution not yet recreated). + if m.workspaceInfoProvider == nil { + return false + } + info, err := m.workspaceInfoProvider.GetWorkspaceInfoForSession(ctx, "", sessionID) + if err != nil || info == nil { + return false + } + if models.IsContainerizedExecutorType(models.ExecutorType(info.ExecutorType)) { + return true + } + // Backwards compatibility: old records may only have RuntimeName set. + return info.RuntimeName == string(executor.NameDocker) || + info.RuntimeName == string(executor.NameSprites) || + info.RuntimeName == string(executor.NameRemoteDocker) +} + // GetAvailableCommandsForSession returns the available slash commands for a session. // Returns nil if the session doesn't exist or has no commands stored. // diff --git a/apps/backend/internal/agent/lifecycle/manager_interaction_test.go b/apps/backend/internal/agent/lifecycle/manager_interaction_test.go index 8cc6099bbf..664f5ceb26 100644 --- a/apps/backend/internal/agent/lifecycle/manager_interaction_test.go +++ b/apps/backend/internal/agent/lifecycle/manager_interaction_test.go @@ -621,6 +621,114 @@ func TestIsRemoteSession(t *testing.T) { }) } +func TestShouldUseContainerShell(t *testing.T) { + t.Run("in-memory execution with docker runtime", func(t *testing.T) { + store := NewExecutionStore() + store.Add(&AgentExecution{ + ID: "exec-1", + SessionID: "session-1", + RuntimeName: string(executor.NameDocker), + Status: v1.AgentStatusRunning, + }) + mgr := &Manager{executionStore: store} + require.True(t, mgr.ShouldUseContainerShell(context.Background(), "session-1")) + }) + + t.Run("in-memory execution with sprites runtime", func(t *testing.T) { + store := NewExecutionStore() + store.Add(&AgentExecution{ + ID: "exec-2", + SessionID: "session-2", + RuntimeName: string(executor.NameSprites), + Status: v1.AgentStatusRunning, + }) + mgr := &Manager{executionStore: store} + require.True(t, mgr.ShouldUseContainerShell(context.Background(), "session-2")) + }) + + t.Run("in-memory execution with is_remote metadata", func(t *testing.T) { + store := NewExecutionStore() + store.Add(&AgentExecution{ + ID: "exec-3", + SessionID: "session-3", + RuntimeName: string(executor.NameStandalone), + Status: v1.AgentStatusRunning, + Metadata: map[string]interface{}{MetadataKeyIsRemote: true}, + }) + mgr := &Manager{executionStore: store} + require.True(t, mgr.ShouldUseContainerShell(context.Background(), "session-3")) + }) + + t.Run("in-memory execution with standalone runtime", func(t *testing.T) { + store := NewExecutionStore() + store.Add(&AgentExecution{ + ID: "exec-4", + SessionID: "session-4", + RuntimeName: string(executor.NameStandalone), + Status: v1.AgentStatusRunning, + }) + mgr := &Manager{executionStore: store} + require.False(t, mgr.ShouldUseContainerShell(context.Background(), "session-4")) + }) + + t.Run("not in memory, DB returns local_docker executor type", func(t *testing.T) { + store := NewExecutionStore() + provider := &mockWorkspaceInfoProvider{ + infos: map[string]*WorkspaceInfo{ + "session-5": {ExecutorType: string(models.ExecutorTypeLocalDocker)}, + }, + } + mgr := &Manager{executionStore: store, workspaceInfoProvider: provider} + require.True(t, mgr.ShouldUseContainerShell(context.Background(), "session-5")) + }) + + t.Run("not in memory, DB returns sprites executor type", func(t *testing.T) { + store := NewExecutionStore() + provider := &mockWorkspaceInfoProvider{ + infos: map[string]*WorkspaceInfo{ + "session-6": {ExecutorType: string(models.ExecutorTypeSprites)}, + }, + } + mgr := &Manager{executionStore: store, workspaceInfoProvider: provider} + require.True(t, mgr.ShouldUseContainerShell(context.Background(), "session-6")) + }) + + t.Run("not in memory, DB returns docker runtime name", func(t *testing.T) { + store := NewExecutionStore() + provider := &mockWorkspaceInfoProvider{ + infos: map[string]*WorkspaceInfo{ + "session-7": {RuntimeName: string(executor.NameDocker)}, + }, + } + mgr := &Manager{executionStore: store, workspaceInfoProvider: provider} + require.True(t, mgr.ShouldUseContainerShell(context.Background(), "session-7")) + }) + + t.Run("not in memory, DB returns local type", func(t *testing.T) { + store := NewExecutionStore() + provider := &mockWorkspaceInfoProvider{ + infos: map[string]*WorkspaceInfo{ + "session-8": {ExecutorType: string(models.ExecutorTypeLocal)}, + }, + } + mgr := &Manager{executionStore: store, workspaceInfoProvider: provider} + require.False(t, mgr.ShouldUseContainerShell(context.Background(), "session-8")) + }) + + t.Run("not in memory, DB error returns false", func(t *testing.T) { + store := NewExecutionStore() + provider := &mockWorkspaceInfoProvider{err: fmt.Errorf("db error")} + mgr := &Manager{executionStore: store, workspaceInfoProvider: provider} + require.False(t, mgr.ShouldUseContainerShell(context.Background(), "session-9")) + }) + + t.Run("nil workspaceInfoProvider returns false", func(t *testing.T) { + store := NewExecutionStore() + mgr := &Manager{executionStore: store} + require.False(t, mgr.ShouldUseContainerShell(context.Background(), "nonexistent")) + }) +} + func TestFallbackAuthMethods(t *testing.T) { t.Run("claude-acp returns auth login method", func(t *testing.T) { methods := fallbackAuthMethods("claude-acp") diff --git a/apps/backend/internal/gateway/websocket/terminal_handler.go b/apps/backend/internal/gateway/websocket/terminal_handler.go index 9f6c1b7c44..2828d6ce64 100644 --- a/apps/backend/internal/gateway/websocket/terminal_handler.go +++ b/apps/backend/internal/gateway/websocket/terminal_handler.go @@ -19,7 +19,6 @@ import ( gorillaws "github.com/gorilla/websocket" "go.uber.org/zap" - "github.com/kandev/kandev/internal/agent/executor" "github.com/kandev/kandev/internal/agent/lifecycle" "github.com/kandev/kandev/internal/agentctl/server/process" "github.com/kandev/kandev/internal/common/logger" @@ -213,24 +212,9 @@ func (h *TerminalHandler) HandleTerminalWS(c *gin.Context) { } func (h *TerminalHandler) shouldUseWorkspaceShell(ctx context.Context, sessionID string) bool { - // Check in-memory execution first (fast path when execution is already running). - execution, exists := h.lifecycleMgr.GetExecutionBySessionID(sessionID) - if exists { - if execution.RuntimeName == string(executor.NameSprites) { - return true - } - if execution.Metadata != nil { - if isRemote, ok := execution.Metadata[lifecycle.MetadataKeyIsRemote].(bool); ok && isRemote { - return true - } - } - return false - } - - // Execution not in memory — check DB records. After a backend restart the - // execution may not exist yet (resume in progress), but the session's - // executor type is persisted in the database. - return h.lifecycleMgr.IsRemoteSession(ctx, sessionID) + // Delegates to lifecycle manager which checks both in-memory execution + // and database records for containerized/remote executors. + return h.lifecycleMgr.ShouldUseContainerShell(ctx, sessionID) } // waitForRemoteExecution polls the lifecycle manager for the session's execution, diff --git a/apps/backend/internal/task/models/models.go b/apps/backend/internal/task/models/models.go index 40d6bdae69..6ae0b8bfb8 100644 --- a/apps/backend/internal/task/models/models.go +++ b/apps/backend/internal/task/models/models.go @@ -397,6 +397,18 @@ func IsRemoteExecutorType(t ExecutorType) bool { } } +// IsContainerizedExecutorType reports whether the given executor type runs +// in a container/sandbox where shells must be executed inside the container +// via agentctl, not on the host. +func IsContainerizedExecutorType(t ExecutorType) bool { + switch t { + case ExecutorTypeLocalDocker, ExecutorTypeSprites, ExecutorTypeRemoteDocker, ExecutorTypeMockRemote: + return true + default: + return false + } +} + // IsAlwaysResumableRuntime reports whether the given runtime string represents // an executor that can always be resumed even without an explicit resume token. func IsAlwaysResumableRuntime(runtime string) bool {