From d3d4d9d04eedb121c69d89e483045b6b89a9db99 Mon Sep 17 00:00:00 2001 From: Henry Date: Mon, 24 Aug 2026 20:34:52 +0000 Subject: [PATCH] fix(office): require agents to explicitly signal before advancing the Work step (#2972) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 Co-authored-by: Carlos Florencio --- apps/backend/config/prompts/office-context.md | 2 + apps/backend/config/workflows/loader_test.go | 39 +++++ .../config/workflows/office-default.yml | 1 + .../backend/internal/mcp/handlers/handlers.go | 79 +++++++-- .../mcp/handlers/step_complete_test.go | 45 ++++++ apps/backend/internal/mcp/server/server.go | 2 +- .../internal/mcp/server/server_test.go | 24 ++- .../mcp/server/sysprompt_sync_test.go | 2 - .../event_handlers_step_completion_test.go | 150 ++++++++++++++++++ .../orchestrator/event_handlers_workflow.go | 2 +- .../event_handlers_workflow_profile_test.go | 4 +- .../internal/orchestrator/task_operations.go | 12 +- apps/backend/internal/sysprompt/sysprompt.go | 33 +++- .../internal/sysprompt/sysprompt_test.go | 18 ++- .../task/handlers/message_handlers.go | 2 +- .../handlers/message_handlers_blocked_test.go | 6 +- .../task/repository/sqlite/base_schema.go | 1 + .../sqlite/builtin_workflow_test.go | 138 +++++++++++++++- .../task/repository/sqlite/defaults.go | 27 ++++ .../task/repository/sqlite/session.go | 70 ++++++++ .../task/repository/sqlite/session_test.go | 43 +++++ docs/public/automation-and-mcp.md | 6 +- docs/specs/office/system-design/agents-02.md | 5 +- docs/specs/office/system-design/agents-03.md | 2 +- .../workflow-explicit-completion-signal.md | 19 +-- 25 files changed, 683 insertions(+), 49 deletions(-) diff --git a/apps/backend/config/prompts/office-context.md b/apps/backend/config/prompts/office-context.md index a95a28a73a..eb50f65831 100644 --- a/apps/backend/config/prompts/office-context.md +++ b/apps/backend/config/prompts/office-context.md @@ -17,6 +17,8 @@ Available tools: - get_task_document_kandev: Read one document on an accessible related task. Required: task_id, document_key. - write_task_document_kandev: Create or replace a document on the current task or an ancestor. Required: task_id, document_key, content. Optional: title, type. - record_step_decision_kandev: Record an approve/reject decision for the current workflow step, as a reviewer or approver. Required: decision ("approved" or "rejected"), reason. Not idempotent: a second call supersedes the first. +- step_complete_kandev: Signal that every requirement for the current workflow step is satisfied when the step requires an explicit signal. Required: summary. +{step_complete_instruction} Office state changes are performed through `$KANDEV_CLI kandev ...`, subject to this agent's runtime permissions. Use the injected Office skills for exact commands and do not search for additional Kandev MCP tools. Workspace administration is outside this run. diff --git a/apps/backend/config/workflows/loader_test.go b/apps/backend/config/workflows/loader_test.go index 9572112f64..c6180ff7fa 100644 --- a/apps/backend/config/workflows/loader_test.go +++ b/apps/backend/config/workflows/loader_test.go @@ -270,6 +270,45 @@ func TestLoadTemplates_HiddenFlag(t *testing.T) { } } +// TestLoadTemplates_OfficeDefaultWorkStepRequiresSignal verifies that the +// office-default template's `work` step gates its turn-end auto-advance +// (Work -> Review) on the ADR 0015 declarative completion signal, now that +// step_complete_kandev is registered for the Office MCP surface. Without +// this flag the new signal would be decorative: the step would still +// advance on bare turn-end regardless of whether the agent called the tool. +func TestLoadTemplates_OfficeDefaultWorkStepRequiresSignal(t *testing.T) { + templates, err := LoadTemplates() + if err != nil { + t.Fatalf("LoadTemplates() returned error: %v", err) + } + + var officeDefault *models.WorkflowTemplate + for _, tmpl := range templates { + if tmpl.ID == "office-default" { + officeDefault = tmpl + break + } + } + if officeDefault == nil { + t.Fatal("office-default template not found") + } + + var work *models.StepDefinition + for i := range officeDefault.Steps { + if officeDefault.Steps[i].ID == "work" { + work = &officeDefault.Steps[i] + break + } + } + if work == nil { + t.Fatal("office-default template step \"work\" not found") + } + + if got := boolFieldForTest(t, work, "AutoAdvanceRequiresSignal"); !got { + t.Error("office-default template step \"work\" must set auto_advance_requires_signal: true") + } +} + func TestLoadTemplates_ReportKandevIssuePromptContract(t *testing.T) { templates, err := LoadTemplates() if err != nil { diff --git a/apps/backend/config/workflows/office-default.yml b/apps/backend/config/workflows/office-default.yml index 41309ab55f..5b2e4fe86f 100644 --- a/apps/backend/config/workflows/office-default.yml +++ b/apps/backend/config/workflows/office-default.yml @@ -28,6 +28,7 @@ steps: is_start_step: true show_in_command_panel: true allow_manual_move: true + auto_advance_requires_signal: true events: on_enter: - type: auto_start_agent diff --git a/apps/backend/internal/mcp/handlers/handlers.go b/apps/backend/internal/mcp/handlers/handlers.go index d8f51f6fe8..9c3f6745b1 100644 --- a/apps/backend/internal/mcp/handlers/handlers.go +++ b/apps/backend/internal/mcp/handlers/handlers.go @@ -102,6 +102,24 @@ type SessionRepository interface { SetSessionMetadataKeyIfAbsentOrDifferentStep(ctx context.Context, sessionID, key, stepID string, value interface{}) (bool, error) } +// stepCompletionTurnReader exposes the immutable workflow-step stamp on the +// latest turn. Production SQLite implements this through the turn repository, +// while small handler fakes can omit it and retain legacy behavior. +type stepCompletionTurnReader interface { + ListTurnsBySession(ctx context.Context, sessionID string) ([]*models.Turn, error) +} + +// stepCompletionSignalClaimer binds the metadata write to the task step that +// launched the calling turn. Production SQLite performs the task-step check +// and metadata claim in one transaction. +type stepCompletionSignalClaimer interface { + SetSessionMetadataKeyIfAbsentOrDifferentStepIfTaskAtStep( + ctx context.Context, + taskID, sessionID, key, stepID string, + value interface{}, + ) (bool, error) +} + // conditionalSessionStateUpdater is implemented by repositories that can // reject stale session-state writers. Keeping it optional preserves existing // handler fakes and alternate repositories while production SQLite gets CAS @@ -2205,21 +2223,27 @@ func (h *Handlers) handleStepComplete(ctx context.Context, msg *ws.Message) (*ws return errMsg, err } + launchStepID, err := h.stepCompletionLaunchStep(ctx, req.SessionID, task.WorkflowStepID) + if err != nil { + h.logger.Error("failed to resolve step-completion turn", + zap.String("task_id", req.TaskID), + zap.String("session_id", req.SessionID), + zap.Error(err)) + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "failed to resolve calling turn", nil) + } + if launchStepID != task.WorkflowStepID { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, "workflow step changed before signal was recorded", nil) + } + signal := models.PendingStepCompletionSignal{ - StepID: task.WorkflowStepID, + StepID: launchStepID, Source: models.StepCompletionSourceAgent, Summary: strings.TrimSpace(req.Summary), Handoff: strings.TrimSpace(req.Handoff), Blockers: strings.TrimSpace(req.Blockers), SignaledAt: time.Now().UTC(), } - stored, err := h.sessionRepo.SetSessionMetadataKeyIfAbsentOrDifferentStep( - ctx, - req.SessionID, - models.SessionMetaKeyPendingStepCompletion, - task.WorkflowStepID, - signal, - ) + stored, err := h.claimStepCompletionSignal(ctx, req.TaskID, req.SessionID, launchStepID, signal) if err != nil { h.logger.Error("failed to persist step-completion signal", zap.String("task_id", req.TaskID), @@ -2228,7 +2252,7 @@ func (h *Handlers) handleStepComplete(ctx context.Context, msg *ws.Message) (*ws return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "failed to record signal", nil) } if !stored { - return h.handleDuplicateStepComplete(ctx, msg, req.TaskID, req.SessionID, task.WorkflowStepID, session) + return h.handleDuplicateStepComplete(ctx, msg, req.TaskID, req.SessionID, launchStepID, session) } // Counted here, at the durable bag write, not after publishStepCompletionEvent @@ -2255,6 +2279,43 @@ func (h *Handlers) handleStepComplete(ctx context.Context, msg *ws.Message) (*ws }) } +func (h *Handlers) stepCompletionLaunchStep(ctx context.Context, sessionID, fallback string) (string, error) { + reader, ok := h.sessionRepo.(stepCompletionTurnReader) + if !ok { + return fallback, nil + } + turns, err := reader.ListTurnsBySession(ctx, sessionID) + if err != nil { + return "", err + } + if len(turns) == 0 { + return fallback, nil + } + latest := turns[len(turns)-1] + if latest == nil { + return "", errors.New("latest turn is missing") + } + if stepID := models.StringFromAny(latest.Metadata[models.TurnMetaKeyWorkflowStepIDAtStart]); stepID != "" { + return stepID, nil + } + return "", errors.New("latest turn has no workflow-step stamp") +} + +func (h *Handlers) claimStepCompletionSignal( + ctx context.Context, + taskID, sessionID, stepID string, + signal models.PendingStepCompletionSignal, +) (bool, error) { + if claimer, ok := h.sessionRepo.(stepCompletionSignalClaimer); ok { + return claimer.SetSessionMetadataKeyIfAbsentOrDifferentStepIfTaskAtStep( + ctx, taskID, sessionID, models.SessionMetaKeyPendingStepCompletion, stepID, signal, + ) + } + return h.sessionRepo.SetSessionMetadataKeyIfAbsentOrDifferentStep( + ctx, sessionID, models.SessionMetaKeyPendingStepCompletion, stepID, signal, + ) +} + func (h *Handlers) handleDuplicateStepComplete( ctx context.Context, msg *ws.Message, diff --git a/apps/backend/internal/mcp/handlers/step_complete_test.go b/apps/backend/internal/mcp/handlers/step_complete_test.go index 96fa9e1852..b08bc70f93 100644 --- a/apps/backend/internal/mcp/handlers/step_complete_test.go +++ b/apps/backend/internal/mcp/handlers/step_complete_test.go @@ -227,6 +227,51 @@ func TestHandleStepComplete_TerminalSessionRejected(t *testing.T) { } } +// TestHandleStepComplete_RejectsSignalFromMovedTurn prevents a stale reviewer +// turn from satisfying the successor Work step after a rejection moves the +// task back. The completion signal must belong to the step stamped on the +// turn that called the tool, not only to the task's current step. +func TestHandleStepComplete_RejectsSignalFromMovedTurn(t *testing.T) { + ctx := context.Background() + svc, repo := newTestTaskService(t) + seedStepCompleteTarget(t, repo, "task-moved", "session-moved", "step-review", models.TaskSessionStateRunning) + + turn := &models.Turn{ + ID: "turn-review", + TaskSessionID: "session-moved", + TaskID: "task-moved", + StartedAt: time.Now().UTC(), + CreatedAt: time.Now().UTC(), + UpdatedAt: time.Now().UTC(), + } + stamped, err := repo.CreateTurnWithStepStamp(ctx, turn) + require.NoError(t, err) + require.True(t, stamped, "review turn must carry its launch step stamp") + + task, err := repo.GetTask(ctx, "task-moved") + require.NoError(t, err) + task.WorkflowStepID = "step-work" + require.NoError(t, repo.UpdateTask(ctx, task)) + + bus := &mcpRecordingEventBus{} + h := newStepCompleteHandler(t, svc, repo, bus) + msg := makeWSMessage(t, ws.ActionMCPStepComplete, map[string]interface{}{ + "task_id": "task-moved", + "session_id": "session-moved", + "summary": "late reviewer signal", + }) + + resp, err := h.handleStepComplete(ctx, msg) + require.NoError(t, err) + assertWSError(t, resp, ws.ErrorCodeValidation) + assert.Empty(t, bus.events, "a signal from a moved turn must not publish") + + session, err := repo.GetTaskSession(ctx, "session-moved") + require.NoError(t, err) + _, hasSignal := models.LoadPendingStepSignal(session.Metadata) + assert.False(t, hasSignal, "a stale reviewer signal must not be persisted") +} + // TestHandleStepComplete_FirstCallAccepted covers the happy path: bag is // written, event is published with the documented payload shape, and the // response reports accepted=true with the persisted step_id + signaled_at. diff --git a/apps/backend/internal/mcp/server/server.go b/apps/backend/internal/mcp/server/server.go index 690736593f..180515792d 100644 --- a/apps/backend/internal/mcp/server/server.go +++ b/apps/backend/internal/mcp/server/server.go @@ -922,7 +922,7 @@ func (s *Server) profileToolGroups() []profileToolGroup { s.registerAddWorkspaceSourcesTool() s.registerUpdateRepositoryBaseBranchTool() }}, - {name: "step-completion", enabled: kanban, register: func(s *Server) { s.registerStepCompleteTool() }}, + {name: "step-completion", enabled: func(ctx mcpprofile.Context) bool { return kanban(ctx) || office(ctx) }, register: func(s *Server) { s.registerStepCompleteTool() }}, {name: "task-title", enabled: andProfilePredicates(kanban, capabilityEnabled(mcpprofile.CapabilityTaskTitle)), register: func(s *Server) { s.registerSetTaskTitleTool() }}, {name: "diagnostics", enabled: kanban, register: func(s *Server) { s.registerDiagnosticBundleTool() }}, } diff --git a/apps/backend/internal/mcp/server/server_test.go b/apps/backend/internal/mcp/server/server_test.go index bc3775b127..ce182af253 100644 --- a/apps/backend/internal/mcp/server/server_test.go +++ b/apps/backend/internal/mcp/server/server_test.go @@ -892,7 +892,7 @@ func TestServerModeTask_ToolCount(t *testing.T) { assert.Equal(t, 38, len(tools)) } -func TestServerStepCompleteTool_TaskOnlyAndDiscoverable(t *testing.T) { +func TestServerStepCompleteTool_TaskAndOfficeOnlyAndDiscoverable(t *testing.T) { log := newTestLogger(t) backend := NewChannelBackendClient(log) defer backend.Close() @@ -907,7 +907,10 @@ func TestServerStepCompleteTool_TaskOnlyAndDiscoverable(t *testing.T) { require.NoError(t, json.Unmarshal(serialized, &tool)) assert.NotContains(t, tool, "_meta", "task tools should use normal client discovery") - for _, mode := range []string{ModeOffice, ModeConfig, ModeExternal} { + officeServer := New(backend, "test-session", "test-task", 10005, log, "", false, ModeOffice) + assert.Contains(t, officeServer.mcpServer.ListTools(), "step_complete_kandev", "office mode must register step_complete_kandev (ADR 0015)") + + for _, mode := range []string{ModeConfig, ModeExternal} { t.Run(mode, func(t *testing.T) { restrictedServer := New(backend, "test-session", "test-task", 10005, log, "", false, mode) assert.NotContains(t, restrictedServer.mcpServer.ListTools(), "step_complete_kandev") @@ -972,6 +975,11 @@ func TestServerModeOffice_RegistersCorrectTools(t *testing.T) { assert.Contains(t, tools, "get_task_document_kandev") assert.Contains(t, tools, "write_task_document_kandev") + // Office mode gets the ADR 0015 declarative completion signal too, so a + // step gated by auto_advance_requires_signal can advance without relying + // solely on the agent process exiting. + assert.Contains(t, tools, "step_complete_kandev") + // Office mode should NOT have kanban tools assert.NotContains(t, tools, "create_task_kandev") assert.NotContains(t, tools, "list_tasks_kandev") @@ -996,10 +1004,11 @@ func TestServerModeOffice_ToolCount(t *testing.T) { s := New(backend, "test-session", "test-task", 10005, log, "", false, ModeOffice) tools := getRegisteredToolNames(s) - // 4 plan + 1 interaction + 1 related-tasks + 3 task-documents + 1 rich-output + 1 decisions = 11 + // 4 plan + 1 interaction + 1 related-tasks + 3 task-documents + 1 rich-output + 1 decisions + // + 1 step_complete (ADR 0015) = 12. // (delegate_task_kandev retired in favour of `agentctl kandev task create …`). - assert.NotContains(t, tools, "step_complete_kandev", "step_complete_kandev is kanban-task-only; office mode advances tasks via its own approval surface") - assert.Equal(t, 11, len(tools)) + assert.Contains(t, tools, "step_complete_kandev", "office mode must register the ADR 0015 completion signal") + assert.Equal(t, 12, len(tools)) } func TestServerModeOffice_DisableAskQuestion(t *testing.T) { @@ -1016,9 +1025,10 @@ func TestServerModeOffice_DisableAskQuestion(t *testing.T) { // delegate_task_kandev was retired from ModeOffice (now lives in // the agentctl CLI as `agentctl kandev task create --parent …`). assert.NotContains(t, tools, "delegate_task_kandev") - // 4 plan + 1 related-tasks + 3 task-documents + 1 rich-output + 1 decisions = 10 + // 4 plan + 1 related-tasks + 3 task-documents + 1 rich-output + 1 decisions + // + 1 step_complete (ADR 0015) = 11 // (no ask_user_question, no delegate) - assert.Equal(t, 10, len(tools)) + assert.Equal(t, 11, len(tools)) } func TestServerModeConstants(t *testing.T) { diff --git a/apps/backend/internal/mcp/server/sysprompt_sync_test.go b/apps/backend/internal/mcp/server/sysprompt_sync_test.go index c0ba81861a..0401fe1ae9 100644 --- a/apps/backend/internal/mcp/server/sysprompt_sync_test.go +++ b/apps/backend/internal/mcp/server/sysprompt_sync_test.go @@ -256,8 +256,6 @@ func TestSyspromptToolNames_ExactlyMatchMCPOfficeMode(t *testing.T) { assert.Equal(t, registered, referenced, "Office first-turn context must advertise exactly the ModeOffice tool inventory") - assert.NotContains(t, referenced, "step_complete_kandev") - assert.NotContains(t, sysprompt.OfficeContext(), "mcp__kandev__step_complete_kandev") } // TestSyspromptToolNames_NoBareToolReferences catches the opposite drift: a diff --git a/apps/backend/internal/orchestrator/event_handlers_step_completion_test.go b/apps/backend/internal/orchestrator/event_handlers_step_completion_test.go index 5532cbf583..7018bde875 100644 --- a/apps/backend/internal/orchestrator/event_handlers_step_completion_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_step_completion_test.go @@ -109,6 +109,116 @@ func TestProcessOnTurnComplete_ExplicitSignalGating(t *testing.T) { }) } +// TestProcessOnTurnComplete_OfficeExplicitSignalGating proves the ADR 0015 +// signal gate (WO-32) blocks an Office task's turn-end transition exactly +// like it does for kanban: a gated step with no pending signal must not +// transition, and the session must be parked WAITING_FOR_INPUT rather than +// silently advancing — the failure mode WO-38 measured for 6.75 hours. +func TestProcessOnTurnComplete_OfficeExplicitSignalGating(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedOfficeSession(t, repo, "t-office-gate", "s-office-gate", "") + + stepID := "wfs-t-office-gate" // matches seedOfficeSession's stepID convention + stepGetter := newMockStepGetter() + stepGetter.steps[stepID] = &wfmodels.WorkflowStep{ + ID: stepID, WorkflowID: "wf-office", Name: "work", Position: 1, + AutoAdvanceRequiresSignal: true, + Events: wfmodels.StepEvents{ + OnTurnComplete: []wfmodels.OnTurnCompleteAction{ + {Type: wfmodels.OnTurnCompleteMoveToNext}, + }, + }, + } + svc := createTestServiceWithAgent(repo, stepGetter, newMockTaskRepo(), &mockAgentManager{}) + + task, err := repo.GetTask(ctx, "t-office-gate") + if err != nil { + t.Fatalf("get task: %v", err) + } + session, err := repo.GetTaskSession(ctx, "s-office-gate") + if err != nil { + t.Fatalf("get session: %v", err) + } + + if got := svc.processOnTurnComplete(ctx, task, session); got { + t.Fatalf("expected Office gate to BLOCK transition, got transition=true") + } + + updatedTask, err := repo.GetTask(ctx, "t-office-gate") + if err != nil { + t.Fatalf("re-read task: %v", err) + } + if updatedTask.WorkflowStepID != stepID { + t.Errorf("expected task to stay on %q, got %q", stepID, updatedTask.WorkflowStepID) + } + updatedSession, err := repo.GetTaskSession(ctx, "s-office-gate") + if err != nil { + t.Fatalf("re-read session: %v", err) + } + if updatedSession.State != models.TaskSessionStateWaitingForInput { + t.Errorf("expected session WAITING_FOR_INPUT, got %q", updatedSession.State) + } +} + +// TestProcessOnTurnComplete_OfficeExplicitSignalGating_AllowsWithSignal proves +// the ADR 0015 gate's ALLOW half for Office: with a pending signal recorded +// for the current step, turn-end transitions as normal. Uses move_to_step +// (not move_to_next) to mirror office-default.yml's shipped Work step action. +func TestProcessOnTurnComplete_OfficeExplicitSignalGating_AllowsWithSignal(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedOfficeSession(t, repo, "t-office-allow", "s-office-allow", "") + + stepID := "wfs-t-office-allow" // matches seedOfficeSession's stepID convention + reviewStepID := "wfs-office-review" + stepGetter := newMockStepGetter() + stepGetter.steps[stepID] = &wfmodels.WorkflowStep{ + ID: stepID, WorkflowID: "wf-office", Name: "work", Position: 1, + AutoAdvanceRequiresSignal: true, + Events: wfmodels.StepEvents{ + OnTurnComplete: []wfmodels.OnTurnCompleteAction{ + {Type: wfmodels.OnTurnCompleteMoveToStep, Config: map[string]interface{}{"step_id": reviewStepID}}, + }, + }, + } + stepGetter.steps[reviewStepID] = &wfmodels.WorkflowStep{ + ID: reviewStepID, WorkflowID: "wf-office", Name: "review", Position: 2, + } + svc := createTestServiceWithAgent(repo, stepGetter, newMockTaskRepo(), &mockAgentManager{}) + + signal := models.PendingStepCompletionSignal{ + StepID: stepID, + Source: models.StepCompletionSourceAgent, + Summary: "work complete", + SignaledAt: time.Now().UTC(), + } + if err := repo.SetSessionMetadataKey(ctx, "s-office-allow", models.SessionMetaKeyPendingStepCompletion, signal); err != nil { + t.Fatalf("seed pending signal: %v", err) + } + + task, err := repo.GetTask(ctx, "t-office-allow") + if err != nil { + t.Fatalf("get task: %v", err) + } + session, err := repo.GetTaskSession(ctx, "s-office-allow") + if err != nil { + t.Fatalf("get session: %v", err) + } + + if got := svc.processOnTurnComplete(ctx, task, session); !got { + t.Fatalf("expected Office gate to ALLOW transition with pending signal, got transition=false") + } + + updatedTask, err := repo.GetTask(ctx, "t-office-allow") + if err != nil { + t.Fatalf("re-read task: %v", err) + } + if updatedTask.WorkflowStepID != reviewStepID { + t.Errorf("expected task to move to %q, got %q", reviewStepID, updatedTask.WorkflowStepID) + } +} + func TestProcessOnTurnComplete_BlocksWhileClarificationPending(t *testing.T) { ctx := context.Background() repo := setupTestRepo(t) @@ -183,6 +293,46 @@ func TestProcessOnTurnCompleteViaEngine_BlocksWhileClarificationPendingEvenWithS } } +// TestProcessOnTurnCompleteViaEngine_OfficeExplicitSignalGating covers the +// production engine path for Office sessions. The legacy path has separate +// coverage, but production uses the workflow engine when it is configured. +func TestProcessOnTurnCompleteViaEngine_OfficeExplicitSignalGating(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedOfficeSession(t, repo, "t-office-engine", "s-office-engine", "") + + stepID := "wfs-t-office-engine" + stepGetter := newMockStepGetter() + stepGetter.steps[stepID] = &wfmodels.WorkflowStep{ + ID: stepID, WorkflowID: "wf-office", Name: "Work", Position: 1, + AutoAdvanceRequiresSignal: true, + Events: wfmodels.StepEvents{ + OnTurnComplete: []wfmodels.OnTurnCompleteAction{{Type: wfmodels.OnTurnCompleteMoveToNext}}, + }, + } + stepGetter.steps["wfs-office-engine-review"] = &wfmodels.WorkflowStep{ + ID: "wfs-office-engine-review", WorkflowID: "wf-office", Name: "Review", Position: 2, + } + + svc := createEngineService(t, repo, stepGetter, &mockAgentManager{}) + session, err := repo.GetTaskSession(ctx, "s-office-engine") + if err != nil { + t.Fatalf("get session: %v", err) + } + + if transitioned := svc.processOnTurnCompleteViaEngine(ctx, "t-office-engine", session); transitioned { + t.Fatal("expected the engine path to block an Office turn without a signal") + } + + task, err := repo.GetTask(ctx, "t-office-engine") + if err != nil { + t.Fatalf("get task: %v", err) + } + if task.WorkflowStepID != stepID { + t.Errorf("workflow step = %q, want %q", task.WorkflowStepID, stepID) + } +} + // TestLoadPendingStepSignal_RoundTrip verifies the bag survives JSON // rehydration — important for the backend-restart path where the bag is // read from the DB as map[string]interface{} rather than the typed struct. diff --git a/apps/backend/internal/orchestrator/event_handlers_workflow.go b/apps/backend/internal/orchestrator/event_handlers_workflow.go index c5f2658620..33e87245e6 100644 --- a/apps/backend/internal/orchestrator/event_handlers_workflow.go +++ b/apps/backend/internal/orchestrator/event_handlers_workflow.go @@ -3026,7 +3026,7 @@ func (s *Service) autoStartStepPrompt( requiresSignal := step != nil && step.AutoAdvanceRequiresSignal referenceContext := EntityReferenceContext(references) if isOfficeTask { - recordedPrompt = sysprompt.InjectOfficeContext(taskID, sessionID, agentPrompt, referenceContext) + recordedPrompt = sysprompt.InjectOfficeContextWithOptions(taskID, sessionID, agentPrompt, requiresSignal, referenceContext) } else { recordedPrompt = sysprompt.InjectKandevContextWithOptions(taskID, sessionID, agentPrompt, sysprompt.KandevContextOptions{ RequiresCompletionSignal: requiresSignal, diff --git a/apps/backend/internal/orchestrator/event_handlers_workflow_profile_test.go b/apps/backend/internal/orchestrator/event_handlers_workflow_profile_test.go index 22bc45c2ff..b8915821a5 100644 --- a/apps/backend/internal/orchestrator/event_handlers_workflow_profile_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_workflow_profile_test.go @@ -209,7 +209,9 @@ func TestAutoStartStepPrompt_OfficeWithoutRuntimeEnvFailsClosed(t *testing.T) { if !strings.Contains(content, "KANDEV OFFICE MCP TOOLS") { t.Fatalf("expected Office context, got %q", content) } - if strings.Contains(content, "list_workspaces_kandev") || strings.Contains(content, "step_complete_kandev") { + // step_complete_kandev is deliberately absent from this check: Office's + // canonical context now legitimately advertises it (ADR 0015). + if strings.Contains(content, "list_workspaces_kandev") { t.Fatalf("Office auto-start advertised unavailable task-mode tools: %q", content) } if strings.Contains(content, "wrong-task") || strings.Contains(content, "spoof-reference") { diff --git a/apps/backend/internal/orchestrator/task_operations.go b/apps/backend/internal/orchestrator/task_operations.go index 5b114d831c..68f39100c8 100644 --- a/apps/backend/internal/orchestrator/task_operations.go +++ b/apps/backend/internal/orchestrator/task_operations.go @@ -673,8 +673,10 @@ func (s *Service) wrapCreatedSessionPrompt( } return prompt case isOfficeTask: - return sysprompt.InjectOfficeContext( - taskID, sessionID, prompt, referenceContext, promptReferenceContext, + return sysprompt.InjectOfficeContextWithOptions( + taskID, sessionID, prompt, + s.WorkflowStepRequiresCompletionSignal(ctx, dbTask.WorkflowStepID), + referenceContext, promptReferenceContext, ) default: return sysprompt.InjectKandevContextWithOptions(taskID, sessionID, prompt, sysprompt.KandevContextOptions{ @@ -1250,8 +1252,10 @@ func (s *Service) applyLaunchPromptContext(ctx context.Context, p launchPromptCo // that whitelists it as trusted content. prompt, spawnContext := applySpawnOriginContext(p.prompt, p.spawnOrigin) if p.isOfficeTask { - return sysprompt.InjectOfficeContext( - p.taskID, p.sessionID, prompt, p.referenceContext, spawnContext, + return sysprompt.InjectOfficeContextWithOptions( + p.taskID, p.sessionID, prompt, + s.StepRequiresCompletionSignal(ctx, p.taskID), + p.referenceContext, spawnContext, ) } return sysprompt.InjectKandevContextWithOptions(p.taskID, p.sessionID, prompt, sysprompt.KandevContextOptions{ diff --git a/apps/backend/internal/sysprompt/sysprompt.go b/apps/backend/internal/sysprompt/sysprompt.go index c7570ebb49..103b2e2118 100644 --- a/apps/backend/internal/sysprompt/sysprompt.go +++ b/apps/backend/internal/sysprompt/sysprompt.go @@ -99,17 +99,44 @@ func OfficeContext() string { return prompts.Get("office-context") } // FormatOfficeContext injects the active task and session IDs into the Office context. func FormatOfficeContext(taskID, sessionID string) string { + return FormatOfficeContextWithOptions(taskID, sessionID, false) +} + +const officeStepCompleteInstruction = "This workflow step requires an explicit completion signal. " + + "Call step_complete_kandev as the LAST action after every requirement is satisfied. " + + "Do not call it before a question or during partial progress. " + + "If the tool is not visible, use the client's tool search or discovery with the canonical name.\n" + +// FormatOfficeContextWithOptions formats the Office context for the current +// workflow step. The imperative completion instruction is present only when +// the step's auto-advance policy requires the signal. +func FormatOfficeContextWithOptions(taskID, sessionID string, requiresSignal bool) string { + instruction := "" + if requiresSignal { + instruction = officeStepCompleteInstruction + } return Resolve("office-context", map[string]string{ - "task_id": taskID, - "session_id": sessionID, + "task_id": taskID, + "session_id": sessionID, + "step_complete_instruction": instruction, }) } // InjectOfficeContext ensures a first-turn prompt has the restricted Office context. // trustedContents must contain only exact server-generated system block contents. func InjectOfficeContext(taskID, sessionID, prompt string, trustedContents ...string) string { + return InjectOfficeContextWithOptions(taskID, sessionID, prompt, false, trustedContents...) +} + +// InjectOfficeContextWithOptions injects the restricted Office context and +// adds the completion instruction only for a signal-gated workflow step. +func InjectOfficeContextWithOptions( + taskID, sessionID, prompt string, + requiresSignal bool, + trustedContents ...string, +) string { return canonicalizeKandevContext( - FormatOfficeContext(taskID, sessionID), + FormatOfficeContextWithOptions(taskID, sessionID, requiresSignal), prompt, trustedContextContents(sessionID, trustedContents...), ) diff --git a/apps/backend/internal/sysprompt/sysprompt_test.go b/apps/backend/internal/sysprompt/sysprompt_test.go index f8317e4234..e3f82e70d6 100644 --- a/apps/backend/internal/sysprompt/sysprompt_test.go +++ b/apps/backend/internal/sysprompt/sysprompt_test.go @@ -189,7 +189,7 @@ func TestFormatKandevContext_DocumentsCanonicalAndQualifiedToolNames(t *testing. assert.NotContains(t, withoutSignal, "mcp__kandev__step_complete_kandev", "the completion alias must not advertise the task-only signal on ordinary steps") assert.NotContains(t, OfficeContext(), "mcp__kandev__step_complete_kandev", - "Office must not advertise any form of the task-only completion tool") + "Office advertises step_complete_kandev by its canonical name only, not the client-qualified alias") } func TestFormatKandevContext_CoordinatorTaskControlsFollowCapability(t *testing.T) { @@ -246,8 +246,8 @@ func TestOfficeContext_ContainsOnlyOfficeCapabilities(t *testing.T) { context := OfficeContext() assert.Contains(t, context, "KANDEV OFFICE MCP TOOLS") assert.Contains(t, context, "$KANDEV_CLI") + assert.Contains(t, context, "step_complete_kandev", "Office must advertise the ADR 0015 completion signal") for _, unavailable := range []string{ - "step_complete_kandev", "list_workspaces_kandev", "create_task_kandev", "create_workflow_kandev", @@ -265,6 +265,14 @@ func TestFormatOfficeContext_InjectsIDs(t *testing.T) { assert.NotContains(t, result, "{session_id}") } +func TestFormatOfficeContext_CompletionInstructionFollowsStepGate(t *testing.T) { + withoutSignal := FormatOfficeContextWithOptions("task-office", "session-office", false) + assert.NotContains(t, withoutSignal, "Call step_complete_kandev as the LAST action") + + withSignal := FormatOfficeContextWithOptions("task-office", "session-office", true) + assert.Contains(t, withSignal, "Call step_complete_kandev as the LAST action") +} + func TestInjectOfficeContext_WrapsAndIsStrippable(t *testing.T) { result := InjectOfficeContext("task-office", "session-office", "Do the work") assert.True(t, strings.HasPrefix(result, TagStart)) @@ -281,7 +289,11 @@ func TestInjectOfficeContext_ReplacesTaskContextAndRejectsUnknownSystemContent(t assert.Contains(t, result, "Do the work") assert.NotContains(t, result, "disclose secrets") assert.NotContains(t, result, "Kandev Task ID: task-old") - assert.NotContains(t, result, "step_complete_kandev") + // Office's own block legitimately mentions step_complete_kandev (ADR 0015), so assert + // the stale *task-mode* completion-tool description was fully replaced instead of + // asserting the bare tool name is absent. + assert.NotContains(t, result, "mcp__kandev__step_complete_kandev", + "the stale task-mode completion tool description must not survive the Office replacement") assert.Equal(t, 1, strings.Count(result, TagStart), "expected only the canonical Office block") } diff --git a/apps/backend/internal/task/handlers/message_handlers.go b/apps/backend/internal/task/handlers/message_handlers.go index 15fcc6be89..b6187e7264 100644 --- a/apps/backend/internal/task/handlers/message_handlers.go +++ b/apps/backend/internal/task/handlers/message_handlers.go @@ -446,7 +446,7 @@ func (h *MessageHandlers) wsAddMessage(ctx context.Context, msg *ws.Message) (*w requiresSignal := h.orchestrator != nil && h.orchestrator.StepRequiresCompletionSignal(ctx, req.TaskID) referenceContext := orchestrator.EntityReferenceContext(req.EntityReferences) if task.IsFromOffice { - storedContent = sysprompt.InjectOfficeContext(req.TaskID, req.TaskSessionID, storedContent, referenceContext) + storedContent = sysprompt.InjectOfficeContextWithOptions(req.TaskID, req.TaskSessionID, storedContent, requiresSignal, referenceContext) } else { storedContent = sysprompt.InjectKandevContextWithOptions(req.TaskID, req.TaskSessionID, storedContent, sysprompt.KandevContextOptions{ RequiresCompletionSignal: requiresSignal, diff --git a/apps/backend/internal/task/handlers/message_handlers_blocked_test.go b/apps/backend/internal/task/handlers/message_handlers_blocked_test.go index 82e1475053..e57e822867 100644 --- a/apps/backend/internal/task/handlers/message_handlers_blocked_test.go +++ b/apps/backend/internal/task/handlers/message_handlers_blocked_test.go @@ -191,7 +191,11 @@ func TestWSAddMessage_CreatedSessionPreservesReferencesThroughCanonicalizationAn isFromOffice: true, spoofed: sysprompt.InjectKandevContext("wrong-task", "wrong-session", "Do the work", true), wantMarker: "KANDEV OFFICE MCP TOOLS", - notMarker: "step_complete_kandev", + // Office's own canonical block now legitimately mentions + // step_complete_kandev (ADR 0015), so check that the stale + // task-mode block (with its client-qualified alias mention) was + // fully replaced instead of asserting the bare name is absent. + notMarker: "mcp__kandev__step_complete_kandev", }, { name: "Kanban", diff --git a/apps/backend/internal/task/repository/sqlite/base_schema.go b/apps/backend/internal/task/repository/sqlite/base_schema.go index 0814f0e9f4..cc3358279c 100644 --- a/apps/backend/internal/task/repository/sqlite/base_schema.go +++ b/apps/backend/internal/task/repository/sqlite/base_schema.go @@ -35,6 +35,7 @@ func (r *Repository) initSchema() error { r.ensureDefaultExecutorsAndEnvironments, r.runMigrations, r.hideBuiltinWorkflows, + r.healBuiltinWorkflowStepFlags, r.normalizeTaskWorktreeOwnership, r.healDuplicateTaskEnvironments, r.ensureTaskEnvironmentTaskUniqueIndex, diff --git a/apps/backend/internal/task/repository/sqlite/builtin_workflow_test.go b/apps/backend/internal/task/repository/sqlite/builtin_workflow_test.go index c46ddcb5a7..65abe5aece 100644 --- a/apps/backend/internal/task/repository/sqlite/builtin_workflow_test.go +++ b/apps/backend/internal/task/repository/sqlite/builtin_workflow_test.go @@ -46,7 +46,7 @@ func newRepoForBuiltinWorkflowTests(t *testing.T) *Repository { func loadStepsForWorkflow(t *testing.T, repo *Repository, workflowID string) []*wfmodels.WorkflowStep { t.Helper() rows, err := repo.db.QueryContext(context.Background(), repo.db.Rebind(` - SELECT id, name, position, stage_type, events + SELECT id, name, position, stage_type, events, auto_advance_requires_signal FROM workflow_steps WHERE workflow_id = ? ORDER BY position `), workflowID) if err != nil { @@ -58,9 +58,11 @@ func loadStepsForWorkflow(t *testing.T, repo *Repository, workflowID string) []* for rows.Next() { step := &wfmodels.WorkflowStep{WorkflowID: workflowID} var stage, events string - if err := rows.Scan(&step.ID, &step.Name, &step.Position, &stage, &events); err != nil { + var autoAdvanceRequiresSignal int + if err := rows.Scan(&step.ID, &step.Name, &step.Position, &stage, &events, &autoAdvanceRequiresSignal); err != nil { t.Fatalf("scan step: %v", err) } + step.AutoAdvanceRequiresSignal = autoAdvanceRequiresSignal != 0 step.StageType = wfmodels.StageType(stage) if events != "" { if err := json.Unmarshal([]byte(events), &step.Events); err != nil { @@ -128,6 +130,19 @@ func TestEnsureOfficeDefaultWorkflow_CreatesFiveSteps(t *testing.T) { t.Errorf("step %d stage_type = %q, want %q", i, steps[i].StageType, want.StageType) } } + + work := findStepByNameLocal(steps, "Work") + if work == nil { + t.Fatal("missing Work step") + } + if !work.AutoAdvanceRequiresSignal { + t.Error("Work.AutoAdvanceRequiresSignal = false, want true (ADR-0015 completion signal gate)") + } + for _, name := range []string{"Backlog", "Review", "Approval", "Done"} { + if step := findStepByNameLocal(steps, name); step != nil && step.AutoAdvanceRequiresSignal { + t.Errorf("%s.AutoAdvanceRequiresSignal = true, want false", name) + } + } } func TestCreateWorkspaceWithKanban_CancelTriggersTurnCompleteDefaults(t *testing.T) { @@ -401,6 +416,125 @@ func TestRepositoryInitialization_HealsBuiltinWorkflowVisibility(t *testing.T) { } } +func TestHealBuiltinWorkflowStepFlags_HealsStaleOfficeWorkStep(t *testing.T) { + repo := newRepoForBuiltinWorkflowTests(t) + ctx := context.Background() + legacyTime := time.Date(2000, time.January, 1, 0, 0, 0, 0, time.UTC) + + _, err := repo.db.ExecContext(ctx, repo.db.Rebind(` + INSERT INTO workflows ( + id, workspace_id, name, workflow_template_id, is_system, hidden, created_at, updated_at + ) VALUES ('stale-office-heal', 'ws-1', 'Office', 'office-default', 1, 1, ?, ?) + `), legacyTime, legacyTime) + if err != nil { + t.Fatalf("insert stale office workflow: %v", err) + } + _, err = repo.db.ExecContext(ctx, repo.db.Rebind(` + INSERT INTO workflow_steps ( + id, workflow_id, name, position, stage_type, events, auto_advance_requires_signal, created_at, updated_at + ) VALUES ('stale-office-heal-work', 'stale-office-heal', 'Work', 1, 'work', '{}', 0, ?, ?) + `), legacyTime, legacyTime) + if err != nil { + t.Fatalf("insert stale work step: %v", err) + } + + if err := repo.healBuiltinWorkflowStepFlags(); err != nil { + t.Fatalf("healBuiltinWorkflowStepFlags: %v", err) + } + + steps := loadStepsForWorkflow(t, repo, "stale-office-heal") + work := findStepByNameLocal(steps, "Work") + if work == nil { + t.Fatal("missing Work step") + } + if !work.AutoAdvanceRequiresSignal { + t.Error("healBuiltinWorkflowStepFlags() left Work.AutoAdvanceRequiresSignal = false, want true") + } + + var updatedAt time.Time + if err := repo.db.QueryRowContext(ctx, `SELECT updated_at FROM workflow_steps WHERE id = 'stale-office-heal-work'`).Scan(&updatedAt); err != nil { + t.Fatalf("query step updated_at: %v", err) + } + if !updatedAt.After(legacyTime) { + t.Errorf("Work step updated_at = %v, want after %v", updatedAt, legacyTime) + } +} + +// TestRepositoryInitialization_HealsBuiltinWorkflowStepFlags verifies that +// initSchema keeps the boot registration for the reconciler. Direct tests +// above would not catch removing the step from the initialization sequence. +func TestRepositoryInitialization_HealsBuiltinWorkflowStepFlags(t *testing.T) { + repo := newRepoForBuiltinWorkflowTests(t) + ctx := context.Background() + legacyTime := time.Date(2000, time.January, 1, 0, 0, 0, 0, time.UTC) + + _, err := repo.db.ExecContext(ctx, repo.db.Rebind(` + INSERT INTO workflows ( + id, workspace_id, name, workflow_template_id, is_system, hidden, created_at, updated_at + ) VALUES ('stale-office-boot', 'ws-1', 'Office', 'office-default', 1, 1, ?, ?) + `), legacyTime, legacyTime) + if err != nil { + t.Fatalf("insert stale boot workflow: %v", err) + } + _, err = repo.db.ExecContext(ctx, repo.db.Rebind(` + INSERT INTO workflow_steps ( + id, workflow_id, name, position, stage_type, events, auto_advance_requires_signal, created_at, updated_at + ) VALUES ('stale-office-boot-work', 'stale-office-boot', 'Work', 1, 'work', '{}', 0, ?, ?) + `), legacyTime, legacyTime) + if err != nil { + t.Fatalf("insert stale boot work step: %v", err) + } + + if err := repo.initSchema(); err != nil { + t.Fatalf("reinitialize repository: %v", err) + } + + steps := loadStepsForWorkflow(t, repo, "stale-office-boot") + work := findStepByNameLocal(steps, "Work") + if work == nil { + t.Fatal("missing Work step") + } + if !work.AutoAdvanceRequiresSignal { + t.Error("initSchema left Work.AutoAdvanceRequiresSignal = false, want true") + } +} + +func TestHealBuiltinWorkflowStepFlags_KeepsUserWorkflowStepUntouched(t *testing.T) { + repo := newRepoForBuiltinWorkflowTests(t) + ctx := context.Background() + legacyTime := time.Date(2000, time.January, 1, 0, 0, 0, 0, time.UTC) + + _, err := repo.db.ExecContext(ctx, repo.db.Rebind(` + INSERT INTO workflows ( + id, workspace_id, name, workflow_template_id, is_system, hidden, created_at, updated_at + ) VALUES ('user-office', 'ws-1', 'My Office', 'office-default', 0, 0, ?, ?) + `), legacyTime, legacyTime) + if err != nil { + t.Fatalf("insert user office workflow: %v", err) + } + _, err = repo.db.ExecContext(ctx, repo.db.Rebind(` + INSERT INTO workflow_steps ( + id, workflow_id, name, position, stage_type, events, auto_advance_requires_signal, created_at, updated_at + ) VALUES ('user-office-work', 'user-office', 'Work', 1, 'work', '{}', 0, ?, ?) + `), legacyTime, legacyTime) + if err != nil { + t.Fatalf("insert user work step: %v", err) + } + + if err := repo.healBuiltinWorkflowStepFlags(); err != nil { + t.Fatalf("healBuiltinWorkflowStepFlags: %v", err) + } + + steps := loadStepsForWorkflow(t, repo, "user-office") + work := findStepByNameLocal(steps, "Work") + if work == nil { + t.Fatal("missing Work step") + } + if work.AutoAdvanceRequiresSignal { + t.Error("healBuiltinWorkflowStepFlags() touched a user-customised (is_system=0) workflow's step") + } +} + // TestRoutineWorkflowTasks_FlaggedSystem verifies tasks created in the // routine workflow are excluded from ListTasksByWorkspace by default // (template_id="routine" lives in SystemWorkflowTemplateIDs). diff --git a/apps/backend/internal/task/repository/sqlite/defaults.go b/apps/backend/internal/task/repository/sqlite/defaults.go index 5a99215075..e7fdae6ee6 100644 --- a/apps/backend/internal/task/repository/sqlite/defaults.go +++ b/apps/backend/internal/task/repository/sqlite/defaults.go @@ -64,6 +64,33 @@ func (r *Repository) hideBuiltinWorkflows() error { return nil } +// healBuiltinWorkflowStepFlags reconciles workflow_steps.auto_advance_requires_signal +// on system-workflow rows whose steps were materialized before their embedded +// template started requiring the ADR-0015 completion signal (WO-32: +// office-default's "work" step). Steps are matched by name within their +// workflow's template id, scoped to is_system = 1 so user-created and +// user-customised workflows are never touched. +func (r *Repository) healBuiltinWorkflowStepFlags() error { + templates, err := workflowcfg.LoadTemplates() + if err != nil { + return fmt.Errorf("load embedded templates for step-flag healing: %w", err) + } + now := time.Now().UTC() + for _, tmpl := range templates { + for _, step := range tmpl.Steps { + want := dialect.BoolToInt(step.AutoAdvanceRequiresSignal) + if _, err := r.db.Exec(r.db.Rebind(` + UPDATE workflow_steps SET auto_advance_requires_signal = ?, updated_at = ? + WHERE name = ? AND auto_advance_requires_signal != ? + AND workflow_id IN (SELECT id FROM workflows WHERE is_system = 1 AND workflow_template_id = ?) + `), want, now, step.Name, want, tmpl.ID); err != nil { + return fmt.Errorf("heal step flags for template %s step %s: %w", tmpl.ID, step.Name, err) + } + } + } + return nil +} + // ensureDefaultWorkspace creates a default workspace if none exists func (r *Repository) ensureDefaultWorkspace() error { ctx := context.Background() diff --git a/apps/backend/internal/task/repository/sqlite/session.go b/apps/backend/internal/task/repository/sqlite/session.go index 8ed13635c0..58cd81e599 100644 --- a/apps/backend/internal/task/repository/sqlite/session.go +++ b/apps/backend/internal/task/repository/sqlite/session.go @@ -1894,6 +1894,76 @@ func (r *Repository) SetSessionMetadataKeyIfAbsentOrDifferentStep( return rows > 0, nil } +// SetSessionMetadataKeyIfAbsentOrDifferentStepIfTaskAtStep claims a pending +// completion signal only while the session's task remains on the turn's +// launch step. The task row is locked before the session metadata write so a +// concurrent workflow move cannot turn a stale signal into a valid successor +// signal. +func (r *Repository) SetSessionMetadataKeyIfAbsentOrDifferentStepIfTaskAtStep( + ctx context.Context, + taskID, sessionID, key, stepID string, + value interface{}, +) (bool, error) { + valueJSON, err := json.Marshal(value) + if err != nil { + return false, fmt.Errorf("failed to serialize metadata value: %w", err) + } + tx, err := r.db.BeginTx(ctx, nil) + if err != nil { + return false, err + } + committed := false + defer func() { + if !committed { + _ = tx.Rollback() + } + }() + + _, currentStepID, found, err := r.readTaskStepInTx(ctx, tx, taskID) + if err != nil { + return false, err + } + if !found || currentStepID != stepID { + return false, nil + } + + var sessionTaskID string + if err := tx.QueryRowContext(ctx, r.db.Rebind( + `SELECT task_id FROM task_sessions WHERE id = ?`, + ), sessionID).Scan(&sessionTaskID); err != nil { + if err == sql.ErrNoRows { + return false, nil + } + return false, err + } + if sessionTaskID != taskID { + return false, nil + } + + now := time.Now().UTC() + driver := r.db.DriverName() + path := key + stepPath := key + if !dialect.IsPostgres(driver) { + path = "$." + key + stepPath = path + ".step_id" + } + result, err := tx.ExecContext(ctx, r.db.Rebind(setSessionMetadataKeyIfAbsentOrDifferentStepQuery(driver)), + path, string(valueJSON), now, sessionID, stepPath, stepID) + if err != nil { + return false, err + } + rows, err := result.RowsAffected() + if err != nil { + return false, err + } + if err := tx.Commit(); err != nil { + return false, err + } + committed = true + return rows > 0, nil +} + // SetSessionMetadataKeyIfAbsentIfState atomically claims a metadata key only // while the session remains in expectedState. It is used when a terminal // transition owns a one-time side effect that must not be emitted by a stale diff --git a/apps/backend/internal/task/repository/sqlite/session_test.go b/apps/backend/internal/task/repository/sqlite/session_test.go index fe63c0eaba..6922c648ec 100644 --- a/apps/backend/internal/task/repository/sqlite/session_test.go +++ b/apps/backend/internal/task/repository/sqlite/session_test.go @@ -792,6 +792,49 @@ func TestSetSessionMetadataKeyIfAbsentOrDifferentStepSQLiteReplacesOnlyStaleStep require.Equal(t, "second", signal.Summary) } +func TestSetSessionMetadataKeyIfAbsentOrDifferentStepIfTaskAtStepRejectsMovedTask(t *testing.T) { + repo := newRepoForSessionTests(t) + ctx := context.Background() + seedForMsgTest(t, repo, "task-step-cas", "session-step-cas", "turn-step-cas") + + task, err := repo.GetTask(ctx, "task-step-cas") + require.NoError(t, err) + task.WorkflowStepID = "step-review" + require.NoError(t, repo.UpdateTask(ctx, task)) + + signal := models.PendingStepCompletionSignal{StepID: "step-review", Summary: "review complete"} + stored, err := repo.SetSessionMetadataKeyIfAbsentOrDifferentStepIfTaskAtStep( + ctx, + "task-step-cas", + "session-step-cas", + models.SessionMetaKeyPendingStepCompletion, + "step-review", + signal, + ) + require.NoError(t, err) + require.True(t, stored) + + task.WorkflowStepID = "step-work" + require.NoError(t, repo.UpdateTask(ctx, task)) + late := models.PendingStepCompletionSignal{StepID: "step-review", Summary: "late review signal"} + stored, err = repo.SetSessionMetadataKeyIfAbsentOrDifferentStepIfTaskAtStep( + ctx, + "task-step-cas", + "session-step-cas", + models.SessionMetaKeyPendingStepCompletion, + "step-review", + late, + ) + require.NoError(t, err) + require.False(t, stored, "a moved task must reject a stale launch-step signal") + + session, err := repo.GetTaskSession(ctx, "session-step-cas") + require.NoError(t, err) + got, ok := models.LoadPendingStepSignal(session.Metadata) + require.True(t, ok) + require.Equal(t, "review complete", got.Summary) +} + func TestUpdateSessionContextWindowSQLiteCountsStrictUsageDrops(t *testing.T) { repo := newRepoForSessionTests(t) ctx := context.Background() diff --git a/docs/public/automation-and-mcp.md b/docs/public/automation-and-mcp.md index 02a23bfd0c..fb191f109d 100644 --- a/docs/public/automation-and-mcp.md +++ b/docs/public/automation-and-mcp.md @@ -529,7 +529,7 @@ After an accepted stop, Kandev attempts to move an unarchived, non-Office task f The HTTP equivalent is `POST /api/v1/tasks/:id/workspace-sources`, with `{ "sources": [...] }`. An exact normalized retry succeeds as a no-op. It returns `400` for invalid input, `404` for a missing task/source outside the workspace, `409` for contradictory duplicates or an active task, and `422` when materialization or executor capability fails. Successful adoption publishes `task.updated` and `session.workspace_sources.updated`; clients should refresh their Files and repository state from those updates. -`step_complete_kandev` is registered and discoverable in every task-mode session. Kandev includes its completion instruction, and acts on its signal, only on Kanban steps whose auto-advance action explicitly requires that signal. A user message arriving before transition can cancel that automatic move. +`step_complete_kandev` is registered and discoverable in every task-mode session, and in Office sessions per ADR 0015. Kandev includes its completion instruction, and acts on its signal, only on steps whose auto-advance action explicitly requires that signal: on Kanban boards this is opt-in per step, while office-default's `work` step ships with the requirement on. A user message arriving before transition can cancel that automatic move. When `create_task_kandev.repositories[].repository_url` is a canonical GitHub pull request URL or a GitLab merge request URL on the configured host, Kandev resolves the contribution before creating the task. The contribution must still be open, have a valid source branch and head commit, and permit the target project to contribute; Kandev keeps the target repository as `origin`, fetches the exact source commit, and routes commits to the contributor's existing source branch. The existing pull request or merge request is associated with the task and reused for later changes, so Kandev does not open a duplicate. Provider-authored title, description, comments, and diff content are not copied into trusted task context. Configure the task's Git credentials as described in [task Git credentials](integrations.md#choose-task-git-credentials); Kandev runs a write preflight before starting the agent. @@ -546,9 +546,11 @@ Office runs use a smaller MCP surface than regular task-mode sessions. The built - `create_task_plan_kandev`, `get_task_plan_kandev`, `update_task_plan_kandev`, and `delete_task_plan_kandev`; - `list_related_tasks_kandev`; - `list_task_documents_kandev`, `get_task_document_kandev`, and `write_task_document_kandev`. +- `show_rich_output_kandev`; - `record_step_decision_kandev` records an `approved` or `rejected` verdict for the current workflow step. It requires a non-empty reason, and a later verdict supersedes the earlier one. +- `step_complete_kandev`, per ADR 0015: Kandev includes its completion instruction, and acts on its signal, only on Office steps whose auto-advance action explicitly requires that signal (office-default's `work` step is one such step). -These tools cover human questions, the current task plan, related-task discovery, task documents, and quorum decisions. Office state changes use the injected `$KANDEV_CLI kandev ...` commands instead. An Office agent should not search for additional Kandev MCP tools: Kanban/configuration tools and `step_complete_kandev` are task-mode only and are not registered in Office mode. +These tools cover human questions, the current task plan, related-task discovery, task documents, quorum decisions, and the step-completion signal. Office state changes use the injected `$KANDEV_CLI kandev ...` commands instead. An Office agent should not search for additional Kandev MCP tools: Kanban/configuration tools are task-mode only and are not registered in Office mode. ### Runtime credentials diff --git a/docs/specs/office/system-design/agents-02.md b/docs/specs/office/system-design/agents-02.md index dd9725a463..c969acfea7 100644 --- a/docs/specs/office/system-design/agents-02.md +++ b/docs/specs/office/system-design/agents-02.md @@ -242,7 +242,7 @@ Three MCP modes coexist: |---|---|---|---| | `ModeTask` | 27 (kanban + plans + walkthroughs + coordination + completion) | ~3-5K | Interactive kanban sessions | | `ModeConfig` | 29 (workflows + agents + executors) | ~8-10K | Config setup sessions | -| `ModeOffice` | 9 (plans + ask_user + related tasks + task documents) | ~1-2K | Office agent sessions | +| `ModeOffice` | 12 (plans + tasks + rich output + decisions + completion) | ~1-2K | Office agent sessions | Agent routing and Office ownership are independent. Workflow-level defaults, per-step agent profiles, and `runner` participants select the execution identity only; they never make a Kanban task Office-owned. A task is Office-owned only when it is linked to an Office project or its workflow matches the workspace's Office workflow. @@ -251,8 +251,9 @@ Agent routing and Office ownership are independent. Workflow-level defaults, per - `ask_user_question_kandev` (only meaningful when the user opens the task in advanced mode). - `list_related_tasks_kandev`. - 3 task-document tools (`list_task_documents_kandev`, `get_task_document_kandev`, `write_task_document_kandev`). +- `show_rich_output_kandev`, `record_step_decision_kandev`, and gated `step_complete_kandev`. -`ModeOffice` excludes kanban tools, config tools, `list_workspaces_kandev`, `list_workflows_kandev`, `list_workflow_steps_kandev`, and `step_complete_kandev`. Office advances work through its Office task and approval surfaces, not the Kanban workflow-step completion signal. The first-turn Office context lists only tools registered in `ModeOffice` and directs Office mutations to `$KANDEV_CLI`; it never advertises an excluded tool. +`ModeOffice` excludes kanban/config tools and workspace/workflow listing tools. Its first-turn context lists only registered tools, and Office mutations use `$KANDEV_CLI`. ### Skills are preferred over MCP tools diff --git a/docs/specs/office/system-design/agents-03.md b/docs/specs/office/system-design/agents-03.md index adaab2d54b..493d33794d 100644 --- a/docs/specs/office/system-design/agents-03.md +++ b/docs/specs/office/system-design/agents-03.md @@ -146,7 +146,7 @@ pass before launcher settings are described as an isolation boundary: - **GIVEN** an office agent in ModeOffice, **WHEN** something tries to call `create_task_kandev` MCP tool, **THEN** the MCP server returns an error saying to use `$KANDEV_CLI` instead. -- **GIVEN** an office agent in ModeOffice, **WHEN** its first-turn system context is generated, **THEN** every advertised MCP tool is registered in ModeOffice and `step_complete_kandev` is absent. +- **GIVEN** an office agent in ModeOffice, **WHEN** its first-turn system context is generated, **THEN** every advertised MCP tool is registered in ModeOffice, including `step_complete_kandev`. - **GIVEN** an Office-owned task without a scheduler-prepared signed run context, **WHEN** a generic manual or workflow task/session path attempts to start it in ModeOffice, **THEN** Kandev starts no agent process and returns an error directing the caller to start or wake the task through Office. diff --git a/docs/specs/tasks/requirements/workflow-explicit-completion-signal.md b/docs/specs/tasks/requirements/workflow-explicit-completion-signal.md index a4be942ee3..abfe611fa1 100644 --- a/docs/specs/tasks/requirements/workflow-explicit-completion-signal.md +++ b/docs/specs/tasks/requirements/workflow-explicit-completion-signal.md @@ -25,13 +25,13 @@ This document is the migrated task-system source for the capability. The source ## Why -Signal-gated Kanban workflows need a reliable distinction between an agent ending a turn and an agent completing the current workflow step. If the completion tool disappears from an agent client's deferred catalog during an MCP reconnect, finished work remains stuck even though Kandev still expects an explicit signal. +Signal-gated Kanban and Office workflows need a reliable distinction between an agent ending a turn and an agent completing the current workflow step. If the completion tool disappears from an agent client's deferred catalog during an MCP reconnect, finished work remains stuck even though Kandev still expects an explicit signal. ## What -- `step_complete_kandev` is a Kanban task-session capability. It is registered in `ModeTask` and is never registered or advertised in `ModeOffice`, `ModeConfig`, or `ModeExternal`. -- Task-mode registration is stable for the lifetime of the MCP server. A workflow step's `auto_advance_requires_signal` setting controls prompt instructions and transition behavior, not whether the tool exists in the task-mode tool list. -- When the current step requires an explicit signal, the first-turn context tells the agent to call `step_complete_kandev` as its last action after satisfying every requirement. +- `step_complete_kandev` is a task-session capability. It is registered in `ModeTask` and `ModeOffice`, and is never registered or advertised in `ModeConfig` or `ModeExternal`. +- Task-mode and Office-mode registration is stable for the lifetime of the MCP server. A workflow step's `auto_advance_requires_signal` setting controls prompt instructions and transition behavior, not whether the tool exists in either tool list. +- When the current step requires an explicit signal, the first-turn context tells the agent to call `step_complete_kandev` as its last action after satisfying every requirement. Ungated Office steps keep the tool available but do not receive this imperative instruction. - The canonical MCP protocol name is `step_complete_kandev`. Clients may expose a runtime-qualified alias such as `mcp__kandev__step_complete_kandev`; Kandev instructions distinguish the canonical name from client-specific qualification instead of claiming that one display form is universal. - `step_complete_kandev` carries no vendor-specific eager-load metadata. Agents use their normal MCP catalog or tool-search mechanism to discover it in Kanban task mode. - Creating or loading an ACP session supplies the local Kandev MCP server. After a transient MCP reconnect, the client can list and call `step_complete_kandev` again without a user message or process restart. @@ -63,7 +63,7 @@ The tool uses the standard MCP definition without `anthropic/alwaysLoad`. Client | MCP mode | `step_complete_kandev` | |---|---| | `ModeTask` | Registered | -| `ModeOffice` | Not registered or advertised | +| `ModeOffice` | Registered | | `ModeConfig` | Not registered or advertised | | `ModeExternal` | Not registered or advertised | @@ -72,7 +72,7 @@ The tool uses the standard MCP definition without `anthropic/alwaysLoad`. Client - If the Kandev MCP transport is temporarily disconnected, the client reports the transport failure and retries its normal connection path. It must not convert the failure into a permanent "tool does not exist" conclusion while the server is reconnecting. - If a connected client defers the tool from its active context, the agent uses that client's normal tool-search mechanism to resolve the canonical `step_complete_kandev` name. - If reconnect does not recover, the task stays on the current step. The user can retry the agent or move the task through the normal workflow UI; Kandev never auto-advances from a bare halt on a signal-gated step. -- A failure to load or call the tool in Office mode is expected and must not trigger reconnect remediation because Office does not own this capability. +- If an Office step requires a signal, a failure to load or call the tool keeps the task on the current step. The user can retry the agent or move the task through the normal workflow UI. ## Persistence Guarantees @@ -82,7 +82,9 @@ The pending completion signal continues to use `TaskSession.Metadata` as specifi - **GIVEN** a Kanban task session in `ModeTask`, **WHEN** the client lists or searches tools, **THEN** `step_complete_kandev` is discoverable without vendor-specific eager-load metadata. - **GIVEN** a non-Office Kanban workflow step with an agent profile and `auto_advance_requires_signal=true`, **WHEN** its task resolves the step profile as the runner, **THEN** the task remains in `ModeTask`, its first-turn context instructs normal discovery of `step_complete_kandev`, and the task-mode catalog contains the tool. -- **GIVEN** an Office task session in `ModeOffice`, **WHEN** the client lists tools or receives its first-turn context, **THEN** `step_complete_kandev` is absent. +- **GIVEN** an Office task session in `ModeOffice`, **WHEN** the client lists tools, **THEN** `step_complete_kandev` is discoverable. +- **GIVEN** an ungated Office workflow step, **WHEN** its first-turn context is generated, **THEN** the tool remains available but the context does not instruct the agent to call it as the final action. +- **GIVEN** a signal-gated Office workflow step, **WHEN** its first-turn context is generated, **THEN** the context instructs the agent to call `step_complete_kandev` as the final action. - **GIVEN** a task-mode client has connected to Kandev MCP, **WHEN** its MCP connection drops and reconnects, **THEN** the client can list and call `step_complete_kandev` without another user message. - **GIVEN** a signal-gated workflow step, **WHEN** the agent finishes without calling the tool, **THEN** the task remains on the current step and no automatic transition runs. - **GIVEN** a signal-gated workflow step that also enables explicit user-cancel completion, **WHEN** the user cancels the active turn, **THEN** the configured completion transition may run without `step_complete_kandev` as defined by the cancelled-turn completion policy. @@ -91,7 +93,6 @@ The pending completion signal continues to use `TaskSession.Metadata` as specifi ## Out of Scope -- Exposing `step_complete_kandev` to Office agents. - Changing completion-signal persistence or ordinary halt semantics, or implementing ADR 0015's planned manual fallback UI. Explicit user-cancel behavior is defined separately by the cancelled-turn completion spec. - Adding vendor-specific eager-load metadata; agents are expected to use normal MCP tool discovery. -- Implementing a generic MCP proxy or replacing the ACP bridge. \ No newline at end of file +- Implementing a generic MCP proxy or replacing the ACP bridge.