diff --git a/apps/backend/internal/agent/agents/agent.go b/apps/backend/internal/agent/agents/agent.go index fe895df0fd..9e8e7910c0 100644 --- a/apps/backend/internal/agent/agents/agent.go +++ b/apps/backend/internal/agent/agents/agent.go @@ -152,7 +152,7 @@ type CommandOptions struct { AgentType string // for --agent flag (e.g. "task" for subagent) // CLIFlagTokens are user-configured CLI flag argv tokens derived from // AgentProfile.CLIFlags (only Enabled entries, shell-tokenised). Appended - // verbatim to the built command by every agent's BuildCommand. + // verbatim by lifecycle.CommandBuilder after the agent builds its command. CLIFlagTokens []string // CommandPrefixTokens are user-configured launcher tokens derived from // AgentProfile.CommandPrefix (shell-tokenised). Prepended to the built @@ -184,8 +184,9 @@ type PassthroughOptions struct { // mcp_servers.…" overrides). Appended to the built command. MCPArgs []string // CLIFlagTokens are user-configured CLI flag argv tokens derived from - // AgentProfile.CLIFlags (only Enabled entries, shell-tokenised). Appended - // verbatim to the built passthrough command, mirroring CommandOptions. + // AgentProfile.CLIFlags (only Enabled entries, shell-tokenised). Unlike ACP + // commands, which lifecycle.CommandBuilder appends centrally, passthrough + // agents opt in by appending these tokens in BuildPassthroughCommand. CLIFlagTokens []string } diff --git a/apps/backend/internal/agent/runtime/agentctl/agent_test.go b/apps/backend/internal/agent/runtime/agentctl/agent_test.go index 4e89792ef2..0549876880 100644 --- a/apps/backend/internal/agent/runtime/agentctl/agent_test.go +++ b/apps/backend/internal/agent/runtime/agentctl/agent_test.go @@ -3,6 +3,7 @@ package client import ( "context" "encoding/json" + "io" "net/http" "net/http/httptest" "strings" @@ -23,6 +24,46 @@ func newTestLogger() *logger.Logger { return log } +func TestConfigureAgent_SerializesStructuredArgsAndOmitsAbsentFallback(t *testing.T) { + var bodies [][]byte + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + if err != nil { + t.Fatalf("read configure request: %v", err) + } + bodies = append(bodies, body) + _, _ = w.Write([]byte(`{"success":true}`)) + })) + defer server.Close() + + client := &Client{baseURL: server.URL, httpClient: server.Client()} + args := []string{"runner", "two words", "", `C:\tools\agent.exe`} + continueArgs := []string{"runner", "continue", "thread id"} + if err := client.ConfigureAgent(context.Background(), "runner two words C:\\tools\\agent.exe", args, nil, "", "runner continue thread id", continueArgs); err != nil { + t.Fatalf("configure structured args: %v", err) + } + if err := client.ConfigureAgent(context.Background(), "legacy --flag", nil, nil, "", "", nil); err != nil { + t.Fatalf("configure legacy fallback: %v", err) + } + + var structured map[string]json.RawMessage + if err := json.Unmarshal(bodies[0], &structured); err != nil { + t.Fatalf("decode structured request: %v", err) + } + for _, field := range []string{"agent_args", "continue_args"} { + if _, ok := structured[field]; !ok { + t.Fatalf("structured request omitted %s: %s", field, bodies[0]) + } + } + var legacy map[string]json.RawMessage + if err := json.Unmarshal(bodies[1], &legacy); err != nil { + t.Fatalf("decode legacy request: %v", err) + } + if _, ok := legacy["agent_args"]; ok { + t.Fatalf("legacy fallback unexpectedly sent agent_args: %s", bodies[1]) + } +} + // wsTestServer creates a test WebSocket server that echoes back response messages. // The handler receives each request and should return a response message. type wsTestServer struct { diff --git a/apps/backend/internal/agent/runtime/agentctl/client.go b/apps/backend/internal/agent/runtime/agentctl/client.go index 7d3b04aa76..4893ef3b54 100644 --- a/apps/backend/internal/agent/runtime/agentctl/client.go +++ b/apps/backend/internal/agent/runtime/agentctl/client.go @@ -258,13 +258,15 @@ func (c *Client) GetStatus(ctx context.Context) (*StatusResponse, error) { // ConfigureAgent configures the agent command and optional approval policy. Must be called before Start(). // continueCommand is optional — when set, the adapter uses it for one-shot follow-up prompts. -func (c *Client) ConfigureAgent(ctx context.Context, command string, env map[string]string, approvalPolicy, continueCommand string) error { +func (c *Client) ConfigureAgent(ctx context.Context, command string, agentArgs []string, env map[string]string, approvalPolicy, continueCommand string, continueArgs []string) error { ctx, span := tracing.TraceHTTPRequest(ctx, "POST", "/api/v1/agent/configure", c.executionID) defer span.End() payload := struct { Command string `json:"command"` + AgentArgs *[]string `json:"agent_args,omitempty"` ContinueCommand string `json:"continue_command,omitempty"` + ContinueArgs *[]string `json:"continue_args,omitempty"` Env map[string]string `json:"env,omitempty"` ApprovalPolicy string `json:"approval_policy,omitempty"` }{ @@ -273,6 +275,12 @@ func (c *Client) ConfigureAgent(ctx context.Context, command string, env map[str Env: env, ApprovalPolicy: approvalPolicy, } + if agentArgs != nil { + payload.AgentArgs = &agentArgs + } + if continueArgs != nil { + payload.ContinueArgs = &continueArgs + } body, err := json.Marshal(payload) if err != nil { diff --git a/apps/backend/internal/agent/runtime/lifecycle/command.go b/apps/backend/internal/agent/runtime/lifecycle/command.go index 9b497cdfd9..be7e6d1558 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/command.go +++ b/apps/backend/internal/agent/runtime/lifecycle/command.go @@ -4,7 +4,6 @@ package lifecycle import ( "os" - "strings" "github.com/kandev/kandev/internal/agent/agents" ) @@ -44,20 +43,16 @@ func prependCommandPrefix(cmd agents.Command, prefixTokens []string) agents.Comm return agents.NewCommand(args...) } -// BuildCommandString builds a command as a single string (for standalone mode) -func (cb *CommandBuilder) BuildCommandString(ag agents.Agent, opts agents.CommandOptions) string { - cmd := cb.BuildCommand(ag, opts) - return strings.Join(cmd.Args(), " ") +// BuildCommandArgs builds the executable argv for an agent launch. +func (cb *CommandBuilder) BuildCommandArgs(ag agents.Agent, opts agents.CommandOptions) []string { + return cb.BuildCommand(ag, opts).Args() } -// BuildContinueCommandString builds the continue command with the same model/permission -// flags as the initial command. Used by one-shot agents (Amp) where each follow-up prompt -// needs a separate "threads continue" command. Returns empty string if the agent has no -// ContinueSessionCmd configured. -func (cb *CommandBuilder) BuildContinueCommandString(ag agents.Agent, opts agents.CommandOptions) string { +// BuildContinueCommandArgs builds argv for a one-shot follow-up command. +func (cb *CommandBuilder) BuildContinueCommandArgs(ag agents.Agent, opts agents.CommandOptions) []string { sessionCfg := ag.Runtime().SessionConfig if sessionCfg.ContinueSessionCmd.IsEmpty() { - return "" + return nil } // Start from the continue command base and apply the same flags as BuildCommand @@ -68,7 +63,7 @@ func (cb *CommandBuilder) BuildContinueCommandString(ag agents.Agent, opts agent Build() cmd = prependCommandPrefix(cmd, opts.CommandPrefixTokens) - return strings.Join(cmd.Args(), " ") + return cmd.Args() } // ExpandSessionDir resolves the host-side directory that should be bind- diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_docker_test.go b/apps/backend/internal/agent/runtime/lifecycle/executor_docker_test.go index 94eac7504e..69e2b04e54 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_docker_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_docker_test.go @@ -3,11 +3,15 @@ package lifecycle import ( "context" "fmt" + "net/http" + "net/http/httptest" "reflect" "strings" "sync/atomic" "testing" + "github.com/stretchr/testify/require" + "github.com/kandev/kandev/internal/agent/agents" "github.com/kandev/kandev/internal/agent/docker" "github.com/kandev/kandev/internal/agent/executor" @@ -164,6 +168,38 @@ func TestDockerStopInstanceStopsContainerOnStaleCleanup(t *testing.T) { } } +func TestRollbackLaunchExecutionForceKillsUnregisteredDockerContainer(t *testing.T) { + requests := make(chan string, 1) + dockerDaemon := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodHead && r.URL.Path == "/_ping" { + w.WriteHeader(http.StatusOK) + return + } + requests <- r.Method + " " + r.URL.Path + w.WriteHeader(http.StatusNoContent) + })) + t.Cleanup(dockerDaemon.Close) + + dockerExec := NewDockerExecutor(config.DockerConfig{ + Host: "tcp://" + strings.TrimPrefix(dockerDaemon.URL, "http://"), + }, "", newTestDockerLogger()) + t.Cleanup(func() { require.NoError(t, dockerExec.Close()) }) + + mgr := newTestManager(t) + mgr.rollbackLaunchExecution(context.Background(), dockerExec, &ExecutorInstance{ + InstanceID: "failed-instance", + ContainerID: "failed-container", + }, &AgentExecution{ID: "failed-execution", SessionID: "failed-session"}, "command construction failed") + + select { + case got := <-requests: + require.Equal(t, http.MethodPost, strings.Fields(got)[0]) + require.True(t, strings.HasSuffix(got, "/containers/failed-container/kill"), got) + default: + t.Fatal("rollback must force Docker cleanup for an unregistered failed instance") + } +} + func TestDockerCleanupContextIgnoresCanceledParentAfterAgentStopFailed(t *testing.T) { parent, parentCancel := context.WithCancel(context.Background()) parentCancel() diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_execution.go b/apps/backend/internal/agent/runtime/lifecycle/manager_execution.go index 480db0b591..99dee438b2 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_execution.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_execution.go @@ -619,7 +619,7 @@ func (m *Manager) rollbackRacedExecution(ctx context.Context, rt ExecutorBackend zap.String("execution_id", execution.ID), zap.String("session_id", execution.SessionID)) if rt != nil && runtimeInstance != nil { - if stopErr := rt.StopInstance(ctx, runtimeInstance, false); stopErr != nil { + if stopErr := rt.StopInstance(ctx, runtimeInstance, true); stopErr != nil { m.logger.Warn("failed to stop raced runtime instance during rollback", zap.String("execution_id", execution.ID), zap.Error(stopErr)) diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_execution_test.go b/apps/backend/internal/agent/runtime/lifecycle/manager_execution_test.go index e8bccde3b0..e67f2e299d 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_execution_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_execution_test.go @@ -892,6 +892,7 @@ type createInstanceExecutor struct { client *agentctl.Client createCount atomic.Int32 stopCount atomic.Int32 + forceStopped atomic.Bool lastRequest *ExecutorCreateRequest authToken string nonce string @@ -950,6 +951,7 @@ func (e *createInstanceExecutor) CreateInstance(ctx context.Context, req *Execut func (e *createInstanceExecutor) StopInstance(ctx context.Context, instance *ExecutorInstance, force bool) error { e.stopCount.Add(1) + e.forceStopped.Store(force) return nil } diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go index 287119e0f9..ed551628ff 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go @@ -625,20 +625,9 @@ func (m *Manager) RestartAgentProcess(ctx context.Context, executionID string) e return fmt.Errorf("execution %q has no agentctl client", executionID) } - m.logger.Info("restarting agent process for context reset", - zap.String("execution_id", executionID), - zap.String("task_id", execution.TaskID), - zap.String("session_id", execution.SessionID)) - - // Capture the active session mode before the restart so it can be re-applied to - // the fresh ACP session (issue #1183). Capture now, before the streams reconnect - // and the restarted agent reports its default mode (which would overwrite the cache). - prevMode := execution.GetModeState() - - // Resolve agent config early — needed for both command rebuild and ACP session init - agentConfig, err := m.getAgentConfigForExecution(execution) + preparation, err := m.prepareAgentRestart(ctx, execution) if err != nil { - return fmt.Errorf("failed to get agent config for restart: %w", err) + return err } // 1. Close WebSocket streams (updates + workspace). Use per-stream Close @@ -657,36 +646,8 @@ func (m *Manager) RestartAgentProcess(ctx context.Context, executionID string) e // Continue — the process may already be stopped } - // 3. Rebuild agent command without resume flags and reset execution state. - // Fail closed: if the launcher prefix can't be resolved, abort the restart - // rather than relaunch a sandboxed agent unwrapped. - freshCmd, freshContinueCmd, err := m.buildFreshAgentCommand(ctx, execution, agentConfig) - if err != nil { - return fmt.Errorf("failed to rebuild agent command for restart: %w", err) - } - _ = m.executionStore.WithLock(executionID, func(exec *AgentExecution) { - exec.ACPSessionID = "" - exec.Status = v1.AgentStatusStarting - exec.ErrorMessage = "" - exec.needsResumeContext = false - exec.resumeContextInjected = false - exec.sessionInitialized = false - exec.AgentCommand = freshCmd - exec.ContinueCommand = freshContinueCmd - - exec.messageMu.Lock() - exec.messageBuffer.Reset() - exec.thinkingBuffer.Reset() - exec.currentMessageID = "" - exec.currentThinkingID = "" - exec.messageMu.Unlock() - - // Drain any stale prompt completion signal - select { - case <-exec.promptDoneCh: - default: - } - }) + // 3. Reset execution state after the replacement command has been validated. + m.resetAgentRestartState(executionID, preparation.commands) // 4. Wait for agentctl to be ready (it should still be running) if err := execution.agentctl.WaitForReady(ctx, 30*time.Second); err != nil { @@ -708,18 +669,18 @@ func (m *Manager) RestartAgentProcess(ctx context.Context, executionID string) e zap.Error(err)) } - mcpServers, err := m.resolveMcpServers(ctx, execution, agentConfig) + mcpServers, err := m.resolveMcpServers(ctx, execution, preparation.agentConfig) if err != nil { return fmt.Errorf("failed to resolve MCP config for restart: %w", err) } - if err := m.initializeACPSessionForRestart(ctx, execution, agentConfig, mcpServers); err != nil { + if err := m.initializeACPSessionForRestart(ctx, execution, preparation.agentConfig, mcpServers); err != nil { m.updateExecutionError(executionID, "failed to initialize ACP session after restart: "+err.Error()) return fmt.Errorf("failed to initialize ACP session after restart: %w", err) } // Restore the user's session permission mode onto the fresh ACP session. - m.reapplySessionModeAfterReset(ctx, execution, execution.ACPSessionID, prevMode) + m.reapplySessionModeAfterReset(ctx, execution, execution.ACPSessionID, preparation.previousMode) m.logger.Info("agent process restarted with fresh context", zap.String("execution_id", executionID), @@ -730,6 +691,59 @@ func (m *Manager) RestartAgentProcess(ctx context.Context, executionID string) e return nil } +type agentRestartPreparation struct { + agentConfig agents.Agent + commands agentCommands + previousMode *CachedModeState +} + +// prepareAgentRestart builds and validates the replacement before touching the current process. +func (m *Manager) prepareAgentRestart(ctx context.Context, execution *AgentExecution) (agentRestartPreparation, error) { + m.logger.Info("restarting agent process for context reset", + zap.String("execution_id", execution.ID), + zap.String("task_id", execution.TaskID), + zap.String("session_id", execution.SessionID)) + + agentConfig, err := m.getAgentConfigForExecution(execution) + if err != nil { + return agentRestartPreparation{}, fmt.Errorf("failed to get agent config for restart: %w", err) + } + commands, err := m.buildFreshAgentCommand(ctx, execution, agentConfig) + if err != nil { + return agentRestartPreparation{}, fmt.Errorf("failed to rebuild agent command for restart: %w", err) + } + return agentRestartPreparation{ + agentConfig: agentConfig, + commands: commands, + previousMode: execution.GetModeState(), + }, nil +} + +func (m *Manager) resetAgentRestartState(executionID string, commands agentCommands) { + _ = m.executionStore.WithLock(executionID, func(exec *AgentExecution) { + exec.ACPSessionID = "" + exec.Status = v1.AgentStatusStarting + exec.ErrorMessage = "" + exec.needsResumeContext = false + exec.resumeContextInjected = false + exec.sessionInitialized = false + exec.AgentCommand = commands.initial + exec.ContinueCommand = commands.continue_ + exec.AgentArgs = commands.args + exec.ContinueArgs = commands.continueArgs + exec.messageMu.Lock() + exec.messageBuffer.Reset() + exec.thinkingBuffer.Reset() + exec.currentMessageID = "" + exec.currentThinkingID = "" + exec.messageMu.Unlock() + select { + case <-exec.promptDoneCh: + default: + } + }) +} + // initializeACPSessionForRestart connects streams and creates a new ACP session without // sending an initial prompt. The caller (workflow processOnEnter) handles prompting separately. func (m *Manager) initializeACPSessionForRestart( @@ -1495,13 +1509,13 @@ func (m *Manager) stopPassthroughProcess(ctx context.Context, executionID string } // buildFreshAgentCommand rebuilds the agent command without resume flags by going through -// the standard BuildCommandString pipeline with an empty SessionID. This works for all +// the standard command-building pipeline with an empty SessionID. This works for all // agent types because each agent's BuildCommand respects SessionID="" to skip resume flags. // // It fails closed: if the profile cannot be resolved, or a configured // command_prefix cannot be tokenised, it returns an error so a context reset -// never relaunches a sandboxed agent without its wrapper. -func (m *Manager) buildFreshAgentCommand(ctx context.Context, execution *AgentExecution, agentConfig agents.Agent) (initial, continueCmd string, err error) { +// never relaunches a configured agent without its wrapper. +func (m *Manager) buildFreshAgentCommand(ctx context.Context, execution *AgentExecution, agentConfig agents.Agent) (agentCommands, error) { var profileInfo *AgentProfileInfo if execution.AgentProfileID != "" && m.profileResolver != nil { pi, resolveErr := m.profileResolver.ResolveProfile(ctx, execution.AgentProfileID) @@ -1509,7 +1523,7 @@ func (m *Manager) buildFreshAgentCommand(ctx context.Context, execution *AgentEx // A profile was expected but could not be resolved. We cannot tell // whether it carried a sandbox prefix, so refuse to relaunch rather // than risk running unwrapped. - return "", "", fmt.Errorf("resolve profile %s for restart: %w", execution.AgentProfileID, resolveErr) + return agentCommands{}, fmt.Errorf("resolve profile %s for restart: %w", execution.AgentProfileID, resolveErr) } profileInfo = pi } @@ -1530,10 +1544,10 @@ func (m *Manager) buildFreshAgentCommand(ctx context.Context, execution *AgentEx // Preserve the profile's cli_flags and command_prefix across restarts. A // context reset that dropped the launcher prefix would relaunch a - // sandboxed agent unwrapped — the exact protection the prefix exists for. + // configured agent unwrapped. cliFlagTokens, commandPrefixTokens, err := m.resolveProfileLaunchTokens(profileInfo) if err != nil { - return "", "", err + return agentCommands{}, err } opts := agents.CommandOptions{ @@ -1548,6 +1562,10 @@ func (m *Manager) buildFreshAgentCommand(ctx context.Context, execution *AgentEx // an absolute host path. Runtime: execution.RuntimeName, } - return m.commandBuilder.BuildCommandString(agentConfig, opts), - m.commandBuilder.BuildContinueCommandString(agentConfig, opts), nil + args := m.commandBuilder.BuildCommandArgs(agentConfig, opts) + continueArgs := m.commandBuilder.BuildContinueCommandArgs(agentConfig, opts) + if err := validateBuiltAgentCommands(args, continueArgs); err != nil { + return agentCommands{}, err + } + return newAgentCommands(args, continueArgs), nil } diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction_test.go b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction_test.go index 6449c31d39..ad8698646b 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction_test.go @@ -3,6 +3,7 @@ package lifecycle import ( "context" "encoding/json" + "errors" "fmt" "net/http" "net/http/httptest" @@ -14,6 +15,7 @@ import ( "github.com/gorilla/websocket" "github.com/stretchr/testify/require" + "github.com/kandev/kandev/internal/agent/agents" "github.com/kandev/kandev/internal/agent/executor" agentctl "github.com/kandev/kandev/internal/agent/runtime/agentctl" "github.com/kandev/kandev/internal/agentctl/server/process" @@ -24,6 +26,15 @@ import ( ws "github.com/kandev/kandev/pkg/websocket" ) +type restartProfileResolver struct { + profile *AgentProfileInfo + err error +} + +func (r *restartProfileResolver) ResolveProfile(_ context.Context, _ string) (*AgentProfileInfo, error) { + return r.profile, r.err +} + type restartMockAgentctlServer struct { server *httptest.Server @@ -287,6 +298,159 @@ func TestManager_RestartAgentProcess_Success(t *testing.T) { } } +func TestManager_RestartAgentProcess_InvalidReplacementLeavesCurrentProcessUntouched(t *testing.T) { + tests := []struct { + name string + resolver ProfileResolver + }{ + { + name: "unresolvable profile", + resolver: &restartProfileResolver{err: errors.New("profile unavailable")}, + }, + { + name: "malformed persisted command prefix", + resolver: &restartProfileResolver{profile: &AgentProfileInfo{ + ProfileID: "profile-1", + AgentName: "auggie", + CommandPrefix: `greywall "unterminated`, + }}, + }, + { + name: "persisted prefix has an empty quoted executable", + resolver: &restartProfileResolver{profile: &AgentProfileInfo{ + ProfileID: "profile-1", + AgentName: "auggie", + CommandPrefix: `'' --arg`, + }}, + }, + { + name: "persisted prefix has a whitespace executable", + resolver: &restartProfileResolver{profile: &AgentProfileInfo{ + ProfileID: "profile-1", + AgentName: "auggie", + CommandPrefix: `" " --arg`, + }}, + }, + { + name: "persisted prefix starts with a flag", + resolver: &restartProfileResolver{profile: &AgentProfileInfo{ + ProfileID: "profile-1", + AgentName: "auggie", + CommandPrefix: `--not-a-launcher`, + }}, + }, + { + name: "persisted prefix starts with a space-prefixed flag", + resolver: &restartProfileResolver{profile: &AgentProfileInfo{ + ProfileID: "profile-1", + AgentName: "auggie", + CommandPrefix: `" --not-a-launcher"`, + }}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mgr := newTestManager(t) + mgr.profileResolver = tt.resolver + mock := newRestartMockAgentctlServer(t, false, false) + client := createTestClient(t, mock.server.URL) + t.Cleanup(client.Close) + + execution := &AgentExecution{ + ID: "exec-invalid-restart", + TaskID: "task-1", + SessionID: "session-1", + AgentProfileID: "profile-1", + ACPSessionID: "old-acp-session", + AgentCommand: "auggie --resume old-acp-session", + ContinueCommand: "auggie continue old-acp-session", + AgentArgs: []string{"auggie", "--resume", "old-acp-session"}, + ContinueArgs: []string{"auggie", "continue", "old-acp-session"}, + Status: v1.AgentStatusRunning, + ErrorMessage: "existing error", + agentctl: client, + promptDoneCh: make(chan PromptCompletionSignal, 1), + } + require.NoError(t, mgr.executionStore.Add(execution)) + + err := mgr.RestartAgentProcess(context.Background(), execution.ID) + require.Error(t, err) + require.Empty(t, mock.getHTTPActions(), "invalid replacement must not stop, configure, or start the current process") + + current, found := mgr.executionStore.Get(execution.ID) + require.True(t, found) + require.Equal(t, v1.AgentStatusRunning, current.Status) + require.Equal(t, "old-acp-session", current.ACPSessionID) + require.Equal(t, "auggie --resume old-acp-session", current.AgentCommand) + require.Equal(t, []string{"auggie", "--resume", "old-acp-session"}, current.AgentArgs) + require.Equal(t, "existing error", current.ErrorMessage) + }) + } +} + +func TestManager_RestartAgentProcess_InvalidBuiltReplacementLeavesCurrentProcessUntouched(t *testing.T) { + tests := []struct { + name string + agent *invalidCommandTestAgent + }{ + { + name: "initial executable is a flag", + agent: &invalidCommandTestAgent{command: agents.NewCommand("--not-an-executable")}, + }, + { + name: "continue executable is whitespace", + agent: &invalidCommandTestAgent{ + command: agents.NewCommand("agent"), + testAgent: testAgent{runtimeConfig: &agents.RuntimeConfig{SessionConfig: agents.SessionConfig{ + ContinueSessionCmd: agents.NewCommand(" "), + }}}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mgr := newTestManager(t) + tt.agent.id = "invalid-restart" + require.NoError(t, mgr.registry.Register(tt.agent)) + mgr.profileResolver = &restartProfileResolver{profile: &AgentProfileInfo{ + ProfileID: "profile-1", + AgentName: tt.agent.ID(), + }} + mock := newRestartMockAgentctlServer(t, false, false) + client := createTestClient(t, mock.server.URL) + t.Cleanup(client.Close) + + execution := &AgentExecution{ + ID: "exec-invalid-built-restart", + TaskID: "task-1", + SessionID: "session-1", + AgentProfileID: "profile-1", + ACPSessionID: "old-acp-session", + AgentCommand: "agent --resume old-acp-session", + AgentArgs: []string{"agent", "--resume", "old-acp-session"}, + Status: v1.AgentStatusRunning, + ErrorMessage: "existing error", + agentctl: client, + promptDoneCh: make(chan PromptCompletionSignal, 1), + } + require.NoError(t, mgr.executionStore.Add(execution)) + + err := mgr.RestartAgentProcess(context.Background(), execution.ID) + require.Error(t, err) + require.Empty(t, mock.getHTTPActions()) + + current, found := mgr.executionStore.Get(execution.ID) + require.True(t, found) + require.Equal(t, v1.AgentStatusRunning, current.Status) + require.Equal(t, "old-acp-session", current.ACPSessionID) + require.Equal(t, []string{"agent", "--resume", "old-acp-session"}, current.AgentArgs) + require.Equal(t, "existing error", current.ErrorMessage) + }) + } +} + func TestManager_RestartAgentProcess_StopErrorIsNonFatal(t *testing.T) { mgr := newTestManager(t) mock := newRestartMockAgentctlServer(t, true, false) diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go b/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go index ac8ae10506..9be8dd68a8 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_launch.go @@ -204,21 +204,32 @@ func baseBranchMetadataKey(spec RepoLaunchSpec) string { return repoName + "-" + branchSlug } -// agentCommands holds the initial and continue command strings for an agent execution. +// agentCommands holds both the display strings and structured argv for an agent execution. type agentCommands struct { - initial string - continue_ string // continue command for one-shot agents (empty if not applicable) + initial string + continue_ string // continue command for one-shot agents (empty if not applicable) + args []string + continueArgs []string +} + +func newAgentCommands(args, continueArgs []string) agentCommands { + return agentCommands{ + initial: strings.Join(args, " "), + continue_: strings.Join(continueArgs, " "), + args: args, + continueArgs: continueArgs, + } } // resolveProfileLaunchTokens resolves the user-configured cli_flags argv tokens // and the launcher command_prefix tokens for a profile. Both must be applied on // every command build — the initial launch AND fresh restarts (context reset) — -// or a sandboxed profile would silently relaunch without its wrapper. +// or a configured profile could silently relaunch without its wrapper. // // cli_flags are best-effort: a malformed entry is logged and dropped so a typo -// doesn't block the task. command_prefix is a sandbox boundary, so it fails -// CLOSED: a profile that configured a prefix which cannot be resolved returns an -// error and aborts the launch rather than running the agent unwrapped. +// doesn't block the task. command_prefix is launcher policy, so it fails closed: +// a profile that configured a prefix which cannot be resolved returns an error +// and aborts the launch rather than running the agent unwrapped. func (m *Manager) resolveProfileLaunchTokens(profileInfo *AgentProfileInfo) (cliFlagTokens, commandPrefixTokens []string, err error) { if profileInfo == nil { return nil, nil, nil @@ -231,12 +242,15 @@ func (m *Manager) resolveProfileLaunchTokens(profileInfo *AgentProfileInfo) (cli cliFlagTokens = tokens } if strings.TrimSpace(profileInfo.CommandPrefix) != "" { - tokens, tokErr := cliflags.Tokenise(profileInfo.CommandPrefix) - if tokErr != nil || len(tokens) == 0 { + if validateErr := cliflags.ValidateCommandPrefix(profileInfo.CommandPrefix); validateErr != nil { return nil, nil, fmt.Errorf("resolve command_prefix for profile %s: %w", - profileInfo.ProfileID, tokErr) + profileInfo.ProfileID, validateErr) + } + commandPrefixTokens, err = cliflags.Tokenise(profileInfo.CommandPrefix) + if err != nil { + return nil, nil, fmt.Errorf("resolve command_prefix for profile %s: %w", + profileInfo.ProfileID, err) } - commandPrefixTokens = tokens } return cliFlagTokens, commandPrefixTokens, nil } @@ -244,7 +258,7 @@ func (m *Manager) resolveProfileLaunchTokens(profileInfo *AgentProfileInfo) (cli // buildAgentCommand builds the agent command strings for the execution. // Returns both the initial command and the continue command (for one-shot agents like Amp). // Returns an error when a configured command_prefix cannot be resolved, so a -// sandboxed profile fails closed instead of launching unwrapped. +// configured profile fails closed instead of launching unwrapped. func (m *Manager) buildAgentCommand(req *LaunchRequest, profileInfo *AgentProfileInfo, agentConfig agents.Agent, preferNative bool) (agentCommands, error) { model := "" autoApprove := false @@ -281,10 +295,24 @@ func (m *Manager) buildAgentCommand(req *LaunchRequest, profileInfo *AgentProfil Runtime: models.ExecutorType(req.ExecutorType).Runtime(), PreferNativeBinary: preferNative, } - return agentCommands{ - initial: m.commandBuilder.BuildCommandString(agentConfig, cmdOpts), - continue_: m.commandBuilder.BuildContinueCommandString(agentConfig, cmdOpts), - }, nil + args := m.commandBuilder.BuildCommandArgs(agentConfig, cmdOpts) + continueArgs := m.commandBuilder.BuildContinueCommandArgs(agentConfig, cmdOpts) + if err := validateBuiltAgentCommands(args, continueArgs); err != nil { + return agentCommands{}, err + } + return newAgentCommands(args, continueArgs), nil +} + +func validateBuiltAgentCommands(args, continueArgs []string) error { + if err := cliflags.ValidateCommandArgs(args); err != nil { + return fmt.Errorf("validate agent command: %w", err) + } + if continueArgs != nil { + if err := cliflags.ValidateCommandArgs(continueArgs); err != nil { + return fmt.Errorf("validate continue command: %w", err) + } + } + return nil } // launchResolveWorkspacePath resolves the effective workspace path for non-worktree executors. @@ -925,6 +953,8 @@ func (m *Manager) promoteWorkspaceExecution(ctx context.Context, execution *Agen } execution.AgentCommand = cmds.initial execution.ContinueCommand = cmds.continue_ + execution.AgentArgs = cmds.args + execution.ContinueArgs = cmds.continueArgs if req.ACPSessionID != "" && execution.ACPSessionID == "" { execution.ACPSessionID = req.ACPSessionID } @@ -936,6 +966,8 @@ func (m *Manager) promoteWorkspaceExecution(ctx context.Context, execution *Agen if err := m.materializeRuntimeProjectMCP(sharedCtx, execution, agentConfig); err != nil { execution.AgentCommand = "" execution.ContinueCommand = "" + execution.AgentArgs = nil + execution.ContinueArgs = nil execution.isResumedSession = false execution.IsPassthrough = false return nil, err @@ -1055,12 +1087,15 @@ func (m *Manager) launchInternal(ctx context.Context, req *LaunchRequest) (*Agen // instance directly to avoid leaking it, then fail closed. if rt != nil && execInstance != nil { cleanupCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - if stopErr := rt.StopInstance(cleanupCtx, execInstance, false); stopErr != nil { + if stopErr := rt.StopInstance(cleanupCtx, execInstance, true); stopErr != nil { m.logger.Warn("failed to stop runtime instance after command resolution error", zap.Error(stopErr)) } cancel() } + if execInstance != nil && execInstance.Client != nil { + execInstance.Client.Close() + } return nil, err } if profileInfo != nil && len(profileInfo.EnvVars) > 0 { @@ -1118,6 +1153,8 @@ func (m *Manager) buildExecutionFromInstance( } execution.AgentCommand = cmds.initial execution.ContinueCommand = cmds.continue_ + execution.AgentArgs = cmds.args + execution.ContinueArgs = cmds.continueArgs return execution, nil } @@ -1167,7 +1204,7 @@ func (m *Manager) rollbackLaunchExecution(_ context.Context, rt ExecutorBackend, if rt != nil && execInstance != nil { cleanupCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - if stopErr := rt.StopInstance(cleanupCtx, execInstance, false); stopErr != nil { + if stopErr := rt.StopInstance(cleanupCtx, execInstance, true); stopErr != nil { m.logger.Warn("failed to stop runtime instance during launch rollback", zap.String("execution_id", execution.ID), zap.Error(stopErr)) @@ -1322,7 +1359,7 @@ func (m *Manager) configureAndStartAgent(ctx context.Context, execution *AgentEx return "", fmt.Errorf("failed to prepare agent env: %w", err) } - if err := execution.agentctl.ConfigureAgent(ctx, execution.AgentCommand, env, approvalPolicy, execution.ContinueCommand); err != nil { + if err := execution.agentctl.ConfigureAgent(ctx, execution.AgentCommand, execution.AgentArgs, env, approvalPolicy, execution.ContinueCommand, execution.ContinueArgs); err != nil { return "", fmt.Errorf("failed to configure agent: %w", err) } diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_launch_test.go b/apps/backend/internal/agent/runtime/lifecycle/manager_launch_test.go index b5c1a1771e..2f67bfea3d 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_launch_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_launch_test.go @@ -11,8 +11,10 @@ import ( "os" "os/exec" "path/filepath" + "reflect" "strconv" "strings" + "sync/atomic" "testing" "time" @@ -136,6 +138,15 @@ func (a *cliFlagTestAgent) BuildCommand(_ agents.CommandOptions) agents.Command return agents.Cmd("copilot", "--acp").Build() } +type invalidCommandTestAgent struct { + testAgent + command agents.Command +} + +func (a *invalidCommandTestAgent) BuildCommand(_ agents.CommandOptions) agents.Command { + return a.command +} + func TestBuildAgentCommand_CLIFlagsAppended(t *testing.T) { mgr := newTestManager(t) ag := &cliFlagTestAgent{} @@ -242,6 +253,141 @@ func TestBuildAgentCommand_CommandPrefix(t *testing.T) { }) } +func TestBuildAgentCommand_RejectsInvalidBuiltArgv(t *testing.T) { + mgr := newTestManager(t) + tests := []struct { + name string + agent *invalidCommandTestAgent + }{ + { + name: "initial executable is a flag", + agent: &invalidCommandTestAgent{command: agents.NewCommand("--not-an-executable")}, + }, + { + name: "continue executable is whitespace", + agent: &invalidCommandTestAgent{ + command: agents.NewCommand("agent"), + testAgent: testAgent{runtimeConfig: &agents.RuntimeConfig{SessionConfig: agents.SessionConfig{ + ContinueSessionCmd: agents.NewCommand(" "), + }}}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := mgr.buildAgentCommand(&LaunchRequest{}, nil, tt.agent, false) + require.Error(t, err) + }) + } +} + +func TestManager_LaunchRejectsInvalidBuiltArgvBeforeRegistration(t *testing.T) { + tests := []struct { + name string + agent *invalidCommandTestAgent + }{ + { + name: "initial executable is a flag", + agent: &invalidCommandTestAgent{ + testAgent: testAgent{id: "invalid-command", enabled: true}, + command: agents.NewCommand("--not-an-executable"), + }, + }, + { + name: "continue executable is whitespace", + agent: &invalidCommandTestAgent{ + testAgent: testAgent{ + id: "invalid-command", + enabled: true, + runtimeConfig: &agents.RuntimeConfig{SessionConfig: agents.SessionConfig{ + ContinueSessionCmd: agents.NewCommand(" "), + }}, + }, + command: agents.NewCommand("agent"), + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resolver := &countingProfileResolver{info: &AgentProfileInfo{ + ProfileID: "profile-invalid-command", + AgentName: tt.agent.ID(), + }} + mgr, backend := newEnvironmentExecutionTestManagerWithProfileResolver(t, nil, resolver) + require.NoError(t, mgr.registry.Register(tt.agent)) + var configureCalls, startCalls atomic.Int32 + backend.client = newLaunchRollbackAgentctlClient(t, mgr.logger, &configureCalls, &startCalls) + + execution, err := mgr.Launch(context.Background(), &LaunchRequest{ + TaskID: "task-invalid-command", + SessionID: "session-invalid-command", + AgentProfileID: "profile-invalid-command", + ExecutorType: string(models.ExecutorTypeLocal), + IsEphemeral: true, + }) + require.Nil(t, execution) + require.ErrorContains(t, err, "validate") + require.Equal(t, int32(1), backend.createCount.Load()) + require.Equal(t, int32(1), backend.stopCount.Load()) + require.True(t, backend.forceStopped.Load()) + _, found := mgr.executionStore.GetBySessionID("session-invalid-command") + require.False(t, found) + require.Zero(t, configureCalls.Load()) + require.Zero(t, startCalls.Load()) + }) + } +} + +func TestManager_LaunchBuildExecutionFailureRollsBackCreatedInstance(t *testing.T) { + resolver := &countingProfileResolver{info: &AgentProfileInfo{ + ProfileID: "profile-1", + AgentName: "auggie", + CommandPrefix: `greywall "unterminated`, + }} + mgr, backend := newEnvironmentExecutionTestManagerWithProfileResolver(t, nil, resolver) + var configureCalls, startCalls atomic.Int32 + backend.client = newLaunchRollbackAgentctlClient(t, mgr.logger, &configureCalls, &startCalls) + + execution, err := mgr.Launch(context.Background(), &LaunchRequest{ + TaskID: "task-launch-rollback", + SessionID: "session-launch-rollback", + AgentProfileID: "profile-1", + ExecutorType: string(models.ExecutorTypeLocal), + IsEphemeral: true, + }) + require.Nil(t, execution) + require.Error(t, err) + require.ErrorContains(t, err, "resolve command_prefix") + require.Equal(t, int32(1), backend.createCount.Load(), "instance must be created before command construction fails") + require.Equal(t, int32(1), backend.stopCount.Load(), "created instance must be stopped exactly once") + require.True(t, backend.forceStopped.Load(), "failed instance cleanup must force runtime teardown") + _, found := mgr.executionStore.GetBySessionID("session-launch-rollback") + require.False(t, found, "failed execution must never be registered") + require.Zero(t, configureCalls.Load(), "command construction failure must not configure agentctl") + require.Zero(t, startCalls.Load(), "command construction failure must not start agentctl") +} + +func newLaunchRollbackAgentctlClient(t *testing.T, log *logger.Logger, configureCalls, startCalls *atomic.Int32) *agentctl.Client { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/health": + w.WriteHeader(http.StatusOK) + case "/api/v1/agent/configure": + configureCalls.Add(1) + _, _ = w.Write([]byte(`{"success":true}`)) + case "/api/v1/start": + startCalls.Add(1) + _, _ = w.Write([]byte(`{"success":true}`)) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + t.Cleanup(server.Close) + + return newTestAgentctlClient(t, server.URL, log) +} + func TestBuildEnvForExecution_ResolvesSecretBackedProfileEnv(t *testing.T) { store := newInMemorySecretStore() if err := store.Create(context.Background(), &secrets.SecretWithValue{ @@ -491,6 +637,30 @@ func TestConfigureAndStartAgent_DoesNotSendTaskDescriptionEnv(t *testing.T) { } } +func TestConfigureAndStartAgent_SendsStructuredArgv(t *testing.T) { + mgr := newTestManager(t) + var captured []string + client := newConfigureArgsCaptureAgentctlClient(t, newTestLogger(), &captured) + execution := &AgentExecution{ + ID: "exec-argv", + TaskID: "task-1", + SessionID: "session-1", + AgentProfileID: "profile-1", + AgentCommand: "runner two words C:\\tools\\agent.exe", + AgentArgs: []string{"runner", "two words", "", `C:\tools\agent.exe`}, + WorkspacePath: t.TempDir(), + agentctl: client, + } + + if _, err := mgr.configureAndStartAgent(context.Background(), execution, "never"); err != nil { + t.Fatalf("configure and start agent: %v", err) + } + want := []string{"runner", "two words", "", `C:\tools\agent.exe`} + if !reflect.DeepEqual(captured, want) { + t.Fatalf("configured argv = %#v, want %#v", captured, want) + } +} + func TestConfigureAndStartAgent_SpillsLargeWakePayloadEnv(t *testing.T) { mgr := newTestManager(t) var configuredEnv map[string]string @@ -586,7 +756,38 @@ func newConfigureCaptureAgentctlClient(t *testing.T, log *logger.Logger, capture })) t.Cleanup(server.Close) - parsed, err := url.Parse(server.URL) + return newTestAgentctlClient(t, server.URL, log) +} + +func newConfigureArgsCaptureAgentctlClient(t *testing.T, log *logger.Logger, captured *[]string) *agentctl.Client { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v1/agent/configure": + var req struct { + AgentArgs []string `json:"agent_args"` + } + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + t.Errorf("decode configure request: %v", err) + w.WriteHeader(http.StatusBadRequest) + return + } + *captured = req.AgentArgs + _, _ = w.Write([]byte(`{"success":true}`)) + case "/api/v1/start": + _, _ = w.Write([]byte(`{"success":true,"command":"runner"}`)) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + t.Cleanup(server.Close) + + return newTestAgentctlClient(t, server.URL, log) +} + +func newTestAgentctlClient(t *testing.T, serverURL string, log *logger.Logger) *agentctl.Client { + t.Helper() + parsed, err := url.Parse(serverURL) if err != nil { t.Fatalf("parse test server URL: %v", err) } @@ -1011,6 +1212,11 @@ func TestLaunchResolveWorkspacePath_WorktreeWithoutRepoFallsBackToScratch(t *tes // path and StartAgentProcess() then failed with "no agent command configured". func TestLaunch_PromotesWorkspaceOnlyExecution(t *testing.T) { mgr := newTestManager(t) + mgr.profileResolver = &countingProfileResolver{info: &AgentProfileInfo{ + ProfileID: "profile-1", + AgentName: "auggie", + CommandPrefix: `"/opt/wrapper dir/wrapper" --`, + }} // Inject a workspace-only execution: AgentCommand is intentionally empty, // matching what createExecution produces when called from @@ -1035,6 +1241,9 @@ func TestLaunch_PromotesWorkspaceOnlyExecution(t *testing.T) { require.NoError(t, err) require.Same(t, existing, got, "Launch must reuse the workspace-only execution, not create a new one") require.NotEmpty(t, got.AgentCommand, "AgentCommand must be populated by promotion") + require.GreaterOrEqual(t, len(got.AgentArgs), 2, "promotion must populate structured argv") + require.Equal(t, []string{"/opt/wrapper dir/wrapper", "--"}, got.AgentArgs[:2], + "promotion must preserve the prefix argv token containing spaces") require.Equal(t, "acp-session-abc", got.ACPSessionID, "ACPSessionID must be carried over from the request") require.True(t, got.isResumedSession, "isResumedSession must be set when PreviousExecutionID is non-empty") } diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_project_mcp_test.go b/apps/backend/internal/agent/runtime/lifecycle/manager_project_mcp_test.go index 867d783008..ccb38031ad 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_project_mcp_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_project_mcp_test.go @@ -144,6 +144,12 @@ func TestPromoteWorkspaceExecutionResetsCommandWhenProjectMCPFails(t *testing.T) if execution.ContinueCommand != "" { t.Fatalf("ContinueCommand = %q, want reset to empty", execution.ContinueCommand) } + if execution.AgentArgs != nil { + t.Fatalf("AgentArgs = %#v, want nil after failed promotion", execution.AgentArgs) + } + if execution.ContinueArgs != nil { + t.Fatalf("ContinueArgs = %#v, want nil after failed promotion", execution.ContinueArgs) + } if execution.IsPassthrough { t.Fatal("IsPassthrough should be reset after failed promotion") } diff --git a/apps/backend/internal/agent/runtime/lifecycle/types.go b/apps/backend/internal/agent/runtime/lifecycle/types.go index 4d997f9027..c5caf11724 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/types.go +++ b/apps/backend/internal/agent/runtime/lifecycle/types.go @@ -43,6 +43,8 @@ type AgentExecution struct { ACPSessionID string // ACP session ID to resume, if available AgentCommand string // Command to start the agent subprocess ContinueCommand string // Command for follow-up prompts (one-shot agents like Amp) + AgentArgs []string // Structured argv for AgentCommand + ContinueArgs []string // Structured argv for ContinueCommand RuntimeName agentruntime.Runtime // Name of the runtime used (e.g., "docker", "standalone") Status v1.AgentStatus StartedAt time.Time diff --git a/apps/backend/internal/agent/settings/cliflags/tokenise.go b/apps/backend/internal/agent/settings/cliflags/tokenise.go index 73cb035f51..8008323233 100644 --- a/apps/backend/internal/agent/settings/cliflags/tokenise.go +++ b/apps/backend/internal/agent/settings/cliflags/tokenise.go @@ -35,6 +35,41 @@ func Tokenise(raw string) ([]string, error) { return st.tokens, nil } +// ValidateCommandPrefix verifies an optional launcher command prefix. Empty +// input means no launcher; otherwise argv[0] must be a non-blank executable +// name rather than a flag. This is shared by profile save/preview and runtime +// launch paths so legacy persisted profiles cannot bypass launcher policy. +func ValidateCommandPrefix(prefix string) error { + if strings.TrimSpace(prefix) == "" { + return nil + } + tokens, err := Tokenise(prefix) + if err != nil { + return err + } + if err := ValidateCommandArgs(tokens); err != nil { + return err + } + return nil +} + +// ValidateCommandArgs rejects argv that cannot safely identify an executable. +// It intentionally permits empty secondary arguments because they can be +// meaningful values passed to an otherwise valid command. +func ValidateCommandArgs(args []string) error { + if len(args) == 0 { + return fmt.Errorf("agent command cannot be empty") + } + executable := strings.TrimSpace(args[0]) + if executable == "" { + return fmt.Errorf("agent command executable is invalid") + } + if strings.HasPrefix(executable, "-") { + return fmt.Errorf("first token %q must be a launcher command, not a flag", args[0]) + } + return nil +} + // tokeniseState carries the scanner state between step calls. type tokeniseState struct { tokens []string diff --git a/apps/backend/internal/agent/settings/cliflags/tokenise_test.go b/apps/backend/internal/agent/settings/cliflags/tokenise_test.go index 76fa4f63c4..390894dbf5 100644 --- a/apps/backend/internal/agent/settings/cliflags/tokenise_test.go +++ b/apps/backend/internal/agent/settings/cliflags/tokenise_test.go @@ -51,6 +51,58 @@ func TestTokenise(t *testing.T) { } } +func TestValidateCommandPrefix(t *testing.T) { + cases := []struct { + name string + prefix string + wantErr bool + }{ + {name: "empty means no launcher"}, + {name: "whitespace means no launcher", prefix: " \t "}, + {name: "launcher command", prefix: "greywall --"}, + {name: "unterminated quote", prefix: `greywall "unterminated`, wantErr: true}, + {name: "empty quoted executable", prefix: `'' --arg`, wantErr: true}, + {name: "whitespace executable", prefix: `" " --arg`, wantErr: true}, + {name: "flag executable", prefix: "--not-a-launcher", wantErr: true}, + {name: "leading-space flag executable", prefix: `" --not-a-launcher"`, wantErr: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := ValidateCommandPrefix(tc.prefix) + if tc.wantErr && err == nil { + t.Fatal("expected error") + } + if !tc.wantErr && err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + } +} + +func TestValidateCommandArgs(t *testing.T) { + cases := []struct { + name string + args []string + wantErr bool + }{ + {name: "valid executable with empty secondary argument", args: []string{"runner", ""}}, + {name: "empty argv", wantErr: true}, + {name: "whitespace executable", args: []string{" \t "}, wantErr: true}, + {name: "flag executable", args: []string{" --not-a-launcher"}, wantErr: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := ValidateCommandArgs(tc.args) + if tc.wantErr && err == nil { + t.Fatal("expected error") + } + if !tc.wantErr && err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + } +} + func TestResolve(t *testing.T) { flags := []models.CLIFlag{ {Flag: "--allow-all-tools", Enabled: true}, diff --git a/apps/backend/internal/agent/settings/controller/agent_config.go b/apps/backend/internal/agent/settings/controller/agent_config.go index 15280a1871..ae805d2239 100644 --- a/apps/backend/internal/agent/settings/controller/agent_config.go +++ b/apps/backend/internal/agent/settings/controller/agent_config.go @@ -174,6 +174,9 @@ type CommandPreviewRequest struct { // PreviewAgentCommand generates a preview of the CLI command that will be executed func (c *Controller) PreviewAgentCommand(ctx context.Context, agentName string, req CommandPreviewRequest) (*dto.CommandPreviewResponse, error) { + if err := validateCommandPrefix(req.CommandPrefix); err != nil { + return nil, err + } agentConfig, ok := c.agentRegistry.Get(agentName) if !ok { return nil, fmt.Errorf("agent type %q not found in registry", agentName) @@ -209,10 +212,9 @@ func (c *Controller) PreviewAgentCommand(ctx context.Context, agentName string, cmd = cmd.With().Flag(cliFlagTokens...).Build() } // Prepend the launcher prefix last, mirroring - // lifecycle.CommandBuilder.BuildCommand. Tolerate malformed input - // silently — the preview is informational. Passthrough never applies - // the prefix (neither does the launch path), so this stays in the ACP - // branch only. + // lifecycle.CommandBuilder.BuildCommand. Prefixes were validated above; + // passthrough never applies one (neither does the launch path), so this + // stays in the ACP branch only. if prefixTokens, err := cliflags.Tokenise(req.CommandPrefix); err == nil && len(prefixTokens) > 0 { cmd = agents.NewCommand(append(prefixTokens, cmd.Args()...)...) } diff --git a/apps/backend/internal/agent/settings/controller/controller_test.go b/apps/backend/internal/agent/settings/controller/controller_test.go index f2abf5f7ca..77d664b266 100644 --- a/apps/backend/internal/agent/settings/controller/controller_test.go +++ b/apps/backend/internal/agent/settings/controller/controller_test.go @@ -2,6 +2,7 @@ package controller import ( "context" + "errors" "os" "path/filepath" "strings" @@ -172,6 +173,26 @@ func TestController_PreviewAgentCommand_StandardCommand(t *testing.T) { } } +func TestController_PreviewAgentCommand_RejectsInvalidCommandPrefix(t *testing.T) { + controller := newTestController(map[string]agents.Agent{ + "test-agent": &testAgent{ + id: "test-agent", + name: "test-agent", + enabled: true, + runtime: &agents.RuntimeConfig{Cmd: agents.NewCommand("test-cli")}, + }, + }) + + for _, prefix := range []string{`greywall "unterminated`, `'' --arg`, `--not-a-launcher`} { + _, err := controller.PreviewAgentCommand(context.Background(), "test-agent", CommandPreviewRequest{ + CommandPrefix: prefix, + }) + if !errors.Is(err, ErrInvalidCommandPrefix) { + t.Errorf("prefix %q: expected ErrInvalidCommandPrefix, got %v", prefix, err) + } + } +} + func TestController_PreviewAgentCommand_PassthroughCommand(t *testing.T) { agentList := map[string]agents.Agent{ "claude-code": &testAgent{ diff --git a/apps/backend/internal/agent/settings/controller/profile_crud.go b/apps/backend/internal/agent/settings/controller/profile_crud.go index 15b4cf821b..3f690be2fa 100644 --- a/apps/backend/internal/agent/settings/controller/profile_crud.go +++ b/apps/backend/internal/agent/settings/controller/profile_crud.go @@ -234,23 +234,9 @@ func validateCLIFlagDTOs(in []dto.CLIFlagDTO) error { // launch path's cliflags.Tokenise error branch unreachable in practice, so a // bad prefix surfaces at save time rather than silently dropping at task start. func validateCommandPrefix(prefix string) error { - if strings.TrimSpace(prefix) == "" { - return nil - } - tokens, err := cliflags.Tokenise(prefix) - if err != nil { + if err := cliflags.ValidateCommandPrefix(prefix); err != nil { return fmt.Errorf("%w: %v", ErrInvalidCommandPrefix, err) } - if len(tokens) == 0 || tokens[0] == "" { - return fmt.Errorf("%w: must start with a launcher command", ErrInvalidCommandPrefix) - } - // The first token is the launcher executable. A leading '-' means the user - // typed a flag first (e.g. "--foo greywall"), which would run the flag as - // the program — almost certainly a mistake, and a confusing failure mode - // for a sandbox boundary. - if strings.HasPrefix(tokens[0], "-") { - return fmt.Errorf("%w: first token %q must be a launcher command, not a flag", ErrInvalidCommandPrefix, tokens[0]) - } return nil } diff --git a/apps/backend/internal/agent/settings/controller/profile_crud_test.go b/apps/backend/internal/agent/settings/controller/profile_crud_test.go index 7a902a5b72..6ce2d60186 100644 --- a/apps/backend/internal/agent/settings/controller/profile_crud_test.go +++ b/apps/backend/internal/agent/settings/controller/profile_crud_test.go @@ -137,6 +137,7 @@ func TestValidateCommandPrefix(t *testing.T) { {name: "unterminated quote rejected", prefix: `greywall "unterminated`, wantErr: true}, {name: "trailing backslash rejected", prefix: `greywall foo\`, wantErr: true}, {name: "only-empty-quotes rejected", prefix: `""`, wantErr: true}, + {name: "whitespace executable rejected", prefix: `" " --arg`, wantErr: true}, {name: "leading flag rejected", prefix: `--foo greywall`, wantErr: true}, {name: "leading dash rejected", prefix: `-x sandbox`, wantErr: true}, } @@ -325,4 +326,35 @@ func TestCreateAgentProfiles_RejectsBadCommandPrefix(t *testing.T) { if !errors.Is(err, ErrInvalidCommandPrefix) { t.Fatalf("expected ErrInvalidCommandPrefix, got %v", err) } + if len(st.created) != 0 { + t.Fatalf("bad nested profile create made %d profile writes", len(st.created)) + } +} + +func TestCreateAgent_RejectsBadNestedProfileBeforeAnyWrite(t *testing.T) { + t.Setenv("KANDEV_E2E_MOCK", "true") + ctrl := newTestController(map[string]agents.Agent{ + "test-agent": &testAgent{ + id: "test-agent", + name: "test-agent", + displayName: "Test Agent", + enabled: true, + }, + }) + st := newFakeStore() + ctrl.repo = st + + _, err := ctrl.CreateAgent(context.Background(), CreateAgentRequest{ + Name: "test-agent", + Profiles: []CreateAgentProfileRequest{{ + Name: "Bad", + CommandPrefix: `--not-a-launcher`, + }}, + }) + if !errors.Is(err, ErrInvalidCommandPrefix) { + t.Fatalf("expected ErrInvalidCommandPrefix, got %v", err) + } + if len(st.agents) != 0 || len(st.created) != 0 { + t.Fatalf("bad create made writes: agents=%d profiles=%d", len(st.agents), len(st.created)) + } } diff --git a/apps/backend/internal/agent/settings/handlers/handlers.go b/apps/backend/internal/agent/settings/handlers/handlers.go index 40229d1f2c..c6c1e8322e 100644 --- a/apps/backend/internal/agent/settings/handlers/handlers.go +++ b/apps/backend/internal/agent/settings/handlers/handlers.go @@ -13,6 +13,7 @@ import ( "github.com/kandev/kandev/internal/agent/mcpconfig" "github.com/kandev/kandev/internal/agent/settings/controller" "github.com/kandev/kandev/internal/agent/settings/dto" + "github.com/kandev/kandev/internal/common/httpmw" "github.com/kandev/kandev/internal/common/logger" ws "github.com/kandev/kandev/pkg/websocket" "go.uber.org/zap" @@ -26,25 +27,27 @@ type Handlers struct { controller *controller.Controller hub Broadcaster logger *logger.Logger + interlock gin.HandlerFunc } type Broadcaster interface { Broadcast(msg *ws.Message) } -func NewHandlers(ctrl *controller.Controller, hub Broadcaster, log *logger.Logger) *Handlers { +func NewHandlers(ctrl *controller.Controller, hub Broadcaster, log *logger.Logger, interlockToken string) *Handlers { return &Handlers{ controller: ctrl, hub: hub, logger: log.WithFields(zap.String("component", "agent-settings-handlers")), + interlock: httpmw.InterimSettingsInterlock(interlockToken), } } -func RegisterRoutes(router *gin.Engine, ctrl *controller.Controller, hub Broadcaster, log *logger.Logger) { +func RegisterRoutes(router *gin.Engine, ctrl *controller.Controller, hub Broadcaster, log *logger.Logger, interlockToken string) { // Wire the install job store with the same broadcaster used by other // agent-settings notifications so streaming install events reach the UI. ctrl.SetJobBroadcaster(hub) - handlers := NewHandlers(ctrl, hub, log) + handlers := NewHandlers(ctrl, hub, log, interlockToken) handlers.registerHTTP(router) } @@ -54,22 +57,22 @@ func (h *Handlers) registerHTTP(router *gin.Engine) { api.GET("/agents/available", h.httpListAvailableAgents) api.GET("/agents/usage", h.httpAgentSubscriptionUsage) api.GET("/agents", h.httpListAgents) - api.POST("/agents", h.httpCreateAgent) - api.POST("/agents/tui", h.httpCreateCustomTUIAgent) + api.POST("/agents", h.interlock, h.httpCreateAgent) + api.POST("/agents/tui", h.interlock, h.httpCreateCustomTUIAgent) api.GET("/agents/:id", h.httpGetAgent) - api.PATCH("/agents/:id", h.httpUpdateAgent) - api.DELETE("/agents/:id", h.httpDeleteAgent) - api.POST("/agents/:id/profiles", h.httpCreateProfile) + api.PATCH("/agents/:id", h.interlock, h.httpUpdateAgent) + api.DELETE("/agents/:id", h.interlock, h.httpDeleteAgent) + api.POST("/agents/:id/profiles", h.interlock, h.httpCreateProfile) api.GET("/agents/:id/logo", h.httpGetAgentLogo) api.GET("/agent-models/:agentName", h.httpGetAgentModels) api.POST("/agent-command-preview/:agentName", h.httpPreviewAgentCommand) - api.POST("/agent-install/:agentName", h.httpInstallAgent) + api.POST("/agent-install/:agentName", h.interlock, h.httpInstallAgent) api.GET("/agent-install/jobs", h.httpListInstallJobs) api.GET("/agent-install/jobs/:id", h.httpGetInstallJob) - api.PATCH("/agent-profiles/:id", h.httpUpdateProfile) - api.DELETE("/agent-profiles/:id", h.httpDeleteProfile) + api.PATCH("/agent-profiles/:id", h.interlock, h.httpUpdateProfile) + api.DELETE("/agent-profiles/:id", h.interlock, h.httpDeleteProfile) api.GET("/agent-profiles/:id/mcp-config", h.httpGetProfileMcpConfig) - api.POST("/agent-profiles/:id/mcp-config", h.httpUpdateProfileMcpConfig) + api.POST("/agent-profiles/:id/mcp-config", h.interlock, h.httpUpdateProfileMcpConfig) } func (h *Handlers) httpDiscoverAgents(c *gin.Context) { diff --git a/apps/backend/internal/agent/settings/handlers/interim_settings_interlock_test.go b/apps/backend/internal/agent/settings/handlers/interim_settings_interlock_test.go new file mode 100644 index 0000000000..d32f5203ad --- /dev/null +++ b/apps/backend/internal/agent/settings/handlers/interim_settings_interlock_test.go @@ -0,0 +1,67 @@ +package handlers + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gin-gonic/gin" + "github.com/kandev/kandev/internal/common/logger" +) + +func TestRegisterRoutesRejectsMutationWithoutInterimSettingsInterlock(t *testing.T) { + gin.SetMode(gin.TestMode) + log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"}) + if err != nil { + t.Fatalf("NewLogger: %v", err) + } + + router := gin.New() + NewHandlers(nil, nil, log, "test-interlock").registerHTTP(router) + + request := httptest.NewRequest(http.MethodPost, "/api/v1/agents", strings.NewReader("{")) + request.Header.Set("Content-Type", "application/json") + response := httptest.NewRecorder() + router.ServeHTTP(response, request) + + if response.Code != http.StatusForbidden { + t.Fatalf("status = %d, want %d", response.Code, http.StatusForbidden) + } +} + +func TestRegisterRoutesProtectsEveryStateChangingAgentSettingsRoute(t *testing.T) { + gin.SetMode(gin.TestMode) + log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json"}) + if err != nil { + t.Fatalf("NewLogger: %v", err) + } + + router := gin.New() + NewHandlers(nil, nil, log, "test-interlock").registerHTTP(router) + requests := []struct { + method string + path string + }{ + {method: http.MethodPost, path: "/api/v1/agents"}, + {method: http.MethodPost, path: "/api/v1/agents/tui"}, + {method: http.MethodPatch, path: "/api/v1/agents/agent-1"}, + {method: http.MethodDelete, path: "/api/v1/agents/agent-1"}, + {method: http.MethodPost, path: "/api/v1/agents/agent-1/profiles"}, + {method: http.MethodPost, path: "/api/v1/agent-install/agent-1"}, + {method: http.MethodPatch, path: "/api/v1/agent-profiles/profile-1"}, + {method: http.MethodDelete, path: "/api/v1/agent-profiles/profile-1"}, + {method: http.MethodPost, path: "/api/v1/agent-profiles/profile-1/mcp-config"}, + } + + for _, requestSpec := range requests { + t.Run(requestSpec.method+" "+requestSpec.path, func(t *testing.T) { + request := httptest.NewRequest(requestSpec.method, requestSpec.path, nil) + response := httptest.NewRecorder() + router.ServeHTTP(response, request) + if response.Code != http.StatusForbidden { + t.Fatalf("status = %d, want %d", response.Code, http.StatusForbidden) + } + }) + } +} diff --git a/apps/backend/internal/agentctl/server/api/agent_test.go b/apps/backend/internal/agentctl/server/api/agent_test.go index c2c7162770..94e8c07565 100644 --- a/apps/backend/internal/agentctl/server/api/agent_test.go +++ b/apps/backend/internal/agentctl/server/api/agent_test.go @@ -7,6 +7,7 @@ import ( "io" "net/http" "net/http/httptest" + "slices" "strings" "testing" "time" @@ -43,6 +44,60 @@ func newTestServer(t *testing.T) *Server { return NewServer(cfg, procMgr, nil, nil, log) } +func TestHandleAgentConfigure_PrefersPresentStructuredArgs(t *testing.T) { + log := newTestLogger() + cfg := &config.InstanceConfig{Port: 0, WorkDir: t.TempDir()} + procMgr := process.NewManager(cfg, log) + server := NewServer(cfg, procMgr, nil, nil, log) + + body := strings.NewReader(`{"command":"legacy command","agent_args":["runner","two words","","C:\\tools\\agent.exe"]}`) + req := httptest.NewRequest(http.MethodPost, "/api/v1/agent/configure", body) + req.Header.Set("Content-Type", "application/json") + resp := httptest.NewRecorder() + + server.router.ServeHTTP(resp, req) + + if resp.Code != http.StatusOK { + t.Fatalf("configure status = %d, want %d: %s", resp.Code, http.StatusOK, resp.Body.String()) + } + want := []string{"runner", "two words", "", `C:\tools\agent.exe`} + if got := cfg.AgentArgs; !slices.Equal(got, want) { + t.Fatalf("AgentArgs = %#v, want %#v", got, want) + } +} + +func TestHandleAgentConfigure_UsesLegacyOnlyWhenArgsAbsent(t *testing.T) { + log := newTestLogger() + cfg := &config.InstanceConfig{Port: 0, WorkDir: t.TempDir()} + server := NewServer(cfg, process.NewManager(cfg, log), nil, nil, log) + + for _, tc := range []struct { + name string + body string + code int + want []string + }{ + {name: "legacy absent", body: `{"command":"legacy --flag"}`, code: http.StatusOK, want: []string{"legacy", "--flag"}}, + {name: "present empty rejects", body: `{"command":"legacy --flag","agent_args":[]}`, code: http.StatusInternalServerError}, + {name: "present flag executable rejects", body: `{"command":"legacy --flag","agent_args":["--flag"]}`, code: http.StatusInternalServerError}, + {name: "present null rejects", body: `{"command":"legacy --flag","agent_args":null}`, code: http.StatusInternalServerError}, + {name: "present empty continue rejects", body: `{"command":"runner","agent_args":["runner"],"continue_command":"legacy continue","continue_args":[]}`, code: http.StatusInternalServerError}, + } { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/api/v1/agent/configure", strings.NewReader(tc.body)) + req.Header.Set("Content-Type", "application/json") + resp := httptest.NewRecorder() + server.router.ServeHTTP(resp, req) + if resp.Code != tc.code { + t.Fatalf("configure status = %d, want %d: %s", resp.Code, tc.code, resp.Body.String()) + } + if tc.want != nil && !slices.Equal(cfg.AgentArgs, tc.want) { + t.Fatalf("AgentArgs = %#v, want %#v", cfg.AgentArgs, tc.want) + } + }) + } +} + // dialTestWS connects a WebSocket client to the test server's /api/v1/agent/stream endpoint. func dialTestWS(t *testing.T, server *httptest.Server) *websocket.Conn { t.Helper() diff --git a/apps/backend/internal/agentctl/server/api/server.go b/apps/backend/internal/agentctl/server/api/server.go index f8be960bf1..5a8d25e3de 100644 --- a/apps/backend/internal/agentctl/server/api/server.go +++ b/apps/backend/internal/agentctl/server/api/server.go @@ -374,11 +374,23 @@ func (s *Server) handleStart(c *gin.Context) { // AgentConfigure request/response - configures the agent command before starting type AgentConfigureRequest struct { Command string `json:"command"` + AgentArgs optionalArgs `json:"agent_args"` ContinueCommand string `json:"continue_command,omitempty"` // For one-shot agents (Amp): command for follow-up prompts + ContinueArgs optionalArgs `json:"continue_args"` Env map[string]string `json:"env,omitempty"` ApprovalPolicy string `json:"approval_policy,omitempty"` // For Codex: "untrusted", "on-failure", "on-request", "never" } +type optionalArgs struct { + Present bool + Args []string +} + +func (a *optionalArgs) UnmarshalJSON(data []byte) error { + a.Present = true + return json.Unmarshal(data, &a.Args) +} + type AgentConfigureResponse struct { Success bool `json:"success"` Message string `json:"message,omitempty"` @@ -395,7 +407,7 @@ func (s *Server) handleAgentConfigure(c *gin.Context) { return } - if req.Command == "" { + if !req.AgentArgs.Present && req.Command == "" { c.JSON(http.StatusBadRequest, AgentConfigureResponse{ Success: false, Error: "command is required", @@ -403,7 +415,7 @@ func (s *Server) handleAgentConfigure(c *gin.Context) { return } - if err := s.procMgr.Configure(req.Command, req.Env, req.ApprovalPolicy, req.ContinueCommand); err != nil { + if err := s.procMgr.Configure(req.Command, req.AgentArgs.Args, req.AgentArgs.Present, req.Env, req.ApprovalPolicy, req.ContinueCommand, req.ContinueArgs.Args, req.ContinueArgs.Present); err != nil { s.logger.Error("failed to configure agent", zap.Error(err), zap.String("command", req.Command)) c.JSON(http.StatusInternalServerError, AgentConfigureResponse{ Success: false, diff --git a/apps/backend/internal/agentctl/server/config/config.go b/apps/backend/internal/agentctl/server/config/config.go index 4a994dfb67..6b92f98178 100644 --- a/apps/backend/internal/agentctl/server/config/config.go +++ b/apps/backend/internal/agentctl/server/config/config.go @@ -14,6 +14,7 @@ import ( "crypto/rand" "crypto/subtle" "encoding/hex" + "fmt" "os" "strconv" "strings" @@ -195,6 +196,9 @@ type InstanceConfig struct { // continuation (thread ID appended at runtime). Only used by Amp. ContinueCommand string + // ContinueArgs is the structured argv for ContinueCommand. + ContinueArgs []string + // VscodeCommand is the command to run the VS Code server (e.g., "code-server") VscodeCommand string @@ -457,6 +461,18 @@ func ParseCommand(cmd string) []string { return strings.Fields(cmd) } +// ValidateCommandArgs rejects argv that cannot safely identify an executable. +func ValidateCommandArgs(args []string) error { + if len(args) == 0 { + return fmt.Errorf("agent command cannot be empty") + } + executable := strings.TrimSpace(args[0]) + if executable == "" || strings.HasPrefix(executable, "-") { + return fmt.Errorf("agent command executable is invalid") + } + return nil +} + // CollectAgentEnv collects environment variables to pass to the agent. // It filters out AGENTCTL_* variables and optionally merges additional env vars. func CollectAgentEnv(additional map[string]string) []string { diff --git a/apps/backend/internal/agentctl/server/config/config_test.go b/apps/backend/internal/agentctl/server/config/config_test.go index e2a8a49e6a..e84504a075 100644 --- a/apps/backend/internal/agentctl/server/config/config_test.go +++ b/apps/backend/internal/agentctl/server/config/config_test.go @@ -2,6 +2,27 @@ package config import "testing" +func TestValidateCommandArgs(t *testing.T) { + for _, tc := range []struct { + name string + args []string + want bool + }{ + {name: "valid preserves empty argument", args: []string{"runner", "", "two words"}}, + {name: "empty argv", args: nil, want: true}, + {name: "empty executable", args: []string{"", "arg"}, want: true}, + {name: "whitespace executable", args: []string{" \t", "arg"}, want: true}, + {name: "flag executable", args: []string{"--runner", "arg"}, want: true}, + } { + t.Run(tc.name, func(t *testing.T) { + err := ValidateCommandArgs(tc.args) + if (err != nil) != tc.want { + t.Fatalf("ValidateCommandArgs(%#v) error = %v, want error=%v", tc.args, err, tc.want) + } + }) + } +} + func TestConsumeNonce(t *testing.T) { t.Run("valid nonce returns token and burns nonce", func(t *testing.T) { cfg := &Config{ diff --git a/apps/backend/internal/agentctl/server/process/manager.go b/apps/backend/internal/agentctl/server/process/manager.go index 3b4d1f7916..95bd880ef3 100644 --- a/apps/backend/internal/agentctl/server/process/manager.go +++ b/apps/backend/internal/agentctl/server/process/manager.go @@ -798,9 +798,9 @@ func (m *Manager) Start(ctx context.Context) error { m.exitCode.Store(-1) m.exitErr.Store(errorWrapper{err: nil}) - if len(m.cfg.AgentArgs) == 0 { + if err := config.ValidateCommandArgs(m.cfg.AgentArgs); err != nil { m.status.Store(StatusError) - return fmt.Errorf("no agent command configured") + return err } // Build adapter config and create protocol adapter @@ -941,10 +941,14 @@ func (m *Manager) buildAdapterConfig() error { // Configure one-shot mode when a continue command is provided. // One-shot adapters (e.g., Amp) spawn a new subprocess per prompt. - if m.cfg.ContinueCommand != "" { + continueArgs := m.cfg.ContinueArgs + if continueArgs == nil && m.cfg.ContinueCommand != "" { + continueArgs = config.ParseCommand(m.cfg.ContinueCommand) + } + if len(continueArgs) > 0 { m.adapterCfg.OneShotConfig = &adapter.OneShotConfig{ InitialArgs: m.cfg.AgentArgs, - ContinueArgs: config.ParseCommand(m.cfg.ContinueCommand), + ContinueArgs: continueArgs, Env: m.cfg.AgentEnv, WorkDir: m.cfg.WorkDir, } @@ -1160,7 +1164,7 @@ func lookupEnvValue(env []string, key string) string { // Configure sets the agent command and optional environment variables. // This must be called before Start() if the instance was created without a command. // continueCommand is optional — when set, the adapter uses it for one-shot follow-up prompts. -func (m *Manager) Configure(command string, env map[string]string, approvalPolicy, continueCommand string) error { +func (m *Manager) Configure(command string, agentArgs []string, agentArgsPresent bool, env map[string]string, approvalPolicy, continueCommand string, continueArgs []string, continueArgsPresent bool) error { m.startMu.Lock() defer m.startMu.Unlock() @@ -1168,14 +1172,22 @@ func (m *Manager) Configure(command string, env map[string]string, approvalPolic return fmt.Errorf("cannot configure while agent is running") } - if command == "" { - return fmt.Errorf("agent command cannot be empty") + args := agentArgs + if !agentArgsPresent { + args = config.ParseCommand(command) } - - // Parse the command string and update config - args := config.ParseCommand(command) - if len(args) == 0 { - return fmt.Errorf("failed to parse agent command") + if err := config.ValidateCommandArgs(args); err != nil { + return err + } + if continueArgsPresent { + if err := config.ValidateCommandArgs(continueArgs); err != nil { + return fmt.Errorf("invalid continue command: %w", err) + } + } else if continueCommand != "" { + continueArgs = config.ParseCommand(continueCommand) + if err := config.ValidateCommandArgs(continueArgs); err != nil { + return fmt.Errorf("invalid continue command: %w", err) + } } m.cfg.AgentCommand = command @@ -1187,8 +1199,12 @@ func (m *Manager) Configure(command string, env map[string]string, approvalPolic } // Store continue command for one-shot adapters (e.g., Amp) - if continueCommand != "" { + if continueArgsPresent { m.cfg.ContinueCommand = continueCommand + m.cfg.ContinueArgs = continueArgs + } else if continueCommand != "" { + m.cfg.ContinueCommand = continueCommand + m.cfg.ContinueArgs = continueArgs } // Merge additional env vars diff --git a/apps/backend/internal/backendapp/helpers.go b/apps/backend/internal/backendapp/helpers.go index d75331cceb..a3799ac791 100644 --- a/apps/backend/internal/backendapp/helpers.go +++ b/apps/backend/internal/backendapp/helpers.go @@ -453,42 +453,43 @@ func sessionACPConfigBaseline(session *models.TaskSession) map[string]string { // routeParams holds all dependencies needed for HTTP and WebSocket route registration. type routeParams struct { - router *gin.Engine - gateway *gateways.Gateway - taskSvc *taskservice.Service - taskRepo *sqliterepo.Repository - officeRepo *officesqlite.Repository - analyticsRepo analyticsrepository.Repository - orchestratorSvc *orchestrator.Service - lifecycleMgr *lifecycle.Manager - hostUtilityMgr *hostutility.Manager - eventBus bus.EventBus - services *Services - systemSvc *systemsvc.Service - workspaceRestorer taskhandlers.WorkspaceQuarantineRestorer - runtimeFlagsSvc *runtimeflags.Service - agentSettingsController *agentsettingscontroller.Controller - agentSettingsRepo settingsstore.Repository - agentList taskhandlers.AgentLister - agentRegistry *registry.Registry - userCtrl *usercontroller.Controller - notificationCtrl *notificationcontroller.Controller - editorCtrl *editorcontroller.Controller - promptCtrl *promptcontroller.Controller - utilityCtrl *utilitycontroller.Controller - msgCreator *messageCreatorAdapter - secretsSvc *secrets.Service - secretStore secrets.SecretStore - mcpConfigSvc *mcpconfig.Service - addCleanup func(func() error) - repoCloner *repoclone.Cloner - version string - webInternalURL string - devMode bool - httpPort int - features config.FeaturesConfig - voice config.VoiceConfig - log *logger.Logger + router *gin.Engine + gateway *gateways.Gateway + taskSvc *taskservice.Service + taskRepo *sqliterepo.Repository + officeRepo *officesqlite.Repository + analyticsRepo analyticsrepository.Repository + orchestratorSvc *orchestrator.Service + lifecycleMgr *lifecycle.Manager + hostUtilityMgr *hostutility.Manager + eventBus bus.EventBus + services *Services + systemSvc *systemsvc.Service + workspaceRestorer taskhandlers.WorkspaceQuarantineRestorer + runtimeFlagsSvc *runtimeflags.Service + agentSettingsController *agentsettingscontroller.Controller + agentSettingsRepo settingsstore.Repository + agentList taskhandlers.AgentLister + agentRegistry *registry.Registry + userCtrl *usercontroller.Controller + notificationCtrl *notificationcontroller.Controller + editorCtrl *editorcontroller.Controller + promptCtrl *promptcontroller.Controller + utilityCtrl *utilitycontroller.Controller + msgCreator *messageCreatorAdapter + secretsSvc *secrets.Service + secretStore secrets.SecretStore + mcpConfigSvc *mcpconfig.Service + addCleanup func(func() error) + repoCloner *repoclone.Cloner + version string + webInternalURL string + devMode bool + httpPort int + features config.FeaturesConfig + voice config.VoiceConfig + interimSettingsInterlockToken string + log *logger.Logger } // registerRoutes sets up all HTTP and WebSocket routes on the given router. @@ -696,6 +697,7 @@ func bootPayload(ctx context.Context, req *http.Request, p routeParams, route we ) payload.RouteData = bootRouteData(ctx, req, p, route) payload.Plugins = bootActivePlugins(p) + payload.InterimSettingsInterlockToken = p.interimSettingsInterlockToken return payload } @@ -915,7 +917,7 @@ func registerSecondaryRoutes( workflowhandlers.RegisterRoutes(p.router, p.gateway.Dispatcher, workflowCtrl, p.eventBus, p.log) p.log.Info("Registered Workflow handlers (HTTP + WebSocket)") - agentsettingshandlers.RegisterRoutes(p.router, p.agentSettingsController, p.gateway.Hub, p.log) + agentsettingshandlers.RegisterRoutes(p.router, p.agentSettingsController, p.gateway.Hub, p.log, p.interimSettingsInterlockToken) p.log.Debug("Registered Agent Settings handlers (HTTP)") // Login PTY: spawns agent login commands under a PTY on the kandev host diff --git a/apps/backend/internal/backendapp/httpserver_test.go b/apps/backend/internal/backendapp/httpserver_test.go index 901df176a1..6f4a08be26 100644 --- a/apps/backend/internal/backendapp/httpserver_test.go +++ b/apps/backend/internal/backendapp/httpserver_test.go @@ -2,17 +2,34 @@ package backendapp import ( "context" + "errors" "fmt" "net" "net/http" "testing" "time" + "github.com/kandev/kandev/internal/common/config" "github.com/kandev/kandev/internal/common/logger" "go.uber.org/goleak" "go.uber.org/zap" ) +func TestBuildHTTPServerAbortsWhenInterlockTokenGenerationFails(t *testing.T) { + wantErr := errors.New("entropy unavailable") + original := newInterimSettingsInterlockToken + newInterimSettingsInterlockToken = func() (string, error) { return "", wantErr } + t.Cleanup(func() { newInterimSettingsInterlockToken = original }) + + server, err := buildHTTPServer(&config.Config{}, testLogger(t), nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) + if !errors.Is(err, wantErr) { + t.Fatalf("buildHTTPServer error = %v, want %v", err, wantErr) + } + if server != nil { + t.Fatal("buildHTTPServer returned a server after token generation failed") + } +} + func testLogger(t *testing.T) *logger.Logger { t.Helper() log, err := logger.NewFromZap(zap.NewNop()) diff --git a/apps/backend/internal/backendapp/main.go b/apps/backend/internal/backendapp/main.go index 9cee05b032..d8a8535766 100644 --- a/apps/backend/internal/backendapp/main.go +++ b/apps/backend/internal/backendapp/main.go @@ -788,14 +788,15 @@ func startGatewayAndServe( // ============================================ // HTTP SERVER // ============================================ - server := buildHTTPServer(cfg, log, gateway, repos, services, agentSettingsController, + server, err := buildHTTPServer(cfg, log, gateway, repos, services, agentSettingsController, lifecycleMgr, eventBus, orchestratorSvc, notificationCtrl, msgCreator, agentRegistry, hostUtilityMgr, addCleanup, repoCloner, systemSvc, storageComposition.workspaceRestorer) - - port := cfg.Server.Port - if port == 0 { - port = ports.Backend + if err != nil { + log.Error("Failed to build HTTP server", zap.Error(err)) + return false } + + port := resolvedHTTPPort(cfg) hosts, err := cfg.Server.ResolvedBinds() if err != nil { log.Error("Invalid server bind configuration", zap.Error(err)) @@ -1644,6 +1645,15 @@ func buildOfficeDashboardService( } // buildHTTPServer creates the HTTP server with all routes registered. +var newInterimSettingsInterlockToken = httpmw.NewInterimSettingsInterlockToken + +func resolvedHTTPPort(cfg *config.Config) int { + if cfg.Server.Port != 0 { + return cfg.Server.Port + } + return ports.Backend +} + func buildHTTPServer( cfg *config.Config, log *logger.Logger, @@ -1662,56 +1672,56 @@ func buildHTTPServer( repoCloner *repoclone.Cloner, systemSvc *systemsvc.Service, workspaceRestorer taskhandlers.WorkspaceQuarantineRestorer, -) *http.Server { +) (*http.Server, error) { gin.SetMode(gin.ReleaseMode) router := gin.New() router.Use(httpmw.RequestLogger(log, "kandev")) router.Use(httpmw.OtelTracing("kandev")) router.Use(gin.Recovery()) router.Use(corsMiddleware()) - - port := cfg.Server.Port - if port == 0 { - port = ports.Backend + interimSettingsInterlockToken, err := newInterimSettingsInterlockToken() + if err != nil { + return nil, fmt.Errorf("generate interim settings interlock token: %w", err) } registerRoutes(routeParams{ - router: router, - gateway: gateway, - taskSvc: services.Task, - taskRepo: repos.Task, - officeRepo: repos.Office, - analyticsRepo: repos.Analytics, - orchestratorSvc: orchestratorSvc, - lifecycleMgr: lifecycleMgr, - hostUtilityMgr: hostUtilityMgr, - eventBus: eventBus, - services: services, - systemSvc: systemSvc, - workspaceRestorer: workspaceRestorer, - runtimeFlagsSvc: services.RuntimeFlags, - agentSettingsController: agentSettingsController, - agentSettingsRepo: repos.AgentSettings, - agentList: agentRegistry, - agentRegistry: agentRegistry, - userCtrl: usercontroller.NewController(services.User), - notificationCtrl: notificationCtrl, - editorCtrl: editorcontroller.NewController(services.Editor), - promptCtrl: promptcontroller.NewController(services.Prompts), - utilityCtrl: utilitycontroller.NewController(services.Utility), - msgCreator: msgCreator, - secretsSvc: secrets.NewService(repos.Secrets, log), - secretStore: repos.Secrets, - mcpConfigSvc: mcpconfig.NewService(repos.AgentSettings), - addCleanup: addCleanup, - repoCloner: repoCloner, - version: Version, - webInternalURL: cfg.Server.WebInternalURL, - devMode: cfg.Debug.DevMode || cfg.Debug.PprofEnabled, - httpPort: port, - features: cfg.Features, - voice: cfg.Voice, - log: log, + router: router, + gateway: gateway, + taskSvc: services.Task, + taskRepo: repos.Task, + officeRepo: repos.Office, + analyticsRepo: repos.Analytics, + orchestratorSvc: orchestratorSvc, + lifecycleMgr: lifecycleMgr, + hostUtilityMgr: hostUtilityMgr, + eventBus: eventBus, + services: services, + systemSvc: systemSvc, + workspaceRestorer: workspaceRestorer, + runtimeFlagsSvc: services.RuntimeFlags, + agentSettingsController: agentSettingsController, + agentSettingsRepo: repos.AgentSettings, + agentList: agentRegistry, + agentRegistry: agentRegistry, + userCtrl: usercontroller.NewController(services.User), + notificationCtrl: notificationCtrl, + editorCtrl: editorcontroller.NewController(services.Editor), + promptCtrl: promptcontroller.NewController(services.Prompts), + utilityCtrl: utilitycontroller.NewController(services.Utility), + msgCreator: msgCreator, + secretsSvc: secrets.NewService(repos.Secrets, log), + secretStore: repos.Secrets, + mcpConfigSvc: mcpconfig.NewService(repos.AgentSettings), + addCleanup: addCleanup, + repoCloner: repoCloner, + version: Version, + webInternalURL: cfg.Server.WebInternalURL, + devMode: cfg.Debug.DevMode || cfg.Debug.PprofEnabled, + httpPort: resolvedHTTPPort(cfg), + features: cfg.Features, + voice: cfg.Voice, + interimSettingsInterlockToken: interimSettingsInterlockToken, + log: log, }) // Addr is intentionally left unset: bind addresses are resolved from @@ -1722,7 +1732,7 @@ func buildHTTPServer( Handler: router, ReadTimeout: cfg.Server.ReadTimeoutDuration(), WriteTimeout: cfg.Server.WriteTimeoutDuration(), - } + }, nil } // awaitShutdown waits for an OS signal then performs graceful shutdown. diff --git a/apps/backend/internal/backendapp/middleware.go b/apps/backend/internal/backendapp/middleware.go index 380b1b699e..6b7c7bc363 100644 --- a/apps/backend/internal/backendapp/middleware.go +++ b/apps/backend/internal/backendapp/middleware.go @@ -37,7 +37,7 @@ func corsMiddleware() gin.HandlerFunc { c.Header("Access-Control-Allow-Origin", "*") } c.Header("Access-Control-Allow-Methods", "GET, POST, PATCH, PUT, DELETE, OPTIONS") - c.Header("Access-Control-Allow-Headers", "Origin, Content-Type, Authorization, Upgrade, Connection, Sec-WebSocket-Key, Sec-WebSocket-Version, Sec-WebSocket-Protocol") + c.Header("Access-Control-Allow-Headers", "Origin, Content-Type, Authorization, X-Kandev-Interim-Settings-Interlock, Upgrade, Connection, Sec-WebSocket-Key, Sec-WebSocket-Version, Sec-WebSocket-Protocol") if c.Request.Method == "OPTIONS" { c.AbortWithStatus(http.StatusNoContent) diff --git a/apps/backend/internal/backendapp/middleware_test.go b/apps/backend/internal/backendapp/middleware_test.go index e62e3d15d6..dc309b1f11 100644 --- a/apps/backend/internal/backendapp/middleware_test.go +++ b/apps/backend/internal/backendapp/middleware_test.go @@ -3,6 +3,7 @@ package backendapp import ( "net/http" "net/http/httptest" + "strings" "testing" "github.com/gin-gonic/gin" @@ -30,6 +31,28 @@ func TestCORSMiddlewareEchoesOriginForCredentialedRequests(t *testing.T) { } } +func TestCORSMiddlewareAllowsInterimSettingsInterlockHeader(t *testing.T) { + gin.SetMode(gin.TestMode) + router := gin.New() + router.Use(corsMiddleware()) + router.POST("/api/v1/agents", func(c *gin.Context) { c.Status(http.StatusNoContent) }) + + req := httptest.NewRequest(http.MethodOptions, "/api/v1/agents", nil) + req.Host = "localhost:38429" + req.Header.Set("Origin", "http://localhost:37429") + req.Header.Set("Access-Control-Request-Method", http.MethodPost) + req.Header.Set("Access-Control-Request-Headers", "X-Kandev-Interim-Settings-Interlock") + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusNoContent { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusNoContent) + } + if !strings.Contains(rec.Header().Get("Access-Control-Allow-Headers"), "X-Kandev-Interim-Settings-Interlock") { + t.Fatalf("Access-Control-Allow-Headers = %q, want interim settings interlock header", rec.Header().Get("Access-Control-Allow-Headers")) + } +} + func TestCORSMiddlewareAllowsLoopbackAliasOriginForCredentialedRequests(t *testing.T) { gin.SetMode(gin.TestMode) router := gin.New() diff --git a/apps/backend/internal/common/httpmw/interim_settings_interlock.go b/apps/backend/internal/common/httpmw/interim_settings_interlock.go new file mode 100644 index 0000000000..dd147a8a00 --- /dev/null +++ b/apps/backend/internal/common/httpmw/interim_settings_interlock.go @@ -0,0 +1,53 @@ +package httpmw + +import ( + "crypto/rand" + "crypto/subtle" + "encoding/base64" + "net/http" + "strings" + + "github.com/gin-gonic/gin" +) + +// InterimSettingsInterlockHeader carries the replayable, per-boot SPA +// interlock used to reduce accidental settings mutations. It is not an +// authentication credential. +const InterimSettingsInterlockHeader = "X-Kandev-Interim-Settings-Interlock" + +// NewInterimSettingsInterlockToken returns a random token for one backend +// boot. Callers must fail closed when token generation fails. +func NewInterimSettingsInterlockToken() (string, error) { + value := make([]byte, 32) + if _, err := rand.Read(value); err != nil { + return "", err + } + return base64.RawURLEncoding.EncodeToString(value), nil +} + +// InterimSettingsInterlock rejects agent/settings mutations without the +// current per-boot token and rejects Office bearer credentials outright. The +// token is intentionally replayable by a client that can read the SPA boot +// payload, so this is an interim CSRF/accidental-mutation interlock only. +func InterimSettingsInterlock(token string) gin.HandlerFunc { + return func(c *gin.Context) { + if strings.TrimSpace(token) == "" || hasBearerAuthorization(c.GetHeader("Authorization")) || + !constantTimeEqual(c.GetHeader(InterimSettingsInterlockHeader), token) { + c.AbortWithStatusJSON(http.StatusForbidden, gin.H{"error": "interim settings interlock required"}) + return + } + c.Next() + } +} + +func hasBearerAuthorization(value string) bool { + parts := strings.Fields(value) + return len(parts) > 0 && strings.EqualFold(parts[0], "Bearer") +} + +func constantTimeEqual(value, expected string) bool { + if value == "" || expected == "" || len(value) != len(expected) { + return false + } + return subtle.ConstantTimeCompare([]byte(value), []byte(expected)) == 1 +} diff --git a/apps/backend/internal/common/httpmw/interim_settings_interlock_test.go b/apps/backend/internal/common/httpmw/interim_settings_interlock_test.go new file mode 100644 index 0000000000..705f67727d --- /dev/null +++ b/apps/backend/internal/common/httpmw/interim_settings_interlock_test.go @@ -0,0 +1,87 @@ +package httpmw + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" +) + +func TestInterimSettingsInterlockRejectsMissingWrongBearerAndEmptyServerToken(t *testing.T) { + gin.SetMode(gin.TestMode) + tests := []struct { + name string + serverToken string + requestToken string + authorization string + }{ + {name: "missing token", serverToken: "per-boot-token"}, + {name: "wrong token", serverToken: "per-boot-token", requestToken: "wrong"}, + {name: "bearer credential", serverToken: "per-boot-token", requestToken: "per-boot-token", authorization: "Bearer office-jwt"}, + {name: "empty server token"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + called := false + router := gin.New() + router.POST("/settings", InterimSettingsInterlock(tc.serverToken), func(c *gin.Context) { + called = true + c.Status(http.StatusNoContent) + }) + + request := httptest.NewRequest(http.MethodPost, "/settings", nil) + request.Header.Set(InterimSettingsInterlockHeader, tc.requestToken) + request.Header.Set("Authorization", tc.authorization) + response := httptest.NewRecorder() + router.ServeHTTP(response, request) + + if response.Code != http.StatusForbidden { + t.Fatalf("status = %d, want %d", response.Code, http.StatusForbidden) + } + if called { + t.Fatal("handler was called") + } + }) + } +} + +func TestInterimSettingsInterlockAllowsMatchingTokenWithoutBearerCredential(t *testing.T) { + gin.SetMode(gin.TestMode) + called := false + router := gin.New() + router.POST("/settings", InterimSettingsInterlock("per-boot-token"), func(c *gin.Context) { + called = true + c.Status(http.StatusNoContent) + }) + + request := httptest.NewRequest(http.MethodPost, "/settings", nil) + request.Header.Set(InterimSettingsInterlockHeader, "per-boot-token") + response := httptest.NewRecorder() + router.ServeHTTP(response, request) + + if response.Code != http.StatusNoContent { + t.Fatalf("status = %d, want %d", response.Code, http.StatusNoContent) + } + if !called { + t.Fatal("handler was not called") + } +} + +func TestNewInterimSettingsInterlockTokenIsRandomAndNonEmpty(t *testing.T) { + first, err := NewInterimSettingsInterlockToken() + if err != nil { + t.Fatalf("first token: %v", err) + } + second, err := NewInterimSettingsInterlockToken() + if err != nil { + t.Fatalf("second token: %v", err) + } + if first == "" || second == "" { + t.Fatal("token is empty") + } + if first == second { + t.Fatal("generated tokens match") + } +} diff --git a/apps/backend/internal/webapp/payload.go b/apps/backend/internal/webapp/payload.go index 1c713a75e7..99e6df405a 100644 --- a/apps/backend/internal/webapp/payload.go +++ b/apps/backend/internal/webapp/payload.go @@ -11,6 +11,9 @@ type BootPayload struct { InitialState map[string]any `json:"initialState"` RouteData map[string]any `json:"routeData,omitempty"` Errors []BootError `json:"errors,omitempty"` + // InterimSettingsInterlockToken is a replayable per-boot SPA CSRF and + // accidental-mutation interlock. It is not an authentication credential. + InterimSettingsInterlockToken string `json:"interimSettingsInterlockToken,omitempty"` // Plugins lists every active, UI-bundle-declaring plugin, per // docs/plans/plugins/PLUGIN-API.md ("Loading model"). Populated // only when the plugins feature flag is on; the flag itself is not diff --git a/apps/web/app/actions/agents.ts b/apps/web/app/actions/agents.ts index 596ae2cf83..56d3cb88a2 100644 --- a/apps/web/app/actions/agents.ts +++ b/apps/web/app/actions/agents.ts @@ -1,6 +1,8 @@ -"use server"; +"use client"; import { getBackendConfig } from "@/lib/config"; +import { fetchJson } from "@/lib/api/client"; +import { readInterimSettingsInterlockToken } from "@/src/boot-payload"; import type { Agent, AgentProfile, @@ -17,6 +19,7 @@ import { normalizeAgentProfile } from "@/lib/api/domains/agent-profile-normalize type ProfilePermissions = Record; const { apiBaseUrl } = getBackendConfig(); +const interimSettingsInterlockHeader = "X-Kandev-Interim-Settings-Interlock"; function normalizeAgentInPlace(agent: Agent): Agent { return { @@ -25,7 +28,7 @@ function normalizeAgentInPlace(agent: Agent): Agent { }; } -async function fetchJson(url: string, options?: RequestInit): Promise { +function agentSettingsRequest(url: string, init?: RequestInit): Promise { // Pin the URL origin to the configured backend so a tainted path segment // (agent ID from a form, profile ID from a route param) cannot redirect // the request to a different host. Closes the CodeQL SSRF finding. @@ -34,33 +37,15 @@ async function fetchJson(url: string, options?: RequestInit): Promise { if (parsed.origin !== allowed.origin) { throw new Error(`Refusing to fetch outside configured backend origin: ${parsed.origin}`); } - const response = await fetch(parsed.toString(), { - ...options, - cache: "no-store", - headers: { - "Content-Type": "application/json", - ...(options?.headers ?? {}), - }, - }); - if (!response.ok) { - throw new Error(`Request failed: ${response.status} ${response.statusText}`); - } - if (response.status === 204) { - return undefined as T; - } - const text = await response.text(); - if (!text) { - return undefined as T; - } - return JSON.parse(text) as T; + return fetchJson(parsed.toString(), { cache: "no-store", init }); } export async function listAgentDiscoveryAction(): Promise { - return fetchJson(`${apiBaseUrl}/api/v1/agents/discovery`); + return agentSettingsRequest(`${apiBaseUrl}/api/v1/agents/discovery`); } export async function listAgentsAction(): Promise { - const res = await fetchJson(`${apiBaseUrl}/api/v1/agents`); + const res = await agentSettingsRequest(`${apiBaseUrl}/api/v1/agents`); return { ...res, agents: (res.agents ?? []).map(normalizeAgentInPlace) }; } @@ -79,7 +64,7 @@ export async function createAgentAction(payload: { } & ProfilePermissions >; }): Promise { - const res = await fetchJson(`${apiBaseUrl}/api/v1/agents`, { + const res = await agentSettingsRequest(`${apiBaseUrl}/api/v1/agents`, { method: "POST", body: JSON.stringify(payload), }); @@ -94,7 +79,7 @@ export async function updateAgentAction( mcp_config_path?: string | null; }, ): Promise { - const res = await fetchJson(`${apiBaseUrl}/api/v1/agents/${id}`, { + const res = await agentSettingsRequest(`${apiBaseUrl}/api/v1/agents/${id}`, { method: "PATCH", body: JSON.stringify(payload), }); @@ -102,7 +87,7 @@ export async function updateAgentAction( } export async function deleteAgentAction(id: string) { - await fetchJson(`${apiBaseUrl}/api/v1/agents/${id}`, { method: "DELETE" }); + await agentSettingsRequest(`${apiBaseUrl}/api/v1/agents/${id}`, { method: "DELETE" }); } export async function createAgentProfileAction( @@ -118,10 +103,13 @@ export async function createAgentProfileAction( env_vars?: ProfileEnvVar[]; } & ProfilePermissions, ): Promise { - const raw = await fetchJson(`${apiBaseUrl}/api/v1/agents/${agentId}/profiles`, { - method: "POST", - body: JSON.stringify(payload), - }); + const raw = await agentSettingsRequest( + `${apiBaseUrl}/api/v1/agents/${agentId}/profiles`, + { + method: "POST", + body: JSON.stringify(payload), + }, + ); return normalizeAgentProfile(raw); } @@ -140,7 +128,7 @@ export async function updateAgentProfileAction( env_vars?: ProfileEnvVar[]; }, ): Promise { - const raw = await fetchJson(`${apiBaseUrl}/api/v1/agent-profiles/${id}`, { + const raw = await agentSettingsRequest(`${apiBaseUrl}/api/v1/agent-profiles/${id}`, { method: "PATCH", body: JSON.stringify(payload), }); @@ -168,10 +156,14 @@ export async function deleteAgentProfileAction( force?: boolean, ): Promise { const url = `${apiBaseUrl}/api/v1/agent-profiles/${id}${force ? "?force=true" : ""}`; + const token = readInterimSettingsInterlockToken(); const response = await fetch(url, { method: "DELETE", cache: "no-store", - headers: { "Content-Type": "application/json" }, + headers: { + "Content-Type": "application/json", + ...(token ? { [interimSettingsInterlockHeader]: token } : {}), + }, }); if (!response.ok) { const body = await response.json().catch(() => ({})); @@ -197,7 +189,7 @@ export async function deleteAgentProfileAction( export async function getAgentProfileMcpConfigAction( profileId: string, ): Promise { - return fetchJson( + return agentSettingsRequest( `${apiBaseUrl}/api/v1/agent-profiles/${profileId}/mcp-config`, ); } @@ -210,7 +202,7 @@ export async function updateAgentProfileMcpConfigAction( meta?: Record; }, ): Promise { - return fetchJson( + return agentSettingsRequest( `${apiBaseUrl}/api/v1/agent-profiles/${profileId}/mcp-config`, { method: "POST", @@ -237,7 +229,7 @@ export async function previewAgentCommandAction( agentName: string, payload: CommandPreviewRequest, ): Promise { - return fetchJson( + return agentSettingsRequest( `${apiBaseUrl}/api/v1/agent-command-preview/${agentName}`, { method: "POST", diff --git a/apps/web/app/settings/agents/[agentId]/profiles/[profileId]/command-preview-card.test.tsx b/apps/web/app/settings/agents/[agentId]/profiles/[profileId]/command-preview-card.test.tsx index 1f4acffbc4..43f9755e9a 100644 --- a/apps/web/app/settings/agents/[agentId]/profiles/[profileId]/command-preview-card.test.tsx +++ b/apps/web/app/settings/agents/[agentId]/profiles/[profileId]/command-preview-card.test.tsx @@ -1,5 +1,5 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { cleanup, render, screen, waitFor } from "@testing-library/react"; +import { act, cleanup, render, screen, waitFor } from "@testing-library/react"; import { CommandPreviewCard } from "./command-preview-card"; import type { CommandPreviewResponse } from "@/app/actions/agents"; @@ -60,10 +60,14 @@ describe("CommandPreviewCard", () => { await waitFor(() => expect(screen.getByText("agent --new")).toBeTruthy()); // The stale request finally resolves — it must not clobber the newer preview. - resolveStale(response("agent --stale")); - await new Promise((resolve) => setTimeout(resolve, 50)); + await act(async () => { + resolveStale(response("agent --stale")); + await stalePromise; + }); - expect(screen.getByText("agent --new")).toBeTruthy(); - expect(screen.queryByText("agent --stale")).not.toBeTruthy(); + await waitFor(() => { + expect(screen.getByText("agent --new")).toBeTruthy(); + expect(screen.queryByText("agent --stale")).not.toBeTruthy(); + }); }); }); diff --git a/apps/web/e2e/helpers/api-client.ts b/apps/web/e2e/helpers/api-client.ts index 3af3f657c3..47f14dd8ca 100644 --- a/apps/web/e2e/helpers/api-client.ts +++ b/apps/web/e2e/helpers/api-client.ts @@ -9,6 +9,7 @@ import type { MCPTaskAgentProfileDefault, } from "../../lib/types/http"; import type { Agent, AgentProfile } from "../../lib/types/http-agents"; +import { normalizeAgentProfile } from "../../lib/api/domains/agent-profile-normalize"; import type { TaskCIAutomationOptions, TaskCIAutomationPatch } from "../../lib/types/github"; import type { VoiceModeSettings } from "../../lib/types/http-voice"; import type { @@ -29,6 +30,7 @@ import type { SSHTestRequest, SSHTestResult, } from "../../lib/types/http-ssh"; +import { loadInterimSettingsInterlockToken } from "./interim-settings-interlock"; // --- GitHub Mock Types --- @@ -227,7 +229,7 @@ export class ApiClient { async rawRequest(method: string, path: string, body?: unknown): Promise { return fetch(`${this.baseUrl}${path}`, { method, - headers: body ? { "Content-Type": "application/json" } : undefined, + headers: await this.requestHeaders(method, body), body: body ? JSON.stringify(body) : undefined, }); } @@ -235,7 +237,7 @@ export class ApiClient { private async request(method: string, path: string, body?: unknown): Promise { const res = await fetch(`${this.baseUrl}${path}`, { method, - headers: body ? { "Content-Type": "application/json" } : undefined, + headers: await this.requestHeaders(method, body), body: body ? JSON.stringify(body) : undefined, }); if (!res.ok) { @@ -245,6 +247,19 @@ export class ApiClient { return res.json() as Promise; } + private async requestHeaders( + method: string, + body?: unknown, + ): Promise | undefined> { + const headers: Record = body ? { "Content-Type": "application/json" } : {}; + if (["POST", "PUT", "PATCH", "DELETE"].includes(method.toUpperCase())) { + headers["X-Kandev-Interim-Settings-Interlock"] = await loadInterimSettingsInterlockToken( + this.baseUrl, + ); + } + return Object.keys(headers).length > 0 ? headers : undefined; + } + private async activeWorkspaceId(): Promise { const { settings } = await this.getUserSettings(); const workspaceId = settings.workspace_id; @@ -384,7 +399,17 @@ export class ApiClient { } async listAgents(): Promise<{ agents: Agent[]; total: number }> { - return this.request("GET", "/api/v1/agents"); + const response = await this.request<{ agents: Agent[]; total: number }>( + "GET", + "/api/v1/agents", + ); + return { + ...response, + agents: response.agents.map((agent) => ({ + ...agent, + profiles: (agent.profiles ?? []).map(normalizeAgentProfile), + })), + }; } async deleteAgentProfile(profileId: string, force?: boolean): Promise { @@ -426,11 +451,8 @@ export class ApiClient { command_prefix?: string; env_vars?: Array<{ key: string; value?: string; secret_id?: string }>; }, - ): Promise<{ - id: string; - cli_flags: Array<{ description: string; flag: string; enabled: boolean }>; - }> { - return this.request("POST", `/api/v1/agents/${agentId}/profiles`, { + ): Promise { + const response = await this.request("POST", `/api/v1/agents/${agentId}/profiles`, { name, model: opts.model, mode: opts.mode, @@ -440,6 +462,7 @@ export class ApiClient { command_prefix: opts.command_prefix, env_vars: opts.env_vars, }); + return normalizeAgentProfile(response); } async getAgentProfile(profileId: string): Promise { diff --git a/apps/web/e2e/helpers/interim-settings-interlock.ts b/apps/web/e2e/helpers/interim-settings-interlock.ts new file mode 100644 index 0000000000..e5bc57f00a --- /dev/null +++ b/apps/web/e2e/helpers/interim-settings-interlock.ts @@ -0,0 +1,16 @@ +const appStatePath = "/api/v1/app-state?path=%2Fsettings%2Fagents"; + +export async function loadInterimSettingsInterlockToken(baseUrl: string): Promise { + const response = await fetch(`${baseUrl}${appStatePath}`); + if (!response.ok) { + throw new Error(`Unable to load E2E settings interlock (${response.status})`); + } + const payload = (await response.json()) as { interimSettingsInterlockToken?: unknown }; + if ( + typeof payload.interimSettingsInterlockToken !== "string" || + !payload.interimSettingsInterlockToken + ) { + throw new Error("E2E settings interlock token missing from boot payload"); + } + return payload.interimSettingsInterlockToken; +} diff --git a/apps/web/e2e/tests/settings/agent-install-streaming.spec.ts b/apps/web/e2e/tests/settings/agent-install-streaming.spec.ts index 5c20925ed5..ae13992410 100644 --- a/apps/web/e2e/tests/settings/agent-install-streaming.spec.ts +++ b/apps/web/e2e/tests/settings/agent-install-streaming.spec.ts @@ -12,6 +12,7 @@ import { test, expect } from "../../fixtures/test-base"; */ test.describe("agent install streaming", () => { test("POST returns a job_id, WS streams started+output+finished, GET reflects success", async ({ + apiClient, backend, }) => { test.setTimeout(15_000); @@ -29,9 +30,7 @@ test.describe("agent install streaming", () => { } }); - const enqueueRes = await fetch(`${backend.baseUrl}/api/v1/agent-install/mock-agent`, { - method: "POST", - }); + const enqueueRes = await apiClient.rawRequest("POST", "/api/v1/agent-install/mock-agent"); expect(enqueueRes.status).toBe(202); const job = (await enqueueRes.json()) as { job_id: string; status: string }; expect(job.job_id).toBeTruthy(); @@ -67,7 +66,10 @@ test.describe("agent install streaming", () => { expect(snap.status).toBe("succeeded"); }); - test("POST while a job is running returns the same job_id (idempotent)", async ({ backend }) => { + test("POST while a job is running returns the same job_id (idempotent)", async ({ + apiClient, + backend, + }) => { test.setTimeout(15_000); // Note: mock-agent's script is fast (~10ms), so the second POST usually @@ -75,11 +77,11 @@ test.describe("agent install streaming", () => { // same job_id (still running) OR different job_id with first already in // retention. The contract under test is: no duplicate concurrent runs. const first = (await ( - await fetch(`${backend.baseUrl}/api/v1/agent-install/mock-agent`, { method: "POST" }) + await apiClient.rawRequest("POST", "/api/v1/agent-install/mock-agent") ).json()) as { job_id: string }; const second = (await ( - await fetch(`${backend.baseUrl}/api/v1/agent-install/mock-agent`, { method: "POST" }) + await apiClient.rawRequest("POST", "/api/v1/agent-install/mock-agent") ).json()) as { job_id: string }; // If both raced before finish they must share IDs; if first already @@ -92,13 +94,14 @@ test.describe("agent install streaming", () => { } }); - test("GET /agent-install/jobs lists recent jobs for rehydration", async ({ backend }) => { + test("GET /agent-install/jobs lists recent jobs for rehydration", async ({ + apiClient, + backend, + }) => { test.setTimeout(15_000); // Trigger an install so there's at least one job in retention. - const enqueueRes = await fetch(`${backend.baseUrl}/api/v1/agent-install/mock-agent`, { - method: "POST", - }); + const enqueueRes = await apiClient.rawRequest("POST", "/api/v1/agent-install/mock-agent"); const job = (await enqueueRes.json()) as { job_id: string }; // Wait for it to finish (mock script is fast). @@ -128,6 +131,7 @@ test.describe("agent install streaming", () => { }); test("after install probe completes, agent.available.updated is broadcast", async ({ + apiClient, backend, }) => { test.setTimeout(20_000); @@ -145,7 +149,7 @@ test.describe("agent install streaming", () => { } }); - await fetch(`${backend.baseUrl}/api/v1/agent-install/mock-agent`, { method: "POST" }); + await apiClient.rawRequest("POST", "/api/v1/agent-install/mock-agent"); // The controller fires a `Refresh` after install and then broadcasts // `agent.available.updated`. The mock-agent's host-utility probe may diff --git a/apps/web/e2e/tests/settings/agent-profile-cli-flags.spec.ts b/apps/web/e2e/tests/settings/agent-profile-cli-flags.spec.ts index a966f003ce..4e7bf9c292 100644 --- a/apps/web/e2e/tests/settings/agent-profile-cli-flags.spec.ts +++ b/apps/web/e2e/tests/settings/agent-profile-cli-flags.spec.ts @@ -76,10 +76,10 @@ test.describe("Agent profile — CLI flags", () => { ); // Direct DB-path verification: fetch the profile via the API and assert - // the cli_flags JSON contains our entry, enabled. + // the normalized CLI flags contain our entry, enabled. const stored = await apiClient.getAgentProfile(profile.id); - const found = stored.cli_flags.find((f) => f.flag === flagText); - expect(found, `cli_flags should include ${flagText}`).toBeDefined(); + const found = stored.cliFlags.find((f) => f.flag === flagText); + expect(found, `cliFlags should include ${flagText}`).toBeDefined(); expect(found?.enabled).toBe(true); } finally { await apiClient.deleteAgentProfile(profile.id, true); @@ -123,7 +123,7 @@ test.describe("Agent profile — CLI flags", () => { ); const stored = await apiClient.getAgentProfile(profile.id); - const found = stored.cli_flags.find((f) => f.flag === "--toggle-me"); + const found = stored.cliFlags.find((f) => f.flag === "--toggle-me"); expect(found?.enabled).toBe(false); } finally { await apiClient.deleteAgentProfile(profile.id, true); @@ -166,7 +166,7 @@ test.describe("Agent profile — CLI flags", () => { await expect(testPage.getByTestId("cli-flag-row-1")).toBeHidden(); const stored = await apiClient.getAgentProfile(profile.id); - expect(stored.cli_flags.map((f) => f.flag)).toEqual(["--keep-me"]); + expect(stored.cliFlags.map((f) => f.flag)).toEqual(["--keep-me"]); } finally { await apiClient.deleteAgentProfile(profile.id, true); } diff --git a/apps/web/e2e/tests/settings/mobile-agent-profile-config-selector.spec.ts b/apps/web/e2e/tests/settings/mobile-agent-profile-config-selector.spec.ts index ae4c7f4015..1a3ba3d2a5 100644 --- a/apps/web/e2e/tests/settings/mobile-agent-profile-config-selector.spec.ts +++ b/apps/web/e2e/tests/settings/mobile-agent-profile-config-selector.spec.ts @@ -76,10 +76,9 @@ test.describe("Mobile agent profile config selector", () => { timeout: 15_000, }); - // Direct DB-path verification: fetch the profile and assert the - // persisted command_prefix. + // API-path verification confirms the normalized persisted value. const stored = await apiClient.getAgentProfile(profile.id); - expect((stored as unknown as { command_prefix?: string }).command_prefix).toBe("greywall --"); + expect(stored.commandPrefix).toBe("greywall --"); // Clearing a previously-saved prefix must persist an empty value. await testPage.getByTestId("command-prefix-input").fill(""); @@ -95,9 +94,7 @@ test.describe("Mobile agent profile config selector", () => { timeout: 15_000, }); const storedAfterClear = await apiClient.getAgentProfile(profile.id); - expect( - (storedAfterClear as unknown as { command_prefix?: string }).command_prefix ?? "", - ).toBe(""); + expect(storedAfterClear.commandPrefix).toBeUndefined(); } finally { await apiClient.deleteAgentProfile(profile.id, true); } diff --git a/apps/web/e2e/tests/task/create-task.spec.ts b/apps/web/e2e/tests/task/create-task.spec.ts index f0a3132f65..40c814b8e5 100644 --- a/apps/web/e2e/tests/task/create-task.spec.ts +++ b/apps/web/e2e/tests/task/create-task.spec.ts @@ -111,7 +111,7 @@ test.describe("Task creation", () => { auth_specs: [ { id: agent.name, - display_name: profile.agent_display_name, + display_name: profile.agentDisplayName, methods: [{ method_id: "test-token", type: "env", env_var: "TEST_TOKEN" }], }, ], diff --git a/apps/web/e2e/tests/workflow/workflow-agent-profile.spec.ts b/apps/web/e2e/tests/workflow/workflow-agent-profile.spec.ts index 08f0c23309..a226103ab4 100644 --- a/apps/web/e2e/tests/workflow/workflow-agent-profile.spec.ts +++ b/apps/web/e2e/tests/workflow/workflow-agent-profile.spec.ts @@ -27,7 +27,7 @@ test.describe("Workflow agent profile", () => { const { agents } = await apiClient.listAgents(); const agentProfile = agents.flatMap((a) => a.profiles ?? [])[0]; expect(agentProfile).toBeDefined(); - const profileLabel = `${agentProfile.agent_display_name} \u2022 ${agentProfile.name}`; + const profileLabel = `${agentProfile.agentDisplayName} \u2022 ${agentProfile.name}`; // Open the select and pick the agent profile await profileSelect.click(); @@ -74,7 +74,7 @@ test.describe("Workflow agent profile", () => { const { agents } = await apiClient.listAgents(); const agentProfile = agents.flatMap((a) => a.profiles ?? [])[0]; expect(agentProfile).toBeDefined(); - const profileLabel = `${agentProfile.agent_display_name} \u2022 ${agentProfile.name}`; + const profileLabel = `${agentProfile.agentDisplayName} \u2022 ${agentProfile.name}`; // Select an agent profile for this step await testPage.addStyleTag({ diff --git a/apps/web/lib/api/client.test.ts b/apps/web/lib/api/client.test.ts new file mode 100644 index 0000000000..ce808ed668 --- /dev/null +++ b/apps/web/lib/api/client.test.ts @@ -0,0 +1,65 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { fetchJson } from "./client"; + +const interlockToken = "replayable-per-boot-value"; + +describe("fetchJson", () => { + afterEach(() => { + vi.unstubAllGlobals(); + delete (window as unknown as { __KANDEV_BOOT_PAYLOAD__?: unknown }).__KANDEV_BOOT_PAYLOAD__; + }); + + it("attaches the replayable interim settings interlock to mutations", async () => { + (window as unknown as { __KANDEV_BOOT_PAYLOAD__?: unknown }).__KANDEV_BOOT_PAYLOAD__ = { + interimSettingsInterlockToken: interlockToken, + }; + const fetcher = vi.fn().mockResolvedValue(new Response("{}", { status: 200 })); + vi.stubGlobal("fetch", fetcher); + + await fetchJson("/api/v1/agents", { + baseUrl: "http://backend.test", + init: { method: "POST", body: "{}" }, + }); + + expect(fetcher).toHaveBeenCalledWith("http://backend.test/api/v1/agents", expect.any(Object)); + expect( + new Headers(fetcher.mock.calls[0][1]?.headers).get("X-Kandev-Interim-Settings-Interlock"), + ).toBe(interlockToken); + }); + + it("replaces a lowercase content type without losing mutation headers", async () => { + (window as unknown as { __KANDEV_BOOT_PAYLOAD__?: unknown }).__KANDEV_BOOT_PAYLOAD__ = { + interimSettingsInterlockToken: interlockToken, + }; + const fetcher = vi.fn().mockResolvedValue(new Response("{}", { status: 200 })); + vi.stubGlobal("fetch", fetcher); + + await fetchJson("/api/v1/agents", { + baseUrl: "http://backend.test", + init: { + method: "POST", + body: "{}", + headers: { "content-type": "text/plain", "X-Caller-Header": "preserved" }, + }, + }); + + const headers = fetcher.mock.calls[0][1]?.headers; + const normalizedHeaders = new Headers(headers); + + expect({ + isHeaders: headers instanceof Headers, + contentType: normalizedHeaders.get("content-type"), + contentTypeCount: [...normalizedHeaders.entries()].filter( + ([name]) => name.toLowerCase() === "content-type", + ).length, + callerHeader: normalizedHeaders.get("X-Caller-Header"), + interlock: normalizedHeaders.get("X-Kandev-Interim-Settings-Interlock"), + }).toEqual({ + isHeaders: true, + contentType: "application/json", + contentTypeCount: 1, + callerHeader: "preserved", + interlock: interlockToken, + }); + }); +}); diff --git a/apps/web/lib/api/client.ts b/apps/web/lib/api/client.ts index 68a9569541..6954657d20 100644 --- a/apps/web/lib/api/client.ts +++ b/apps/web/lib/api/client.ts @@ -1,4 +1,7 @@ import { getBackendConfig } from "@/lib/config"; +import { readInterimSettingsInterlockToken } from "@/src/boot-payload"; + +const interimSettingsInterlockHeader = "X-Kandev-Interim-Settings-Interlock"; export type ApiRequestOptions = { baseUrl?: string; @@ -47,13 +50,16 @@ async function throwFromResponse(response: Response): Promise { export async function fetchJson(pathOrUrl: string, options?: ApiRequestOptions): Promise { const baseUrl = options?.baseUrl ?? getBackendConfig().apiBaseUrl; const url = resolveUrl(pathOrUrl, baseUrl); + const headers = requestHeaders(options?.init?.headers); + headers.set("Content-Type", "application/json"); + if (isMutation(options?.init?.method)) { + const token = readInterimSettingsInterlockToken(); + if (token) headers.set(interimSettingsInterlockHeader, token); + } const response = await fetch(url, { ...options?.init, cache: options?.cache, - headers: { - "Content-Type": "application/json", - ...(options?.init?.headers ?? {}), - }, + headers, }); if (!response.ok) await throwFromResponse(response); if (response.status === 204) return undefined as T; @@ -62,6 +68,14 @@ export async function fetchJson(pathOrUrl: string, options?: ApiRequestOption return JSON.parse(text) as T; } +function isMutation(method: string | undefined): boolean { + return ["POST", "PUT", "PATCH", "DELETE"].includes(method?.toUpperCase() ?? "GET"); +} + +function requestHeaders(headers: HeadersInit | undefined): Headers { + return new Headers(headers); +} + /** * Wraps fetchJson with up to `maxAttempts` retries (default 3) and * exponential backoff starting at `baseDelayMs` (default 100 ms). diff --git a/apps/web/lib/api/domains/plugins-api.test.ts b/apps/web/lib/api/domains/plugins-api.test.ts index ff63882f9a..c1fc72f8ed 100644 --- a/apps/web/lib/api/domains/plugins-api.test.ts +++ b/apps/web/lib/api/domains/plugins-api.test.ts @@ -118,7 +118,7 @@ describe("installPluginFromUrl", () => { expect(JSON.parse(String(init?.body))).toEqual({ url: "https://example.test/acme-tools-1.0.0.tar.gz", }); - expect((init?.headers as Record)?.["Content-Type"]).toBe("application/json"); + expect(new Headers(init?.headers).get("Content-Type")).toBe("application/json"); expect(result.plugin.id).toBe(PLUGIN_ID); expect(result.plugin.status).toBe("active"); expect(result.warning).toBeUndefined(); diff --git a/apps/web/lib/api/e2e-api-client.test.ts b/apps/web/lib/api/e2e-api-client.test.ts new file mode 100644 index 0000000000..8f77c2d3b2 --- /dev/null +++ b/apps/web/lib/api/e2e-api-client.test.ts @@ -0,0 +1,27 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { loadInterimSettingsInterlockToken } from "../../e2e/helpers/interim-settings-interlock"; + +describe("E2E interim settings interlock", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("loads the current boot token after boot rotation", async () => { + let bootToken = "first-boot-token"; + vi.stubGlobal( + "fetch", + vi.fn(async (url: string | URL) => { + if (String(url).endsWith("/api/v1/app-state?path=%2Fsettings%2Fagents")) { + return Response.json({ interimSettingsInterlockToken: bootToken }); + } + }), + ); + + const firstToken = await loadInterimSettingsInterlockToken("http://backend.test"); + bootToken = "second-boot-token"; + const secondToken = await loadInterimSettingsInterlockToken("http://backend.test"); + + expect([firstToken, secondToken]).toEqual(["first-boot-token", "second-boot-token"]); + }); +}); diff --git a/apps/web/src/boot-payload.test.ts b/apps/web/src/boot-payload.test.ts index e8677a3f5e..013f318edd 100644 --- a/apps/web/src/boot-payload.test.ts +++ b/apps/web/src/boot-payload.test.ts @@ -27,6 +27,7 @@ describe("readBootPayload", () => { initialState: { tasks: { activeTaskId: "task-1" }, }, + interimSettingsInterlockToken: "replayable-per-boot-value", }, } as unknown as Window; @@ -45,6 +46,7 @@ describe("readBootPayload", () => { initialState: { tasks: { activeTaskId: "task-1" }, }, + interimSettingsInterlockToken: "replayable-per-boot-value", }); }); diff --git a/apps/web/src/boot-payload.ts b/apps/web/src/boot-payload.ts index 2b43dafe55..d7327da7ec 100644 --- a/apps/web/src/boot-payload.ts +++ b/apps/web/src/boot-payload.ts @@ -46,6 +46,8 @@ export type BootPayload = { initialState?: Partial; routeData?: BootRouteData; plugins?: ActivePlugin[]; + /** Replayable per-boot CSRF/accidental-mutation interlock; not authentication. */ + interimSettingsInterlockToken?: string; }; type BootWindow = Window & { @@ -68,9 +70,15 @@ export function readBootPayload(win: Window = window): BootPayload { initialState: isRecord(payload.initialState) ? (payload.initialState as Partial) : {}, routeData: isRecord(payload.routeData) ? (payload.routeData as BootRouteData) : undefined, plugins: Array.isArray(payload.plugins) ? readPlugins(payload.plugins) : undefined, + interimSettingsInterlockToken: readNonEmptyString(payload.interimSettingsInterlockToken), }; } +export function readInterimSettingsInterlockToken(): string | undefined { + if (typeof window === "undefined") return undefined; + return readBootPayload(window).interimSettingsInterlockToken; +} + function readPlugins(value: unknown[]): ActivePlugin[] { return value.filter(isRecord).flatMap((entry) => { const plugin = readPlugin(entry); @@ -133,6 +141,11 @@ function readString(value: unknown): string | undefined { return typeof value === "string" ? value : undefined; } +function readNonEmptyString(value: unknown): string | undefined { + const result = readString(value); + return result ? result : undefined; +} + function isRecord(value: unknown): value is Record { return Boolean(value) && typeof value === "object" && !Array.isArray(value); } diff --git a/docs/decisions/2026-07-24-operator-owned-agent-launcher-settings.md b/docs/decisions/2026-07-24-operator-owned-agent-launcher-settings.md new file mode 100644 index 0000000000..9312a3c560 --- /dev/null +++ b/docs/decisions/2026-07-24-operator-owned-agent-launcher-settings.md @@ -0,0 +1,157 @@ +# ADR-2026-07-24-operator-owned-agent-launcher-settings: Operator-Owned Agent Launcher Settings + +**Status:** accepted +**Date:** 2026-07-24 +**Area:** backend, frontend, protocol + +## Context + +Agent execution profiles contain inputs that affect the process before an ACP +agent is confined, including the launcher command prefix and the environment +inherited by that launcher. Office agents receive a runtime JWT and the backend +API URL, but the general agent-profile mutation routes currently require no +operator authentication; omitting the JWT is treated as an administrative UI +request. Kandev also has no application-level operator session or secure +browser bootstrap: the current web UI, HTTP API, and WebSocket are intentionally +unauthenticated, while the desktop health token and agentctl tokens authenticate +different process boundaries. + +A per-boot value exposed through the SPA shell or `/api/v1/app-state` can +mitigate browser CSRF, but it is not operator authentication. Any agent or +direct client that can reach the backend can read and replay that value. This +limitation is also documented by the unfinished plugin boot-token work in +[PR #1808](https://github.com/kdlbs/kandev/pull/1808). + +## Decision + +Launcher-affecting definitions and agent-profile settings are operator +control-plane data. Agent processes and Office runtime JWTs are data-plane +principals and MUST NOT authorize their mutation. The protected resources and +required authority are: + +| Resource | Examples | Required authority | +| --- | --- | --- | +| Install-wide agent definitions | custom TUI commands in `agents.tui_config`, agent creation/deletion, registry-backed executable selection | install operator | +| Global execution profiles | `agent_profiles` rows with an empty `workspace_id`, including nested MCP configuration | install operator | +| Workspace execution profiles | `agent_profiles` rows with a non-empty `workspace_id`, including nested MCP configuration | operator authorized for that workspace | + +Workspace operators cannot mutate shared global definitions or profiles. +Creating a resource, deleting it, replacing it, or moving it between ownership +scopes requires the authority for both its old and new scope. + +Within those resources, the boundary includes at least: + +- launcher executable and command-prefix argv; +- CLI arguments and mode/configuration that select the launched program's + behavior; +- profile environment values or secret references inherited by the launcher; +- profile creation, deletion, and any replacement operation that can select + different launcher inputs. + +These mutations require a mandatory authenticated operator principal. An +absent credential is unauthenticated, not an implicit administrator. An Office +agent JWT is rejected even when its role is `ceo` or otherwise administrative +inside the Office data plane. Authorization also validates that the operator +can mutate the target workspace/profile when Kandev supports more than one +operator or tenant. + +Full-detail reads of the protected resources require the same operator +authority when they include literal environment values, resolved secret +material, MCP server environment, or MCP headers. Agent/data-plane callers may +receive a separate redacted catalog DTO containing identifiers, names, +compatibility/capability metadata, and secret-reference presence, but never the +literal values used by another execution profile. + +The operator credential and its bootstrap material MUST NOT be placed in agent +process environments, workspaces, executor metadata, logs, Office runtime +responses, or an unauthenticated SPA/boot payload. A same-origin boot token may +still be used as a CSRF control, but it never satisfies this authentication +requirement. Deployments may satisfy the principal requirement with native +Kandev application authentication or with a trusted external identity proxy, +provided direct backend access is blocked and Kandev verifies a non-forgeable +proxy assertion. + +Ambient browser credentials are safe only after agent-controlled and +workspace-controlled web content is isolated from the operator origin. Preview +ports, task applications, plugin content, and other untrusted scripts MUST use +a separate origin that cannot read operator boot/session state or send an +operator credential to the control-plane origin. Cookie path scoping, +iframe sandbox flags combined with `allow-same-origin`, and CSRF headers do not +replace this origin boundary. + +Until that operator-authentication surface exists, command prefixes are a +launch customization feature, not a security boundary. A partial profile-route +guard based on optional Office JWT middleware, `Origin`/CORS, client IP, +user-agent headers, or a secret returned by an unauthenticated endpoint MUST +NOT be described or tested as operator authentication. + +As an interim risk reduction, state-changing agent/settings routes may require +a random per-boot token exposed to the same-origin SPA and reject requests that +carry an Office bearer token. This interlock reduces accidental agent writes, +straightforward use of the injected runtime JWT, and cross-origin request +forgery. It does not stop an intentional agent or direct client from fetching +the unauthenticated boot payload and replaying the token, and therefore does not +satisfy the operator principal, secret-read, workspace-authorization, or +untrusted-origin requirements above. Tests and public documentation for the +interlock must preserve that distinction. + +Execution-critical commands cross lifecycle and agentctl boundaries as +structured argv. The additive configure fields are `agent_args []string` and +`continue_args []string`; the existing `command` and `continue_command` fields +remain display/legacy compatibility strings. During migration, senders +dual-write both representations. Receivers prefer structured argv whenever its +field is present, reject present-but-empty or otherwise invalid argv without +falling back, and parse the legacy string only when the corresponding argv +field is absent. The same precedence applies to initial launch, +continue/one-shot execution, restart/context reset, and recovered executions. +Human-readable command strings are never reparsed when structured argv is +available. Legacy fields may be removed only after all supported lifecycle and +agentctl version combinations send and consume structured argv. + +## Consequences + +- Correctly hardening agent-profile mutation depends on a broader + application-authentication and browser-session design. That work must cover + native launcher, desktop WebView, ordinary browser, headless service, and + mobile access without putting the credential in agent-visible surfaces. It + must also move untrusted preview/content rendering off the operator origin. +- The post-merge launcher hardening cannot claim a security boundary until + that dependency lands. Structured argv, restart ordering, and rollback tests + remain valid follow-up work, but shipping them alone does not satisfy the + launcher/settings acceptance criteria. +- The interim boot-token interlock is intentionally compatible with later + operator authentication: it remains a CSRF layer or is removed once the + authenticated session supplies a stronger principal. +- Existing settings clients will need an authenticated mutation transport and + explicit handling for unauthenticated/forbidden responses. Read-only + agent-facing discovery requires a dedicated redacted DTO; existing + full-detail profile and MCP responses remain operator-only. +- Profile environment values may continue to be applied to the launcher only + after operator-only mutation is enforced. If agents later gain a supported + way to propose child environment values, those values require a separate + post-confinement channel or an explicit sanitization contract. +- External-proxy deployments must prevent bypassing the proxy by connecting + directly to the backend. + +## Alternatives Considered + +1. **Apply the current optional `AgentAuthMiddleware`.** Rejected because a + caller can omit its bearer token and is then treated as an administrative + UI request. +2. **Expose a random per-boot token in the SPA shell or boot payload.** Rejected + as authentication because an agent can fetch the same unauthenticated + resource and replay the token. Accepted only as the explicitly limited + interim interlock and future CSRF protection described above. +3. **Protect only `command_prefix`.** Rejected because profile environment + values can influence the wrapper through `LD_PRELOAD`, `PATH`, + wrapper-specific variables, and interpreter resolution before confinement. +4. **Rely on CORS, `Origin`, loopback binding, source IP, or browser headers.** + Rejected because non-browser agent clients can omit or forge those signals, + and local executors may share the host network namespace. +5. **Move launcher policy to another unauthenticated HTTP route or table.** + Rejected because changing storage ownership without authenticating the + mutation principal preserves the same confused-deputy boundary. +6. **Use an ambient operator cookie while serving previews on the backend + origin.** Rejected because agent-controlled same-origin scripts can exercise + the credential and read CSRF/bootstrap material. Authentication and + untrusted-content origin isolation must land together. diff --git a/docs/decisions/INDEX.md b/docs/decisions/INDEX.md index a50672cdd2..26da771c58 100644 --- a/docs/decisions/INDEX.md +++ b/docs/decisions/INDEX.md @@ -59,6 +59,7 @@ Read individual ADRs for full context. Create new ones via `/record decision` or | 2026-07-23-opencode-review-evidence-trust | [Trusted OpenCode Review Evidence](2026-07-23-opencode-review-evidence-trust.md) | accepted | workflow, infra | 2026-07-23 | | 2026-07-23-planner-direct-small-work | [Planner Direct Small Work](2026-07-23-planner-direct-small-work.md) | accepted | workflow | 2026-07-23 | | 2026-07-23-post-commit-hook-aware-verification | [Post-Commit Hook-Aware Verification](2026-07-23-post-commit-hook-aware-verification.md) | accepted | workflow | 2026-07-23 | +| 2026-07-24-operator-owned-agent-launcher-settings | [Operator-Owned Agent Launcher Settings](2026-07-24-operator-owned-agent-launcher-settings.md) | accepted | backend, frontend, protocol | 2026-07-24 | | 2026-07-18-turn-configuration-snapshots | [Attribute runtime configuration to turns](2026-07-18-turn-configuration-snapshots.md) | accepted | backend, frontend | 2026-07-18 | | 2026-07-22-gpg-signed-release-tags | [GPG-signed release tags](2026-07-22-gpg-signed-release-tags.md) | accepted | infra, workflow | 2026-07-22 | | 2026-07-19-reject-mcp-actions-on-raw-websocket | [Reject MCP Actions on the Raw WebSocket](2026-07-19-reject-mcp-actions-on-raw-websocket.md) | accepted | backend, protocol | 2026-07-19 | diff --git a/docs/plans/agent-launcher-hardening-followups/plan.md b/docs/plans/agent-launcher-hardening-followups/plan.md new file mode 100644 index 0000000000..979282569f --- /dev/null +++ b/docs/plans/agent-launcher-hardening-followups/plan.md @@ -0,0 +1,140 @@ +--- +spec: docs/specs/office/agents.md +created: 2026-07-24 +status: done +--- + +# Implementation Plan: Agent Launcher Hardening Follow-ups + +## Overview + +Deliver an explicitly limited per-boot settings interlock while preserving the +long-term operator boundary in +`docs/decisions/2026-07-24-operator-owned-agent-launcher-settings.md`. In +parallel, remove lossy command-string execution, then make restart and launch +rollback ordering fail safely. Finish with the verified review cleanups, +mobile settings coverage, and public documentation that accurately distinguishes +risk reduction from authentication. + +## Backend + +### Interim settings interlock + +- Add a process-random boot token and constant-time validation middleware under + `apps/backend/internal/common/httpmw/`. +- Generate the token once in `apps/backend/internal/backendapp/main.go`, pass it + through `routeParams`, and expose it only as the existing unauthenticated SPA + runtime payload. +- Apply the interlock to state-changing agent/settings routes in + `apps/backend/internal/agent/settings/handlers/handlers.go`, including agent + creation/update/delete, custom TUI creation, profile creation/update/delete, + MCP configuration mutation, and agent installation. +- Reject requests carrying a bearer credential on those routes so the injected + Office runtime JWT is never accepted as the UI interlock. Missing/wrong boot + tokens fail closed. + +### Structured command transport + +- Extend lifecycle execution state and the lifecycle-to-agentctl configure + request with `agent_args` and `continue_args`. +- Dual-write structured argv and legacy display strings. Agentctl prefers a + present structured field, rejects present-but-invalid argv, and uses + `strings.Fields` only when the structured field is absent. +- Preserve argv through initial launch, continue/one-shot execution, + restart/context reset, standalone, Docker/remote configuration, and recovery. +- Validate argv through the shared executable-token rules: non-empty argv and a + non-empty, non-flag-like `argv[0]`. + +### Restart and rollback safety + +- Build and validate replacement initial/continue argv before closing streams, + stopping the existing process, changing execution state, or configuring + agentctl. +- When execution construction fails after an executor instance exists, stop the + instance exactly once and never register/configure/start the failed execution. + +## Frontend + +### Interlock transport + +- Add the boot token to the typed runtime payload in + `apps/web/src/boot-payload.ts`. +- Centralize the mutation header builder in the web API layer and apply it to + every state-changing agent/profile action, preserving existing desktop and + mobile save flows without layout changes. + +### Review cleanups + +- Replace the fixed timeout in `command-preview-card.test.tsx` with controlled + promises and `waitFor`. +- Normalize E2E `AgentProfile.commandPrefix` typing and remove wire-shape casts + from the mobile settings test. + +Mobile design contract: the existing mobile agent-profile route, fields, save +action, scroll owner, and touch targets remain unchanged. Only the request +transport and test typing change, so the existing +`mobile-agent-profile-config-selector.spec.ts` remains the mobile parity proof. + +## Tests + +- Interlock middleware: missing, wrong, valid, empty-server-token, and bearer + rejection. +- Settings routes: Office bearer denied; omitted token denied; valid UI token + succeeds; guarded operations make zero writes on rejection. +- Structured argv: spaces, quotes, empty non-executable arguments, + Windows/backslash paths, prefix arguments, CLI flag values, initial, + continue, reset, JSON compatibility, and legacy absence fallback. +- Restart: resolver failure and malformed persisted prefix cause no + Stop/Configure/Start call and no state mutation. +- Launch rollback: instance is stopped once, absent from execution store, and + never configured/started after construction failure. +- Preview validation: empty/flag-like prefix executable rejected using the same + semantics as save/launch. +- Profile creation validation: bad prefixes produce zero store/profile writes. + +## E2E Tests + +- Rebuild backend and Vite production artifacts. +- Run the existing mobile agent-profile settings test through create, set + prefix, save, clear, and save again. The browser must carry the boot token + automatically; direct E2E API setup helpers receive the token through their + controlled test bootstrap where mutations are required. + +## Implementation Waves + +Wave 1 (parallel): + +- [x] [task-01-interim-settings-interlock](task-01-interim-settings-interlock.md) +- [x] [task-02-structured-argv-transport](task-02-structured-argv-transport.md) + +Wave 2: + +- [x] [task-03-restart-and-launch-rollback](task-03-restart-and-launch-rollback.md) + +Wave 3: + +- [x] [task-04-review-cleanups-e2e-docs](task-04-review-cleanups-e2e-docs.md) + +## Risks + +- The boot token is deliberately replayable by an intentional agent and must + never be described as operator authentication. +- Structured-field presence must be distinguishable from an absent JSON field; + a present invalid slice must fail instead of silently falling back. +- Lifecycle task ownership overlaps between tasks 02 and 03, so they run in + separate waves. +- E2E helpers that mutate settings must use the same guarded transport without + weakening production middleware. + +## Verification + +Targeted commands are owned by each task. After all tasks: + +```bash +make fmt +make typecheck test lint +cd apps/web && pnpm e2e:run tests/settings/mobile-agent-profile-config-selector.spec.ts +``` + +Commit through active hooks, run delegated full `/verify`, then push and open a +PR against `main`. diff --git a/docs/plans/agent-launcher-hardening-followups/task-01-interim-settings-interlock.md b/docs/plans/agent-launcher-hardening-followups/task-01-interim-settings-interlock.md new file mode 100644 index 0000000000..832bcd30b2 --- /dev/null +++ b/docs/plans/agent-launcher-hardening-followups/task-01-interim-settings-interlock.md @@ -0,0 +1,55 @@ +--- +id: "01-interim-settings-interlock" +title: "Interim settings interlock" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/office/agents.md" +--- + +# Task 01: Interim settings interlock + +## Acceptance + +- State-changing agent/settings routes reject a missing/wrong boot token and + any bearer credential before handler side effects. +- Legitimate SPA requests attach the per-boot token and existing desktop/mobile + settings saves continue to work. +- Tests and naming describe the control as a replayable interim interlock/CSRF + token, never operator authentication. + +## Verification + +```bash +cd apps/backend && go test ./internal/common/httpmw ./internal/agent/settings/handlers ./internal/backendapp +cd apps && pnpm --filter @kandev/web test -- --run src/boot-payload.test.ts lib/api +``` + +## Files likely touched + +- `apps/backend/internal/common/httpmw/` +- `apps/backend/internal/backendapp/main.go` +- `apps/backend/internal/backendapp/helpers.go` +- `apps/backend/internal/agent/settings/handlers/handlers.go` +- `apps/backend/internal/agent/settings/handlers/*_test.go` +- `apps/backend/internal/webapp/payload.go` +- `apps/web/src/boot-payload.ts` +- `apps/web/src/boot-payload.test.ts` +- `apps/web/lib/api/client.ts` +- `apps/web/app/actions/agents.ts` +- focused API/action tests +- this task file + +## Inputs + +- ADR sections “Decision” and “Consequences”. +- Existing CORS middleware and SPA boot-payload hydration. +- Existing agent settings route registration and action clients. + +## Output contract + +Return a compact handoff capsule with intent/acceptance, base/head SHA, changed +files and entry points, ADR sections, risk tags, exact RED/GREEN verification +commands/results, uncertainties, and this task status set to `done`. Do not edit +`plan.md`. diff --git a/docs/plans/agent-launcher-hardening-followups/task-02-structured-argv-transport.md b/docs/plans/agent-launcher-hardening-followups/task-02-structured-argv-transport.md new file mode 100644 index 0000000000..5b4205582a --- /dev/null +++ b/docs/plans/agent-launcher-hardening-followups/task-02-structured-argv-transport.md @@ -0,0 +1,54 @@ +--- +id: "02-structured-argv-transport" +title: "Structured argv transport" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/office/agents.md" +--- + +# Task 02: Structured argv transport + +## Acceptance + +- Lifecycle stores and sends structured initial/continue argv end to end; + agentctl executes it without reparsing display strings. +- `agent_args`/`continue_args` take strict precedence when present, while legacy + `command`/`continue_command` remain absence-only compatibility fallbacks. +- Tests cover spaces, quotes, empty arguments, Windows paths/backslashes, + prefix/CLI flags, initial, continue, reset/recovery, serialization, and + legacy clients. + +## Verification + +```bash +cd apps/backend && go test ./internal/agent/runtime/agentctl ./internal/agent/runtime/lifecycle ./internal/agentctl/server/api ./internal/agentctl/server/config ./internal/agentctl/server/process +``` + +## Files likely touched + +- `apps/backend/internal/agent/runtime/lifecycle/command.go` +- `apps/backend/internal/agent/runtime/lifecycle/types.go` +- `apps/backend/internal/agent/runtime/lifecycle/manager_launch.go` +- `apps/backend/internal/agent/runtime/lifecycle/manager_startup.go` +- `apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go` +- `apps/backend/internal/agent/runtime/agentctl/client.go` +- `apps/backend/internal/agentctl/server/config/config.go` +- `apps/backend/internal/agentctl/server/api/agent.go` +- `apps/backend/internal/agentctl/server/process/manager.go` +- focused tests beside those files +- this task file + +## Inputs + +- ADR structured-argv paragraph. +- Existing `agents.Command` token slices and lifecycle display-string builders. +- Current agentctl configure JSON and `config.ParseCommand` fallback. + +## Output contract + +Return a compact handoff capsule with intent/acceptance, base/head SHA, changed +files and entry points, ADR sections, risk tags, exact RED/GREEN verification +commands/results, uncertainties, and this task status set to `done`. Do not edit +`plan.md`. diff --git a/docs/plans/agent-launcher-hardening-followups/task-03-restart-and-launch-rollback.md b/docs/plans/agent-launcher-hardening-followups/task-03-restart-and-launch-rollback.md new file mode 100644 index 0000000000..48b7f603bb --- /dev/null +++ b/docs/plans/agent-launcher-hardening-followups/task-03-restart-and-launch-rollback.md @@ -0,0 +1,48 @@ +--- +id: "03-restart-and-launch-rollback" +title: "Restart and launch rollback safety" +status: done +wave: 2 +depends_on: ["02-structured-argv-transport"] +plan: "plan.md" +spec: "../../specs/office/agents.md" +--- + +# Task 03: Restart and launch rollback safety + +## Acceptance + +- Replacement commands are fully resolved and validated before the existing + process, streams, execution state, or agentctl configuration changes. +- Resolver or persisted-prefix failure produces no Stop/Configure/Start calls + and leaves execution state unchanged. +- Construction failure after instance creation stops the instance exactly once, + never registers the execution, and never configures/starts agentctl. + +## Verification + +```bash +cd apps/backend && go test -run 'TestManager_(RestartAgentProcess|Launch|BuildExecution)' ./internal/agent/runtime/lifecycle +``` + +## Files likely touched + +- `apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go` +- `apps/backend/internal/agent/runtime/lifecycle/manager_interaction_test.go` +- `apps/backend/internal/agent/runtime/lifecycle/manager_launch.go` +- `apps/backend/internal/agent/runtime/lifecycle/manager_launch_test.go` +- lifecycle deterministic fakes/helpers +- this task file + +## Inputs + +- Completed task 02 structured execution state. +- `RestartAgentProcess`, `buildFreshAgentCommand`, launch cleanup and execution + store registration ordering. +- Review thread `discussion_r3641362228`. + +## Output contract + +Return a compact handoff capsule with intent/acceptance, base/head SHA, changed +files and entry points, risk tags, exact RED/GREEN verification commands/results, +uncertainties, and this task status set to `done`. Do not edit `plan.md`. diff --git a/docs/plans/agent-launcher-hardening-followups/task-04-review-cleanups-e2e-docs.md b/docs/plans/agent-launcher-hardening-followups/task-04-review-cleanups-e2e-docs.md new file mode 100644 index 0000000000..4d6565b599 --- /dev/null +++ b/docs/plans/agent-launcher-hardening-followups/task-04-review-cleanups-e2e-docs.md @@ -0,0 +1,64 @@ +--- +id: "04-review-cleanups-e2e-docs" +title: "Review cleanups, E2E, and docs" +status: done +wave: 3 +depends_on: + - "01-interim-settings-interlock" + - "03-restart-and-launch-rollback" +plan: "plan.md" +spec: "../../specs/office/agents.md" +--- + +# Task 04: Review cleanups, E2E, and docs + +## Acceptance + +- Preview/save/launch prefix validation is identical; bad create requests make + zero writes; the stale ownership comment and E2E typing casts are removed. +- Frontend async tests use controlled promises/`act`/`waitFor`, with no fixed + sleep. +- The rebuilt mobile settings E2E passes, and public docs accurately describe + ACP-only scope, quoting/argv preservation, fail-closed runtime behavior, and + the replayable interim interlock versus the future operator boundary. + +## Verification + +```bash +cd apps/backend && go test ./internal/agent/settings/controller +cd apps && pnpm --filter @kandev/web test -- --run \ + 'app/settings/agents/[agentId]/profiles/[profileId]/command-preview-card.test.tsx' \ + 'lib/api/domains/agent-profile-normalize.test.ts' +cd apps/web && pnpm e2e:run tests/settings/mobile-agent-profile-config-selector.spec.ts +node --test scripts/validate-public-docs.test.mjs +node scripts/validate-public-docs.mjs +``` + +## Files likely touched + +- `apps/backend/internal/agent/settings/controller/agent_config.go` +- `apps/backend/internal/agent/settings/controller/controller_test.go` +- `apps/backend/internal/agent/settings/controller/agent_crud.go` +- `apps/backend/internal/agent/settings/controller/profile_crud_test.go` +- `apps/backend/internal/agent/agents/agent.go` +- `apps/web/app/settings/agents/[agentId]/profiles/[profileId]/command-preview-card.test.tsx` +- `apps/web/e2e/helpers/api-client.ts` +- `apps/web/e2e/tests/settings/mobile-agent-profile-config-selector.spec.ts` +- `apps/web/lib/types/agent-profile.ts` +- `docs/public/agents-and-profiles.md` +- `docs/public/security.md` +- `docs/specs/office/agents.md` +- this task file + +## Inputs + +- Completed tasks 01 and 03. +- Review threads named in the parent task. +- `/mobile-parity`, `/e2e`, and `/docs-maintainer`. + +## Output contract + +Return a compact handoff capsule with intent/acceptance, base/head SHA, changed +files and entry points, risk tags, exact RED/GREEN/E2E/docs verification +commands/results, uncertainties, and this task status set to `done`. Do not edit +`plan.md`. diff --git a/docs/public/agents-and-profiles.md b/docs/public/agents-and-profiles.md index 0430d143ae..b5ac1264f0 100644 --- a/docs/public/agents-and-profiles.md +++ b/docs/public/agents-and-profiles.md @@ -38,7 +38,8 @@ Select an agent, create a profile, then open **Settings > Agents > _Agent_ > _Pr | Model | Requested through ACP when the agent supports model selection. Leaving it unset uses the agent's default where the form allows that. | | Mode | Requested with ACP `session/set_mode`. The choices come from the installed agent. | | Configuration options | Dynamic ACP values requested with `session/set_config_option`. | -| CLI flags | Enabled entries are tokenized and appended to the launch command. | +| CLI flags | Enabled entries are tokenized and appended to the ACP launch command. | +| Command prefix | Optional ACP-only launcher argv prepended to the command, for example `greywall --`. | | Environment | Literal values or references to Kandev secrets, resolved when the process starts. | | CLI passthrough | Uses the CLI's native terminal interface instead of a structured ACP conversation. | | Auto-approve all permissions | Answers automatically: the first `allow_once`/`allow_always` option, otherwise the first option supplied by the agent; no options cancels. It is off by default. | @@ -60,6 +61,12 @@ The field is not a shell script. Pipes, redirects, variable expansion, and comma Some older profiles contain compatibility fields such as Auggie's `allow_indexing`; current launch behavior is represented by the active profile settings and flags. +### ACP command prefixes + +**Command prefix** is available for ACP launches, not terminal-passthrough launches. Kandev parses the prefix into structured argv rather than running a shell: quote a path containing spaces, for example `"/opt/launchers/safe wrapper" --`. The resulting argv is prefixed to the agent command, so shell features such as pipes, redirects, variable expansion, and command substitution are not evaluated. + +The prefix must contain a nonempty first argv element that is not flag-like. Malformed quotes, a trailing escape, an empty launcher, or a prefix beginning with `-` is rejected when you save or preview it. If an older persisted profile contains an invalid prefix, Kandev fails the launch rather than silently running the agent without its configured launcher. Check the command preview after changing a prefix. + ## Environment variables and secrets Create reusable secrets at **Settings > Secrets** (`/settings/general/secrets`), then select a secret reference in a profile environment entry. Secret names are 1–100 characters and values are 1–10,000 characters. Editing a secret with a blank value keeps the saved value. @@ -89,7 +96,7 @@ Workspace automation selectors do not offer passthrough agent profiles or Local ## Structured ACP and terminal passthrough -ACP sessions can expose typed messages, tool updates, permission requests, models, modes, dynamic configuration, todos, usage, and resume metadata. Each capability depends on the agent's actual ACP implementation. +ACP sessions can expose typed messages, tool updates, permission requests, models, modes, dynamic configuration, todos, usage, and resume metadata. Each capability depends on the agent's actual ACP implementation. ACP-only profile settings, including command prefixes and structured configuration, do not add those capabilities to a terminal-passthrough CLI. Passthrough preserves the CLI's native PTY interface. It is useful when the native terminal has features that ACP does not expose, but Kandev cannot manufacture structured capabilities that are absent. Custom TUI profiles are locked to passthrough. Profile-specific MCP injection also varies by CLI; verify the command preview and the MCP section before depending on it. diff --git a/docs/public/security.md b/docs/public/security.md index 2add0dd9b6..f95077dd35 100644 --- a/docs/public/security.md +++ b/docs/public/security.md @@ -51,6 +51,19 @@ Use separate profiles for different trust levels. Do not give a routine document An agent profile combines a CLI, model, mode, flags, environment values, secret references, permissions, and optional MCP servers. Treat it as a reusable authority package. +The configurable ACP command prefix is currently a launch-customization +feature, not an isolation boundary. Kandev does not yet have a separate +authenticated operator session for profile mutations, so an agent that can +reach the main HTTP API may attempt the same profile edits as the UI. Do not +rely on the prefix to confine a hostile agent until the operator-owned settings +boundary in +[ADR-2026-07-24-operator-owned-agent-launcher-settings](../decisions/2026-07-24-operator-owned-agent-launcher-settings.md) +is enforced. The current per-boot token is a deliberately replayable interim +CSRF and accidental-mutation interlock, not authentication: an intentional +agent can fetch and replay it. It does not close this direct-client path, and +an ambient browser login is insufficient while agent-controlled previews can +share the operator origin. + 1. Create a profile for one purpose, such as local implementation, read-focused review, or unattended maintenance. 2. Select the least-privileged Git, provider, and cloud credentials that purpose needs. 3. Leave approval or sandbox bypasses disabled unless the executor is disposable and the task is trusted. diff --git a/docs/specs/office/agents.md b/docs/specs/office/agents.md index 83860aad9e..1155d4532f 100644 --- a/docs/specs/office/agents.md +++ b/docs/specs/office/agents.md @@ -171,6 +171,30 @@ Service-layer permission checks run on every mutating endpoint. Task scope is en When a CEO calls `POST /office/agents`: must have `can_create_agents`; must specify `role` (defaults applied automatically); may pass `permissions` overrides, but cannot grant permissions it doesn't have itself (no privilege escalation). +### Required operator boundary (not yet implemented) + +The no-JWT UI convention must not apply to execution-profile mutations. +Launcher definitions, executable/prefix argv, CLI configuration, and +environment values are operator control-plane settings. Once operator +authentication is implemented, creating, updating, or deleting those resources +requires an authenticated operator principal; an omitted credential and an +Office agent JWT are both rejected. Operator credentials are never included in +Office runtime environment, workspace files, executor metadata, logs, or +unauthenticated boot/runtime APIs. Full-detail profile and MCP reads containing +literal environment values, headers, or resolved secrets are operator-only; +agent-facing discovery uses a redacted catalog shape. Agent/workspace preview +content runs on an origin that cannot exercise ambient operator credentials or +read operator session/bootstrap state. Until these requirements are enforced, +launcher prefixes are customization rather than an isolation boundary. +Decision: +[ADR-2026-07-24-operator-owned-agent-launcher-settings](../../decisions/2026-07-24-operator-owned-agent-launcher-settings.md). + +The interim risk-reduction guard requires a per-boot SPA token on +state-changing agent/settings requests and rejects Office bearer tokens. Because +an intentional agent can fetch and replay the unauthenticated boot payload, +this guard is a CSRF and accidental-mutation interlock only; it does not satisfy +the operator-boundary scenarios below. + ### Hire flow When the CEO (or any instance with `can_create_agents`) creates a new agent instance, a hire request is submitted: @@ -567,6 +591,29 @@ There are no TTLs on agent rows, runtime rows, instructions, skills, run history - **GIVEN** an agent run scoped to task `KAN-1`, **WHEN** the agent tries to update `KAN-2` without explicit scope, **THEN** the runtime denies the action and no task mutation is attempted. +The following operator-boundary scenarios are acceptance criteria that must +pass before launcher settings are described as an isolation boundary: + +- **GIVEN** a running Office agent that can reach the backend, **WHEN** it + submits an execution-profile mutation with its runtime JWT or without a + credential, **THEN** the backend rejects the request and persists no launcher + or environment change. + +- **GIVEN** an authenticated operator authorized for the target profile, + **WHEN** the settings UI saves an execution-profile change, **THEN** the + backend persists the change without exposing the operator credential to any + agent runtime surface. + +- **GIVEN** an Office agent requests execution-profile discovery, **WHEN** the + backend returns the agent-facing catalog, **THEN** the response contains no + literal profile environment values, MCP environment values, MCP headers, or + resolved secret material. + +- **GIVEN** agent-controlled JavaScript is rendered in a task preview, **WHEN** + it attempts to use the operator session, **THEN** browser origin isolation + prevents it from reading operator state or issuing an authenticated + control-plane mutation. + - **GIVEN** an agent run with `create_subtask` capability and mutation scope for a parent task, **WHEN** it creates a subtask under that parent, **THEN** Office creates the task through the runtime action surface and preserves the caller agent identity. - **GIVEN** an agent run without `create_subtask` capability or without mutation scope for the requested parent, **WHEN** it attempts parented task creation, **THEN** Office rejects the request before reading parent relationships or creating any task.