From 9a91446b729de500eaec8728f9c4e22b64fb2aaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Fri, 18 Sep 2026 10:33:43 +0100 Subject: [PATCH] fix: recover stale workflow completion signals (#3772) (#3788) --- .../backend/internal/mcp/handlers/handlers.go | 8 +- .../handlers/step_complete_recovery_test.go | 140 ++++++++++++++++++ apps/backend/internal/mcp/server/server.go | 2 +- .../mcp/server/step_complete_recovery_test.go | 46 ++++++ .../sysprompt/step_complete_recovery_test.go | 37 +++++ apps/backend/internal/sysprompt/sysprompt.go | 11 +- .../plan.md | 139 +++++++++++++++++ .../task-01-completion-recovery.md | 104 +++++++++++++ docs/public/automation-and-mcp.md | 2 + docs/public/tasks-and-workflows.md | 2 + .../workflow-explicit-completion-signal.md | 31 ++++ .../workflow-explicit-completion-signal.md | 89 +++++++++++ 12 files changed, 607 insertions(+), 4 deletions(-) create mode 100644 apps/backend/internal/mcp/handlers/step_complete_recovery_test.go create mode 100644 apps/backend/internal/mcp/server/step_complete_recovery_test.go create mode 100644 apps/backend/internal/sysprompt/step_complete_recovery_test.go create mode 100644 docs/plans/step-completion-stale-turn-recovery/plan.md create mode 100644 docs/plans/step-completion-stale-turn-recovery/task-01-completion-recovery.md create mode 100644 docs/specs/tasks/system-design/workflow-explicit-completion-signal.md diff --git a/apps/backend/internal/mcp/handlers/handlers.go b/apps/backend/internal/mcp/handlers/handlers.go index f5d81540d0..c2c0fd9324 100644 --- a/apps/backend/internal/mcp/handlers/handlers.go +++ b/apps/backend/internal/mcp/handlers/handlers.go @@ -2359,7 +2359,13 @@ func (h *Handlers) handleStepComplete(ctx context.Context, msg *ws.Message) (*ws 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) + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, fmt.Sprintf( + "workflow step changed before signal was recorded. This turn started in step %s. "+ + "The current step is %s. No signal was recorded. Retrying in this turn cannot recover. "+ + "End this turn and ask the user to resume this session for the current step. "+ + "After satisfying that step, signal completion from the new turn. "+ + "Do not move the task solely to bypass this error.", + launchStepID, task.WorkflowStepID), nil) } boundedHandoff, handoffTruncated := boundStepCompletionSignalField(strings.TrimSpace(req.Handoff)) diff --git a/apps/backend/internal/mcp/handlers/step_complete_recovery_test.go b/apps/backend/internal/mcp/handlers/step_complete_recovery_test.go new file mode 100644 index 0000000000..552670d645 --- /dev/null +++ b/apps/backend/internal/mcp/handlers/step_complete_recovery_test.go @@ -0,0 +1,140 @@ +package handlers + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/kandev/kandev/internal/task/models" + sqliterepo "github.com/kandev/kandev/internal/task/repository/sqlite" + ws "github.com/kandev/kandev/pkg/websocket" +) + +// TestHandleStepComplete_StaleTurnRecoveryGuidance covers AC-003.1, AC-003.2, +// and AC-003.5: a stale turn remains harmless in every non-terminal session +// state and explains the fresh-turn recovery path. +func TestHandleStepComplete_StaleTurnRecoveryGuidance(t *testing.T) { + cases := []struct { + name string + state models.TaskSessionState + }{ + {name: "running", state: models.TaskSessionStateRunning}, + {name: "waiting for input", state: models.TaskSessionStateWaitingForInput}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + svc, repo := newTestTaskService(t) + seedStepCompleteTarget(t, repo, "task-stale-recovery", "session-stale-recovery", "step-review", tc.state) + createStepCompleteRecoveryTurn(t, repo, "turn-review", "task-stale-recovery", "session-stale-recovery") + + task, err := repo.GetTask(ctx, "task-stale-recovery") + 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-stale-recovery", + "session_id": "session-stale-recovery", + "summary": "late reviewer signal", + }) + + for attempt := 0; attempt < 2; attempt++ { + resp, callErr := h.handleStepComplete(ctx, msg) + require.NoError(t, callErr) + require.NotNil(t, resp) + + var payload ws.ErrorPayload + require.NoError(t, json.Unmarshal(resp.Payload, &payload)) + assert.Equal(t, ws.ErrorCodeValidation, payload.Code) + assert.Contains(t, payload.Message, "workflow step changed before signal was recorded") + assert.Contains(t, payload.Message, "turn started in step step-review") + assert.Contains(t, payload.Message, "current step is step-work") + assert.Contains(t, payload.Message, "No signal was recorded") + assert.Contains(t, payload.Message, "Retrying in this turn cannot recover") + assert.Contains(t, payload.Message, "End this turn and ask the user to resume") + assert.Contains(t, payload.Message, "Do not move the task solely to bypass this error") + } + + assert.Empty(t, bus.events, "stale calls must not publish a completion event") + session, err := repo.GetTaskSession(ctx, "session-stale-recovery") + require.NoError(t, err) + _, hasSignal := models.LoadPendingStepSignal(session.Metadata) + assert.False(t, hasSignal, "stale calls must not persist a completion signal") + }) + } +} + +// TestHandleStepComplete_FreshTurnAfterStepChange covers AC-003.3: a new +// turn stamped with the current step can complete normally after an older +// turn was rejected. +func TestHandleStepComplete_FreshTurnAfterStepChange(t *testing.T) { + ctx := context.Background() + svc, repo := newTestTaskService(t) + seedStepCompleteTarget(t, repo, "task-fresh-recovery", "session-fresh-recovery", "step-review", models.TaskSessionStateRunning) + createStepCompleteRecoveryTurn(t, repo, "turn-review", "task-fresh-recovery", "session-fresh-recovery") + + task, err := repo.GetTask(ctx, "task-fresh-recovery") + require.NoError(t, err) + task.WorkflowStepID = "step-work" + require.NoError(t, repo.UpdateTask(ctx, task)) + + bus := &mcpRecordingEventBus{} + h := newStepCompleteHandler(t, svc, repo, bus) + staleMsg := makeWSMessage(t, ws.ActionMCPStepComplete, map[string]interface{}{ + "task_id": "task-fresh-recovery", + "session_id": "session-fresh-recovery", + "summary": "late reviewer signal", + }) + staleResp, err := h.handleStepComplete(ctx, staleMsg) + require.NoError(t, err) + assertWSError(t, staleResp, ws.ErrorCodeValidation) + + createStepCompleteRecoveryTurn(t, repo, "turn-work", "task-fresh-recovery", "session-fresh-recovery") + freshMsg := makeWSMessage(t, ws.ActionMCPStepComplete, map[string]interface{}{ + "task_id": "task-fresh-recovery", + "session_id": "session-fresh-recovery", + "summary": "work step finished", + }) + freshResp, err := h.handleStepComplete(ctx, freshMsg) + require.NoError(t, err) + + var payload map[string]interface{} + require.NoError(t, json.Unmarshal(freshResp.Payload, &payload)) + assert.Equal(t, true, payload["accepted"]) + assert.Equal(t, "step-work", payload["step_id"]) + require.Len(t, bus.events, 1, "the fresh current-step turn must publish one event") + + session, err := repo.GetTaskSession(ctx, "session-fresh-recovery") + require.NoError(t, err) + signal, ok := models.LoadPendingStepSignal(session.Metadata) + require.True(t, ok, "the fresh turn must persist its completion signal") + assert.Equal(t, "step-work", signal.StepID) +} + +func createStepCompleteRecoveryTurn( + t *testing.T, + repo *sqliterepo.Repository, + turnID, taskID, sessionID string, +) { + t.Helper() + now := time.Now().UTC() + turn := &models.Turn{ + ID: turnID, + TaskSessionID: sessionID, + TaskID: taskID, + StartedAt: now, + CreatedAt: now, + UpdatedAt: now, + } + stamped, err := repo.CreateTurnWithStepStamp(context.Background(), turn) + require.NoError(t, err) + require.True(t, stamped, "turn must carry its launch step stamp") +} diff --git a/apps/backend/internal/mcp/server/server.go b/apps/backend/internal/mcp/server/server.go index ab77e17481..2a6450150d 100644 --- a/apps/backend/internal/mcp/server/server.go +++ b/apps/backend/internal/mcp/server/server.go @@ -1736,7 +1736,7 @@ func (s *Server) updateRepositoryBaseBranchHandler() server.ToolHandlerFunc { func (s *Server) registerStepCompleteTool() { s.mcpServer.AddTool( mcp.NewTool("step_complete_kandev", - mcp.WithDescription(`Signal that every requirement for the current workflow step is complete. Call this as the step's final action; do not call before asking the user or during partial work. If you cannot make further progress without input, describe the issue in blockers. The signal is idempotent within a step, and any configured transition runs asynchronously at turn end. A new user message cancels a pending signal. The summary is shown to the user and is recorded on the step-transition history.`), + mcp.WithDescription(`Complete the current workflow step as your final action. Do not call before asking the user or during partial work; put blockers in blockers. The signal is idempotent, transitions run asynchronously, and a new user message cancels it. If the response says the workflow step changed, this turn is stale: retries cannot recover. End it and ask the user to resume the session for the current step, then complete it in the new turn. This differs from already_signaled, which means the signal was accepted. Do not move the task solely to bypass a stale-turn error.`), mcp.WithString("summary", mcp.Required(), mcp.Description("One-paragraph plain-text summary of what was done in this step. Shown to the user.")), mcp.WithString("handoff", mcp.Description("Optional context for the immediately-following step's agent, delivered once in that step's first prompt and not carried beyond it. Up to 8,192 bytes; longer values are truncated.")), mcp.WithString("blockers", mcp.Description("Optional list of known unresolved issues. Recorded on this step's transition history, not delivered to the next step's agent — do not use it to pass context forward. Use sparingly, only when you cannot make further progress without input. Up to 8,192 bytes; longer values are truncated.")), diff --git a/apps/backend/internal/mcp/server/step_complete_recovery_test.go b/apps/backend/internal/mcp/server/step_complete_recovery_test.go new file mode 100644 index 0000000000..8c654e1882 --- /dev/null +++ b/apps/backend/internal/mcp/server/step_complete_recovery_test.go @@ -0,0 +1,46 @@ +package mcp + +import ( + "errors" + "testing" + + mcplib "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestStepCompleteToolDescription_ExplainsStaleTurnRecovery covers AC-003.4: +// the tool distinguishes a rejected stale turn from an accepted duplicate and +// does not turn a stale error into move authority. +func TestStepCompleteToolDescription_ExplainsStaleTurnRecovery(t *testing.T) { + s := newTaskModeServer(t, &testBackend{}, "task-current") + tool, ok := s.mcpServer.ListTools()["step_complete_kandev"] + require.True(t, ok) + + description := tool.Tool.Description + assert.Contains(t, description, "workflow step changed") + assert.Contains(t, description, "stale") + assert.Contains(t, description, "retries cannot recover") + assert.Contains(t, description, "resume the session for the current step") + assert.Contains(t, description, "already_signaled") + assert.Contains(t, description, "Do not move the task solely to bypass a stale-turn error") +} + +// TestStepCompleteTool_ForwardsStaleTurnRecoveryGuidance covers AC-003.4 at +// the MCP boundary: the backend diagnostic reaches the agent as tool-error +// text instead of being hidden in a transport-only field. +func TestStepCompleteTool_ForwardsStaleTurnRecoveryGuidance(t *testing.T) { + const guidance = "workflow step changed before signal was recorded. This turn started in step step-review. The current step is step-work. No signal was recorded. Retrying in this turn cannot recover. End this turn and ask the user to resume this session for the current step." + backend := &testBackend{err: errors.New(guidance)} + s := newTaskModeServer(t, backend, "task-current") + + result := callTool(t, s, "step_complete_kandev", map[string]interface{}{ + "summary": "finished", + }) + + require.True(t, result.IsError) + require.Len(t, result.Content, 1) + content, ok := result.Content[0].(mcplib.TextContent) + require.True(t, ok) + assert.Equal(t, guidance, content.Text) +} diff --git a/apps/backend/internal/sysprompt/step_complete_recovery_test.go b/apps/backend/internal/sysprompt/step_complete_recovery_test.go new file mode 100644 index 0000000000..ed09c18ec5 --- /dev/null +++ b/apps/backend/internal/sysprompt/step_complete_recovery_test.go @@ -0,0 +1,37 @@ +package sysprompt + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestCompletionInstructionsExplainStaleTurnRecovery covers AC-003.4: both +// signal-gated prompt surfaces preserve final-action and question barriers, +// distinguish stale rejection from already_signaled, and avoid move authority. +func TestCompletionInstructionsExplainStaleTurnRecovery(t *testing.T) { + cases := map[string]string{ + "task": FormatKandevContext("task-recovery", "session-recovery", true), + "office": FormatOfficeContextWithOptions("task-recovery", "session-recovery", true), + } + for name, context := range cases { + t.Run(name, func(t *testing.T) { + if name == "task" { + assert.Contains(t, context, "Call it as the LAST action") + } else { + assert.Contains(t, context, "Call step_complete_kandev as the LAST action") + } + assert.Contains(t, context, "before a question") + assert.Contains(t, context, "workflow step changed") + assert.Contains(t, context, "retries in this turn cannot recover") + assert.Contains(t, context, "End the turn and have the user resume the session") + assert.Contains(t, context, "already_signaled") + assert.Contains(t, context, "Do not move the task solely to bypass a stale-turn error") + }) + } +} + +func TestCompletionInstructionsOmitRecoveryGuidanceWhenUngated(t *testing.T) { + assert.NotContains(t, FormatKandevContext("task", "session", false), "workflow step changed") + assert.NotContains(t, FormatOfficeContextWithOptions("task", "session", false), "workflow step changed") +} diff --git a/apps/backend/internal/sysprompt/sysprompt.go b/apps/backend/internal/sysprompt/sysprompt.go index 6fbf21ebdc..3fdf41b6e3 100644 --- a/apps/backend/internal/sysprompt/sysprompt.go +++ b/apps/backend/internal/sysprompt/sysprompt.go @@ -166,10 +166,16 @@ func FormatOfficeContext(taskID, sessionID string) string { return FormatOfficeContextWithOptions(taskID, sessionID, false) } +const stepCompleteRecoveryInstruction = "If it reports that the workflow step changed, retries in this turn cannot recover. " + + "End the turn and have the user resume the session for the current step, then complete it in the new turn. " + + "already_signaled means the signal was accepted. " + + "Do not move the task solely to bypass a stale-turn error.\n" + 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" + "If the tool is not visible, use the client's tool search or discovery with the canonical name.\n" + + stepCompleteRecoveryInstruction // FormatOfficeContextWithOptions formats the Office context for the current // workflow step. The imperative completion instruction is present only when @@ -256,7 +262,8 @@ const richOutputSection = `- show_rich_output_kandev: For a chart, graph, plot, const stepCompleteSection = "- step_complete_kandev: Signal that every requirement for the CURRENT workflow step is satisfied. " + "Call it as the LAST action, never before a question or during partial progress. " + "If it is not visible, use the client's tool search/discovery with the canonical name; some clients display mcp__kandev__step_complete_kandev. " + - "Required param: summary.\n" + "Required param: summary.\n" + + stepCompleteRecoveryInstruction // coordinatorTaskControlSection documents task-mode-only parent/child controls. // Restricted MCP modes omit the section because neither message_task_kandev nor diff --git a/docs/plans/step-completion-stale-turn-recovery/plan.md b/docs/plans/step-completion-stale-turn-recovery/plan.md new file mode 100644 index 0000000000..8c818da624 --- /dev/null +++ b/docs/plans/step-completion-stale-turn-recovery/plan.md @@ -0,0 +1,139 @@ +--- +created: 2026-09-18 +status: done +requirements: + - REQ-TASKS-WORKFLOW-EXPLICIT-COMPLETION-SIGNAL-003 +system_design: + - ../../specs/tasks/system-design/workflow-explicit-completion-signal.md +legacy_specs: [] +--- + +# Implementation plan: completion recovery after a step change + +## Overview + +[Issue #3772](https://github.com/kdlbs/kandev/issues/3772) reports repeated completion +rejections after a reviewer moves the task during a worker turn. +This package makes that rejection actionable while preserving stale-reviewer protection. +One sequential work order delivers diagnostics, instructions, and regression evidence. + +## Evidence and root cause + +Investigation base: `81b59416ab`. +`handleStepComplete` compares the latest turn's launch stamp with the current task step. +A mismatch always returns the same opaque validation error before claiming a signal. +Repeating the call or changing session state does not change that stamp. + +The existing `TestHandleStepComplete_RejectsSignalFromMovedTurn` reproduces this +condition with a real SQLite repository. It passed during investigation. +Its purpose is to stop a stale reviewer from completing the rejected Work step. +Removing the comparison would regress that protection. + +The issue's production timeline is reported evidence. The precise upstream +interleaving that applies a deferred move during worker execution was not reproduced. +The current deferred-move path has additional queue and session-identity guards. +This package does not claim to repair or fully explain that separate race. + +The missing behavior is recovery guidance, covered by the proposed requirement +addition. The immutable turn stamp and safe rejection remain correct. +The current source does not contain the issue's exact "CALL THIS EXACTLY ONCE" +wording in the built-in completion sections. User-authored prompts are not rewritten. + +## Scope + +### In scope + +- Step IDs and fresh-turn recovery instructions in the mismatch error. +- Consistent tool, task-mode, and Office instructions. +- Tests for rejection, fresh-turn recovery, and MCP error forwarding. +- Public recovery instructions for the implemented behavior. + +### Out of scope + +- Automatic restart, synthetic completion, or a default direct-move fallback. +- Restamping old turns or accepting stale signals from idle sessions. +- Deferred-move queue changes, issue #3716, and issue #3712. +- UI layout, migrations, new flags, or rewriting saved user prompts. + +## Technical approach + +The [system design](../../specs/tasks/system-design/workflow-explicit-completion-signal.md) +owns the response and recovery contract. +Update only the mismatch message in `internal/mcp/handlers/handlers.go`. +Keep its validation code and existing prefix. Include recovery text in the message +that `stepCompleteHandler` forwards through MCP. + +Update `registerStepCompleteTool`, `stepCompleteSection`, and +`officeStepCompleteInstruction`. Preserve description budgets and conditional injection. +Do not change claims, subscriber behavior, or the task move API. + +Public docs updates belong in `docs/public/tasks-and-workflows.md` and +`docs/public/automation-and-mcp.md`. These reference pages need a short recovery +note, not a new tutorial. Update them with implementation, not during this design turn. + +## Tests + +| Acceptance | Regression evidence | +| --- | --- | +| 003.1, 003.2 | `step_complete_recovery_test.go:TestHandleStepComplete_StaleTurnRecoveryGuidance`, table-driven RUNNING and WAITING_FOR_INPUT cases | +| 003.3 | `step_complete_recovery_test.go:TestHandleStepComplete_FreshTurnAfterStepChange` | +| 003.4 | `step_complete_recovery_test.go:TestStepCompleteHandler_ForwardsRecoveryGuidance` in the MCP server package, plus sysprompt recovery tests | +| 003.5 | Public docs validators and review against the proven fresh-turn sequence | + +All short acceptance suffixes refer to +`AC-TASKS-WORKFLOW-EXPLICIT-COMPLETION-SIGNAL-003.*`. +The first new handler test must fail on the existing opaque response before implementation. +Retain the existing moved-turn, duplicate, concurrent-claim, and prompt-size tests. + +## End-to-end evidence + +The changed surface is MCP text. The server test sends a completion request through +`stepCompleteHandler` and proves that backend recovery text reaches the MCP error. +The SQLite handler test proves rejection followed by fresh-turn acceptance. +No browser test is required because this package changes no rendered UI behavior. +Automated text tests do not prove that every model obeys recovery instructions. + +## Work orders + +- [x] [Task 01: expose safe completion recovery](task-01-completion-recovery.md) + +## Verification results + +Investigation passed: + +```bash +(cd apps/backend && go test ./internal/mcp/handlers -run TestHandleStepComplete_RejectsSignalFromMovedTurn -count=1) +``` + +Package validation passed: + +- `python3 scripts/list-docs.py validate`: 288 decisions and 1003 specifications. +- `python3 scripts/lint-spec-files.test.py`: 36 tests passed. +- `python3 scripts/lint-spec-files.py --all`: all specifications passed. +- `git diff --check -- docs/specs docs/plans/step-completion-stale-turn-recovery`: passed. +- `git status --short -- docs/plans/step-completion-stale-turn-recovery`: both package files present. + +Issue #3772 was assigned to `carlosflorencio` during investigation. + +Implementation completed: + +- Added launch-step and current-step IDs plus fresh-turn recovery guidance to + stale completion errors while preserving validation and signal-claim guards. +- Updated MCP tool metadata, signal-gated task and Office prompts, and public + task/MCP reference pages with the same recovery sequence. +- Added SQLite handler, MCP forwarding, prompt, and metadata regressions. +- `go test ./internal/mcp/handlers -run 'TestHandleStepComplete_' -count=1` + passed. +- `go test ./internal/mcp/server -count=1` and `go test ./internal/sysprompt + -count=1` passed. +- `go test -race ./internal/mcp/handlers -run 'TestHandleStepComplete_' -count=1` + passed. +- Public-doc validators, specification catalog/lint checks, `git diff --check`, + and `make -C apps/backend build` passed. + +## Risks + +- Recovery still needs a fresh user-started turn or an operator's manual move. +- A task can change steps again after recovery. The mismatch must remain safe. +- Saved prompts can contradict new built-in guidance. This package does not overwrite them. +- Broadening the engine fix requires separate evidence for the deferred-move race. diff --git a/docs/plans/step-completion-stale-turn-recovery/task-01-completion-recovery.md b/docs/plans/step-completion-stale-turn-recovery/task-01-completion-recovery.md new file mode 100644 index 0000000000..c44eeac66d --- /dev/null +++ b/docs/plans/step-completion-stale-turn-recovery/task-01-completion-recovery.md @@ -0,0 +1,104 @@ +--- +id: "01-completion-recovery" +title: "Expose safe completion recovery" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-TASKS-WORKFLOW-EXPLICIT-COMPLETION-SIGNAL-003 +acceptance_criteria: + - AC-TASKS-WORKFLOW-EXPLICIT-COMPLETION-SIGNAL-003.1 + - AC-TASKS-WORKFLOW-EXPLICIT-COMPLETION-SIGNAL-003.2 + - AC-TASKS-WORKFLOW-EXPLICIT-COMPLETION-SIGNAL-003.3 + - AC-TASKS-WORKFLOW-EXPLICIT-COMPLETION-SIGNAL-003.4 + - AC-TASKS-WORKFLOW-EXPLICIT-COMPLETION-SIGNAL-003.5 +system_design: + - ../../specs/tasks/system-design/workflow-explicit-completion-signal.md +--- + +# Task 01: expose safe completion recovery + +## Summary + +Make stale-turn completion errors explain the cause and the fresh-turn recovery path. +Preserve the rejection and document the distinction from a duplicate accepted signal. + +## In scope + +- Add the plan's handler regressions before changing the error. +- Use `seedStepCompleteTarget` and `CreateTurnWithStepStamp` for Review to Work scenarios. +- Assert both IDs and recovery text in repeated rejected responses. +- Assert no event or pending signal after stale calls, in running and idle states. +- Create a later Work turn and assert ordinary completion acceptance. +- Exercise MCP forwarding with the existing `testBackend` server fixture. +- Cover task and Office prompt guidance, gating, final-action rules, and size limits. +- Update the two public reference pages with the tested recovery sequence. + +## Out of scope + +Queue or orchestrator changes, automatic task moves, turn restamping, UI, and migrations. + +## Acceptance + +1. The new guidance test fails on the old response, then passes with the correction. +2. Stale calls remain harmless, while a fresh current-step turn can signal normally. +3. The MCP response, built-in prompts, and public docs describe the same recovery behavior. + +## Verification + +Run the new handler test first and record its expected failure. Then implement +and run these commands from the repository root: + +```bash +(cd apps/backend && go test ./internal/mcp/handlers -run 'TestHandleStepComplete_' -count=1) +(cd apps/backend && go test ./internal/mcp/server ./internal/sysprompt -count=1) +node --test scripts/validate-public-docs.test.mjs +node scripts/validate-public-docs.mjs +python3 scripts/list-docs.py validate +python3 scripts/lint-spec-files.test.py +python3 scripts/lint-spec-files.py --all +git diff --check +``` + +## Files likely touched + +- `apps/backend/internal/mcp/handlers/handlers.go` +- `apps/backend/internal/mcp/handlers/step_complete_recovery_test.go` (new) +- `apps/backend/internal/mcp/server/server.go` +- `apps/backend/internal/mcp/server/step_complete_recovery_test.go` (new) +- `apps/backend/internal/sysprompt/sysprompt.go` +- `apps/backend/internal/sysprompt/step_complete_recovery_test.go` (new) +- `docs/public/tasks-and-workflows.md` +- `docs/public/automation-and-mcp.md` + +## Dependencies + +None. Implementation completed in this work order after the explicit implementation request. + +## Risks + +Keep the tool description within the existing metadata budget. +Do not equate `already_signaled` with a stale rejection. +Do not add a backend side effect to make the text recovery test pass. + +## Parallelism + +`sequential` + +## Inputs + +- [Requirement 003](../../specs/tasks/requirements/workflow-explicit-completion-signal.md) +- [System design](../../specs/tasks/system-design/workflow-explicit-completion-signal.md) +- `apps/backend/internal/mcp/handlers/step_complete_test.go` +- `apps/backend/internal/mcp/server/step_complete_description_test.go` +- `apps/backend/internal/sysprompt/mcp_discovery_test.go` +- [ADR 0015](../../decisions/0015-explicit-completion-signal-for-auto-advance.md) + +## Results + +Completed. The stale-turn rejection now includes both step IDs and directs the +agent to end the stale turn and have the user resume the session. A fresh turn +stamped with the current step can signal normally. MCP forwarding, tool +metadata, task and Office prompts, public docs, and the existing stale-turn, +duplicate, concurrency, and prompt-size coverage remain aligned. diff --git a/docs/public/automation-and-mcp.md b/docs/public/automation-and-mcp.md index 75722617d5..971aa7e5cd 100644 --- a/docs/public/automation-and-mcp.md +++ b/docs/public/automation-and-mcp.md @@ -684,6 +684,8 @@ The HTTP equivalent is `POST /api/v1/tasks/:id/workspace-sources`, with `{ "sour `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 the response says that the workflow step changed, the calling turn is stale. The error identifies the launch and current steps, and a retry in that turn cannot recover. End the turn and have the user resume the session, then complete the current step and call the tool from the fresh turn. A normal manual workflow move remains available to an operator after verifying the work and destination. This recovery path does not grant an agent automatic move authority. + 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. The task server runs inside agentctl's local runtime boundary. Its MCP routes do not use a separate bearer token. Do not expose agentctl ports; rely on the executor's process/network isolation and Kandev's session scoping. diff --git a/docs/public/tasks-and-workflows.md b/docs/public/tasks-and-workflows.md index 07f9a96e00..c342d9d43d 100644 --- a/docs/public/tasks-and-workflows.md +++ b/docs/public/tasks-and-workflows.md @@ -680,6 +680,8 @@ Generic comment, blocker-resolution, approval, heartbeat, budget, and error trig When **On Turn Complete** moves a task, **Wait for agent completion signal** is available. With it enabled, a bare turn end leaves the task waiting; the agent must call `step_complete_kandev`. The call requires a summary and can include a handoff or blockers. It is idempotent within the step, runs asynchronously, and a user message sent before the transition is applied cancels that pending signal. Without the option, turn end counts as completion. +If the task changes steps before the agent sends the signal, Kandev rejects the call and names both the turn's launch step and the current step. Retrying in that same turn cannot recover. End the turn and have the user resume the session, then satisfy the current step and call `step_complete_kandev` from the fresh turn. An operator can also use the normal manual workflow move after checking the work and destination. Waiting, reconnecting, or retrying the stale turn does not change its eligibility. + **Run completion actions when a turn is cancelled** is available beneath a configured turn-complete transition. It applies only when a user explicitly presses **Cancel** on the active turn. The normal completion pipeline still applies, including `on_exit`, the configured transition, and the destination step's `on_enter` actions; an `auto_start_agent` action there can start another turn immediately. An eligible explicit cancellation bypasses the `auto_advance_requires_signal` / `step_complete_kandev` gate, but a pending clarification still blocks the transition. The setting does not turn other interruption or failure paths into completion events. When the setting is off, an explicit cancel leaves the task in its current step and ready for input. The built-in **Kanban** workflow enables this policy on **Backlog** and **In Progress** and leaves it disabled on its other steps. Custom steps and imported definitions default to disabled unless they set the field explicitly. diff --git a/docs/specs/tasks/requirements/workflow-explicit-completion-signal.md b/docs/specs/tasks/requirements/workflow-explicit-completion-signal.md index 0061a003e8..8b28f9f70a 100644 --- a/docs/specs/tasks/requirements/workflow-explicit-completion-signal.md +++ b/docs/specs/tasks/requirements/workflow-explicit-completion-signal.md @@ -55,6 +55,37 @@ when the agent did not emit its completion signal. the clarification barrier clears, the surfaces shall reevaluate visibility and show the action when the signal-gated idle conditions are satisfied. +### REQ-TASKS-WORKFLOW-EXPLICIT-COMPLETION-SIGNAL-003: Recovery after a step change + +**Status:** shipped. + +**Intent:** A caller can distinguish a stale completion attempt from a duplicate +signal and recover without completing the wrong step. + +#### Acceptance criteria + +- **AC-TASKS-WORKFLOW-EXPLICIT-COMPLETION-SIGNAL-003.1:** When the calling turn + belongs to a different step, completion remains rejected. The error identifies + the turn's step and the current step. No completion signal or event is created. +- **AC-TASKS-WORKFLOW-EXPLICIT-COMPLETION-SIGNAL-003.2:** The rejection explains + that retries in the same turn cannot recover. It directs the caller to end + that turn and have the user resume the session for the current step. +- **AC-TASKS-WORKFLOW-EXPLICIT-COMPLETION-SIGNAL-003.3:** After a fresh turn starts + on the current step, a new completion attempt is eligible under the normal + completion rules. A previous rejection does not permanently disable completion. +- **AC-TASKS-WORKFLOW-EXPLICIT-COMPLETION-SIGNAL-003.4:** Tool and signal-gated + prompt instructions distinguish stale rejection from `already_signaled`. + Completion remains the final action after all current-step requirements are + satisfied. Pending questions remain barriers. The instructions do not authorize + a task move as an automatic substitute for a rejected signal. +- **AC-TASKS-WORKFLOW-EXPLICIT-COMPLETION-SIGNAL-003.5:** User recovery guidance + explains fresh-turn recovery and the existing manual workflow move option. + It does not claim that waiting, reconnecting, or retrying the stale turn changes + its completion eligibility. + +This addition specifies recovery guidance. Automatic turn restart, deferred-move +scheduling repairs, and changes to completion authorization are outside its scope. + ## Migrated source detail ## Why diff --git a/docs/specs/tasks/system-design/workflow-explicit-completion-signal.md b/docs/specs/tasks/system-design/workflow-explicit-completion-signal.md new file mode 100644 index 0000000000..e7e3a1c3f3 --- /dev/null +++ b/docs/specs/tasks/system-design/workflow-explicit-completion-signal.md @@ -0,0 +1,89 @@ +--- +status: current +system: tasks +requirements: + - REQ-TASKS-WORKFLOW-EXPLICIT-COMPLETION-SIGNAL-003 +--- + +# Completion signal recovery system design + +## Purpose and boundaries + +The tasks system owns completion eligibility and its recovery instructions. +This design adds diagnostics to the existing completion contract. +It preserves the current turn stamp, signal claim, and workflow transition rules. + +## Requirement mapping + +| Requirement | Design sections | +| --- | --- | +| REQ-TASKS-WORKFLOW-EXPLICIT-COMPLETION-SIGNAL-003 | Diagnostic response, Recovery instructions, Verification | + +## Diagnostic response + +`internal/mcp/handlers/handlers.go:handleStepComplete` compares the launch step +from `stepCompletionLaunchStep` with the task's current workflow step. +The mismatch continues to return `VALIDATION_ERROR` before the signal claim. + +The error retains its existing prefix for compatibility. Its text includes both +step IDs and the recovery instruction. This information belongs in the message, +because `internal/mcp/server/server.go:stepCompleteHandler` forwards `err.Error()`. +A details-only change would not reliably reach the agent. + +Suggested message: + +> workflow step changed before signal was recorded. This turn started in step +> . The current step is . No signal was recorded. +> Retrying in this turn cannot recover. End this turn and ask the user to resume +> this session for the current step. After satisfying that step, signal completion +> from the new turn. Do not move the task solely to bypass this error. + +A missing stamp or failed turn read remains a separate internal error. +The message does not promise that a concurrent future move cannot occur. + +## Recovery instructions + +The registered tool description explains the distinction between a stale-turn +error and `already_signaled`. A duplicate preserves the existing accepted signal. +A stale-turn rejection records nothing and requires a fresh turn. + +`internal/sysprompt/sysprompt.go` supplies concise recovery guidance in +`stepCompleteSection` and `officeStepCompleteInstruction`. Gating and mode +visibility stay unchanged. The long explanation resides in the returned error. +This keeps prompt and tool-description size budgets intact. + +The agent ends its stale turn with a clear recovery request. A user resumes the +same session on the current step. Normal turn creation stamps that step. +The agent evaluates current-step work before its final completion call. +No tool call silently creates a turn, restamps an old turn, or moves the task. + +The existing operator move remains documented as a separate manual decision. +An error does not grant an agent new authority to use `move_task_kandev`. + +## Persistence and compatibility + +There is no schema change, new response envelope, or new MCP argument. +`workflow_step_id_at_start` remains an immutable historical fact. +The task-at-step conditional signal claim and duplicate handling remain intact. +RUNNING and WAITING_FOR_INPUT sessions both retain the mismatch guard. +Idleness does not prove that work belongs to the new step. + +## Verification + +Handler tests use the SQLite-backed existing fixtures. They reproduce Review to +Work movement after turn creation, reject repeated stale calls, and accept a +call after a new Work turn starts. Both session states retain the guard. + +Server tests exercise error forwarding through the MCP handler. Prompt tests +cover gated task and Office contexts, omission from ungated instructions, and +existing size budgets. These are protocol tests, not browser layout changes. + +## Related decisions and contracts + +- [ADR 0015](../../../decisions/0015-explicit-completion-signal-for-auto-advance.md) +- [Turn stamp design](workflow-task-step-transition-ledger.md) +- [Requirements](../requirements/workflow-explicit-completion-signal.md) + +## Implementation plans + +- [Issue 3772 recovery package](../../../plans/step-completion-stale-turn-recovery/plan.md)