diff --git a/apps/backend/cmd/kandev/adapters.go b/apps/backend/cmd/kandev/adapters.go index 9317f0fdf7..f80614622e 100644 --- a/apps/backend/cmd/kandev/adapters.go +++ b/apps/backend/cmd/kandev/adapters.go @@ -341,6 +341,11 @@ func (a *lifecycleAdapter) SetSessionModelBySessionID(ctx context.Context, sessi return a.mgr.SetSessionModelBySessionID(ctx, sessionID, modelID) } +// SetSessionModeBySessionID applies a session permission mode via ACP session/set_mode. +func (a *lifecycleAdapter) SetSessionModeBySessionID(ctx context.Context, sessionID, modeID string) error { + return a.mgr.SetSessionModeBySessionID(ctx, sessionID, modeID) +} + // RespondToPermissionBySessionID sends a response to a permission request for a session func (a *lifecycleAdapter) RespondToPermissionBySessionID(ctx context.Context, sessionID, pendingID, optionID string, cancelled bool) error { return a.mgr.RespondToPermissionBySessionID(sessionID, pendingID, optionID, cancelled) diff --git a/apps/backend/config/workflows/loader.go b/apps/backend/config/workflows/loader.go index 6c40bfa0c7..fcdb5f9dfb 100644 --- a/apps/backend/config/workflows/loader.go +++ b/apps/backend/config/workflows/loader.go @@ -205,6 +205,7 @@ var ( string(models.OnEnterEnablePlanMode): true, string(models.OnEnterAutoStartAgent): true, string(models.OnEnterResetAgentContext): true, + string(models.OnEnterSetSessionMode): true, string(models.OnEnterClearDecisions): true, string(models.OnEnterQueueRunForEachParticipant): true, string(models.OnEnterQueueRun): true, @@ -231,6 +232,16 @@ func convertEvents(e stepEventsYAML) (models.StepEvents, error) { if !validOnEnter[a.Type] { return events, fmt.Errorf("invalid on_enter action type %q", a.Type) } + // set_session_mode must carry a non-empty string mode. Modes are + // agent-specific (reported dynamically by the agent), so there is no + // global allow-list to check against — but a missing or non-string + // value would otherwise be silently dropped at compile time, so reject + // it here to fail misconfigured templates loudly. + if a.Type == string(models.OnEnterSetSessionMode) { + if mode, _ := a.Config["mode"].(string); mode == "" { + return events, fmt.Errorf("on_enter set_session_mode requires a non-empty string \"mode\" config") + } + } events.OnEnter = append(events.OnEnter, models.OnEnterAction{ Type: models.OnEnterActionType(a.Type), Config: a.Config, diff --git a/apps/backend/config/workflows/loader_test.go b/apps/backend/config/workflows/loader_test.go index 5bd908425a..308eefdb4b 100644 --- a/apps/backend/config/workflows/loader_test.go +++ b/apps/backend/config/workflows/loader_test.go @@ -3,6 +3,9 @@ package workflows import ( "fmt" "testing" + + "github.com/kandev/kandev/internal/workflow/models" + "gopkg.in/yaml.v3" ) func TestLoadTemplates_AllValid(t *testing.T) { @@ -26,6 +29,57 @@ func TestLoadTemplates_AllValid(t *testing.T) { } } +// TestConvertEvents_SetSessionMode round-trips a set_session_mode on_enter +// action through the YAML loader: the action type passes the allow-list and its +// "mode" config survives into the typed StepEvents. See issue #1183. +func TestConvertEvents_SetSessionMode(t *testing.T) { + const yamlDoc = ` +on_enter: + - type: set_session_mode + config: + mode: acceptEdits +` + var e stepEventsYAML + if err := yaml.Unmarshal([]byte(yamlDoc), &e); err != nil { + t.Fatalf("unmarshal yaml: %v", err) + } + events, err := convertEvents(e) + if err != nil { + t.Fatalf("convertEvents returned error: %v", err) + } + if len(events.OnEnter) != 1 { + t.Fatalf("expected 1 on_enter action, got %d", len(events.OnEnter)) + } + if events.OnEnter[0].Type != models.OnEnterSetSessionMode { + t.Fatalf("unexpected action type %q", events.OnEnter[0].Type) + } + if mode, _ := events.OnEnter[0].Config["mode"].(string); mode != "acceptEdits" { + t.Fatalf("expected mode=acceptEdits, got %q", mode) + } +} + +// TestConvertEvents_SetSessionModeRejectsMissingMode verifies the loader fails +// fast when a set_session_mode action has no usable "mode" config, rather than +// silently dropping it at compile time. See issue #1183. +func TestConvertEvents_SetSessionModeRejectsMissingMode(t *testing.T) { + cases := map[string]string{ + "no config": "on_enter:\n - type: set_session_mode\n", + "empty mode": "on_enter:\n - type: set_session_mode\n config:\n mode: \"\"\n", + "non-string": "on_enter:\n - type: set_session_mode\n config:\n mode: 3\n", + } + for name, doc := range cases { + t.Run(name, func(t *testing.T) { + var e stepEventsYAML + if err := yaml.Unmarshal([]byte(doc), &e); err != nil { + t.Fatalf("unmarshal yaml: %v", err) + } + if _, err := convertEvents(e); err == nil { + t.Fatal("expected convertEvents to reject set_session_mode with no usable mode") + } + }) + } +} + func TestLoadTemplates_EachHasStartStep(t *testing.T) { templates, err := LoadTemplates() if err != nil { diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go index 739a3a5217..68190edecc 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go @@ -293,6 +293,52 @@ func (m *Manager) AuthenticateBySessionID(ctx context.Context, sessionID, method return execution.agentctl.Authenticate(ctx, methodID) } +// reapplySessionModeAfterReset re-applies the active session permission mode +// (e.g. auto / accept-edits) to a freshly (re)initialized ACP session so the +// user's choice survives a context reset instead of silently reverting to the +// agent's default. +// +// The mode to restore is resolved from the persisted session_mode in the DB +// (the authoritative, synchronously-written source — see persistSessionMode and +// the set_session_mode action), falling back to prev (the in-memory mode state +// captured before the reset) only when no provider is wired or nothing is +// persisted. Preferring the DB avoids re-applying a stale in-memory mode when a +// set_session_mode action persisted a newer mode in the same on_enter batch +// before its agent mode event updated modeState. A nil/empty resolved mode is a +// no-op. Addresses issue #1183. +func (m *Manager) reapplySessionModeAfterReset(ctx context.Context, execution *AgentExecution, newSessionID string, prev *CachedModeState) { + if execution.agentctl == nil { + return + } + fallback := "" + var availableModes []streams.SessionModeInfo + if prev != nil { + fallback = prev.CurrentModeID + availableModes = prev.AvailableModes + } + mode := m.effectiveSessionMode(ctx, execution, fallback) + if mode == "" { + return + } + if err := execution.agentctl.SetMode(ctx, newSessionID, mode); err != nil { + m.logger.Warn("failed to re-apply session mode after context reset", + zap.String("execution_id", execution.ID), + zap.String("mode", mode), + zap.Error(err)) + return + } + // Restore the cache too: the fresh session would otherwise report the agent's + // default mode, leaving modeState stale relative to what we just re-applied. + execution.SetModeState(&CachedModeState{ + CurrentModeID: mode, + AvailableModes: availableModes, + }) + m.logger.Info("re-applied session mode after context reset", + zap.String("execution_id", execution.ID), + zap.String("session_id", execution.SessionID), + zap.String("mode", mode)) +} + // ResetAgentContext resets the agent's conversation context. For ACP agents that support // session reset, this creates a new session on the existing connection (fast, no process restart). // For all other agents, this falls back to RestartAgentProcess (full subprocess restart). @@ -311,6 +357,11 @@ func (m *Manager) ResetAgentContext(ctx context.Context, executionID string) err return fmt.Errorf("execution %q has no agentctl client", executionID) } + // Capture the active session mode before the reset so it can be re-applied to + // the fresh ACP session (issue #1183). The agent's new session starts at its + // default mode, so without this the user's chosen mode is lost. + prevMode := execution.GetModeState() + // Resolve agent config and MCP servers for session reset agentConfig, err := m.getAgentConfigForExecution(execution) if err != nil { @@ -355,6 +406,9 @@ func (m *Manager) ResetAgentContext(ctx context.Context, executionID string) err } }) + // Restore the user's session permission mode onto the fresh ACP session. + m.reapplySessionModeAfterReset(ctx, execution, newSessionID, prevMode) + m.logger.Info("agent context reset via session (no process restart)", zap.String("execution_id", executionID), zap.String("session_id", execution.SessionID), @@ -510,6 +564,11 @@ func (m *Manager) RestartAgentProcess(ctx context.Context, executionID string) e 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) if err != nil { @@ -585,6 +644,9 @@ func (m *Manager) RestartAgentProcess(ctx context.Context, executionID string) e 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.logger.Info("agent process restarted with fresh context", zap.String("execution_id", executionID), zap.String("session_id", execution.SessionID), 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 93a130f1d8..8335bc3975 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction_test.go @@ -15,6 +15,7 @@ import ( "github.com/stretchr/testify/require" "github.com/kandev/kandev/internal/agent/executor" + agentctl "github.com/kandev/kandev/internal/agent/runtime/agentctl" "github.com/kandev/kandev/internal/agentctl/server/process" "github.com/kandev/kandev/internal/agentctl/types/streams" "github.com/kandev/kandev/internal/events" @@ -112,6 +113,15 @@ func newRestartMockAgentctlServer(t *testing.T, failStop, failSessionNew bool) * "session_id": "new-session-123", }) } + case "agent.session.reset": + resp, _ = ws.NewResponse(msg.ID, msg.Action, map[string]interface{}{ + "success": true, + "session_id": "reset-session-456", + }) + case "agent.session.set_mode": + resp, _ = ws.NewResponse(msg.ID, msg.Action, map[string]interface{}{ + "success": true, + }) default: resp, _ = ws.NewError(msg.ID, msg.Action, ws.ErrorCodeUnknownAction, "unknown action", nil) } @@ -343,6 +353,137 @@ func TestManager_RestartAgentProcess_SessionInitFailure(t *testing.T) { } } +// --- Session mode preservation across reset (issue #1183) --- + +// TestManager_RestartAgentProcess_ReappliesSessionMode is the regression test for +// the full-process-restart path: a non-default session permission mode (auto / +// accept-edits) chosen by the user must be re-applied to the fresh ACP session +// after restart instead of silently reverting to the agent's default. +func TestManager_RestartAgentProcess_ReappliesSessionMode(t *testing.T) { + mgr := newTestManager() + mock := newRestartMockAgentctlServer(t, false, false) + + client := createTestClient(t, mock.server.URL) + t.Cleanup(client.Close) + + exec := &AgentExecution{ + ID: "exec-mode-restart", + TaskID: "task-1", + SessionID: "session-1", + AgentProfileID: "profile-1", + ACPSessionID: "old-session", + AgentCommand: "auggie --model test", + Status: v1.AgentStatusRunning, + WorkspacePath: "/workspace", + agentctl: client, + promptDoneCh: make(chan PromptCompletionSignal, 1), + } + exec.SetModeState(&CachedModeState{ + CurrentModeID: "acceptEdits", + AvailableModes: []streams.SessionModeInfo{{ID: "default"}, {ID: "acceptEdits"}}, + }) + require.NoError(t, mgr.executionStore.Add(exec)) + + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + + require.NoError(t, mgr.RestartAgentProcess(ctx, exec.ID)) + + require.Contains(t, mock.getWSActions(), "agent.session.set_mode", + "restart must re-apply the previously-active session mode to the new session") + require.NotNil(t, exec.GetModeState()) + require.Equal(t, "acceptEdits", exec.GetModeState().CurrentModeID, + "cached mode state must reflect the re-applied mode after restart") +} + +// TestManager_ResetAgentContext_ReappliesSessionMode is the regression test for +// the ACP fast-path (session reset without a process restart): the user's chosen +// session permission mode must deterministically survive the reset. +func TestManager_ResetAgentContext_ReappliesSessionMode(t *testing.T) { + mgr := newTestManager() + mock := newRestartMockAgentctlServer(t, false, false) + + client := createTestClient(t, mock.server.URL) + t.Cleanup(client.Close) + + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + + // The ACP fast-path reuses the existing agent stream, so connect it first. + require.NoError(t, client.StreamUpdates(ctx, func(agentctl.AgentEvent) {}, nil, nil)) + + exec := &AgentExecution{ + ID: "exec-mode-reset", + TaskID: "task-1", + SessionID: "session-1", + AgentProfileID: "profile-1", + ACPSessionID: "old-session", + AgentCommand: "auggie --model test", + Status: v1.AgentStatusRunning, + WorkspacePath: "/workspace", + sessionInitialized: true, + agentctl: client, + promptDoneCh: make(chan PromptCompletionSignal, 1), + } + exec.SetModeState(&CachedModeState{ + CurrentModeID: "acceptEdits", + AvailableModes: []streams.SessionModeInfo{{ID: "default"}, {ID: "acceptEdits"}}, + }) + require.NoError(t, mgr.executionStore.Add(exec)) + + require.NoError(t, mgr.ResetAgentContext(ctx, exec.ID)) + + require.Equal(t, "reset-session-456", exec.ACPSessionID, + "fast-path reset should create a new ACP session, not restart the process") + require.Contains(t, mock.getWSActions(), "agent.session.set_mode", + "fast-path reset must re-apply the previously-active session mode") + require.NotNil(t, exec.GetModeState()) + require.Equal(t, "acceptEdits", exec.GetModeState().CurrentModeID) +} + +// TestManager_RestartAgentProcess_PrefersPersistedModeOverStaleCache is the +// regression for the ordering hazard flagged on #1188: when a set_session_mode +// action persists a newer mode to the DB in the same on_enter batch just before +// reset_agent_context, the in-memory modeState is still the old value (its agent +// mode event is async). The restart must restore the persisted (DB) mode, not the +// stale cached one. +func TestManager_RestartAgentProcess_PrefersPersistedModeOverStaleCache(t *testing.T) { + mgr := newTestManager() + mgr.workspaceInfoProvider = &mockWorkspaceInfoProvider{ + infos: map[string]*WorkspaceInfo{ + "session-1": {SessionID: "session-1", SessionMode: "acceptEdits"}, + }, + } + mock := newRestartMockAgentctlServer(t, false, false) + client := createTestClient(t, mock.server.URL) + t.Cleanup(client.Close) + + exec := &AgentExecution{ + ID: "exec-mode-stale", + TaskID: "task-1", + SessionID: "session-1", + AgentProfileID: "profile-1", + ACPSessionID: "old-session", + AgentCommand: "auggie --model test", + Status: v1.AgentStatusRunning, + WorkspacePath: "/workspace", + agentctl: client, + promptDoneCh: make(chan PromptCompletionSignal, 1), + } + // Stale in-memory cache: the previous (pre-action) mode. + exec.SetModeState(&CachedModeState{CurrentModeID: "default"}) + require.NoError(t, mgr.executionStore.Add(exec)) + + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + require.NoError(t, mgr.RestartAgentProcess(ctx, exec.ID)) + + require.Contains(t, mock.getWSActions(), "agent.session.set_mode") + require.NotNil(t, exec.GetModeState()) + require.Equal(t, "acceptEdits", exec.GetModeState().CurrentModeID, + "restart must restore the persisted DB mode, not the stale in-memory cache") +} + // --- SetSessionModel passthrough tests --- // TestManager_SetSessionModel_Passthrough_PersistsOverride is the regression test for @@ -561,6 +702,41 @@ func TestIsAgentRunningForSession(t *testing.T) { }) } +// TestEffectiveSessionMode covers the fresh-launch mode propagation for issue +// #1183: a persisted session_mode (e.g. from a set_session_mode workflow step) +// must override the profile default at ACP session init, while a missing +// provider / lookup error / empty session mode falls back to the profile mode. +func TestEffectiveSessionMode(t *testing.T) { + exec := &AgentExecution{ID: "exec-1", TaskID: "task-1", SessionID: "session-1"} + + t.Run("session mode overrides profile mode", func(t *testing.T) { + mgr := newTestManager() + mgr.workspaceInfoProvider = &mockWorkspaceInfoProvider{ + infos: map[string]*WorkspaceInfo{"session-1": {SessionID: "session-1", SessionMode: "acceptEdits"}}, + } + require.Equal(t, "acceptEdits", mgr.effectiveSessionMode(context.Background(), exec, "default")) + }) + + t.Run("falls back to profile mode when no session mode set", func(t *testing.T) { + mgr := newTestManager() + mgr.workspaceInfoProvider = &mockWorkspaceInfoProvider{ + infos: map[string]*WorkspaceInfo{"session-1": {SessionID: "session-1"}}, + } + require.Equal(t, "default", mgr.effectiveSessionMode(context.Background(), exec, "default")) + }) + + t.Run("falls back to profile mode on provider error", func(t *testing.T) { + mgr := newTestManager() + mgr.workspaceInfoProvider = &mockWorkspaceInfoProvider{err: fmt.Errorf("boom")} + require.Equal(t, "default", mgr.effectiveSessionMode(context.Background(), exec, "default")) + }) + + t.Run("falls back to profile mode when no provider wired", func(t *testing.T) { + mgr := newTestManager() + require.Equal(t, "default", mgr.effectiveSessionMode(context.Background(), exec, "default")) + }) +} + // --- IsRemoteSession tests --- type mockWorkspaceInfoProvider struct { diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_profile.go b/apps/backend/internal/agent/runtime/lifecycle/manager_profile.go index 715eb81ce4..9d454b5064 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_profile.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_profile.go @@ -164,5 +164,23 @@ func (m *Manager) resolveProfileModelAndMode(ctx context.Context, profileID stri // MarkReady fires later from handleCompleteEvent — that path is the true turn-end. func (m *Manager) initializeACPSession(ctx context.Context, execution *AgentExecution, agentConfig agents.Agent, taskDescription string, attachments []MessageAttachment, mcpServers []agentctltypes.McpServer) error { profileModel, profileMode := m.resolveProfileModelAndMode(ctx, execution.AgentProfileID) - return m.sessionManager.InitializeAndPrompt(ctx, execution, agentConfig, taskDescription, attachments, mcpServers, m.MarkBootReady, profileModel, profileMode) + mode := m.effectiveSessionMode(ctx, execution, profileMode) + return m.sessionManager.InitializeAndPrompt(ctx, execution, agentConfig, taskDescription, attachments, mcpServers, m.MarkBootReady, profileModel, mode) +} + +// effectiveSessionMode prefers a session-level permission mode persisted in the +// session metadata (session_mode — declared via the set_session_mode workflow +// action or a user toggle) over the agent profile's default mode. This makes a +// fresh launch start in the declared mode before the first prompt, rather than +// reverting to the profile default. Falls back to profileMode when no provider +// is wired, the lookup fails, or no session mode is set. See issue #1183. +func (m *Manager) effectiveSessionMode(ctx context.Context, execution *AgentExecution, profileMode string) string { + if m.workspaceInfoProvider == nil { + return profileMode + } + info, err := m.workspaceInfoProvider.GetWorkspaceInfoForSession(ctx, execution.TaskID, execution.SessionID) + if err != nil || info == nil || info.SessionMode == "" { + return profileMode + } + return info.SessionMode } diff --git a/apps/backend/internal/agent/runtime/lifecycle/types.go b/apps/backend/internal/agent/runtime/lifecycle/types.go index 9951fd0fa9..1009a8281c 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/types.go +++ b/apps/backend/internal/agent/runtime/lifecycle/types.go @@ -490,6 +490,11 @@ type WorkspaceInfo struct { AgentProfileID string // Optional - agent profile for the task AgentID string // Agent type ID (e.g., "auggie", "codex") - required for runtime creation ACPSessionID string // Agent's session ID for conversation resumption (from session metadata) + // SessionMode is the persisted session permission mode (e.g. "acceptEdits") + // from session metadata, declared via the set_session_mode workflow action or + // a user toggle. Applied as a mode override at ACP session init so a fresh + // launch starts in the declared mode before the first prompt. See issue #1183. + SessionMode string // Executor-aware fields for correct runtime selection and remote reconnection ExecutorType string // Executor type (e.g., "local_pc", "sprites") diff --git a/apps/backend/internal/integration/simulated_agent_manager_test.go b/apps/backend/internal/integration/simulated_agent_manager_test.go index 61907f8c89..98dfcf6430 100644 --- a/apps/backend/internal/integration/simulated_agent_manager_test.go +++ b/apps/backend/internal/integration/simulated_agent_manager_test.go @@ -485,6 +485,10 @@ func (s *SimulatedAgentManagerClient) SetSessionModelBySessionID(_ context.Conte return fmt.Errorf("not supported") } +func (s *SimulatedAgentManagerClient) SetSessionModeBySessionID(_ context.Context, _, _ string) error { + return fmt.Errorf("not supported") +} + func (s *SimulatedAgentManagerClient) WasSessionInitialized(_ string) bool { return false } func (s *SimulatedAgentManagerClient) IsPassthroughSession(ctx context.Context, sessionID string) bool { return false diff --git a/apps/backend/internal/orchestrator/event_handlers_streaming.go b/apps/backend/internal/orchestrator/event_handlers_streaming.go index 5d9ab7271d..0ebec0f7d1 100644 --- a/apps/backend/internal/orchestrator/event_handlers_streaming.go +++ b/apps/backend/internal/orchestrator/event_handlers_streaming.go @@ -894,6 +894,14 @@ func (s *Service) handleSessionModeEvent(ctx context.Context, payload *lifecycle if sessionID == "" || s.eventBus == nil { return } + // Persist the agent-reported mode to session metadata so the user's chosen + // permission mode survives a backend restart / SSR reload, mirroring how the + // current model is persisted. Only non-empty modes are stored — an empty + // CurrentModeID means the agent left a special mode, with nothing sticky to keep. + if mode := payload.Data.CurrentModeID; mode != "" { + s.persistSessionMode(ctx, sessionID, mode) + } + eventPayload := lifecycle.SessionModeEventPayload{ TaskID: payload.TaskID, SessionID: sessionID, @@ -906,6 +914,21 @@ func (s *Service) handleSessionModeEvent(ctx context.Context, payload *lifecycle _ = s.eventBus.Publish(ctx, subject, bus.NewEvent(events.SessionModeChanged, "orchestrator", eventPayload)) } +// persistSessionMode stores the agent-reported session permission mode in the +// session metadata using a targeted json_set, so other metadata keys (plan_mode, +// acp_session_id, …) are preserved. See issue #1183. +func (s *Service) persistSessionMode(ctx context.Context, sessionID, modeID string) { + if s.repo == nil { + return + } + if err := s.repo.SetSessionMetadataKey(ctx, sessionID, models.SessionMetaKeySessionMode, modeID); err != nil { + s.logger.Warn("failed to persist session mode to metadata", + zap.String("session_id", sessionID), + zap.String("mode", modeID), + zap.Error(err)) + } +} + // handleAgentCapabilitiesEvent broadcasts agent_capabilities events to the WebSocket. func (s *Service) handleAgentCapabilitiesEvent(ctx context.Context, payload *lifecycle.AgentStreamEventPayload) { sessionID := payload.SessionID diff --git a/apps/backend/internal/orchestrator/event_handlers_streaming_test.go b/apps/backend/internal/orchestrator/event_handlers_streaming_test.go index 4ce1aeaef0..862f4cddc6 100644 --- a/apps/backend/internal/orchestrator/event_handlers_streaming_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_streaming_test.go @@ -115,6 +115,57 @@ func TestHandleSessionModeEvent(t *testing.T) { require.Empty(t, eb.events) }) + + // Regression for issue #1183: a non-empty mode is persisted to session + // metadata (so it survives backend restart / SSR) without clobbering other + // keys such as plan_mode. + t.Run("persists non-empty mode without clobbering plan_mode", func(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + require.NoError(t, repo.UpdateSessionMetadata(ctx, "s1", map[string]interface{}{"plan_mode": true})) + + eb := &recordingEventBus{} + svc := &Service{logger: testLogger(), eventBus: eb, repo: repo} + + svc.handleSessionModeEvent(ctx, &lifecycle.AgentStreamEventPayload{ + TaskID: "t1", + SessionID: "s1", + AgentID: "a1", + Data: &lifecycle.AgentStreamEventData{CurrentModeID: "acceptEdits"}, + }) + + updated, err := repo.GetTaskSession(ctx, "s1") + require.NoError(t, err) + require.Equal(t, "acceptEdits", updated.Metadata[models.SessionMetaKeySessionMode], + "session mode must be persisted to metadata") + pm, _ := updated.Metadata["plan_mode"].(bool) + require.True(t, pm, "plan_mode and other metadata keys must be preserved") + }) + + // An empty CurrentModeID (agent left a special mode) must not overwrite a + // previously-stored sticky mode. + t.Run("empty mode does not overwrite stored mode", func(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + require.NoError(t, repo.UpdateSessionMetadata(ctx, "s1", + map[string]interface{}{models.SessionMetaKeySessionMode: "acceptEdits"})) + + eb := &recordingEventBus{} + svc := &Service{logger: testLogger(), eventBus: eb, repo: repo} + + svc.handleSessionModeEvent(ctx, &lifecycle.AgentStreamEventPayload{ + TaskID: "t1", + SessionID: "s1", + AgentID: "a1", + Data: &lifecycle.AgentStreamEventData{CurrentModeID: ""}, + }) + + updated, err := repo.GetTaskSession(ctx, "s1") + require.NoError(t, err) + require.Equal(t, "acceptEdits", updated.Metadata[models.SessionMetaKeySessionMode]) + }) } // TestToolEventsWakeSessionAndTaskTogether locks in the fix for the diff --git a/apps/backend/internal/orchestrator/event_handlers_test.go b/apps/backend/internal/orchestrator/event_handlers_test.go index 9f4e35a9ad..aead4133be 100644 --- a/apps/backend/internal/orchestrator/event_handlers_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_test.go @@ -174,6 +174,17 @@ type mockAgentManager struct { cancelAgentCalls atomic.Int32 cancelAgentBlock chan struct{} cancelAgentEntered chan struct{} + + // set_session_mode tracking (issue #1183). Records (sessionID, modeID) for + // every SetSessionModeBySessionID call. setSessionModeErr, when set, is + // returned to simulate "no running agent". + setSessionModeCalls []sessionModeCall + setSessionModeErr error +} + +type sessionModeCall struct { + SessionID string + ModeID string } type stopAgentCall struct { @@ -282,6 +293,12 @@ func (m *mockAgentManager) SetExecutionEnv(_ context.Context, _ string, _ map[st func (m *mockAgentManager) SetSessionModelBySessionID(_ context.Context, _, _ string) error { return fmt.Errorf("not supported") } +func (m *mockAgentManager) SetSessionModeBySessionID(_ context.Context, sessionID, modeID string) error { + m.mu.Lock() + defer m.mu.Unlock() + m.setSessionModeCalls = append(m.setSessionModeCalls, sessionModeCall{SessionID: sessionID, ModeID: modeID}) + return m.setSessionModeErr +} func (m *mockAgentManager) SetMcpMode(_ context.Context, _ string, _ string) error { return nil } diff --git a/apps/backend/internal/orchestrator/event_handlers_workflow.go b/apps/backend/internal/orchestrator/event_handlers_workflow.go index 1e6dafc395..0c6760addd 100644 --- a/apps/backend/internal/orchestrator/event_handlers_workflow.go +++ b/apps/backend/internal/orchestrator/event_handlers_workflow.go @@ -862,6 +862,9 @@ func (s *Service) processOnEnter(ctx context.Context, taskID string, session *mo if !isPassthrough && hasPlanMode { s.setSessionPlanMode(ctx, session, true) } + case wfmodels.OnEnterSetSessionMode: + mode, _ := action.Config["mode"].(string) + s.applyStepSessionMode(ctx, session, mode, isPassthrough) case wfmodels.OnEnterAutoStartAgent: hasAutoStart = true } @@ -1727,6 +1730,35 @@ func (s *Service) setSessionPlanMode(ctx context.Context, session *models.TaskSe } } +// applyStepSessionMode applies a workflow-declared session permission mode to a +// session entering a step (set_session_mode action, issue #1183). It persists the +// mode to metadata (durable + restored on reset) and best-effort applies it to a +// running agent via ACP session/set_mode. Passthrough sessions manage their own +// mode in the underlying CLI and are skipped, mirroring plan-mode handling. +func (s *Service) applyStepSessionMode(ctx context.Context, session *models.TaskSession, mode string, isPassthrough bool) { + if mode == "" || isPassthrough { + return + } + // Persist for durability (SSR / backend restart) and so Part 1's reset + // re-apply has the right value. Mirror the in-memory struct for callers + // that read session.Metadata afterwards. + if session.Metadata == nil { + session.Metadata = make(map[string]interface{}) + } + session.Metadata[models.SessionMetaKeySessionMode] = mode + s.persistSessionMode(ctx, session.ID, mode) + + // Apply live when an agent is running. When none is (e.g. the step also + // auto-starts the agent fresh), this is a no-op and the profile default + // governs the new session — the declared mode stays persisted. + if err := s.agentManager.SetSessionModeBySessionID(ctx, session.ID, mode); err != nil { + s.logger.Debug("set_session_mode: could not apply mode to a live agent (persisted for next launch/reset)", + zap.String("session_id", session.ID), + zap.String("mode", mode), + zap.Error(err)) + } +} + // processTurnCompleteActions processes on_turn_complete actions for a step: // it executes side-effect actions and returns the first eligible transition action. func (s *Service) processTurnCompleteActions(ctx context.Context, session *models.TaskSession, step *wfmodels.WorkflowStep) *wfmodels.OnTurnCompleteAction { diff --git a/apps/backend/internal/orchestrator/event_handlers_workflow_triggers_test.go b/apps/backend/internal/orchestrator/event_handlers_workflow_triggers_test.go index 2552dd86aa..005eefbfc5 100644 --- a/apps/backend/internal/orchestrator/event_handlers_workflow_triggers_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_workflow_triggers_test.go @@ -298,6 +298,41 @@ func TestProcessOnTurnStart(t *testing.T) { }) } +// TestProcessOnEnterSetSessionMode verifies the set_session_mode on_enter action +// (issue #1183): it persists the declared mode to metadata and applies it live to +// the running agent. +func TestProcessOnEnterSetSessionMode(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + + agentMgr := &mockAgentManager{} + svc := createTestServiceWithAgent(repo, newMockStepGetter(), newMockTaskRepo(), agentMgr) + + step := &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Implement", + Events: wfmodels.StepEvents{ + OnEnter: []wfmodels.OnEnterAction{ + {Type: wfmodels.OnEnterSetSessionMode, Config: map[string]interface{}{"mode": "acceptEdits"}}, + }, + }, + } + + session, _ := repo.GetTaskSession(ctx, "s1") + svc.processOnEnter(ctx, "t1", session, step, "test task") + + updated, _ := repo.GetTaskSession(ctx, "s1") + if got, _ := updated.Metadata[models.SessionMetaKeySessionMode].(string); got != "acceptEdits" { + t.Errorf("expected session_mode metadata acceptEdits, got %q", got) + } + if len(agentMgr.setSessionModeCalls) != 1 { + t.Fatalf("expected 1 live set-mode call, got %d", len(agentMgr.setSessionModeCalls)) + } + if c := agentMgr.setSessionModeCalls[0]; c.SessionID != "s1" || c.ModeID != "acceptEdits" { + t.Errorf("unexpected live set-mode call: %+v", c) + } +} + func TestProcessOnEnter(t *testing.T) { ctx := context.Background() diff --git a/apps/backend/internal/orchestrator/executor/executor.go b/apps/backend/internal/orchestrator/executor/executor.go index 405144967c..a049c2b1c4 100644 --- a/apps/backend/internal/orchestrator/executor/executor.go +++ b/apps/backend/internal/orchestrator/executor/executor.go @@ -139,6 +139,11 @@ type AgentManagerClient interface { // Returns an error if the agent doesn't support in-place model switching. SetSessionModelBySessionID(ctx context.Context, sessionID, modelID string) error + // SetSessionModeBySessionID applies a session permission mode (e.g. "default", + // "acceptEdits") to the running agent via ACP session/set_mode. Returns an + // error when no agent is running for the session. See issue #1183. + SetSessionModeBySessionID(ctx context.Context, sessionID, modeID string) error + // WasSessionInitialized reports whether the given execution completed session initialization. // Used to distinguish launch-phase failures from normal prompt failures. WasSessionInitialized(executionID string) bool diff --git a/apps/backend/internal/orchestrator/executor/executor_mocks_test.go b/apps/backend/internal/orchestrator/executor/executor_mocks_test.go index 150e9b2c27..58641715fb 100644 --- a/apps/backend/internal/orchestrator/executor/executor_mocks_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_mocks_test.go @@ -116,6 +116,10 @@ func (m *mockAgentManager) SetSessionModelBySessionID(ctx context.Context, sessi return fmt.Errorf("not supported") } +func (m *mockAgentManager) SetSessionModeBySessionID(ctx context.Context, sessionID, modeID string) error { + return fmt.Errorf("not supported") +} + func (m *mockAgentManager) IsAgentRunningForSession(ctx context.Context, sessionID string) bool { m.isAgentRunningForSessionCallArgs = append(m.isAgentRunningForSessionCallArgs, sessionID) if m.isAgentRunningForSessionFunc != nil { diff --git a/apps/backend/internal/orchestrator/scheduler/scheduler_test.go b/apps/backend/internal/orchestrator/scheduler/scheduler_test.go index afe4be5aa4..68e2ab3b7a 100644 --- a/apps/backend/internal/orchestrator/scheduler/scheduler_test.go +++ b/apps/backend/internal/orchestrator/scheduler/scheduler_test.go @@ -91,6 +91,9 @@ func (m *mockAgentManager) ResetAgentContext(ctx context.Context, agentExecution func (m *mockAgentManager) SetSessionModelBySessionID(_ context.Context, _, _ string) error { return errors.New("not supported") } +func (m *mockAgentManager) SetSessionModeBySessionID(_ context.Context, _, _ string) error { + return errors.New("not supported") +} func (m *mockAgentManager) IsAgentRunningForSession(ctx context.Context, sessionID string) bool { return false diff --git a/apps/backend/internal/orchestrator/workflow_callbacks.go b/apps/backend/internal/orchestrator/workflow_callbacks.go index 842ac4e642..32ea7426a1 100644 --- a/apps/backend/internal/orchestrator/workflow_callbacks.go +++ b/apps/backend/internal/orchestrator/workflow_callbacks.go @@ -24,6 +24,7 @@ func buildWorkflowCallbacks(svc *Service) engine.MapRegistry { engine.ActionResetAgentContext: &resetAgentContextCallback{svc: svc}, engine.ActionAutoStartAgent: &autoStartAgentCallback{svc: svc}, engine.ActionSetWorkflowData: &setWorkflowDataCallback{}, + engine.ActionSetSessionMode: &setSessionModeCallback{svc: svc}, } if svc.engineRunQueue != nil { r[engine.ActionQueueRun] = engine.QueueRunCallback{ @@ -109,6 +110,30 @@ func (c *disablePlanModeCallback) Execute(ctx context.Context, in engine.ActionI return engine.ActionResult{}, nil } +// setSessionModeCallback applies a workflow-declared session permission mode +// (e.g. "acceptEdits") when entering a step. See issue #1183. +type setSessionModeCallback struct { + svc *Service +} + +func (c *setSessionModeCallback) Execute(ctx context.Context, in engine.ActionInput) (engine.ActionResult, error) { + // Skip before any DB lookup: passthrough sessions manage their own mode in + // the CLI, and an action with no mode is a no-op. Guarding here keeps a + // skipped action from failing on a session-load error, and mirrors the + // enable/disable plan-mode callbacks. + if in.Action.SetSessionMode == nil || in.State.IsPassthrough { + return engine.ActionResult{}, nil + } + session, err := c.svc.repo.GetTaskSession(ctx, in.State.SessionID) + if err != nil { + return engine.ActionResult{}, fmt.Errorf("load session for set session mode: %w", err) + } + // Passthrough is already excluded above, so pass false explicitly; the + // isPassthrough parameter exists for the legacy processOnEnter call site. + c.svc.applyStepSessionMode(ctx, session, in.Action.SetSessionMode.Mode, false) + return engine.ActionResult{}, nil +} + // resetAgentContextCallback restarts the agent subprocess with a fresh ACP session. type resetAgentContextCallback struct { svc *Service diff --git a/apps/backend/internal/orchestrator/workflow_callbacks_test.go b/apps/backend/internal/orchestrator/workflow_callbacks_test.go new file mode 100644 index 0000000000..dcf3fac49e --- /dev/null +++ b/apps/backend/internal/orchestrator/workflow_callbacks_test.go @@ -0,0 +1,81 @@ +package orchestrator + +import ( + "context" + "testing" + + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/workflow/engine" +) + +// TestSetSessionModeCallback_Execute covers the engine callback wired for the +// set_session_mode action (issue #1183): for a non-passthrough session it +// persists the declared mode to metadata and applies it live to the agent; +// passthrough sessions and actions with no payload are no-ops. +func TestSetSessionModeCallback_Execute(t *testing.T) { + ctx := context.Background() + + newInput := func(passthrough bool, mode string) engine.ActionInput { + in := engine.ActionInput{ + Trigger: engine.TriggerOnEnter, + State: engine.MachineState{TaskID: "t1", SessionID: "s1", IsPassthrough: passthrough}, + Action: engine.Action{Kind: engine.ActionSetSessionMode}, + } + if mode != "" { + in.Action.SetSessionMode = &engine.SetSessionModeAction{Mode: mode} + } + return in + } + + t.Run("applies and persists for a non-passthrough session", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + agentMgr := &mockAgentManager{} + svc := createTestServiceWithAgent(repo, newMockStepGetter(), newMockTaskRepo(), agentMgr) + + if _, err := (&setSessionModeCallback{svc: svc}).Execute(ctx, newInput(false, "acceptEdits")); err != nil { + t.Fatalf("Execute returned error: %v", err) + } + + updated, _ := repo.GetTaskSession(ctx, "s1") + if got, _ := updated.Metadata[models.SessionMetaKeySessionMode].(string); got != "acceptEdits" { + t.Errorf("expected persisted session_mode acceptEdits, got %q", got) + } + if len(agentMgr.setSessionModeCalls) != 1 || agentMgr.setSessionModeCalls[0].ModeID != "acceptEdits" { + t.Errorf("expected one live set-mode call for acceptEdits, got %+v", agentMgr.setSessionModeCalls) + } + }) + + t.Run("skips passthrough sessions", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + agentMgr := &mockAgentManager{isPassthrough: true} + svc := createTestServiceWithAgent(repo, newMockStepGetter(), newMockTaskRepo(), agentMgr) + + if _, err := (&setSessionModeCallback{svc: svc}).Execute(ctx, newInput(true, "acceptEdits")); err != nil { + t.Fatalf("Execute returned error: %v", err) + } + + updated, _ := repo.GetTaskSession(ctx, "s1") + if _, ok := updated.Metadata[models.SessionMetaKeySessionMode]; ok { + t.Error("passthrough session must not persist session_mode") + } + if len(agentMgr.setSessionModeCalls) != 0 { + t.Errorf("passthrough session must not apply mode live, got %+v", agentMgr.setSessionModeCalls) + } + }) + + t.Run("no-op when action carries no mode", func(t *testing.T) { + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + agentMgr := &mockAgentManager{} + svc := createTestServiceWithAgent(repo, newMockStepGetter(), newMockTaskRepo(), agentMgr) + + if _, err := (&setSessionModeCallback{svc: svc}).Execute(ctx, newInput(false, "")); err != nil { + t.Fatalf("Execute returned error: %v", err) + } + if len(agentMgr.setSessionModeCalls) != 0 { + t.Errorf("expected no live set-mode call when mode is empty, got %+v", agentMgr.setSessionModeCalls) + } + }) +} diff --git a/apps/backend/internal/task/models/models.go b/apps/backend/internal/task/models/models.go index 37aa0b22f7..a5a8626416 100644 --- a/apps/backend/internal/task/models/models.go +++ b/apps/backend/internal/task/models/models.go @@ -71,6 +71,11 @@ const ( SessionCreatedByWorkflowSwitch = "workflow_switch" ) +// SessionMetaKeySessionMode records the agent's last-known session permission +// mode (auto / default / accept-edits, etc.) so it survives a backend restart or +// SSR reload, alongside the in-memory re-apply on context reset. See issue #1183. +const SessionMetaKeySessionMode = "session_mode" + // Task origin values for the Origin field. const ( TaskOriginManual = "manual" diff --git a/apps/backend/internal/task/service/service_turns.go b/apps/backend/internal/task/service/service_turns.go index 9ab3f4ef67..e2ecc1b412 100644 --- a/apps/backend/internal/task/service/service_turns.go +++ b/apps/backend/internal/task/service/service_turns.go @@ -373,12 +373,15 @@ func (s *Service) GetWorkspaceInfoForSession(ctx context.Context, taskID, sessio } } - // Get ACP session ID from metadata - var acpSessionID string + // Get ACP session ID and persisted session permission mode from metadata. + var acpSessionID, sessionMode string if session.Metadata != nil { if id, ok := session.Metadata["acp_session_id"].(string); ok { acpSessionID = id } + if mode, ok := session.Metadata[models.SessionMetaKeySessionMode].(string); ok { + sessionMode = mode + } } info := &lifecycle.WorkspaceInfo{ @@ -389,6 +392,7 @@ func (s *Service) GetWorkspaceInfoForSession(ctx context.Context, taskID, sessio AgentProfileID: session.AgentProfileID, AgentID: agentID, ACPSessionID: acpSessionID, + SessionMode: sessionMode, } var taskEnv *models.TaskEnvironment diff --git a/apps/backend/internal/task/service/service_turns_test.go b/apps/backend/internal/task/service/service_turns_test.go index 15f3d075d8..c25766a95c 100644 --- a/apps/backend/internal/task/service/service_turns_test.go +++ b/apps/backend/internal/task/service/service_turns_test.go @@ -78,6 +78,37 @@ func TestGetWorkspaceInfoForSession_BasicFields(t *testing.T) { } } +// TestGetWorkspaceInfoForSession_IncludesSessionMode verifies the persisted +// session permission mode is surfaced on WorkspaceInfo so the lifecycle can apply +// it as a mode override on a fresh launch. See issue #1183. +func TestGetWorkspaceInfoForSession_IncludesSessionMode(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + + setupTestTask(t, repo) + now := time.Now().UTC() + + session := &models.TaskSession{ + ID: "session-1", + TaskID: "task-123", + State: models.TaskSessionStateRunning, + Metadata: map[string]interface{}{models.SessionMetaKeySessionMode: "acceptEdits"}, + StartedAt: now, + UpdatedAt: now, + } + if err := repo.CreateTaskSession(ctx, session); err != nil { + t.Fatalf("failed to create session: %v", err) + } + + info, err := svc.GetWorkspaceInfoForSession(ctx, "task-123", "session-1") + if err != nil { + t.Fatalf("GetWorkspaceInfoForSession returned error: %v", err) + } + if info.SessionMode != "acceptEdits" { + t.Errorf("expected SessionMode 'acceptEdits', got %q", info.SessionMode) + } +} + func TestGetWorkspaceInfoForSession_InfersTaskID(t *testing.T) { svc, _, repo := createTestService(t) ctx := context.Background() diff --git a/apps/backend/internal/workflow/engine/engine_test.go b/apps/backend/internal/workflow/engine/engine_test.go index 40a1ba7b26..11e9ea0501 100644 --- a/apps/backend/internal/workflow/engine/engine_test.go +++ b/apps/backend/internal/workflow/engine/engine_test.go @@ -4,6 +4,8 @@ import ( "context" "errors" "testing" + + wfmodels "github.com/kandev/kandev/internal/workflow/models" ) type fakeStore struct { @@ -81,6 +83,57 @@ func (c *fakeCallback) Execute(_ context.Context, _ ActionInput) (ActionResult, return c.result, nil } +// recordingCallback captures the ActionInput it was invoked with so tests can +// assert the engine dispatched the right typed action to the callback. +type recordingCallback struct { + calls []ActionInput +} + +func (c *recordingCallback) Execute(_ context.Context, in ActionInput) (ActionResult, error) { + c.calls = append(c.calls, in) + return ActionResult{}, nil +} + +// TestHandleTrigger_SetSessionMode_InvokesCallback exercises the full engine path +// for the set_session_mode action (issue #1183): a step compiled from a +// set_session_mode on_enter action dispatches to the registered callback with the +// typed mode carried through. This is the engine-level end-to-end of the feature. +func TestHandleTrigger_SetSessionMode_InvokesCallback(t *testing.T) { + compiled := CompileStep(&wfmodels.WorkflowStep{ + ID: "step-1", WorkflowID: "wf1", + Events: wfmodels.StepEvents{ + OnEnter: []wfmodels.OnEnterAction{ + {Type: wfmodels.OnEnterSetSessionMode, Config: map[string]any{"mode": "acceptEdits"}}, + }, + }, + }) + store := &fakeStore{ + state: MachineState{TaskID: "t1", SessionID: "s1", WorkflowID: "wf1", CurrentStepID: "step-1"}, + stepsByID: map[string]StepSpec{"step-1": compiled}, + applied: map[string]bool{}, + } + + cb := &recordingCallback{} + eng := New(store, MapRegistry{ActionSetSessionMode: cb}) + + if _, err := eng.HandleTrigger(context.Background(), HandleInput{ + TaskID: "t1", SessionID: "s1", Trigger: TriggerOnEnter, + }); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(cb.calls) != 1 { + t.Fatalf("expected set_session_mode callback to fire once, got %d", len(cb.calls)) + } + got := cb.calls[0].Action + if got.Kind != ActionSetSessionMode { + t.Fatalf("unexpected action kind dispatched: %s", got.Kind) + } + if got.SetSessionMode == nil || got.SetSessionMode.Mode != "acceptEdits" { + t.Fatalf("expected dispatched mode acceptEdits, got %+v", got.SetSessionMode) + } +} + func TestHandleTrigger_FirstTransitionWins(t *testing.T) { store := &fakeStore{ state: MachineState{TaskID: "t1", SessionID: "s1", WorkflowID: "wf1", CurrentStepID: "step-1"}, diff --git a/apps/backend/internal/workflow/engine/types.go b/apps/backend/internal/workflow/engine/types.go index 1df2c23d24..54440bb4d1 100644 --- a/apps/backend/internal/workflow/engine/types.go +++ b/apps/backend/internal/workflow/engine/types.go @@ -42,6 +42,7 @@ const ( ActionAutoStartAgent ActionKind = "auto_start_agent" ActionResetAgentContext ActionKind = "reset_agent_context" ActionSetWorkflowData ActionKind = "set_workflow_data" + ActionSetSessionMode ActionKind = "set_session_mode" // New Phase 2 action kinds (ADR-0004). Defined and exposed via callbacks // that intentionally return ErrActionNotYetWired — they will be wired @@ -81,6 +82,7 @@ type Action struct { MoveToStep *MoveToStepAction AutoStartAgent *AutoStartAgentAction SetWorkflowData *SetWorkflowDataAction + SetSessionMode *SetSessionModeAction QueueRun *QueueRunAction ClearDecisions *ClearDecisionsAction QueueRunForEachParticipant *QueueRunForEachParticipantAction @@ -131,6 +133,13 @@ type SetWorkflowDataAction struct { Value any } +// SetSessionModeAction declares the agent session permission mode to apply when +// the step is entered (e.g. "default", "acceptEdits"). Mode is read from the +// action config's "mode" key by CompileStep. See issue #1183. +type SetSessionModeAction struct { + Mode string +} + // QueueRunAction represents the Phase 2 "queue a run on a target task/agent" // action. The action is declared but its callback is not yet wired into the // engine; see PlaceholderQueueRunCallback. @@ -282,6 +291,12 @@ func compileOnEnter(step *wfmodels.WorkflowStep) []Action { actions = append(actions, Action{Kind: ActionAutoStartAgent, AutoStartAgent: &AutoStartAgentAction{QueueIfBusy: true}}) case wfmodels.OnEnterResetAgentContext: actions = append(actions, Action{Kind: ActionResetAgentContext}) + case wfmodels.OnEnterSetSessionMode: + mode := readSessionMode(action.Config) + if mode == "" { + continue // skip set_session_mode actions with no target mode + } + actions = append(actions, Action{Kind: ActionSetSessionMode, SetSessionMode: &SetSessionModeAction{Mode: mode}}) case wfmodels.OnEnterClearDecisions: actions = append(actions, Action{ Kind: ActionClearDecisions, @@ -421,6 +436,16 @@ func readQueueRunForEachParticipantConfig(config map[string]any) *QueueRunForEac } } +// readSessionMode reads the target mode for a set_session_mode action from its +// config map. Returns "" when the config is missing or the mode key is unset. +func readSessionMode(config map[string]any) string { + if config == nil { + return "" + } + mode, _ := config["mode"].(string) + return mode +} + func readStepID(config map[string]any) (string, error) { if config == nil { return "", fmt.Errorf("missing move_to_step config") diff --git a/apps/backend/internal/workflow/engine/types_test.go b/apps/backend/internal/workflow/engine/types_test.go index 8502161e22..84c2915dae 100644 --- a/apps/backend/internal/workflow/engine/types_test.go +++ b/apps/backend/internal/workflow/engine/types_test.go @@ -46,6 +46,48 @@ func TestCompileStep_CompilesLegacyActionsToTypedActions(t *testing.T) { } } +func TestCompileStep_SetSessionMode(t *testing.T) { + t.Run("compiles set_session_mode with mode config", func(t *testing.T) { + step := &wfmodels.WorkflowStep{ + ID: "s1", + WorkflowID: "wf1", + Name: "Implement", + Events: wfmodels.StepEvents{ + OnEnter: []wfmodels.OnEnterAction{ + {Type: wfmodels.OnEnterSetSessionMode, Config: map[string]any{"mode": "acceptEdits"}}, + }, + }, + } + + spec := CompileStep(step) + actions := spec.Events[TriggerOnEnter] + if len(actions) != 1 { + t.Fatalf("expected 1 on_enter action, got %d", len(actions)) + } + if actions[0].Kind != ActionSetSessionMode { + t.Fatalf("unexpected action kind: %s", actions[0].Kind) + } + if actions[0].SetSessionMode == nil || actions[0].SetSessionMode.Mode != "acceptEdits" { + t.Fatalf("expected compiled set_session_mode mode=acceptEdits, got %+v", actions[0].SetSessionMode) + } + }) + + t.Run("skips set_session_mode with no mode", func(t *testing.T) { + step := &wfmodels.WorkflowStep{ + ID: "s1", + Events: wfmodels.StepEvents{ + OnEnter: []wfmodels.OnEnterAction{ + {Type: wfmodels.OnEnterSetSessionMode}, + }, + }, + } + spec := CompileStep(step) + if len(spec.Events[TriggerOnEnter]) != 0 { + t.Fatalf("expected set_session_mode with no mode to be skipped, got %d actions", len(spec.Events[TriggerOnEnter])) + } + }) +} + func TestCompileStep_RequiresApproval(t *testing.T) { step := &wfmodels.WorkflowStep{ ID: "s1", diff --git a/apps/backend/internal/workflow/models/export.go b/apps/backend/internal/workflow/models/export.go index 76ebda2b49..5279324c21 100644 --- a/apps/backend/internal/workflow/models/export.go +++ b/apps/backend/internal/workflow/models/export.go @@ -130,6 +130,9 @@ func (e *WorkflowExport) Validate() error { return fmt.Errorf("workflow %d: duplicate step position %d", i, step.Position) } positions[step.Position] = true + if err := validateOnEnterActions(step); err != nil { + return fmt.Errorf("workflow %d step %d: %w", i, j, err) + } } // Validate that move_to_step references point to valid positions. if err := validateStepPositionRefs(wf.Steps, positions); err != nil { @@ -139,6 +142,23 @@ func (e *WorkflowExport) Validate() error { return nil } +// validateOnEnterActions rejects malformed on_enter actions in a portable step. +// Currently this guards set_session_mode, which requires a non-empty string +// "mode" config — without it the action is silently dropped at compile time, so +// an import would "succeed" with an inert action. See issue #1183. This mirrors +// the embedded-YAML loader's allow-list check. +func validateOnEnterActions(step StepPortable) error { + for _, a := range step.Events.OnEnter { + if a.Type != OnEnterSetSessionMode { + continue + } + if mode, _ := a.Config["mode"].(string); mode == "" { + return fmt.Errorf("step %q on_enter: set_session_mode requires a non-empty string \"mode\" config", step.Name) + } + } + return nil +} + func validateStepPositionRefs(steps []StepPortable, validPositions map[int]bool) error { for _, step := range steps { for _, a := range step.Events.OnTurnStart { diff --git a/apps/backend/internal/workflow/models/export_test.go b/apps/backend/internal/workflow/models/export_test.go index 9074a92c64..9329fea7cf 100644 --- a/apps/backend/internal/workflow/models/export_test.go +++ b/apps/backend/internal/workflow/models/export_test.go @@ -137,6 +137,34 @@ func TestValidate(t *testing.T) { assert.NoError(t, e.Validate()) }) + t.Run("set_session_mode with mode passes", func(t *testing.T) { + e := validExport() + e.Workflows[0].Steps[0].Events = StepEvents{ + OnEnter: []OnEnterAction{ + {Type: OnEnterSetSessionMode, Config: map[string]any{"mode": "acceptEdits"}}, + }, + } + assert.NoError(t, e.Validate()) + }) + + t.Run("set_session_mode without mode fails", func(t *testing.T) { + e := validExport() + e.Workflows[0].Steps[0].Events = StepEvents{ + OnEnter: []OnEnterAction{{Type: OnEnterSetSessionMode}}, + } + assert.ErrorContains(t, e.Validate(), "set_session_mode requires a non-empty string") + }) + + t.Run("set_session_mode with non-string mode fails", func(t *testing.T) { + e := validExport() + e.Workflows[0].Steps[0].Events = StepEvents{ + OnEnter: []OnEnterAction{ + {Type: OnEnterSetSessionMode, Config: map[string]any{"mode": 3}}, + }, + } + assert.ErrorContains(t, e.Validate(), "set_session_mode requires a non-empty string") + }) + t.Run("invalid move_to_step position ref fails", func(t *testing.T) { e := validExport() e.Workflows[0].Steps[0].Events = StepEvents{ diff --git a/apps/backend/internal/workflow/models/models.go b/apps/backend/internal/workflow/models/models.go index 039e2173de..f047300a65 100644 --- a/apps/backend/internal/workflow/models/models.go +++ b/apps/backend/internal/workflow/models/models.go @@ -12,6 +12,10 @@ const ( OnEnterEnablePlanMode OnEnterActionType = "enable_plan_mode" OnEnterAutoStartAgent OnEnterActionType = "auto_start_agent" OnEnterResetAgentContext OnEnterActionType = "reset_agent_context" + // OnEnterSetSessionMode declares the agent's session permission mode (e.g. + // "default", "acceptEdits") for a step on entry. The target mode is carried + // in the action Config under the "mode" key. See issue #1183. + OnEnterSetSessionMode OnEnterActionType = "set_session_mode" // Phase 2 (ADR-0004) — generic actions are also permitted on on_enter // so review/approval steps can clear decisions and fan out runs to diff --git a/docs/workflow-import-export.md b/docs/workflow-import-export.md index 1d7a45cff9..49674efffb 100644 --- a/docs/workflow-import-export.md +++ b/docs/workflow-import-export.md @@ -166,7 +166,7 @@ import/export: | Trigger | Allowed action `type`s | `config` | |---------|------------------------|----------| -| `on_enter` | `enable_plan_mode`, `auto_start_agent`, `reset_agent_context`, `clear_decisions`, `queue_run`, `queue_run_for_each_participant` | first four take no config; `queue_run` / `queue_run_for_each_participant` use the same config keys as the office triggers (see [Office triggers](#office--phase-2-triggers-intended-format--see-caveat)) | +| `on_enter` | `enable_plan_mode`, `auto_start_agent`, `reset_agent_context`, `set_session_mode`, `clear_decisions`, `queue_run`, `queue_run_for_each_participant` | the first three take no config; `set_session_mode` takes `mode` (the agent permission mode to apply, e.g. `acceptEdits`); `queue_run` / `queue_run_for_each_participant` use the same config keys as the office triggers (see [Office triggers](#office--phase-2-triggers-intended-format--see-caveat)) | | `on_turn_start` | `move_to_next`, `move_to_previous`, `move_to_step` | `move_to_step` needs `step_position` | | `on_turn_complete` | `move_to_next`, `move_to_previous`, `move_to_step`, `disable_plan_mode` | `move_to_step` needs `step_position` | | `on_exit` | `disable_plan_mode` | — |