From 09b5c4df7aa51cc75bbe434b7e82c45d430b2603 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Tue, 28 Jul 2026 21:19:27 +0100 Subject: [PATCH] feat: propagate managed git environment to terminals (#2010) --- .../runtime/lifecycle/execution_store.go | 1 + .../runtime/lifecycle/execution_store_test.go | 14 ++ .../runtime/lifecycle/executor_backend.go | 4 +- .../lifecycle/executor_backend_test.go | 15 ++ .../runtime/lifecycle/manager_passthrough.go | 5 +- .../lifecycle/manager_passthrough_test.go | 29 ++++ .../internal/agent/runtime/lifecycle/types.go | 43 +++++- .../agentctl/server/process/manager.go | 76 +++++++--- .../server/process/manager_temp_test.go | 105 ++++++++++++++ .../internal/agentctl/server/shell/session.go | 15 ++ .../gateway/websocket/terminal_handler.go | 18 ++- .../websocket/terminal_handler_test.go | 63 +++++++++ .../task-terminal-git-environment/plan.md | 132 ++++++++++++++++++ .../task-01-runtime-terminal-environment.md | 58 ++++++++ ...k-02-agentctl-shell-process-environment.md | 55 ++++++++ .../task-03-document-terminal-git-routing.md | 53 +++++++ docs/public/developer-tools.md | 5 + docs/public/executors.md | 6 + .../integrations/github-authentication.md | 23 ++- 19 files changed, 694 insertions(+), 26 deletions(-) create mode 100644 docs/plans/task-terminal-git-environment/plan.md create mode 100644 docs/plans/task-terminal-git-environment/task-01-runtime-terminal-environment.md create mode 100644 docs/plans/task-terminal-git-environment/task-02-agentctl-shell-process-environment.md create mode 100644 docs/plans/task-terminal-git-environment/task-03-document-terminal-git-routing.md diff --git a/apps/backend/internal/agent/runtime/lifecycle/execution_store.go b/apps/backend/internal/agent/runtime/lifecycle/execution_store.go index 6dce8cb3a7..2d25690107 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/execution_store.go +++ b/apps/backend/internal/agent/runtime/lifecycle/execution_store.go @@ -106,6 +106,7 @@ func (s *ExecutionStore) Remove(executionID string) { if execution.ContainerID != "" { delete(s.byContainer, execution.ContainerID) } + execution.clearRuntimeEnvironment() // Remove from primary map delete(s.executions, executionID) diff --git a/apps/backend/internal/agent/runtime/lifecycle/execution_store_test.go b/apps/backend/internal/agent/runtime/lifecycle/execution_store_test.go index cb8133b2b7..c631825577 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/execution_store_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/execution_store_test.go @@ -70,6 +70,20 @@ func TestExecutionStore_AddReplaceAfterRemove(t *testing.T) { } } +func TestExecutionStore_RemoveClearsRuntimeEnvironment(t *testing.T) { + store := NewExecutionStore() + execution := &AgentExecution{ID: "exec-1"} + execution.setRuntimeEnvironment(map[string]string{"BROKER": "runtime-only"}) + if err := store.Add(execution); err != nil { + t.Fatalf("Add() error = %v", err) + } + + store.Remove(execution.ID) + if got := execution.RuntimeEnvironment(); got != nil { + t.Fatalf("RuntimeEnvironment() after Remove = %#v, want nil", got) + } +} + func TestExecutionStore_AddNoSessionIDAlwaysSucceeds(t *testing.T) { store := NewExecutionStore() diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_backend.go b/apps/backend/internal/agent/runtime/lifecycle/executor_backend.go index 80246f14c5..086d54277b 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_backend.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_backend.go @@ -388,7 +388,7 @@ func (ri *ExecutorInstance) ToAgentExecution(req *ExecutorCreateRequest) *AgentE } } - return &AgentExecution{ + execution := &AgentExecution{ ID: ri.InstanceID, TaskID: req.TaskID, SessionID: req.SessionID, @@ -410,4 +410,6 @@ func (ri *ExecutorInstance) ToAgentExecution(req *ExecutorCreateRequest) *AgentE historyEnabled: historyEnabled, promptDoneCh: make(chan PromptCompletionSignal, 1), } + execution.setRuntimeEnvironment(req.Env) + return execution } diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_backend_test.go b/apps/backend/internal/agent/runtime/lifecycle/executor_backend_test.go index 6b430b9c85..cf8c9be5b0 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_backend_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_backend_test.go @@ -77,3 +77,18 @@ func TestToAgentExecutionRecordsHistoryForWorkspaceRebindFallback(t *testing.T) require.True(t, execution.historyEnabled) } + +func TestToAgentExecutionCapturesDefensiveRuntimeEnvironment(t *testing.T) { + reqEnv := map[string]string{ + "KANDEV_GITHUB_CREDENTIAL_BROKER_URL": "http://127.0.0.1:9876", + "PATH": "/tmp/kandev-shim:/usr/bin", + } + execution := (&ExecutorInstance{InstanceID: "execution"}).ToAgentExecution(&ExecutorCreateRequest{Env: reqEnv}) + + reqEnv["PATH"] = "/usr/bin" + got := execution.RuntimeEnvironment() + require.Equal(t, "/tmp/kandev-shim:/usr/bin", got["PATH"]) + + got["PATH"] = "/mutated" + require.Equal(t, "/tmp/kandev-shim:/usr/bin", execution.RuntimeEnvironment()["PATH"]) +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_passthrough.go b/apps/backend/internal/agent/runtime/lifecycle/manager_passthrough.go index 359ff78012..8e78970ba0 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_passthrough.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_passthrough.go @@ -138,7 +138,10 @@ func (m *Manager) GetPassthroughBuffer(ctx context.Context, sessionID string) (s // buildPassthroughEnv builds the environment map for a passthrough session, // including Kandev metadata and required credentials from the agent runtime config. func (m *Manager) buildPassthroughEnv(ctx context.Context, execution *AgentExecution, requiredEnv []string) map[string]string { - env := make(map[string]string) + env := execution.RuntimeEnvironment() + if env == nil { + env = make(map[string]string) + } env["KANDEV_TASK_ID"] = execution.TaskID env["KANDEV_SESSION_ID"] = execution.SessionID env["KANDEV_AGENT_PROFILE_ID"] = execution.officeProfileID() diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_passthrough_test.go b/apps/backend/internal/agent/runtime/lifecycle/manager_passthrough_test.go index d2daeb734a..e66fbf7d73 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_passthrough_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_passthrough_test.go @@ -1094,6 +1094,35 @@ func TestBuildPassthroughEnv_MergesProfileEnvVars(t *testing.T) { } } +func TestBuildPassthroughEnvIncludesEffectiveRuntimeEnv(t *testing.T) { + mgr := newTestManager(t) + execution := &AgentExecution{ + TaskID: "task-1", + SessionID: "session-1", + AgentProfileID: "profile-1", + } + execution.setRuntimeEnvironment(map[string]string{ + "KANDEV_GITHUB_CREDENTIAL_BROKER_URL": "http://127.0.0.1:9876", + "GIT_CONFIG_COUNT": "1", + "GIT_CONFIG_KEY_0": "credential.helper", + "GIT_CONFIG_VALUE_0": "!agentctl git-credential", + "PATH": "/tmp/kandev-shim:/usr/bin", + }) + + env := mgr.buildPassthroughEnv(context.Background(), execution, nil) + for key, want := range map[string]string{ + "KANDEV_GITHUB_CREDENTIAL_BROKER_URL": "http://127.0.0.1:9876", + "GIT_CONFIG_COUNT": "1", + "GIT_CONFIG_KEY_0": "credential.helper", + "GIT_CONFIG_VALUE_0": "!agentctl git-credential", + "PATH": "/tmp/kandev-shim:/usr/bin", + } { + if env[key] != want { + t.Fatalf("passthrough env[%q] = %q, want %q (env=%#v)", key, env[key], want, env) + } + } +} + func TestBuildInteractiveStartRequestCarriesStripEnv(t *testing.T) { stripEnv := []string{"ACP_BACKEND"} req := buildInteractiveStartRequest( diff --git a/apps/backend/internal/agent/runtime/lifecycle/types.go b/apps/backend/internal/agent/runtime/lifecycle/types.go index 133fc7ae82..6f2541162b 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/types.go +++ b/apps/backend/internal/agent/runtime/lifecycle/types.go @@ -53,8 +53,14 @@ type AgentExecution struct { ExitCode *int ErrorMessage string Metadata map[string]interface{} - promptGeneration uint64 - promptLifecycleMu sync.Mutex + // runtimeEnv is the effective environment used to create the task's + // runtime instance. It is kept in memory only so authorized task-scoped + // terminals and passthrough processes can inherit the same credentials and + // PATH as the agent subprocess without persisting secrets in metadata. + runtimeEnv map[string]string + runtimeEnvMu sync.RWMutex + promptGeneration uint64 + promptLifecycleMu sync.Mutex // PrepareResult carries the environment preparation result back to the caller // so it can be persisted synchronously before UpdateTaskSession clobbers metadata. @@ -181,6 +187,39 @@ type AgentExecution struct { sessionSpanMu sync.RWMutex } +// setRuntimeEnvironment stores a defensive copy of the effective task runtime +// environment. Callers must use RuntimeEnvironment when reading it. +func (e *AgentExecution) setRuntimeEnvironment(env map[string]string) { + if e == nil { + return + } + e.runtimeEnvMu.Lock() + e.runtimeEnv = cloneStringMap(env) + e.runtimeEnvMu.Unlock() +} + +// RuntimeEnvironment returns a defensive copy of the effective task runtime +// environment. The lifecycle manager only exposes an execution after the +// caller's task/session ownership check has succeeded. +func (e *AgentExecution) RuntimeEnvironment() map[string]string { + if e == nil { + return nil + } + e.runtimeEnvMu.RLock() + env := cloneStringMap(e.runtimeEnv) + e.runtimeEnvMu.RUnlock() + return env +} + +func (e *AgentExecution) clearRuntimeEnvironment() { + if e == nil { + return + } + e.runtimeEnvMu.Lock() + e.runtimeEnv = nil + e.runtimeEnvMu.Unlock() +} + func (e *AgentExecution) officeProfileID() string { if e == nil { return "" diff --git a/apps/backend/internal/agentctl/server/process/manager.go b/apps/backend/internal/agentctl/server/process/manager.go index 818ab6083e..f3ffc72c86 100644 --- a/apps/backend/internal/agentctl/server/process/manager.go +++ b/apps/backend/internal/agentctl/server/process/manager.go @@ -25,6 +25,7 @@ import ( "github.com/kandev/kandev/internal/agentctl/types" "github.com/kandev/kandev/internal/agentctl/types/streams" "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/gitconfigenv" tools "github.com/kandev/kandev/internal/tools/installer" "go.uber.org/zap" ) @@ -677,7 +678,11 @@ func (m *Manager) StartProcess(ctx context.Context, req StartProcessRequest) (*P if m.processRunner == nil { return nil, fmt.Errorf("process runner not available") } - return m.processRunner.Start(ctx, req) + effectiveReq, err := m.buildProcessRequest(req) + if err != nil { + return nil, fmt.Errorf("prepare process environment: %w", err) + } + return m.processRunner.Start(ctx, effectiveReq) } // StopProcess stops a running process by ID. @@ -1220,8 +1225,13 @@ func (m *Manager) startAgentShell() { if !m.cfg.ShellEnabled { return } - shellCfg := shell.DefaultConfig(m.cfg.WorkDir) - shellCfg.ShellCommand = preferredShellCommand(m.cfg.AgentEnv) + // Start calls this while holding startMu. Copy AgentEnv directly instead of + // calling agentEnvSnapshot, which would attempt to acquire the same mutex. + shellCfg, err := m.buildShellConfigWithAgentEnv(append([]string(nil), m.cfg.AgentEnv...)) + if err != nil { + m.logger.Warn("failed to prepare shell environment", zap.Error(err)) + return + } shellSession, err := shell.NewSession(shellCfg, m.logger) if err != nil { m.logger.Warn("failed to create shell session", zap.Error(err)) @@ -1231,6 +1241,31 @@ func (m *Manager) startAgentShell() { m.logger.Info("shell session auto-created") } +// buildShellConfig creates the embedded shell configuration with the same +// effective instance environment used by agent and terminal processes. +func (m *Manager) buildShellConfig() (shell.Config, error) { + return m.buildShellConfigWithAgentEnv(m.agentEnvSnapshot()) +} + +func (m *Manager) buildShellConfigWithAgentEnv(agentEnv []string) (shell.Config, error) { + shellCfg := shell.DefaultConfig(m.cfg.WorkDir) + shellCfg.ShellCommand = preferredShellCommand(agentEnv) + var err error + shellCfg.Env, err = mergeAgentEnvIntoShellConfigWithError(agentEnv, nil) + if err != nil { + return shell.Config{}, err + } + return shellCfg, nil +} + +// buildProcessRequest applies the instance environment to a task-scoped +// process request. Explicit request values remain authoritative. +func (m *Manager) buildProcessRequest(req StartProcessRequest) (StartProcessRequest, error) { + var err error + req.Env, err = mergeAgentEnvIntoShellConfigWithError(m.agentEnvSnapshot(), req.Env) + return req, err +} + func preferredShellCommand(env []string) string { if value := lookupEnvValue(env, "AGENTCTL_SHELL_COMMAND"); value != "" { return value @@ -2231,8 +2266,10 @@ func (m *Manager) StartShell() error { if !m.cfg.ShellEnabled { return nil } - shellCfg := shell.DefaultConfig(m.cfg.WorkDir) - shellCfg.ShellCommand = preferredShellCommand(m.cfg.AgentEnv) + shellCfg, err := m.buildShellConfig() + if err != nil { + return fmt.Errorf("prepare shell environment: %w", err) + } shellSession, err := shell.NewSession(shellCfg, m.logger) if err != nil { return fmt.Errorf("failed to create shell session: %w", err) @@ -2253,7 +2290,10 @@ func (m *Manager) StartTerminalShell(terminalID string, cfg shell.Config) (*shel return nil, err } defer release() - cfg.Env = mergeAgentEnvIntoShellConfig(m.agentEnvSnapshot(), cfg.Env) + cfg.Env, err = mergeAgentEnvIntoShellConfigWithError(m.agentEnvSnapshot(), cfg.Env) + if err != nil { + return nil, fmt.Errorf("prepare terminal shell environment: %w", err) + } return m.shellMgr.Start(terminalID, cfg) } @@ -2267,27 +2307,27 @@ func (m *Manager) agentEnvSnapshot() []string { // mergeAgentEnvIntoShellConfig folds the instance agent env into the shell's // override map. Explicit overrides take precedence over the inherited value. -// -// With no agent env there is nothing to merge, so the caller's map is returned -// as-is rather than defensively copied: the sole caller holds cfg by value and -// immediately assigns the result back to its own cfg.Env, so no alias outlives -// this call. func mergeAgentEnvIntoShellConfig(agentEnv []string, overrides map[string]string) map[string]string { - if len(agentEnv) == 0 { + merged, err := mergeAgentEnvIntoShellConfigWithError(agentEnv, overrides) + if err != nil { return overrides } - merged := make(map[string]string) + return merged +} + +func mergeAgentEnvIntoShellConfigWithError(agentEnv []string, overrides map[string]string) (map[string]string, error) { + if len(agentEnv) == 0 && len(overrides) == 0 { + return overrides, nil + } + base := make(map[string]string, len(agentEnv)) for _, entry := range agentEnv { eq := strings.IndexByte(entry, '=') if eq <= 0 { continue } - merged[entry[:eq]] = entry[eq+1:] + base[entry[:eq]] = entry[eq+1:] } - for k, v := range overrides { - merged[k] = v - } - return merged + return gitconfigenv.Merge(base, overrides) } // StartVscode starts the code-server process on a random OS-assigned port. diff --git a/apps/backend/internal/agentctl/server/process/manager_temp_test.go b/apps/backend/internal/agentctl/server/process/manager_temp_test.go index 9b5088e971..329cd2ddf7 100644 --- a/apps/backend/internal/agentctl/server/process/manager_temp_test.go +++ b/apps/backend/internal/agentctl/server/process/manager_temp_test.go @@ -6,12 +6,14 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strings" "testing" "time" "github.com/kandev/kandev/internal/agentctl/server/config" "github.com/kandev/kandev/internal/agentctl/server/shell" + "github.com/kandev/kandev/pkg/agent" ) func envValue(env []string, key string) string { @@ -86,6 +88,109 @@ func TestManager_BuildFinalCommandLeavesUnsetTempEnvironmentUnset(t *testing.T) assertNoAgentTempRoot(t, serviceTemp) } +func TestManager_StartShellInheritsAgentEnvironment(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("PTY-backed shell sessions are unsupported on Windows") + } + mgr := NewManager(&config.InstanceConfig{ + WorkDir: t.TempDir(), + ShellEnabled: true, + AgentEnv: []string{ + "KANDEV_GITHUB_CREDENTIAL_BROKER_URL=http://127.0.0.1:9876", + "PATH=/tmp/kandev-shim:/usr/bin", + }, + }, newTestLogger(t)) + if err := mgr.StartShell(); err != nil { + t.Fatalf("StartShell() error = %v", err) + } + t.Cleanup(func() { _ = mgr.shell.Stop() }) + cfg := mgr.shell.Config() + if got := cfg.Env["KANDEV_GITHUB_CREDENTIAL_BROKER_URL"]; got != "http://127.0.0.1:9876" { + t.Fatalf("shell broker env = %q, want managed broker URL", got) + } + if got := cfg.Env["PATH"]; got != "/tmp/kandev-shim:/usr/bin" { + t.Fatalf("shell PATH = %q, want shim-first PATH", got) + } +} + +func TestManager_StartProcessInheritsAgentEnvironment(t *testing.T) { + mgr := NewManager(&config.InstanceConfig{ + AgentEnv: []string{ + "KANDEV_GITHUB_CREDENTIAL_BROKER_URL=http://127.0.0.1:9876", + "PATH=/tmp/kandev-shim:/usr/bin", + }, + }, newTestLogger(t)) + req, err := mgr.buildProcessRequest(StartProcessRequest{ + SessionID: "session-1", + Command: "echo ok", + Env: map[string]string{ + "COMMAND_ONLY": "yes", + "PATH": "/request/path", + }, + }) + if err != nil { + t.Fatalf("buildProcessRequest() error = %v", err) + } + if got := req.Env["KANDEV_GITHUB_CREDENTIAL_BROKER_URL"]; got != "http://127.0.0.1:9876" { + t.Fatalf("process broker env = %q, want managed broker URL", got) + } + if got := req.Env["PATH"]; got != "/request/path" { + t.Fatalf("process PATH = %q, want explicit request value", got) + } + if got := req.Env["COMMAND_ONLY"]; got != "yes" { + t.Fatalf("process explicit env = %q, want yes", got) + } +} + +func TestManager_ProcessEnvironmentMergesIndexedGitConfig(t *testing.T) { + mgr := NewManager(&config.InstanceConfig{ + AgentEnv: []string{ + "GIT_CONFIG_COUNT=1", + "GIT_CONFIG_KEY_0=credential.helper", + "GIT_CONFIG_VALUE_0=!agentctl git-credential", + }, + }, newTestLogger(t)) + req, err := mgr.buildProcessRequest(StartProcessRequest{ + SessionID: "session-1", + Command: "echo ok", + Env: map[string]string{ + "GIT_CONFIG_COUNT": "1", + "GIT_CONFIG_KEY_0": "core.hooksPath", + "GIT_CONFIG_VALUE_0": "/tmp/hooks", + }, + }) + if err != nil { + t.Fatalf("buildProcessRequest() error = %v", err) + } + if got := req.Env["GIT_CONFIG_COUNT"]; got != "2" { + t.Fatalf("GIT_CONFIG_COUNT = %q, want merged count 2", got) + } + if got := req.Env["GIT_CONFIG_KEY_1"]; got != "core.hooksPath" { + t.Fatalf("GIT_CONFIG_KEY_1 = %q, want request entry appended", got) + } +} + +func TestManager_StartAutoShellDoesNotDeadlockOnEnvironmentSnapshot(t *testing.T) { + mgr := NewManager(&config.InstanceConfig{ + AgentArgs: []string{"echo"}, + ShellEnabled: true, + Protocol: agent.ProtocolACP, + }, newTestLogger(t)) + mgr.adapter = newOneShotStubAdapter() + t.Cleanup(func() { _ = mgr.Stop(context.Background()) }) + + done := make(chan error, 1) + go func() { done <- mgr.Start(context.Background()) }() + select { + case err := <-done: + if err != nil { + t.Fatalf("Start() error = %v", err) + } + case <-time.After(2 * time.Second): + t.Fatal("Start() deadlocked while creating the auto shell") + } +} + func TestManager_BeginStopWaitsForInFlightAdmission(t *testing.T) { mgr := NewManager(&config.InstanceConfig{WorkDir: t.TempDir()}, newTestLogger(t)) release, err := mgr.admitStart() diff --git a/apps/backend/internal/agentctl/server/shell/session.go b/apps/backend/internal/agentctl/server/shell/session.go index 2ec3c9273b..c023c02959 100644 --- a/apps/backend/internal/agentctl/server/shell/session.go +++ b/apps/backend/internal/agentctl/server/shell/session.go @@ -430,6 +430,21 @@ func (s *Session) Status() Status { } } +// Config returns a defensive copy of the session configuration. +func (s *Session) Config() Config { + s.mu.RLock() + defer s.mu.RUnlock() + cfg := s.config + cfg.ShellArgs = append([]string(nil), s.config.ShellArgs...) + if s.config.Env != nil { + cfg.Env = make(map[string]string, len(s.config.Env)) + for key, value := range s.config.Env { + cfg.Env[key] = value + } + } + return cfg +} + // readOutput continuously reads from PTY and broadcasts to subscribers func (s *Session) readOutput() { buf := make([]byte, 4096) diff --git a/apps/backend/internal/gateway/websocket/terminal_handler.go b/apps/backend/internal/gateway/websocket/terminal_handler.go index 34d35fa39c..91f1e99236 100644 --- a/apps/backend/internal/gateway/websocket/terminal_handler.go +++ b/apps/backend/internal/gateway/websocket/terminal_handler.go @@ -420,8 +420,21 @@ func (h *TerminalHandler) startUserShellProcess( // sees the same variables the agent subprocess and the repository setup // script get. Best-effort: an unresolvable profile just yields no extras. profileEnv := h.lifecycleMgr.ExecutorProfileEnvForSession(c.Request.Context(), sessionID, scopeID) + shellEnv := execution.RuntimeEnvironment() + if shellEnv == nil { + shellEnv = make(map[string]string, len(profileEnv)) + } + // The effective runtime environment is authoritative: it includes managed + // Git credentials and the shim-first PATH selected for this execution. + // Profile resolution remains a best-effort fallback for recovered or older + // executions that do not have an in-memory runtime snapshot. + for key, value := range profileEnv { + if _, exists := shellEnv[key]; !exists { + shellEnv[key] = value + } + } - opts := &process.UserShellOptions{Label: label, InitialCommand: initialCommand, Env: profileEnv} + opts := &process.UserShellOptions{Label: label, InitialCommand: initialCommand, Env: shellEnv} h.logger.Info("handleUserShellWS: calling StartUserShell", zap.String("session_id", sessionID), @@ -430,7 +443,8 @@ func (h *TerminalHandler) startUserShellProcess( zap.String("preferred_shell", preferredShell), zap.String("label", opts.Label), zap.String("initial_command", opts.InitialCommand), - zap.Int("profile_env_count", len(profileEnv))) + zap.Int("profile_env_count", len(profileEnv)), + zap.Int("shell_env_count", len(shellEnv))) info, err := interactiveRunner.StartUserShell( c.Request.Context(), scopeID, sessionID, terminalID, workingDir, preferredShell, opts, diff --git a/apps/backend/internal/gateway/websocket/terminal_handler_test.go b/apps/backend/internal/gateway/websocket/terminal_handler_test.go index 34050c42d6..7eb03915f7 100644 --- a/apps/backend/internal/gateway/websocket/terminal_handler_test.go +++ b/apps/backend/internal/gateway/websocket/terminal_handler_test.go @@ -303,6 +303,9 @@ func TestStartUserShellProcessExportsExecutorProfileEnv(t *testing.T) { }) handler := NewTerminalHandler(manager, nil, nil, log) runner := process.NewInteractiveRunner(nil, log, 2*1024*1024) + t.Cleanup(func() { + _ = runner.StopUserShell(context.Background(), "env-1", "term-1") + }) execution := &lifecycle.AgentExecution{ ID: "exec-1", @@ -328,3 +331,63 @@ func TestStartUserShellProcessExportsExecutorProfileEnv(t *testing.T) { t.Fatalf("shell env = %#v, want executor-profile var exported", env) } } + +func TestStartUserShellProcessExportsEffectiveRuntimeEnv(t *testing.T) { + log := testTerminalLogger(t) + manager := lifecycle.NewManager( + nil, + bus.NewMemoryEventBus(log), + nil, + nil, + nil, + nil, + lifecycle.ExecutorFallbackDeny, + t.TempDir(), + log, + ) + handler := NewTerminalHandler(manager, nil, nil, log) + runner := process.NewInteractiveRunner(nil, log, 2*1024*1024) + t.Cleanup(func() { + _ = runner.StopUserShell(context.Background(), "env-1", "term-1") + }) + + execution := (&lifecycle.ExecutorInstance{ + InstanceID: "exec-1", + WorkspacePath: t.TempDir(), + }).ToAgentExecution(&lifecycle.ExecutorCreateRequest{ + SessionID: "session-1", + TaskEnvironmentID: "env-1", + WorkspacePath: t.TempDir(), + Env: map[string]string{ + "KANDEV_GITHUB_CREDENTIAL_BROKER_URL": "http://127.0.0.1:9876", + "GIT_CONFIG_COUNT": "1", + "GIT_CONFIG_KEY_0": "credential.helper", + "GIT_CONFIG_VALUE_0": "!agentctl git-credential", + "PATH": "/tmp/kandev-shim:/usr/bin", + }, + }) + + gin.SetMode(gin.TestMode) + c, _ := gin.CreateTestContext(httptest.NewRecorder()) + c.Request = httptest.NewRequest(http.MethodGet, "/terminal/environment/env-1?terminalId=term-1", nil) + + processID, status, errMsg := handler.startUserShellProcess(c, execution, "env-1", "term-1", runner) + if errMsg != "" { + t.Fatalf("startUserShellProcess() status=%d error=%q", status, errMsg) + } + env, ok := runner.StartEnvForTesting(processID) + if !ok { + t.Fatalf("process %q not registered", processID) + } + for key, want := range map[string]string{ + "KANDEV_GITHUB_CREDENTIAL_BROKER_URL": "http://127.0.0.1:9876", + "GIT_CONFIG_COUNT": "1", + "GIT_CONFIG_KEY_0": "credential.helper", + "GIT_CONFIG_VALUE_0": "!agentctl git-credential", + "PATH": "/tmp/kandev-shim:/usr/bin", + } { + if env[key] != want { + t.Fatalf("shell env[%q] = %q, want %q (env=%#v)", key, env[key], want, env) + } + } +} diff --git a/docs/plans/task-terminal-git-environment/plan.md b/docs/plans/task-terminal-git-environment/plan.md new file mode 100644 index 0000000000..e790b1ae8b --- /dev/null +++ b/docs/plans/task-terminal-git-environment/plan.md @@ -0,0 +1,132 @@ +--- +spec: docs/specs/integrations/github-authentication.md +created: 2026-07-28 +status: completed +--- + +# Implementation Plan: Managed Task Terminal Git Environment + +## Overview + +Extend the existing managed GitHub credential contract from agent subprocesses to every authorised +task execution surface: local/worktree terminals, remote terminal shells, passthrough-agent PTYs, +legacy agentctl shells, and task-scoped command processes. Reuse the current per-execution +environment and ownership guards; do not persist, log, or expose the broker contract to the +browser. + +## Backend + +### Capture and provide the effective execution environment + +Files: + +- `apps/backend/internal/agent/runtime/lifecycle/types.go` +- `apps/backend/internal/agent/runtime/lifecycle/manager_launch.go` +- `apps/backend/internal/agent/runtime/lifecycle/manager_execution.go` +- `apps/backend/internal/agent/runtime/lifecycle/manager_passthrough.go` +- `apps/backend/internal/gateway/websocket/terminal_handler.go` + +Changes: + +- Keep the already-resolved per-execution environment in memory on `AgentExecution`, using a + defensive copy and clearing it when the execution is removed. +- Provide the effective environment only to lifecycle-owned, already-authorized terminal/process + launch paths; preserve explicit terminal/process overrides as the highest precedence. +- Use it for local/worktree user-shell terminals and passthrough-agent PTYs, including their + managed `GIT_CONFIG_*` block and shim-first `PATH`. +- Preserve executor inheritance: environments without a broker contract retain their current + host/executor behavior and receive no Kandev shim. + +### Make agentctl shell and command launch surfaces inherit instance environment + +Files: + +- `apps/backend/internal/agentctl/server/process/manager.go` +- `apps/backend/internal/agentctl/server/process/runner.go` +- `apps/backend/internal/agentctl/server/process/interactive_shells.go` + +Changes: + +- Merge the instance `AgentEnv` into the legacy `/shell/start` shell, per-terminal agentctl + shells, and `StartProcess` command launches before their explicit call-site environment. +- Retain the existing indexed-Git configuration exactly as one ordered block; do not reconstruct + or log its individual credential values. +- Make process start fail before spawning when effective-environment preparation cannot satisfy + the managed contract, without ambient Git/`gh` fallback. + +### Public documentation + +Files: + +- `docs/public/executors.md` +- `docs/public/developer-tools.md` + +Changes: + +- Document that a terminal opened in a managed task receives the same task-scoped Git/`gh` + routing as its agent, while executor-inheritance mode leaves credentials to the executor. +- State that an already-open terminal keeps its launch environment; open a new terminal after a + session resume or credential-policy change. + +## Frontend + +No UI or browser protocol changes are required. Existing terminal ownership checks and terminal +routes remain the boundary; the backend supplies the process environment after authorization. + +## Tests + +- **What:** a local/worktree terminal and a passthrough-agent PTY inherit a managed broker + contract, Git helper configuration, and shim-first `PATH`; executor-inheritance terminals do + not gain that shim. + **Files:** `apps/backend/internal/agent/runtime/lifecycle/manager_launch_test.go`, + `apps/backend/internal/agent/runtime/lifecycle/manager_passthrough_test.go`, and + `apps/backend/internal/gateway/websocket/terminal_handler_test.go`. + **How:** focused Go tests with a synthetic non-secret broker contract and captured PTY start + environment; assert authorization fails before environment retrieval. +- **What:** legacy agentctl shells, per-terminal agentctl shells, and task-scoped command starts + inherit `AgentEnv`, while explicit call-site values override inherited ones. + **Files:** `apps/backend/internal/agentctl/server/process/runner_shells_test.go` and + `apps/backend/internal/agentctl/server/process/runner_test.go`. + **How:** table-driven environment merge tests and a real short-lived child process that reports + only non-secret marker variables. +- **What:** public documentation remains valid. + **Files:** `docs/public/executors.md`, `docs/public/developer-tools.md`. + **How:** `node --test scripts/validate-public-docs.test.mjs` and + `node scripts/validate-public-docs.mjs`. + +## E2E Tests + +No new Playwright test is planned. The terminal UI and WebSocket contract are unchanged; focused +backend tests exercise the authorization boundary and the actual process environments without +putting a broker contract into an E2E fixture or browser-visible output. + +## Implementation Waves + +Wave 1: + +- [x] [Task 01: Runtime terminal environment propagation](task-01-runtime-terminal-environment.md) + +Wave 2: + +- [x] [Task 02: Agentctl shell and process inheritance](task-02-agentctl-shell-process-environment.md) + +Wave 3: + +- [x] [Task 03: Document and verify terminal Git routing](task-03-document-terminal-git-routing.md) + +Execute sequentially in the primary conversation. No task is delegated unless the user explicitly +authorizes delegation after choosing the implementation model. + +## Risks and non-goals + +- Broker leases and indexed Git configuration are sensitive runtime data. They must remain + in-memory and must not enter terminal metadata, logs, errors, frontend payloads, or command + arguments. +- A terminal already owns host-level execution authority for local/worktree tasks; this change + aligns its credential behavior with the task agent and is not a sandbox boundary. +- This plan does not grant terminal access across task-environment ownership boundaries, change + credential policy selection, persist terminal environments, or support arbitrary host shells. + +## Open Questions + +None. diff --git a/docs/plans/task-terminal-git-environment/task-01-runtime-terminal-environment.md b/docs/plans/task-terminal-git-environment/task-01-runtime-terminal-environment.md new file mode 100644 index 0000000000..b0f4fd6aef --- /dev/null +++ b/docs/plans/task-terminal-git-environment/task-01-runtime-terminal-environment.md @@ -0,0 +1,58 @@ +--- +id: "01-runtime-terminal-environment" +title: "Runtime terminal environment propagation" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/integrations/github-authentication.md" +--- + +# Task 01: Runtime Terminal Environment Propagation + +## Acceptance + +- The lifecycle retains a defensive, runtime-only copy of each execution's resolved environment + and makes it available only to authorised task execution surfaces. +- Local/worktree user-shell terminals and passthrough-agent PTYs receive the same managed Git + broker, indexed configuration, and shimmed `PATH` as their execution's agent subprocess. +- Executor-inheritance mode and unauthorised terminal access do not gain managed broker values. + +## Verification + +```bash +cd apps/backend && rtk go test ./internal/agent/runtime/lifecycle ./internal/gateway/websocket -run 'Test(ExecutionEnvironment|StartUserShellProcess|Passthrough).*ManagedGit' -count=1 +``` + +## Files likely touched + +- `apps/backend/internal/agent/runtime/lifecycle/types.go` +- `apps/backend/internal/agent/runtime/lifecycle/manager_launch.go` +- `apps/backend/internal/agent/runtime/lifecycle/manager_execution.go` +- `apps/backend/internal/agent/runtime/lifecycle/manager_passthrough.go` +- `apps/backend/internal/agent/runtime/lifecycle/manager_launch_test.go` +- `apps/backend/internal/agent/runtime/lifecycle/manager_passthrough_test.go` +- `apps/backend/internal/gateway/websocket/terminal_handler.go` +- `apps/backend/internal/gateway/websocket/terminal_handler_test.go` + +## Dependencies + +None. + +## Parallelism + +Sequential. This task establishes the runtime-only environment ownership used by later process +launch paths. + +## Inputs + +- Spec: `docs/specs/integrations/github-authentication.md` — What, Failure Modes, Scenarios. +- Plan: Runtime environment section. +- Existing patterns: `buildEnvForExecution`, `ExecutorProfileEnvForSession`, and + `StartUserShell`. + +## Output contract + +Report the environment ownership boundary, files changed, focused test output, and any finding +that requires plan/spec revision. Focused lifecycle and gateway tests pass, including the +race-enabled regression set; agentctl process tests are covered by Task 02. diff --git a/docs/plans/task-terminal-git-environment/task-02-agentctl-shell-process-environment.md b/docs/plans/task-terminal-git-environment/task-02-agentctl-shell-process-environment.md new file mode 100644 index 0000000000..5f25e00d4f --- /dev/null +++ b/docs/plans/task-terminal-git-environment/task-02-agentctl-shell-process-environment.md @@ -0,0 +1,55 @@ +--- +id: "02-agentctl-shell-process-environment" +title: "Agentctl shell and process environment" +status: done +wave: 2 +depends_on: ["01-runtime-terminal-environment"] +plan: "plan.md" +spec: "../../specs/integrations/github-authentication.md" +--- + +# Task 02: Agentctl Shell and Process Environment + +## Acceptance + +- The legacy agentctl shell, per-terminal agentctl shells, and task-scoped command processes + inherit the instance `AgentEnv` before explicit call-site environment overrides. +- Managed Git configuration and shim-first `PATH` remain intact as one effective environment; + explicit per-call values retain their established precedence. +- Process startup reports an error before execution if managed environment preparation fails and + never falls back to an ambient Git helper or `gh` login. + +## Verification + +```bash +cd apps/backend && rtk go test ./internal/agentctl/server/process -run 'Test(StartShell|StartTerminalShell|StartProcess).*AgentEnvironment' -count=1 +``` + +## Files likely touched + +- `apps/backend/internal/agentctl/server/process/manager.go` +- `apps/backend/internal/agentctl/server/process/runner.go` +- `apps/backend/internal/agentctl/server/process/interactive_shells.go` +- `apps/backend/internal/agentctl/server/process/runner_shells_test.go` +- `apps/backend/internal/agentctl/server/process/runner_test.go` + +## Dependencies + +Task 01 establishes the canonical runtime environment boundary and its credential-safety tests. + +## Parallelism + +Sequential. + +## Inputs + +- Spec: `docs/specs/integrations/github-authentication.md` — What, Failure Modes, Scenarios. +- Plan: Agentctl shell and command launch section. +- Existing patterns: `mergeAgentEnvIntoShellConfig`, `buildShellEnv`, `mergeEnvWithStrip`, and + `gitEnvironment`. + +## Output contract + +Report precedence behavior, files changed, focused test output, and any need to revise the +security contract. Explicit request values retain precedence over inherited instance values; +focused and package-level process tests pass. diff --git a/docs/plans/task-terminal-git-environment/task-03-document-terminal-git-routing.md b/docs/plans/task-terminal-git-environment/task-03-document-terminal-git-routing.md new file mode 100644 index 0000000000..5d66ae0e4c --- /dev/null +++ b/docs/plans/task-terminal-git-environment/task-03-document-terminal-git-routing.md @@ -0,0 +1,53 @@ +--- +id: "03-document-terminal-git-routing" +title: "Document terminal Git routing" +status: done +wave: 3 +depends_on: ["01-runtime-terminal-environment", "02-agentctl-shell-process-environment"] +plan: "plan.md" +spec: "../../specs/integrations/github-authentication.md" +--- + +# Task 03: Document and Verify Terminal Git Routing + +## Acceptance + +- Public executor and terminal documentation state that a newly opened terminal in a managed task + uses the task's managed Git/`gh` routing, while executor-inheritance mode does not. +- Documentation states that a live terminal keeps its initial environment and must be reopened + after a new launch/resume or policy change. +- Public-doc validation passes without exposing broker variables, helper paths, or credentials. + +## Verification + +```bash +node --test scripts/validate-public-docs.test.mjs +node scripts/validate-public-docs.mjs +``` + +## Files likely touched + +- `docs/public/executors.md` +- `docs/public/developer-tools.md` +- `docs/specs/integrations/github-authentication.md` +- `docs/plans/task-terminal-git-environment/plan.md` + +## Dependencies + +Tasks 01 and 02 must define and verify the final observable behavior. + +## Parallelism + +Sequential. + +## Inputs + +- Spec: `docs/specs/integrations/github-authentication.md` — What and Scenarios. +- Plan: Public documentation section. +- Existing docs: `docs/public/executors.md` and `docs/public/developer-tools.md`. + +## Output contract + +Report documentation changes and validation output. Public executor and developer-tool guidance +now covers managed terminal routing and reopening live PTYs; both public-doc validation commands +pass. diff --git a/docs/public/developer-tools.md b/docs/public/developer-tools.md index f669060834..c8d3d19e5b 100644 --- a/docs/public/developer-tools.md +++ b/docs/public/developer-tools.md @@ -179,6 +179,11 @@ Language-server subprocesses run on the Kandev backend host with the task worksp The task terminal is a PTY in the task environment. Local tasks run through the local environment; Docker, SSH, and other remote executors route the terminal to their configured runtime. +When the workspace uses managed GitHub credentials, a new terminal receives the same task-scoped +Git and GitHub CLI routing as its agent. **Inherit executor Git credentials** leaves that routing +to the selected executor instead. The environment is fixed when the PTY starts, so reopen a +terminal after a session resume or credential-policy change. + On desktop, select **+ > Terminals > New Terminal**. Parked terminal sessions can appear in the same menu for reopening. The tab context menu offers **Rename** and **Terminate**, not a separate Close action. Selecting the tab's **X** deletes the terminal and asks for confirmation when it is busy. Removing the panel through layout management can instead park it and keep its PTY alive; terminating a live terminal or deleting a parked entry destroys it. On mobile, select **Terminal** in task navigation and use the terminal picker to create another. Do not confuse a user terminal with a CLI-passthrough agent tab: both use PTYs, but only the agent tab is the agent's native interface. `Cmd/Ctrl+J` toggles the bottom terminal area. diff --git a/docs/public/executors.md b/docs/public/executors.md index 9f9deaaa9d..4219206752 100644 --- a/docs/public/executors.md +++ b/docs/public/executors.md @@ -99,6 +99,12 @@ the executor before clone or agent startup and require its `204 No Content` read Network failures, redirects, proxy routing errors, and broker server errors stop launch instead of falling back to another GitHub credential. +A newly opened task terminal and a CLI-passthrough agent tab use the same task-scoped Git and +GitHub CLI routing as the agent that owns the execution. This applies only after the task +ownership check and does not change **Inherit executor Git credentials** mode. A terminal that is +already open keeps the environment from its launch; reopen it after a new session launch, resume, +or a Git credential-policy change. + ## Worktree Worktree creates a dedicated host Git worktree and runs the standalone `agentctl` service against it. It separates branches and files between tasks, but the process still has the Kandev user's host permissions, network access, and readable credentials. diff --git a/docs/specs/integrations/github-authentication.md b/docs/specs/integrations/github-authentication.md index 920c9f3938..485ad98845 100644 --- a/docs/specs/integrations/github-authentication.md +++ b/docs/specs/integrations/github-authentication.md @@ -60,6 +60,11 @@ automation under different GitHub Apps without operating separate Kandev deploym only in memory. PAT/CLI tokens retain their provider-granted scope once delivered to a trusted agent subprocess. GitHub HTTPS and the broker-aware `gh` shim fail closed rather than consulting another ambient helper after a managed-helper failure. +- Under managed routing, every authorized task execution surface receives the same current + task-scoped Git environment: the agent subprocess, terminal shells, passthrough-agent PTYs, and + task-scoped command processes. This includes the broker contract, managed indexed Git + configuration, and the `agentctl`/`gh` shim-first `PATH`; it does not grant access to a browser + client, an unrelated host shell, or another workspace's task environment. - Kandev composes the indexed `GIT_CONFIG_COUNT` / `GIT_CONFIG_KEY_` / `GIT_CONFIG_VALUE_` protocol across host, executor, profile, task, and agentctl boundaries. Unrelated entries such as hooks, notes, safe-directory, and URL-rewrite settings survive in their @@ -360,6 +365,9 @@ post-signature processing failures produce `failing`; a later valid successful d prompts, and activates Kandev's `agentctl`/`gh` tool directory only for broker-enabled task instances. It does not claim to prevent a host-authority agent from manually switching a remote to SSH or invoking another credential-bearing tool. +- The effective task Git environment is runtime-only. It is copied only after the existing + task/session or task-environment ownership check, is never persisted in task metadata or terminal + records, and is never written to logs, errors, browser payloads, or process arguments. - Indexed Git configuration is validated and composed as a single ordered block at environment merge boundaries. Kandev never replaces a complete inherited block merely by assigning its own `GIT_CONFIG_COUNT`; managed helper reset semantics are expressed as later Git config entries. @@ -379,6 +387,9 @@ post-signature processing failures produce `failing`; a later valid successful d account; selecting another stored login fails with guidance to activate it or upgrade the CLI. - If the managed `agentctl` helper, broker, or `gh` shim is unavailable, the command fails with a managed-credential error and does not fall through to another HTTPS helper or interactive prompt. +- If an authorized task terminal, passthrough PTY, or task-scoped command cannot receive its + effective managed Git environment, Kandev fails that process start before it runs the requested + command. It does not silently fall back to an ambient credential helper or host `gh` login. - If any environment source supplies a malformed or unreasonably large indexed Git configuration, task environment preparation fails with a sanitized configuration error rather than silently truncating, partially merging, or executing a different block. @@ -457,6 +468,12 @@ registration and never creates a global default. - **GIVEN** a named CLI workspace in managed mode, **WHEN** a task launches, **THEN** Kandev resolves the selected host/login, makes both managed `git` and `gh` available in standalone and remote runtimes, and does not depend on the host's currently active CLI account. +- **GIVEN** an authorized user opens a terminal, uses a passthrough-agent PTY, or starts a + task-scoped command in a managed task, **WHEN** it accesses an attached GitHub repository, + **THEN** it receives the same task-scoped broker helper and `gh` shim environment as the agent + subprocess. +- **GIVEN** an unauthorised user or a terminal for another task environment, **WHEN** it attempts to + open a terminal or start a process, **THEN** it cannot receive the managed task Git environment. - **GIVEN** a workspace selects executor inheritance, **WHEN** a Local/Worktree or remote task launches, **THEN** Kandev injects no broker helper/shim and the task uses host-visible or executor-configured credentials respectively. @@ -505,7 +522,8 @@ registration and never creates a global default. token, or live installation token in logs, API snapshots, redirects, process arguments, or executor environments. - Standalone, container, and remote task tests prove the managed helper is discoverable only for - broker-enabled instances, while executor inheritance receives no Kandev GitHub helper/shim. + broker-enabled instances and their authorized task terminals/processes, while executor + inheritance receives no Kandev GitHub helper/shim. - A real Git subprocess test proves that host/executor indexed hooks and notes config survive managed credential injection, and focused tests prove ordered composition and overlap handling across standalone, container, and remote launch shapes. @@ -530,7 +548,8 @@ See [the original authentication implementation plan](../../plans/github-authent and the [task Git credential policy follow-up plan](../../plans/task-git-credential-policy/plan.md), plus the -[executor clone transport repair plan](../../plans/github-executor-clone-transport/plan.md). +[executor clone transport repair plan](../../plans/github-executor-clone-transport/plan.md), and +the [managed task terminal environment plan](../../plans/task-terminal-git-environment/plan.md). ## Decision