diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index 7ea055093d..4e50d3d730 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -237,6 +237,7 @@ Client (WS) ← Orchestrator ← Lifecycle Manager ←──── stream update - **Execution access:** Workspace-oriented handlers (files, shell, inference, ports, vscode, LSP) MUST use `GetOrEnsureExecution(ctx, sessionID)` — it recovers from backend restarts by creating executions on-demand. Only use `GetExecutionBySessionID` for operations that require a running agent process (prompt, cancel, mode). - **Generated-title ownership:** Claim generated-title ownership only after task, config, and Office eligibility is known. Config tasks, Office/External modes, and workspace-only preparation (`StartAgent: false`) must never claim it; only an eligible agent-start task may claim the generated title. - **Task lifecycle events:** Any code path that mutates a task row must publish via the event bus (`task.created` / `task.updated` / `task.deleted`) — either by going through `Service.CreateTask` / `UpdateTask` / `DeleteTask` / `ArchiveTask`, or by calling `publishTaskEvent` (or one of the `Publish*` helpers in `service_events.go`) directly. Walking `repository.TaskRepository` straight bypasses event publishing and breaks WS-driven UI like the All-Workflows kanban view. `HandoffService`'s cascade methods learned this the hard way — they now require a `TaskEventPublisher` wired via `SetTaskEventPublisher`. New cascade / bulk / cleanup paths must follow the same pattern. **Workflow steps** follow the same rule with their own publisher: step create/update/delete publish `workflow_step.created` / `.updated` / `.deleted` through `internal/workflow/stepevents.Publisher` (the WS gateway fans them out; the orchestrator re-evaluates queue admission off `.updated`). Both mutation surfaces — REST/WS in `internal/workflow/handlers`, MCP in `internal/mcp/handlers` — share that publisher and differ only in the source label they pass, so a new step-mutation path uses it rather than hand-rolling the payload; promoting a start step demotes the previous one, so also publish `.updated` for every entry in `DemotedStartSteps`. +- **Agent task plans:** Agent whole-document writes must carry `expected_version` when a plan already exists. The task service rejects suspicious reductions before storage and requires explicit `allow_truncation` plus verified history for intentional reductions. Exact edits and revision recovery stay task-scoped, use the service lock, and check both the current plan version and the selected revision snapshot. Keep browser DTOs and browser write behavior independent from the agent-only version contract. - **Workspace deletion side tables:** Any workspace-scoped integration side table without a database foreign key/cascade must be deleted explicitly from its `WorkspaceDeleted` handler. Cover the create → delete lifecycle in a test, and include the same cleanup in E2E reset fixtures; do not assume deleting the workspace row removes orphaned integration metadata. - **Testing:** For backend test changes, load [backend-tests.md](../../.agents/skills/tdd/references/backend-tests.md). It covers time, cleanup, filesystem fixtures, environment isolation, and subprocess helpers. @@ -291,7 +292,6 @@ Enforced by `apps/backend/.golangci.yml` (errors on new code only): - Functions: ≤80 lines, ≤50 statements · Cyclomatic complexity: ≤15 · Cognitive complexity: ≤30 · Nesting depth: ≤5 · Naked returns only in functions ≤30 lines · No duplicated blocks (≥150 tokens) · Repeated strings → constants (≥3 occurrences) · Revive's 800-effective-line file limit also applies to test files; put new tests in a new file instead of appending to an already-large test file. When a PR fixup touches backend code, run `golangci-lint run ./... --new-from-rev="" --timeout=5m` from `apps/backend` with the PR base SHA before pushing; CI enforces changed-file complexity thresholds. -## Further scoped notes - `internal/launcher/` — native launcher owning every entrypoint (`dev`, `start`, `run`, `service`); `dev` runs `make -C apps/backend dev` with Vite as a supervised child, state under `/.kandev-dev/`. The root `make dev` prebuilds only the copied launcher; the backend dev target builds the native agentctl and a linux/amd64 helper when the host is not Linux/amd64 (`docs/plans/go-dev-launcher/`). - `internal/agentctl/AGENTS.md` — agentctl server route groups, adapter model, ACP protocol - `internal/agentctl/server/api/AGENTS.md` — reverse-proxy body rewriting (`Accept-Encoding`), iframe-blocking header stripping diff --git a/apps/backend/cmd/mock-agent/script.go b/apps/backend/cmd/mock-agent/script.go index a6e03afe0b..7423fb4e73 100644 --- a/apps/backend/cmd/mock-agent/script.go +++ b/apps/backend/cmd/mock-agent/script.go @@ -19,6 +19,7 @@ func isScriptMode(cmd string) bool { // executeScript processes a multi-line script command. func executeScript(e *emitter, fullPrompt, cmd string) { + planVersion := "" lines := strings.Split(cmd, "\n") for _, line := range lines { if e.ctx != nil && e.ctx.Err() != nil { @@ -28,12 +29,12 @@ func executeScript(e *emitter, fullPrompt, cmd string) { if line == "" || strings.HasPrefix(line, "#") { continue } - executeCommand(e, fullPrompt, line) + executeCommand(e, fullPrompt, line, &planVersion) } } // executeCommand dispatches a single script command line. -func executeCommand(e *emitter, fullPrompt, line string) { +func executeCommand(e *emitter, fullPrompt, line string, planVersion *string) { switch { case strings.HasPrefix(line, "e2e:message("): text := extractStringArg(line, "e2e:message(") @@ -53,7 +54,7 @@ func executeCommand(e *emitter, fullPrompt, line string) { waitForDelay(e.ctx, ms) case strings.HasPrefix(line, "e2e:mcp:"): - executeMCPCommand(e, fullPrompt, line) + executeMCPCommand(e, fullPrompt, line, planVersion) case strings.HasPrefix(line, "e2e:plan("): executePlanCommand(e, line) @@ -128,7 +129,7 @@ func executeMonitorEnd(e *emitter, line string) { } // executeMCPCommand parses and executes: e2e:mcp::() -func executeMCPCommand(e *emitter, fullPrompt, line string) { +func executeMCPCommand(e *emitter, fullPrompt, line string, planVersion *string) { rest := strings.TrimPrefix(line, "e2e:mcp:") colonIdx := strings.Index(rest, ":") @@ -158,7 +159,11 @@ func executeMCPCommand(e *emitter, fullPrompt, line string) { return } - substituteContextPlaceholders(args, fullPrompt) + currentPlanVersion := "" + if planVersion != nil { + currentPlanVersion = *planVersion + } + substituteScriptPlaceholders(args, fullPrompt, currentPlanVersion) toolID := nextToolID() e.startTool(toolID, toolName, acp.ToolKindOther, args) @@ -172,6 +177,11 @@ func executeMCPCommand(e *emitter, fullPrompt, line string) { e.completeTool(toolID, map[string]any{toolKeyError: "MCP error: " + err.Error()}) } else { e.completeTool(toolID, map[string]any{toolKeyResult: result}) + if planVersion != nil { + if version := extractPlanVersion(result); version != "" { + *planVersion = version + } + } } } @@ -350,6 +360,36 @@ func substituteContextPlaceholders(args map[string]any, fullPrompt string) { } } +// substituteScriptPlaceholders replaces the context placeholders and the +// version returned by the most recent scripted plan MCP call. The latter lets +// deterministic E2E scripts perform a sequence of conditional agent writes +// without weakening the production requirement to pass expected_version. +func substituteScriptPlaceholders(args map[string]any, fullPrompt, planVersion string) { + substituteContextPlaceholders(args, fullPrompt) + for k, v := range args { + if s, ok := v.(string); ok { + args[k] = strings.ReplaceAll(s, "{plan_version}", planVersion) + } + } +} + +var ( + planWriteVersionRegex = regexp.MustCompile(`(?:^|[\s,])version=([^\s.,]+)`) + planReadVersionRegex = regexp.MustCompile(`"version"\s*:\s*"([^"]+)"`) +) + +// extractPlanVersion reads the committed version from either a plan-write +// acknowledgement or the metadata block returned by get_task_plan_kandev. +func extractPlanVersion(result string) string { + if match := planWriteVersionRegex.FindStringSubmatch(result); len(match) == 2 { + return match[1] + } + if match := planReadVersionRegex.FindStringSubmatch(result); len(match) == 2 { + return match[1] + } + return "" +} + // extractRegexMatch returns the first capture group match, or empty string. func extractRegexMatch(re *regexp.Regexp, s string) string { m := re.FindStringSubmatch(s) diff --git a/apps/backend/cmd/mock-agent/script_test.go b/apps/backend/cmd/mock-agent/script_test.go index 7dd8dc8ed7..1411fd4150 100644 --- a/apps/backend/cmd/mock-agent/script_test.go +++ b/apps/backend/cmd/mock-agent/script_test.go @@ -214,6 +214,50 @@ e2e:mcp:kandev:create_task_plan_kandev({"task_id":"{task_id}"})` }) } +func TestSubstitutePlanVersionPlaceholder(t *testing.T) { + args := map[string]any{ + "expected_version": "{plan_version}", + "content": "unchanged", + } + + substituteScriptPlaceholders(args, "", "write-v2") + + if args["expected_version"] != "write-v2" { + t.Fatalf("expected_version = %q, want %q", args["expected_version"], "write-v2") + } + if args["content"] != "unchanged" { + t.Fatalf("content = %q, want unchanged", args["content"]) + } +} + +func TestExtractPlanVersion(t *testing.T) { + tests := []struct { + name string + result string + want string + }{ + { + name: "write acknowledgement", + result: "Plan updated, version=write-v2. Plan content is omitted from this response.", + want: "write-v2", + }, + { + name: "read metadata", + result: `Plan metadata:\n{"version":"write-v3","content_bytes":12}\nbody`, + want: "write-v3", + }, + {name: "missing", result: "Plan content without version", want: ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := extractPlanVersion(tt.result); got != tt.want { + t.Fatalf("extractPlanVersion(%q) = %q, want %q", tt.result, got, tt.want) + } + }) + } +} + func TestExtractRegexMatch(t *testing.T) { tests := []struct { name string diff --git a/apps/backend/config/prompts/office-context.md b/apps/backend/config/prompts/office-context.md index 51fcad868e..cec952f0dd 100644 --- a/apps/backend/config/prompts/office-context.md +++ b/apps/backend/config/prompts/office-context.md @@ -7,9 +7,13 @@ Use these IDs when a tool requires task_id or session_id. Available tools: - ask_user_question_kandev: Ask the user one or more clarifying questions in a single call. Required: questions (1-4 items, each with prompt and 2-6 labeled options). Optional: context. -- create_task_plan_kandev: Save an implementation plan for the current task. Required: task_id, content. Optional: title. -- get_task_plan_kandev: Retrieve the current task plan, including user edits. Required: task_id. -- update_task_plan_kandev: Update the current task plan. Required: task_id, content. Optional: title. +- create_task_plan_kandev: Save an implementation plan. This replaces existing content only with the current expected_version; use edit_task_plan_kandev for a local change or update_task_plan_kandev with mode="append" for a new section. Required: task_id, content. Optional: title, expected_version, allow_truncation. +- get_task_plan_kandev: Retrieve the current task plan, including user edits and the current version. Required: task_id. +- update_task_plan_kandev: Update the current task plan. Use mode="replace" with expected_version for the whole document, or mode="append" for a new section without reading first. Required: task_id, content. Optional: title, mode, expected_version, allow_truncation. +- edit_task_plan_kandev: Apply one exact text edit with a current expected_version. The old text must occur exactly once; surrounding bytes and line endings are preserved. Required: task_id, expected_version, old_text, new_text. Optional: allow_truncation. +- list_task_plan_revisions_kandev: List bounded metadata for the task plan's revisions without content. Optional: task_id, before_revision_number, limit. +- get_task_plan_revision_kandev: Read one exact task-plan revision, including its unchanged content and revision_version. Required: task_id, revision_id. +- restore_task_plan_revision_kandev: Restore one task-plan revision as a new revision. Requires expected_version and expected_revision_version. Required: task_id, revision_id, expected_version, expected_revision_version. - delete_task_plan_kandev: Delete the current task plan. Required: task_id. - show_rich_output_kandev: When user asks for chart/graph/plot/file preview/KPI/metrics with data: call now. Do not implement the display as ASCII/SVG/HTML or with another app. Else prose; small text table: Markdown. Send version=1,title,blocks (1-4). Inline: {"type":"chart","chart_type":"bar","title":"T","summary":"S","labels":["A","B"],"series":[{"label":"Count","values":[42,27]}]}. CSV line: {"type":"chart","chart_type":"line","title":"T","summary":"S","csv":{"path":"reports/latency.csv","x_column":"recorded_at","series":[{"column":"p95_ms","label":"p95 (ms)"}]}}. Metrics: {"type":"metrics","items":[{"label":"Passed","value":"38"}]}. Paths workspace-relative. Kandev owns axes/legends/tooltips/layout. Label series with units. - list_related_tasks_kandev: List parent, child, sibling, blocker, and blocked tasks. Optional: task_id (defaults to the current task), verbose (include task descriptions; omitted by default). diff --git a/apps/backend/internal/mcp/handlers/handlers.go b/apps/backend/internal/mcp/handlers/handlers.go index 6a967467f2..f5d81540d0 100644 --- a/apps/backend/internal/mcp/handlers/handlers.go +++ b/apps/backend/internal/mcp/handlers/handlers.go @@ -543,6 +543,10 @@ func (h *Handlers) registerTaskPlanHandlers(d *guardedMCPDispatcher) { d.RegisterFunc(ws.ActionMCPCreateTaskPlan, h.handleCreateTaskPlan) d.RegisterFunc(ws.ActionMCPGetTaskPlan, h.handleGetTaskPlan) d.RegisterFunc(ws.ActionMCPUpdateTaskPlan, h.handleUpdateTaskPlan) + d.RegisterFunc(ws.ActionMCPEditTaskPlan, h.handleEditTaskPlan) + d.RegisterFunc(ws.ActionMCPListTaskPlanRevisions, h.handleListTaskPlanRevisions) + d.RegisterFunc(ws.ActionMCPGetTaskPlanRevision, h.handleGetTaskPlanRevision) + d.RegisterFunc(ws.ActionMCPRestoreTaskPlanRevision, h.handleRestoreTaskPlanRevision) d.RegisterFunc(ws.ActionMCPDeleteTaskPlan, h.handleDeleteTaskPlan) d.RegisterFunc(ws.ActionMCPShowWalkthrough, h.handleShowWalkthrough) d.RegisterFunc(ws.ActionMCPGetWalkthrough, h.handleGetWalkthrough) @@ -4323,10 +4327,12 @@ func (h *Handlers) sessionUpdatedAtForStateEvent(ctx context.Context, sessionID // handleCreateTaskPlan creates a new task plan. func (h *Handlers) handleCreateTaskPlan(ctx context.Context, msg *ws.Message) (*ws.Message, error) { var req struct { - TaskID string `json:"task_id"` - Title string `json:"title"` - Content string `json:"content"` - CreatedBy string `json:"created_by"` + TaskID string `json:"task_id"` + Title string `json:"title"` + Content string `json:"content"` + CreatedBy string `json:"created_by"` + ExpectedVersion string `json:"expected_version"` + AllowTruncation bool `json:"allow_truncation"` } if err := json.Unmarshal(msg.Payload, &req); err != nil { return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "Invalid payload: "+err.Error(), nil) @@ -4346,6 +4352,9 @@ func (h *Handlers) handleCreateTaskPlan(ctx context.Context, msg *ws.Message) (* Content: req.Content, CreatedBy: createdBy, EvaluateTruncation: true, + AgentWrite: true, + ExpectedVersion: req.ExpectedVersion, + AllowTruncation: req.AllowTruncation, }) if err != nil { return planws.CreateError(msg, err) @@ -4355,7 +4364,7 @@ func (h *Handlers) handleCreateTaskPlan(ctx context.Context, msg *ws.Message) (* if result.TruncationDetected { warning = planTruncationWarning(result.ReplacedRunes, result.NewRunes, result.PriorRevisionNumber) } - return ws.NewResponse(msg.ID, msg.Action, planWritePayload(dto.TaskPlanFromModel(result.Plan), warning, result.PriorRevisionNumber)) + return ws.NewResponse(msg.ID, msg.Action, planWritePayload(dto.TaskPlanFromModel(result.Plan), result.Plan.WriteVersion, warning, result.PriorRevisionNumber)) } // handleGetTaskPlan retrieves a task plan. @@ -4365,7 +4374,7 @@ func (h *Handlers) handleGetTaskPlan(ctx context.Context, msg *ws.Message) (*ws. return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "Invalid payload: "+err.Error(), nil) } - plan, err := h.planService.GetPlan(ctx, req.TaskID) + plan, err := h.planService.GetPlanSnapshot(ctx, req.TaskID) if err != nil { return planws.GetError(msg, err) } @@ -4374,7 +4383,7 @@ func (h *Handlers) handleGetTaskPlan(ctx context.Context, msg *ws.Message) (*ws. return ws.NewResponse(msg.ID, msg.Action, map[string]interface{}{}) } - return ws.NewResponse(msg.ID, msg.Action, dto.TaskPlanFromModel(plan)) + return ws.NewResponse(msg.ID, msg.Action, planReadPayload(plan)) } // handleUpdateTaskPlan updates an existing task plan. @@ -4389,11 +4398,13 @@ func (h *Handlers) handleGetTaskPlan(ctx context.Context, msg *ws.Message) (*ws. // it cannot reach. func (h *Handlers) handleUpdateTaskPlan(ctx context.Context, msg *ws.Message) (*ws.Message, error) { var req struct { - TaskID string `json:"task_id"` - Title string `json:"title"` - Content string `json:"content"` - CreatedBy string `json:"created_by"` - Mode string `json:"mode"` + TaskID string `json:"task_id"` + Title string `json:"title"` + Content string `json:"content"` + CreatedBy string `json:"created_by"` + Mode string `json:"mode"` + ExpectedVersion string `json:"expected_version"` + AllowTruncation bool `json:"allow_truncation"` } if err := json.Unmarshal(msg.Payload, &req); err != nil { return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "Invalid payload: "+err.Error(), nil) @@ -4415,6 +4426,9 @@ func (h *Handlers) handleUpdateTaskPlan(ctx context.Context, msg *ws.Message) (* CreatedBy: createdBy, EvaluateTruncation: mode == service.PlanWriteModeReplace, Mode: mode, + AgentWrite: true, + ExpectedVersion: req.ExpectedVersion, + AllowTruncation: req.AllowTruncation, }) if err != nil { return planws.UpdateError(msg, err) @@ -4424,7 +4438,7 @@ func (h *Handlers) handleUpdateTaskPlan(ctx context.Context, msg *ws.Message) (* if result.TruncationDetected { warning = planTruncationWarning(result.ReplacedRunes, result.NewRunes, result.PriorRevisionNumber) } - return ws.NewResponse(msg.ID, msg.Action, planWritePayload(dto.TaskPlanFromModel(result.Plan), warning, result.PriorRevisionNumber)) + return ws.NewResponse(msg.ID, msg.Action, planWritePayload(dto.TaskPlanFromModel(result.Plan), result.Plan.WriteVersion, warning, result.PriorRevisionNumber)) } // handleDeleteTaskPlan deletes a task plan. diff --git a/apps/backend/internal/mcp/handlers/task_plan_append_mode_test.go b/apps/backend/internal/mcp/handlers/task_plan_append_mode_test.go index 24cfe8c30e..e47ed455c9 100644 --- a/apps/backend/internal/mcp/handlers/task_plan_append_mode_test.go +++ b/apps/backend/internal/mcp/handlers/task_plan_append_mode_test.go @@ -20,9 +20,10 @@ import ( func TestMCPUpdateTaskPlanModeValidation(t *testing.T) { h := newMCPPlanTestHandlers(t) ctx := context.Background() - if _, err := h.planService.CreatePlan(ctx, service.CreatePlanRequest{ + created, err := h.planService.CreatePlan(ctx, service.CreatePlanRequest{ TaskID: mcpPlanTaskID, Content: "base", CreatedBy: "agent", - }); err != nil { + }) + if err != nil { t.Fatalf("seed CreatePlan: %v", err) } @@ -65,7 +66,10 @@ func TestMCPUpdateTaskPlanModeValidation(t *testing.T) { t.Run("empty mode defaults to replace", func(t *testing.T) { out, err := h.handleUpdateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPUpdateTaskPlan, - `{"task_id":"`+mcpPlanTaskID+`","content":"whole new document"}`)) + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanTaskID, "content": "whole new document", + "expected_version": created.Plan.WriteVersion, + }))) if err != nil { t.Fatalf("handleUpdateTaskPlan: %v", err) } diff --git a/apps/backend/internal/mcp/handlers/task_plan_edit.go b/apps/backend/internal/mcp/handlers/task_plan_edit.go new file mode 100644 index 0000000000..b493af11e2 --- /dev/null +++ b/apps/backend/internal/mcp/handlers/task_plan_edit.go @@ -0,0 +1,43 @@ +package handlers + +import ( + "context" + "encoding/json" + + "github.com/kandev/kandev/internal/task/dto" + "github.com/kandev/kandev/internal/task/planws" + "github.com/kandev/kandev/internal/task/service" + ws "github.com/kandev/kandev/pkg/websocket" +) + +// handleEditTaskPlan applies one exact, unique agent edit to a task plan. +func (h *Handlers) handleEditTaskPlan(ctx context.Context, msg *ws.Message) (*ws.Message, error) { + var req struct { + TaskID string `json:"task_id"` + ExpectedVersion string `json:"expected_version"` + OldText string `json:"old_text"` + NewText *string `json:"new_text"` + AllowTruncation bool `json:"allow_truncation"` + } + if err := json.Unmarshal(msg.Payload, &req); err != nil { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "Invalid payload: "+err.Error(), nil) + } + if req.NewText == nil { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, + "new_text is required; use an empty string to delete the match", nil) + } + + result, err := h.planService.EditPlan(ctx, service.ExactPlanEditRequest{ + TaskID: req.TaskID, + ExpectedVersion: req.ExpectedVersion, + OldText: req.OldText, + NewText: *req.NewText, + AllowTruncation: req.AllowTruncation, + }) + if err != nil { + return planws.EditError(msg, err) + } + return ws.NewResponse(msg.ID, msg.Action, planWritePayload( + dto.TaskPlanFromModel(result.Plan), result.Plan.WriteVersion, "", result.PriorRevisionNumber, + )) +} diff --git a/apps/backend/internal/mcp/handlers/task_plan_guard.go b/apps/backend/internal/mcp/handlers/task_plan_guard.go index d12c07ea6b..425fc6faf9 100644 --- a/apps/backend/internal/mcp/handlers/task_plan_guard.go +++ b/apps/backend/internal/mcp/handlers/task_plan_guard.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/kandev/kandev/internal/task/dto" + "github.com/kandev/kandev/internal/task/models" ) // planTruncationWarning renders the agent-facing warning appended to a plan @@ -29,15 +30,11 @@ import ( // at 1 (NextTaskPlanRevisionNumber), so 0 is never a real revision. In that // case the warning does not make an unverified preservation claim. // -// It deliberately does NOT tell the caller to "recover" the content by -// calling an MCP tool: none of the four registered plan tools can read a -// past revision (get_task_plan_kandev returns the current, now-truncated, -// HEAD). Telling an agent to "recover it" here would send it to the only -// tool it has, get back the truncated document, and fall back to -// reconstructing the plan from memory — the exact WO-38 failure this guard -// exists to stop. Instead it says where the content lives (revision -// history, Kandev UI only) and that the caller cannot fetch it itself, so -// the caller stops and surfaces the loss instead of guessing. +// This helper remains for compatibility with older non-guarded callers. The +// current agent handlers reject a suspicious reduction before this warning +// can be produced and expose task-scoped revision recovery tools. If a legacy +// caller does receive the warning, it points to those tools and never asks +// the agent to reconstruct content from memory. func planTruncationWarning(replacedRunes, newRunes, priorRevisionNumber int) string { // dropped is always >= 0: the only caller renders this after the plan // service has already confirmed newRunes < replacedRunes. @@ -50,10 +47,10 @@ func planTruncationWarning(replacedRunes, newRunes, priorRevisionNumber int) str "This replace-mode write overwrote the entire document; use "+ "update_task_plan_kandev with mode=\"append\" to add a section without "+ "resubmitting the whole document next time. Kandev could not verify which "+ - "prior revision contains the pre-write content. The MCP plan tools cannot "+ - "fetch past revisions. If this drop was not intentional, stop and inspect "+ - "the task's revision history in the Kandev UI rather than rewriting the "+ - "plan from memory.", + "prior revision contains the pre-write content. Read the current plan and "+ + "list_task_plan_revisions_kandev before retrying. If this drop was not "+ + "intentional, fetch the identified revision when available rather than "+ + "rewriting the plan from memory.", replacedRunes, newRunes, dropped, droppedPct, ) } @@ -63,37 +60,45 @@ func planTruncationWarning(replacedRunes, newRunes, priorRevisionNumber int) str "This replace-mode write overwrote the entire document; use "+ "update_task_plan_kandev with mode=\"append\" to add a section without "+ "resubmitting the whole document next time. The pre-write content is "+ - "preserved in %s — recoverable from the Kandev UI, but NOT fetchable through "+ - "the MCP plan tools (get_task_plan_kandev returns the current, now-truncated, "+ - "content, not that revision). If this drop was not intentional, stop and "+ - "surface the loss rather than rewriting the plan from memory.", + "preserved in %s. Use list_task_plan_revisions_kandev and "+ + "get_task_plan_revision_kandev to inspect that snapshot. If this drop was "+ + "not intentional, restore it only after checking the current plan and both "+ + "snapshot versions; do not rewrite the plan from memory.", replacedRunes, newRunes, dropped, droppedPct, fmt.Sprintf("plan revision %d, in the task's plan revision history", priorRevisionNumber), ) } -// planWriteResponse extends the standard plan DTO with a truncation warning -// for the MCP write actions only. It deliberately does not touch +// planReadResponse extends the standard plan DTO with the agent-only opaque +// write version. It deliberately does not touch // dto.TaskPlanDTO itself — the browser plan editor (which has a visible diff // and revision history, and uses TaskPlanDTO as-is) is unaffected. -// json.Marshal promotes an embedded pointer struct's exported fields to the -// top level, so a non-truncating write still marshals to the identical shape -// callers see today. +type planReadResponse struct { + *dto.TaskPlanDTO + Version string `json:"version,omitempty"` +} + +// planWriteResponse extends the standard plan DTO with the committed version +// and, for compatibility with older callers, the existing truncation fields. type planWriteResponse struct { *dto.TaskPlanDTO + Version string `json:"version,omitempty"` PlanWriteWarning string `json:"plan_write_warning,omitempty"` PriorRevisionNumber int `json:"prior_revision_number,omitempty"` } -// planWritePayload wraps plan in a planWriteResponse when warning is -// non-empty, otherwise returns plan unwrapped so an unaffected write's -// response shape is unchanged. -func planWritePayload(plan *dto.TaskPlanDTO, warning string, priorRevision int) interface{} { - if warning == "" { +func planReadPayload(plan *models.TaskPlan) interface{} { + return planReadResponse{TaskPlanDTO: dto.TaskPlanFromModel(plan), Version: plan.WriteVersion} +} + +// planWritePayload always includes the committed version for agent writes. +func planWritePayload(plan *dto.TaskPlanDTO, version, warning string, priorRevision int) interface{} { + if version == "" && warning == "" { return plan } return planWriteResponse{ TaskPlanDTO: plan, + Version: version, PlanWriteWarning: warning, PriorRevisionNumber: priorRevision, } diff --git a/apps/backend/internal/mcp/handlers/task_plan_guard_test.go b/apps/backend/internal/mcp/handlers/task_plan_guard_test.go index ea9c4823f5..b65c2ca3d3 100644 --- a/apps/backend/internal/mcp/handlers/task_plan_guard_test.go +++ b/apps/backend/internal/mcp/handlers/task_plan_guard_test.go @@ -51,14 +51,9 @@ func TestPlanTruncationWarningUnknownRevisionDoesNotClaimPreservation(t *testing } } -// TestMCPPlanTruncationGuard_WarnsAndPreservesHistory pins the defect this -// card fixes: a write that drops the majority of a substantial plan today -// returns plain success with no signal that anything shrank (WO-38, task -// 809498b3, measured two incidents dropping 76-77% of a 40k+ char plan). -// -// This asserts against a response shape (plan_write_warning, -// prior_revision_number) that does not exist yet, so it fails first. -func TestMCPPlanTruncationGuard_WarnsAndPreservesHistory(t *testing.T) { +// TestMCPPlanTruncationGuard_RejectsUnacknowledgedReduction verifies that a +// suspicious replacement is rejected before it can alter HEAD or history. +func TestMCPPlanTruncationGuard_RejectsUnacknowledgedReduction(t *testing.T) { h := newMCPPlanTestHandlers(t) ctx := context.Background() @@ -75,56 +70,45 @@ func TestMCPPlanTruncationGuard_WarnsAndPreservesHistory(t *testing.T) { t.Fatalf("handleCreateTaskPlan: %v", err) } created := decodeMCPPlanPayload(t, createOut) - if warning, ok := created["plan_write_warning"]; ok { - t.Errorf("unexpected warning on initial create (no prior plan to truncate): %v", warning) + version, _ := created["version"].(string) + if version == "" { + t.Fatal("initial create omitted write version") } updateOut, err := h.handleUpdateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPUpdateTaskPlan, mustMarshalPlanPayload(t, map[string]any{ - "task_id": mcpPlanTaskID, - "content": small, + "task_id": mcpPlanTaskID, "content": small, "expected_version": version, }))) if err != nil { t.Fatalf("handleUpdateTaskPlan: %v", err) } - updated := decodeMCPPlanPayload(t, updateOut) - if updated["content"] != small { - t.Errorf("content = %v, want the new (truncated) content", updated["content"]) + if updateOut.Type != ws.MessageTypeError { + t.Fatalf("type = %q, want an error", updateOut.Type) + } + var rejection ws.ErrorPayload + if err := json.Unmarshal(updateOut.Payload, &rejection); err != nil { + t.Fatalf("decode rejection: %v", err) + } + if rejection.Details["reason"] != "plan_truncation_rejected" || rejection.Details["write_applied"] != false { + t.Fatalf("rejection details = %#v, want truncation/no-write", rejection.Details) + } + if !strings.Contains(rejection.Message, "not changed") { + t.Fatalf("rejection message = %q, want explicit no-write guidance", rejection.Message) } - warning, _ := updated["plan_write_warning"].(string) - if warning == "" { - t.Fatal("expected a truncation warning naming the character drop, got none") - } - if !strings.Contains(warning, "40000") || !strings.Contains(warning, "10000") { - t.Errorf("warning does not name the character drop (40000 -> 10000): %q", warning) - } - if !strings.Contains(strings.ToLower(warning), "entire") && !strings.Contains(strings.ToLower(warning), "whole document") { - t.Errorf("warning does not explain the write replaced the whole document: %q", warning) - } - - priorRev, ok := updated["prior_revision_number"].(float64) - if !ok || int(priorRev) != 1 { - t.Errorf("prior_revision_number = %v, want 1", updated["prior_revision_number"]) - } - - // The truncating write must NOT coalesce: revision 1 must survive as its - // own row with the full pre-truncation content, not be overwritten - // in-place by mergeRevisionInTx. revisions, err := h.planService.ListRevisions(ctx, mcpPlanTaskID) if err != nil { t.Fatalf("ListRevisions: %v", err) } - if len(revisions) != 2 { - t.Fatalf("expected 2 separate revisions (no coalesce), got %d", len(revisions)) + if len(revisions) != 1 { + t.Fatalf("expected one revision after rejected write, got %d", len(revisions)) } - rev1 := revisionWithNumber(t, revisions, 1) - fullRev1, err := h.planService.GetRevision(ctx, rev1.ID) + fullRev1, err := h.planService.GetRevision(ctx, revisions[0].ID) if err != nil { t.Fatalf("GetRevision(rev1): %v", err) } if fullRev1.Content != large { - t.Errorf("revision 1 content was mutated by the truncating write; got len=%d, want len=%d", + t.Errorf("revision content was mutated by the rejected write; got len=%d, want len=%d", len(fullRev1.Content), len(large)) } } @@ -137,7 +121,7 @@ func TestMCPPlanTruncationGuard_SmallDropsAreQuiet(t *testing.T) { ctx := context.Background() t.Run("small plan under the floor", func(t *testing.T) { - _, err := h.handleCreateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPCreateTaskPlan, + createOut, err := h.handleCreateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPCreateTaskPlan, mustMarshalPlanPayload(t, map[string]any{ "task_id": mcpPlanTaskID, "content": "a short plan", @@ -145,10 +129,10 @@ func TestMCPPlanTruncationGuard_SmallDropsAreQuiet(t *testing.T) { if err != nil { t.Fatalf("handleCreateTaskPlan: %v", err) } + version := decodeMCPPlanPayload(t, createOut)["version"] out, err := h.handleUpdateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPUpdateTaskPlan, mustMarshalPlanPayload(t, map[string]any{ - "task_id": mcpPlanTaskID, - "content": "x", + "task_id": mcpPlanTaskID, "content": "x", "expected_version": version, }))) if err != nil { t.Fatalf("handleUpdateTaskPlan: %v", err) @@ -162,7 +146,7 @@ func TestMCPPlanTruncationGuard_SmallDropsAreQuiet(t *testing.T) { t.Run("legitimate prune retains more than half", func(t *testing.T) { large := strings.Repeat("x", 40000) retained := strings.Repeat("y", 25000) // 62.5% retained, above the 50% line - _, err := h.handleCreateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPCreateTaskPlan, + createOut, err := h.handleCreateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPCreateTaskPlan, mustMarshalPlanPayload(t, map[string]any{ "task_id": mcpPlanlessID, "content": large, @@ -170,10 +154,10 @@ func TestMCPPlanTruncationGuard_SmallDropsAreQuiet(t *testing.T) { if err != nil { t.Fatalf("handleCreateTaskPlan: %v", err) } + version := decodeMCPPlanPayload(t, createOut)["version"] out, err := h.handleUpdateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPUpdateTaskPlan, mustMarshalPlanPayload(t, map[string]any{ - "task_id": mcpPlanlessID, - "content": retained, + "task_id": mcpPlanlessID, "content": retained, "expected_version": version, }))) if err != nil { t.Fatalf("handleUpdateTaskPlan: %v", err) @@ -203,7 +187,7 @@ func TestMCPPlanTruncationGuard_NonASCIIUsesCharacterCount(t *testing.T) { // cross the 50% line, but the character ratio must. small := strings.Repeat("好", 800) - _, err := h.handleCreateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPCreateTaskPlan, + createOut, err := h.handleCreateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPCreateTaskPlan, mustMarshalPlanPayload(t, map[string]any{ "task_id": mcpPlanTaskID, "content": large, @@ -211,23 +195,24 @@ func TestMCPPlanTruncationGuard_NonASCIIUsesCharacterCount(t *testing.T) { if err != nil { t.Fatalf("handleCreateTaskPlan: %v", err) } + version := decodeMCPPlanPayload(t, createOut)["version"] out, err := h.handleUpdateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPUpdateTaskPlan, mustMarshalPlanPayload(t, map[string]any{ - "task_id": mcpPlanTaskID, - "content": small, + "task_id": mcpPlanTaskID, "content": small, "expected_version": version, }))) if err != nil { t.Fatalf("handleUpdateTaskPlan: %v", err) } - updated := decodeMCPPlanPayload(t, out) - - warning, _ := updated["plan_write_warning"].(string) - if warning == "" { - t.Fatal("expected a truncation warning for an 80 percent character drop disguised as a 60 percent byte retain, got none") + if out.Type != ws.MessageTypeError { + t.Fatalf("type = %q, want a truncation rejection", out.Type) } - if !strings.Contains(warning, "4000") || !strings.Contains(warning, "800") { - t.Errorf("warning does not name the character counts (4000 -> 800): %q", warning) + var rejection ws.ErrorPayload + if err := json.Unmarshal(out.Payload, &rejection); err != nil { + t.Fatalf("decode rejection: %v", err) + } + if rejection.Details["reason"] != "plan_truncation_rejected" { + t.Fatalf("reason = %v, want plan_truncation_rejected", rejection.Details["reason"]) } } @@ -235,14 +220,14 @@ func TestMCPPlanTruncationGuard_NonASCIIUsesCharacterCount(t *testing.T) { // scope extension in handleCreateTaskPlan: CreatePlan upserts, so a create // call over an existing large plan is the same destructive write as update, // through a different door, and must be guarded identically. -func TestMCPPlanTruncationGuard_CreateOverExistingPlanWarns(t *testing.T) { +func TestMCPPlanTruncationGuard_CreateOverExistingPlanRejects(t *testing.T) { h := newMCPPlanTestHandlers(t) ctx := context.Background() large := strings.Repeat("x", 40000) small := strings.Repeat("y", 10000) - _, err := h.handleCreateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPCreateTaskPlan, + createOut, err := h.handleCreateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPCreateTaskPlan, mustMarshalPlanPayload(t, map[string]any{ "task_id": mcpPlanlessID, "content": large, @@ -250,20 +235,17 @@ func TestMCPPlanTruncationGuard_CreateOverExistingPlanWarns(t *testing.T) { if err != nil { t.Fatalf("handleCreateTaskPlan (initial): %v", err) } + version := decodeMCPPlanPayload(t, createOut)["version"] out, err := h.handleCreateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPCreateTaskPlan, mustMarshalPlanPayload(t, map[string]any{ - "task_id": mcpPlanlessID, - "content": small, + "task_id": mcpPlanlessID, "content": small, "expected_version": version, }))) if err != nil { t.Fatalf("handleCreateTaskPlan (overwrite): %v", err) } - updated := decodeMCPPlanPayload(t, out) - - warning, _ := updated["plan_write_warning"].(string) - if warning == "" { - t.Fatal("expected a truncation warning when create_task_plan_kandev overwrites an existing large plan, got none") + if out.Type != ws.MessageTypeError { + t.Fatalf("type = %q, want a truncation rejection", out.Type) } } @@ -347,21 +329,17 @@ func newMCPPlanTestHandlersWithFailingRevisionLookup(t *testing.T) (*Handlers, * return &Handlers{planService: service.NewPlanService(wrapped, eventBus, log), logger: log}, wrapped } -// TestMCPPlanTruncationGuard_RevisionLookupFailureOmitsRevisionNumber pins -// Review round 2 Finding 3: revision numbering starts at 1 -// (NextTaskPlanRevisionNumber), so "plan revision 0" can never be a real -// revision. When the latest-revision lookup fails on a truncating write, the -// plan service must still force a new revision and still warn — coalescing -// here would overwrite the only surviving copy of the pre-truncation content -// — but the rendered warning must not claim the content lives in revision 0. -func TestMCPPlanTruncationGuard_RevisionLookupFailureOmitsRevisionNumber(t *testing.T) { +// TestMCPPlanTruncationGuard_RevisionLookupFailureRejects verifies that an +// intentional reduction cannot proceed when the predecessor cannot be proven +// to exist in revision history. +func TestMCPPlanTruncationGuard_RevisionLookupFailureRejects(t *testing.T) { h, _ := newMCPPlanTestHandlersWithFailingRevisionLookup(t) ctx := context.Background() large := strings.Repeat("x", 40000) small := strings.Repeat("y", 10000) - _, err := h.handleCreateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPCreateTaskPlan, + createOut, err := h.handleCreateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPCreateTaskPlan, mustMarshalPlanPayload(t, map[string]any{ "task_id": mcpPlanTaskID, "content": large, @@ -369,35 +347,28 @@ func TestMCPPlanTruncationGuard_RevisionLookupFailureOmitsRevisionNumber(t *test if err != nil { t.Fatalf("handleCreateTaskPlan: %v", err) } + version := decodeMCPPlanPayload(t, createOut)["version"] out, err := h.handleUpdateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPUpdateTaskPlan, mustMarshalPlanPayload(t, map[string]any{ - "task_id": mcpPlanTaskID, - "content": small, + "task_id": mcpPlanTaskID, "content": small, "expected_version": version, "allow_truncation": true, }))) if err != nil { t.Fatalf("handleUpdateTaskPlan: %v", err) } - updated := decodeMCPPlanPayload(t, out) - - warning, _ := updated["plan_write_warning"].(string) - if warning == "" { - t.Fatal("expected a truncation warning even when the revision lookup fails, got none") + if out.Type != ws.MessageTypeError { + t.Fatalf("type = %q, want a history rejection", out.Type) } - if strings.Contains(warning, "revision 0") { - t.Errorf("warning names a nonexistent revision 0 (revision numbering starts at 1): %q", warning) + var rejection ws.ErrorPayload + if err := json.Unmarshal(out.Payload, &rejection); err != nil { + t.Fatalf("decode rejection: %v", err) } - if !strings.Contains(warning, "40000") || !strings.Contains(warning, "10000") { - t.Errorf("warning does not name the character drop (40000 -> 10000): %q", warning) - } - - if _, ok := updated["prior_revision_number"]; ok { - t.Errorf("prior_revision_number should be omitted when the revision number is unknown, got %v", - updated["prior_revision_number"]) + if rejection.Details["reason"] != "plan_history_unavailable" || rejection.Details["write_applied"] != false { + t.Fatalf("rejection details = %#v, want history-unavailable/no-write", rejection.Details) } } -func TestMCPPlanTruncationGuard_PlanLookupFailurePreservesHistory(t *testing.T) { +func TestMCPPlanTruncationGuard_PlanLookupFailureRejects(t *testing.T) { h, repo := newMCPPlanTestHandlersWithFailingRevisionLookup(t) ctx := context.Background() @@ -417,23 +388,28 @@ func TestMCPPlanTruncationGuard_PlanLookupFailurePreservesHistory(t *testing.T) out, err := h.handleUpdateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPUpdateTaskPlan, mustMarshalPlanPayload(t, map[string]any{ - "task_id": mcpPlanTaskID, - "content": small, + "task_id": mcpPlanTaskID, "content": small, "expected_version": "unknown", }))) if err != nil { t.Fatalf("handleUpdateTaskPlan: %v", err) } - updated := decodeMCPPlanPayload(t, out) - if warning, ok := updated["plan_write_warning"]; ok { - t.Errorf("unexpected warning when the guard could not read the prior plan: %v", warning) + if out.Type != ws.MessageTypeError { + t.Fatalf("type = %q, want a head-read rejection", out.Type) + } + var rejection ws.ErrorPayload + if err := json.Unmarshal(out.Payload, &rejection); err != nil { + t.Fatalf("decode rejection: %v", err) + } + if rejection.Details["reason"] != "plan_head_unavailable" || rejection.Details["write_applied"] != false { + t.Fatalf("rejection details = %#v, want head-unavailable/no-write", rejection.Details) } revisions, err := repo.Repository.ListTaskPlanRevisions(ctx, mcpPlanTaskID, 0) if err != nil { t.Fatalf("ListTaskPlanRevisions: %v", err) } - if len(revisions) != 2 { - t.Fatalf("expected 2 revisions after a guarded read failure, got %d", len(revisions)) + if len(revisions) != 1 { + t.Fatalf("expected one revision after a guarded read failure, got %d", len(revisions)) } rev1 := revisionWithNumber(t, revisions, 1) fullRev1, err := repo.GetTaskPlanRevision(ctx, rev1.ID) diff --git a/apps/backend/internal/mcp/handlers/task_plan_handlers_test.go b/apps/backend/internal/mcp/handlers/task_plan_handlers_test.go index 571914da76..b8e5e97453 100644 --- a/apps/backend/internal/mcp/handlers/task_plan_handlers_test.go +++ b/apps/backend/internal/mcp/handlers/task_plan_handlers_test.go @@ -188,7 +188,19 @@ func TestMCPPlanActionsRequireContent(t *testing.T) { t.Run("update", func(t *testing.T) { out, err := h.handleUpdateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPUpdateTaskPlan, `{"task_id":"`+mcpPlanTaskID+`"}`)) - assertMCPPlanError(t, out, err, ws.ErrorCodeValidation, "content is required") + if err != nil { + t.Fatalf("handleUpdateTaskPlan returned error: %v", err) + } + if out.Type != ws.MessageTypeError { + t.Fatalf("message type = %q, want %q", out.Type, ws.MessageTypeError) + } + var payload ws.ErrorPayload + if jsonErr := json.Unmarshal(out.Payload, &payload); jsonErr != nil { + t.Fatalf("unmarshal error payload: %v", jsonErr) + } + if payload.Code != ws.ErrorCodeValidation || payload.Details["reason"] != "plan_content_required" || payload.Details["write_applied"] != false { + t.Fatalf("error payload = %#v, want content-required validation with no write", payload) + } }) } @@ -296,15 +308,18 @@ func TestMCPPlanActionsRejectOversizedContent(t *testing.T) { func TestMCPPlanUpdateReportsMissingTask(t *testing.T) { h, repo := newMCPPlanTestHandlersWithRepo(t) ctx := context.Background() - if _, err := h.planService.CreatePlan(ctx, service.CreatePlanRequest{ + created, err := h.planService.CreatePlan(ctx, service.CreatePlanRequest{ TaskID: mcpPlanTaskID, Content: "initial", CreatedBy: "agent", - }); err != nil { + }) + if err != nil { t.Fatalf("CreatePlan: %v", err) } h.planService = service.NewPlanService(&missingTaskOnPlanWriteRepo{Repository: repo}, nil, h.logger) out, err := h.handleUpdateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPUpdateTaskPlan, - `{"task_id":"`+mcpPlanTaskID+`","content":"updated"}`)) + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanTaskID, "content": "updated", "expected_version": created.Plan.WriteVersion, + }))) assertMCPPlanError(t, out, err, ws.ErrorCodeNotFound, "Task not found") if strings.Contains(strings.ToLower(string(out.Payload)), "constraint") { t.Fatalf("error payload leaks storage details: %s", out.Payload) @@ -333,6 +348,7 @@ func (r *missingTaskOnPlanWriteRepo) WritePlanRevision( func TestMCPPlanActionsSucceed(t *testing.T) { h := newMCPPlanTestHandlers(t) ctx := context.Background() + var version string t.Run("get with no plan returns an empty object", func(t *testing.T) { out, err := h.handleGetTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPGetTaskPlan, @@ -361,6 +377,10 @@ func TestMCPPlanActionsSucceed(t *testing.T) { if plan["created_by"] != "agent" { t.Errorf("created_by = %v, want agent", plan["created_by"]) } + version, _ = plan["version"].(string) + if version == "" { + t.Fatal("create response omitted version") + } }) t.Run("get", func(t *testing.T) { @@ -376,7 +396,9 @@ func TestMCPPlanActionsSucceed(t *testing.T) { t.Run("update", func(t *testing.T) { out, err := h.handleUpdateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPUpdateTaskPlan, - `{"task_id":"`+mcpPlanTaskID+`","content":"step two"}`)) + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanTaskID, "content": "step two", "expected_version": version, + }))) if err != nil { t.Fatalf("handleUpdateTaskPlan: %v", err) } diff --git a/apps/backend/internal/mcp/handlers/task_plan_recovery.go b/apps/backend/internal/mcp/handlers/task_plan_recovery.go new file mode 100644 index 0000000000..d8098c195e --- /dev/null +++ b/apps/backend/internal/mcp/handlers/task_plan_recovery.go @@ -0,0 +1,128 @@ +package handlers + +import ( + "context" + "encoding/json" + + "github.com/kandev/kandev/internal/task/dto" + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/planws" + "github.com/kandev/kandev/internal/task/service" + ws "github.com/kandev/kandev/pkg/websocket" +) + +func (h *Handlers) handleListTaskPlanRevisions(ctx context.Context, msg *ws.Message) (*ws.Message, error) { + var req struct { + TaskID string `json:"task_id"` + BeforeRevisionNumber int `json:"before_revision_number"` + Limit int `json:"limit"` + } + if err := json.Unmarshal(msg.Payload, &req); err != nil { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "Invalid payload: "+err.Error(), nil) + } + page, err := h.planService.ListAgentPlanRevisionMetadata( + ctx, req.TaskID, req.BeforeRevisionNumber, req.Limit, + ) + if err != nil { + return planws.Error(msg, err, "Failed to list task plan revisions") + } + revisions := make([]map[string]interface{}, 0, len(page.Revisions)) + for _, revision := range page.Revisions { + revisions = append(revisions, planRevisionMetadataPayload(revision)) + } + payload := map[string]interface{}{ + "task_id": req.TaskID, + "revisions": revisions, + } + if page.NextBeforeRevisionNum > 0 { + payload["next_before_revision_number"] = page.NextBeforeRevisionNum + } + return ws.NewResponse(msg.ID, msg.Action, payload) +} + +func (h *Handlers) handleGetTaskPlanRevision(ctx context.Context, msg *ws.Message) (*ws.Message, error) { + var req struct { + TaskID string `json:"task_id"` + RevisionID string `json:"revision_id"` + } + if err := json.Unmarshal(msg.Payload, &req); err != nil { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "Invalid payload: "+err.Error(), nil) + } + revision, version, err := h.planService.GetAgentPlanRevision(ctx, req.TaskID, req.RevisionID) + if err != nil { + return planws.Error(msg, err, "Failed to get task plan revision") + } + return ws.NewResponse(msg.ID, msg.Action, struct { + *dto.TaskPlanRevisionDTO + RevisionVersion string `json:"revision_version"` + }{ + TaskPlanRevisionDTO: dto.TaskPlanRevisionFromModel(revision), + RevisionVersion: version, + }) +} + +func (h *Handlers) handleRestoreTaskPlanRevision(ctx context.Context, msg *ws.Message) (*ws.Message, error) { + var req struct { + TaskID string `json:"task_id"` + RevisionID string `json:"revision_id"` + ExpectedVersion string `json:"expected_version"` + ExpectedRevisionVersion string `json:"expected_revision_version"` + } + if err := json.Unmarshal(msg.Payload, &req); err != nil { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "Invalid payload: "+err.Error(), nil) + } + result, err := h.planService.RestoreAgentPlanRevision(ctx, service.RestorePlanRequest{ + TaskID: req.TaskID, + RevisionID: req.RevisionID, + ExpectedVersion: req.ExpectedVersion, + ExpectedRevisionVersion: req.ExpectedRevisionVersion, + }) + if err != nil { + return planws.Error(msg, err, "Failed to restore task plan revision") + } + status := "restored" + if result.AlreadyCurrent { + status = "already_current" + } + payload := map[string]interface{}{ + "task_id": req.TaskID, + "status": status, + "title": result.Plan.Title, + "version": result.Plan.WriteVersion, + "revision_id": result.Revision.ID, + "revision_number": result.Revision.RevisionNumber, + "content_bytes": len(result.Plan.Content), + "revision_version": service.PlanRevisionVersion(result.Revision), + } + if result.Revision.RevertOfRevisionID != nil { + payload["revert_of_revision_id"] = *result.Revision.RevertOfRevisionID + } + return ws.NewResponse(msg.ID, msg.Action, payload) +} + +func planRevisionMetadataPayload(revision *models.TaskPlanRevision) map[string]interface{} { + payload := map[string]interface{}{ + "revision_id": revision.ID, + "task_id": revision.TaskID, + "revision_number": revision.RevisionNumber, + "title": revision.Title, + "content_bytes": revision.ContentBytes, + "author_kind": revision.AuthorKind, + "author_name": revision.AuthorName, + "created_at": revision.CreatedAt, + "updated_at": revision.UpdatedAt, + } + if revision.RevertOfRevisionID != nil { + payload["revert_of_revision_id"] = *revision.RevertOfRevisionID + } + if revision.WorkflowStepID != "" { + payload["workflow_step_id"] = revision.WorkflowStepID + } + if revision.WorkflowStepName != "" { + payload["workflow_step_name"] = revision.WorkflowStepName + } + if revision.WorkflowStepColor != "" { + payload["workflow_step_color"] = revision.WorkflowStepColor + } + return payload +} diff --git a/apps/backend/internal/mcp/handlers/task_plan_safe_edits_test.go b/apps/backend/internal/mcp/handlers/task_plan_safe_edits_test.go new file mode 100644 index 0000000000..a3ec506ea7 --- /dev/null +++ b/apps/backend/internal/mcp/handlers/task_plan_safe_edits_test.go @@ -0,0 +1,311 @@ +package handlers + +import ( + "context" + "encoding/json" + "reflect" + "strings" + "testing" + + "github.com/kandev/kandev/internal/events/bus" + "github.com/kandev/kandev/internal/task/service" + ws "github.com/kandev/kandev/pkg/websocket" +) + +type mcpPlanEventRecorder struct { + bus.EventBus + events []*bus.Event +} + +func (r *mcpPlanEventRecorder) Publish(_ context.Context, _ string, event *bus.Event) error { + r.events = append(r.events, event) + return nil +} + +func TestMCPPlanExactEditRejectsMissingOrNullNewTextWithoutMutation(t *testing.T) { + for _, tc := range []struct { + name string + payload string + }{ + {name: "omitted", payload: `{"task_id":"` + mcpPlanTaskID + `","old_text":"remove"}`}, + {name: "null", payload: `{"task_id":"` + mcpPlanTaskID + `","old_text":"remove","new_text":null}`}, + } { + t.Run(tc.name, func(t *testing.T) { + h, repo := newMCPPlanTestHandlersWithRepo(t) + recorder := &mcpPlanEventRecorder{} + h.planService = service.NewPlanService(repo, recorder, h.logger) + ctx := context.Background() + createdOut, err := h.handleCreateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPCreateTaskPlan, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanTaskID, "content": "keep remove", + }))) + if err != nil { + t.Fatalf("handleCreateTaskPlan: %v", err) + } + version := decodeMCPPlanPayload(t, createdOut)["version"].(string) + tc.payload = strings.Replace(tc.payload, `"old_text"`, `"expected_version":"`+version+`","old_text"`, 1) + recorder.events = nil + + beforePlan, err := h.planService.GetPlan(ctx, mcpPlanTaskID) + if err != nil { + t.Fatalf("GetPlan before invalid edit: %v", err) + } + beforeHistory, err := h.planService.ListRevisions(ctx, mcpPlanTaskID) + if err != nil { + t.Fatalf("ListRevisions before invalid edit: %v", err) + } + + out, handleErr := h.handleEditTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPEditTaskPlan, tc.payload)) + if handleErr != nil { + t.Fatalf("handleEditTaskPlan: %v", handleErr) + } + if out.Type != ws.MessageTypeError { + t.Fatalf("message type = %q, want error", out.Type) + } + var payload ws.ErrorPayload + if err := json.Unmarshal(out.Payload, &payload); err != nil { + t.Fatalf("decode error: %v", err) + } + if payload.Code != ws.ErrorCodeValidation || payload.Message != "new_text is required; use an empty string to delete the match" { + t.Fatalf("error payload = %#v, want validation for missing new_text", payload) + } + + gotPlan, err := h.planService.GetPlan(ctx, mcpPlanTaskID) + if err != nil { + t.Fatalf("GetPlan after invalid edit: %v", err) + } + if !reflect.DeepEqual(gotPlan, beforePlan) { + t.Fatalf("plan changed after invalid edit: got %#v, want %#v", gotPlan, beforePlan) + } + gotHistory, err := h.planService.ListRevisions(ctx, mcpPlanTaskID) + if err != nil { + t.Fatalf("ListRevisions after invalid edit: %v", err) + } + if !reflect.DeepEqual(gotHistory, beforeHistory) { + t.Fatalf("history changed after invalid edit: got %#v, want %#v", gotHistory, beforeHistory) + } + if len(recorder.events) != 0 { + t.Fatalf("invalid edit published %d events", len(recorder.events)) + } + }) + } +} + +func TestMCPPlanReplaceRequiresVersionAndReturnsSafeDetails(t *testing.T) { + h := newMCPPlanTestHandlers(t) + ctx := context.Background() + large := strings.Repeat("x", 4000) + createdOut, err := h.handleCreateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPCreateTaskPlan, + mustMarshalPlanPayload(t, map[string]any{"task_id": mcpPlanTaskID, "content": large}))) + if err != nil { + t.Fatalf("handleCreateTaskPlan: %v", err) + } + created := decodeMCPPlanPayload(t, createdOut) + version, ok := created["version"].(string) + if !ok || version == "" { + t.Fatalf("create response version = %v, want a non-empty string", created["version"]) + } + + updateOut, err := h.handleUpdateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPUpdateTaskPlan, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanTaskID, "content": strings.Repeat("y", 800), "mode": "replace", + }))) + if err != nil { + t.Fatalf("handleUpdateTaskPlan: %v", err) + } + var rejection ws.ErrorPayload + if err := json.Unmarshal(updateOut.Payload, &rejection); err != nil { + t.Fatalf("decode rejection: %v", err) + } + if updateOut.Type != ws.MessageTypeError || rejection.Code != ws.ErrorCodeValidation { + t.Fatalf("rejection = type %q/code %q, want error/validation", updateOut.Type, rejection.Code) + } + if rejection.Details["reason"] != "plan_version_required" || rejection.Details["write_applied"] != false { + t.Fatalf("rejection details = %#v, want version-required and no write", rejection.Details) + } + if rejection.Details["next_action"] == "" { + t.Fatal("rejection omitted next_action correction guidance") + } + plan, err := h.planService.GetPlan(ctx, mcpPlanTaskID) + if err != nil { + t.Fatalf("GetPlan after rejection: %v", err) + } + if plan.Content != large || plan.WriteVersion != version { + t.Fatalf("plan changed after rejection: len=%d/version=%q", len(plan.Content), plan.WriteVersion) + } +} + +func TestMCPPlanReplaceEchoesCommittedVersionAndAppendDoesNotRequireIt(t *testing.T) { + h := newMCPPlanTestHandlers(t) + ctx := context.Background() + createdOut, err := h.handleCreateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPCreateTaskPlan, + mustMarshalPlanPayload(t, map[string]any{"task_id": mcpPlanTaskID, "content": "before"}))) + if err != nil { + t.Fatalf("handleCreateTaskPlan: %v", err) + } + created := decodeMCPPlanPayload(t, createdOut) + version := created["version"].(string) + + updatedOut, err := h.handleUpdateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPUpdateTaskPlan, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanTaskID, "content": "after", "mode": "replace", "expected_version": version, + }))) + if err != nil { + t.Fatalf("handleTaskPlan update: %v", err) + } + updated := decodeMCPPlanPayload(t, updatedOut) + updatedVersion, ok := updated["version"].(string) + if !ok || updatedVersion == "" || updatedVersion == version { + t.Fatalf("update response version = %v, want a new non-empty version", updated["version"]) + } + + appendOut, err := h.handleUpdateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPUpdateTaskPlan, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanTaskID, "content": "appended", "mode": "append", + }))) + if err != nil { + t.Fatalf("append without expected version: %v", err) + } + appended := decodeMCPPlanPayload(t, appendOut) + if appended["content"] != "after\n\nappended" { + t.Fatalf("appended content = %v, want composed plan", appended["content"]) + } +} + +func TestMCPPlanExactEditBridgesVersionAndPreservesContent(t *testing.T) { + h := newMCPPlanTestHandlers(t) + ctx := context.Background() + createdOut, err := h.handleCreateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPCreateTaskPlan, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanTaskID, "content": "before\r\n- [ ] task\r\nafter", + }))) + if err != nil { + t.Fatalf("handleCreateTaskPlan: %v", err) + } + version := decodeMCPPlanPayload(t, createdOut)["version"].(string) + + out, err := h.handleEditTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPEditTaskPlan, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanTaskID, "expected_version": version, + "old_text": "- [ ] task", "new_text": "- [x] task", + }))) + if err != nil { + t.Fatalf("handleEditTaskPlan: %v", err) + } + updated := decodeMCPPlanPayload(t, out) + if updated["content"] != "before\r\n- [x] task\r\nafter" { + t.Fatalf("edited content = %v", updated["content"]) + } + if newVersion, ok := updated["version"].(string); !ok || newVersion == version || newVersion == "" { + t.Fatalf("edited response version = %v, want a fresh token", updated["version"]) + } +} + +func TestMCPPlanExactEditRejectsAmbiguousAndMissingMatches(t *testing.T) { + h := newMCPPlanTestHandlers(t) + ctx := context.Background() + createdOut, err := h.handleCreateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPCreateTaskPlan, + mustMarshalPlanPayload(t, map[string]any{"task_id": mcpPlanTaskID, "content": "one\nstone"}))) + if err != nil { + t.Fatalf("handleCreateTaskPlan: %v", err) + } + version := decodeMCPPlanPayload(t, createdOut)["version"].(string) + for _, tc := range []struct { + name string + old string + code string + }{ + {name: "ambiguous", old: "one", code: "plan_edit_ambiguous"}, + {name: "missing", old: "none", code: "plan_edit_not_found"}, + } { + t.Run(tc.name, func(t *testing.T) { + out, err := h.handleEditTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPEditTaskPlan, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanTaskID, "expected_version": version, + "old_text": tc.old, "new_text": "replacement", + }))) + if err != nil { + t.Fatalf("handleEditTaskPlan: %v", err) + } + var payload ws.ErrorPayload + if err := json.Unmarshal(out.Payload, &payload); err != nil { + t.Fatalf("decode error: %v", err) + } + if payload.Details["reason"] != tc.code || payload.Details["write_applied"] != false { + t.Fatalf("details = %#v, want reason %q and no write", payload.Details, tc.code) + } + }) + } +} + +func TestMCPPlanRevisionRecoveryHandlers(t *testing.T) { + h := newMCPPlanTestHandlers(t) + ctx := context.Background() + first, err := h.planService.CreatePlan(ctx, service.CreatePlanRequest{ + TaskID: mcpPlanTaskID, Title: "Original", Content: "complete plan", + }) + if err != nil { + t.Fatalf("CreatePlan(first): %v", err) + } + second, err := h.planService.CreatePlan(ctx, service.CreatePlanRequest{ + TaskID: mcpPlanTaskID, Title: "Current", Content: "short plan", ForceNewRevision: true, + }) + if err != nil { + t.Fatalf("CreatePlan(second): %v", err) + } + history, err := h.planService.ListRevisions(ctx, mcpPlanTaskID) + if err != nil { + t.Fatalf("ListRevisions: %v", err) + } + source := history[len(history)-1] + + listOut, err := h.handleListTaskPlanRevisions(ctx, mcpPlanMsg(t, ws.ActionMCPListTaskPlanRevisions, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanTaskID, "limit": 1, + }))) + if err != nil { + t.Fatalf("handleListTaskPlanRevisions: %v", err) + } + var listPayload struct { + Revisions []map[string]interface{} `json:"revisions"` + } + if err := json.Unmarshal(listOut.Payload, &listPayload); err != nil { + t.Fatalf("decode list: %v", err) + } + if len(listPayload.Revisions) != 1 || listPayload.Revisions[0]["content"] != nil { + t.Fatalf("metadata response = %#v, want one content-free row", listPayload) + } + + getOut, err := h.handleGetTaskPlanRevision(ctx, mcpPlanMsg(t, ws.ActionMCPGetTaskPlanRevision, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanTaskID, "revision_id": source.ID, + }))) + if err != nil { + t.Fatalf("handleGetTaskPlanRevision: %v", err) + } + var sourcePayload map[string]interface{} + if err := json.Unmarshal(getOut.Payload, &sourcePayload); err != nil { + t.Fatalf("decode revision: %v", err) + } + if sourcePayload["content"] != source.Content || sourcePayload["revision_version"] == "" { + t.Fatalf("revision response = %#v, want exact content and token", sourcePayload) + } + + restoreOut, err := h.handleRestoreTaskPlanRevision(ctx, mcpPlanMsg(t, ws.ActionMCPRestoreTaskPlanRevision, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanTaskID, "revision_id": source.ID, + "expected_version": second.Plan.WriteVersion, + "expected_revision_version": sourcePayload["revision_version"], + }))) + if err != nil { + t.Fatalf("handleRestoreTaskPlanRevision: %v", err) + } + restored := decodeMCPPlanPayload(t, restoreOut) + if restored["status"] != "restored" || restored["version"] == second.Plan.WriteVersion { + t.Fatalf("restore response = %#v, want committed fresh version", restored) + } + plan, err := h.planService.GetPlan(ctx, mcpPlanTaskID) + if err != nil || plan.Content != first.Plan.Content { + t.Fatalf("restored plan = %#v, %v; want source content", plan, err) + } +} diff --git a/apps/backend/internal/mcp/server/handlers.go b/apps/backend/internal/mcp/server/handlers.go index 9bd067b5fd..93130db509 100644 --- a/apps/backend/internal/mcp/server/handlers.go +++ b/apps/backend/internal/mcp/server/handlers.go @@ -1228,6 +1228,9 @@ func planWriteAck(action string, result map[string]interface{}, sentContent stri if updatedAt := stringField(result, "updated_at"); updatedAt != "" { ack += ", updated_at=" + updatedAt } + if version := stringField(result, "version"); version != "" { + ack += ", version=" + version + } ack += ". Plan content is omitted from this response; read it back with get_task_plan_kandev if needed." if warning := stringField(result, "plan_write_warning"); warning != "" { ack += "\n\n" + warning @@ -1235,6 +1238,25 @@ func planWriteAck(action string, result map[string]interface{}, sentContent stri return mcp.NewToolResultText(ack) } +// planToolError preserves the structured correction data attached to a safe +// plan-write rejection when the MCP bridge turns a backend error into text. +func planToolError(err error) string { + var backendErr *BackendError + if !errors.As(err, &backendErr) || len(backendErr.Details) == 0 { + return err.Error() + } + parts := make([]string, 0, 5) + for _, key := range []string{"reason", "next_action", "current_version", "current_revision_version", "write_applied"} { + if value, ok := backendErr.Details[key]; ok { + parts = append(parts, fmt.Sprintf("%s=%v", key, value)) + } + } + if len(parts) == 0 { + return err.Error() + } + return err.Error() + " " + strings.Join(parts, "; ") +} + func (s *Server) createTaskPlanHandler() server.ToolHandlerFunc { return func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { taskID, err := s.resolveTaskID(req) @@ -1273,9 +1295,15 @@ func (s *Server) createTaskPlanHandler() server.ToolHandlerFunc { "title": title, "created_by": "agent", } + if expectedVersion := req.GetString("expected_version", ""); expectedVersion != "" { + payload["expected_version"] = expectedVersion + } + if req.GetBool("allow_truncation", false) { + payload["allow_truncation"] = true + } var result map[string]interface{} if err := s.backend.RequestPayload(ctx, ws.ActionMCPCreateTaskPlan, payload, &result); err != nil { - return mcp.NewToolResultError(err.Error()), nil + return mcp.NewToolResultError(planToolError(err)), nil } return planWriteAck("created", result, content), nil } @@ -1291,7 +1319,7 @@ func (s *Server) getTaskPlanHandler() server.ToolHandlerFunc { payload := map[string]string{"task_id": taskID} var result map[string]interface{} if err := s.backend.RequestPayload(ctx, ws.ActionMCPGetTaskPlan, payload, &result); err != nil { - return mcp.NewToolResultError(err.Error()), nil + return mcp.NewToolResultError(planToolError(err)), nil } // Check if plan exists @@ -1299,9 +1327,25 @@ func (s *Server) getTaskPlanHandler() server.ToolHandlerFunc { return mcp.NewToolResultText("No plan exists for this task yet."), nil } - // Return the plan content for easy reading + // Keep metadata separate from the content block. This lets an agent read + // the exact stored document while still receiving the version needed for + // its next conditional write. if content, ok := result["content"].(string); ok { - return mcp.NewToolResultText(content), nil + metadata := make(map[string]interface{}, len(result)) + for key, value := range result { + if key != "content" { + metadata[key] = value + } + } + metadata["content_bytes"] = len(content) + data, marshalErr := json.MarshalIndent(metadata, "", " ") + if marshalErr != nil { + return mcp.NewToolResultError("failed to format plan metadata"), nil + } + return &mcp.CallToolResult{Content: []mcp.Content{ + mcp.NewTextContent("Plan metadata:\n" + string(data)), + mcp.NewTextContent(content), + }}, nil } data, _ := json.MarshalIndent(result, "", " ") @@ -1355,15 +1399,58 @@ func (s *Server) updateTaskPlanHandler() server.ToolHandlerFunc { if mode != "" { payload["mode"] = mode } + if expectedVersion := req.GetString("expected_version", ""); expectedVersion != "" { + payload["expected_version"] = expectedVersion + } + if req.GetBool("allow_truncation", false) { + payload["allow_truncation"] = true + } var result map[string]interface{} if err := s.backend.RequestPayload(ctx, ws.ActionMCPUpdateTaskPlan, payload, &result); err != nil { - return mcp.NewToolResultError(err.Error()), nil + return mcp.NewToolResultError(planToolError(err)), nil } return planWriteAck("updated", result, content), nil } } +func (s *Server) editTaskPlanHandler() server.ToolHandlerFunc { + return func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { + taskID, err := s.resolveTaskID(req) + if err != nil { + return mcp.NewToolResultError("task_id is required"), nil + } + oldText, err := req.RequireString("old_text") + if err != nil { + return mcp.NewToolResultError("old_text is required"), nil + } + newText, err := req.RequireString("new_text") + if err != nil { + return mcp.NewToolResultError("new_text is required; use an empty string to delete the match"), nil + } + expectedVersion, err := req.RequireString("expected_version") + if err != nil { + return mcp.NewToolResultError("expected_version is required"), nil + } + + payload := map[string]interface{}{ + "task_id": taskID, + "old_text": oldText, + "new_text": newText, + "expected_version": expectedVersion, + "created_by": "agent", + } + if req.GetBool("allow_truncation", false) { + payload["allow_truncation"] = true + } + var result map[string]interface{} + if err := s.backend.RequestPayload(ctx, ws.ActionMCPEditTaskPlan, payload, &result); err != nil { + return mcp.NewToolResultError(planToolError(err)), nil + } + return planWriteAck("edited", result, newText), nil + } +} + // planModeArg reads the optional "mode" tool argument. It distinguishes a // value present but not a string from one genuinely absent: silently // treating the former as absent (as GetString would) defaults a malformed diff --git a/apps/backend/internal/mcp/server/server.go b/apps/backend/internal/mcp/server/server.go index 2142c441be..ab77e17481 100644 --- a/apps/backend/internal/mcp/server/server.go +++ b/apps/backend/internal/mcp/server/server.go @@ -1872,10 +1872,12 @@ This tool is available only to autopilot child tasks. It sends a durable questio func (s *Server) registerPlanTools() { s.mcpServer.AddTool( mcp.NewTool("create_task_plan_kandev", - mcp.WithDescription("Create or save a task plan. task_id addresses the plan's task: pass your own task ID for your current task, or another task's ID to write that task's plan (allowed only within your reach — same workspace / task tree; a task outside it is rejected, never silently redirected to your own). This tool always replaces the plan's entire content and has no append mode; it rejects any non-empty mode argument. If a plan already exists and you only want to add a section, use update_task_plan_kandev with mode=\"append\" instead — it composes your addition onto the stored plan without you reading and resending it. To replace the whole document here, read it first with get_task_plan_kandev if you need to preserve any of it."), + mcp.WithDescription("Create or save a task plan. task_id addresses the plan's task: pass your own task ID for your current task, or another task's ID to write that task's plan (allowed only within your reach — same workspace / task tree; a task outside it is rejected, never silently redirected to your own). This tool replaces the entire content. For an existing plan, expected_version from get_task_plan_kandev or a successful write is required; a suspicious reduction is rejected unless allow_truncation=true confirms an intentional reduction. Use edit_task_plan_kandev for a local change or update_task_plan_kandev with mode=\"append\" for a new section."), mcp.WithString("task_id", mcp.Description("The task ID to create a plan for. Defaults to your current task when omitted; pass another task's ID to target it directly.")), - mcp.WithString("content", mcp.Required(), mcp.Description("The full plan content in markdown format. This REPLACES any existing plan whole. To add a section to an existing plan without resending it, use update_task_plan_kandev with mode=\"append\" instead. To replace the whole document here, call get_task_plan_kandev first and include its content plus your additions in this call. Capped at 262,144 bytes (256 KiB) of UTF-8 content; a write over that limit is rejected and stores nothing.")), + mcp.WithString("content", mcp.Required(), mcp.Description("The full plan content in markdown format. This replaces existing content after expected_version is checked. Capped at 262,144 bytes (256 KiB) of UTF-8 content; a write over that limit is rejected and stores nothing.")), mcp.WithString("title", mcp.Description("Optional title for the plan (default: 'Plan')")), + mcp.WithString("expected_version", mcp.Description("Required when replacing an existing plan. Use the version returned by get_task_plan_kandev or a prior successful write.")), + mcp.WithBoolean("allow_truncation", mcp.Description("Acknowledge an intentional large reduction. Requires a matching expected_version; the previous snapshot remains in a new revision.")), // Declared (not just rejected in the handler) so the server's // generic MCP argument-schema validator - which rejects any // property absent from a tool's schema before the handler ever @@ -1896,7 +1898,7 @@ func (s *Server) registerPlanTools() { ) s.mcpServer.AddTool( mcp.NewTool("update_task_plan_kandev", - mcp.WithDescription(`Update an existing task plan. task_id selects the task whose plan to modify: your own task by default, or another task's ID to update that task's plan (allowed only within your reach — same workspace / task tree; a task outside it is rejected, never silently redirected to your own). Set mode="replace" (the default) to submit the whole document, or mode="append" to submit only an addition. In replace mode this call OVERWRITES THE ENTIRE PLAN: sending only a new section instead of the whole document will silently delete everything else, so call get_task_plan_kandev first and send the full document (prior content plus your changes), never just the new section. In append mode you do not need to read the plan first: the server reads the stored plan and stores it, then one blank line, then your content. append is not idempotent — resubmitting the same call adds your content again.`), + mcp.WithDescription(`Update an existing task plan. task_id selects the task whose plan to modify: your own task by default, or another task's ID to update that task's plan (allowed only within your reach — same workspace / task tree; a task outside it is rejected, never silently redirected to your own). Set mode="replace" (the default) to submit the whole document, or mode="append" to submit only an addition. Replace mode requires expected_version from get_task_plan_kandev or a successful write for an existing plan. A suspicious reduction is rejected before mutation; use edit_task_plan_kandev for a local change, or set allow_truncation=true only when the reduction is intentional and the matching version is current. In append mode you do not need to read the plan first: the server reads the stored plan and stores it, then one blank line, then your content. append is not idempotent — resubmitting the same call adds your content again.`), mcp.WithString("task_id", mcp.Description("The task ID to update the plan for. Defaults to your current task when omitted; pass another task's ID to target it directly.")), // Deliberately not mcp.Required(): mode validity is checked before // task-reach authorization, ahead of @@ -1907,8 +1909,10 @@ func (s *Server) registerPlanTools() { // run either - both would report the wrong failure for a call // invalid on more than one axis. content-required is instead // enforced by PlanService.UpdatePlan, after authorization. - mcp.WithString("content", mcp.Description(`In mode="replace" (default): the full plan content in markdown format that REPLACES the entire existing plan. Sending only a new section instead of the whole document will silently delete everything else. Read the current plan with get_task_plan_kandev first and include its content here plus your additions. In mode="append": only the fragment to add — do not include the existing plan; the server composes it onto the stored content for you. Capped at 262,144 bytes (256 KiB) of UTF-8 content measured after composition; a write over that limit is rejected and stores nothing.`)), + mcp.WithString("content", mcp.Description(`In mode="replace" (default): the full plan content in markdown format. expected_version is checked before this document replaces the existing plan. In mode="append": only the fragment to add — do not include the existing plan; the server composes it onto the stored content for you. Capped at 262,144 bytes (256 KiB) of UTF-8 content measured after composition; a write over that limit is rejected and stores nothing.`)), mcp.WithString("title", mcp.Description("Optional new title for the plan")), + mcp.WithString("expected_version", mcp.Description("Required for replace mode. Use the version returned by get_task_plan_kandev or a prior successful write. Optional for append mode.")), + mcp.WithBoolean("allow_truncation", mcp.Description("Acknowledge an intentional large reduction in replace mode. Requires a matching expected_version; the previous snapshot remains in a new revision.")), // Deliberately no mcp.Enum here: the server's generic MCP // argument-schema validator enforces a declared enum strictly // (compiled with additionalProperties:false) and would then @@ -1926,6 +1930,44 @@ func (s *Server) registerPlanTools() { ), s.wrapHandler("update_task_plan_kandev", s.updateTaskPlanHandler()), ) + s.mcpServer.AddTool( + mcp.NewTool("edit_task_plan_kandev", + mcp.WithDescription("Apply one exact text edit to a task plan. Provide the current expected_version, a non-empty old_text that occurs exactly once, and new_text. The edit preserves all surrounding bytes, including line endings; use an empty new_text to delete the match. Ambiguous or stale edits are rejected without a write. For a new section, use update_task_plan_kandev with mode=\"append\"."), + mcp.WithString("task_id", mcp.Description("The task ID whose plan to edit. Defaults to your current task when omitted; pass another task's ID to target it directly.")), + mcp.WithString("expected_version", mcp.Required(), mcp.Description("The version returned by get_task_plan_kandev or a prior successful plan write.")), + mcp.WithString("old_text", mcp.Required(), mcp.Description("The exact non-empty text to replace. It must occur exactly once, including overlapping matches.")), + mcp.WithString("new_text", mcp.Required(), mcp.Description("The exact replacement text. Use an empty string to delete old_text.")), + mcp.WithBoolean("allow_truncation", mcp.Description("Acknowledge an intentional large reduction caused by this edit. Requires the matching expected_version and verified revision history.")), + ), + s.wrapHandler("edit_task_plan_kandev", s.editTaskPlanHandler()), + ) + s.mcpServer.AddTool( + mcp.NewTool("list_task_plan_revisions_kandev", + mcp.WithDescription("List bounded metadata for a task plan's revisions, newest first. Responses contain IDs, authors, timestamps, titles, and byte sizes, but no revision content. Use get_task_plan_revision_kandev for one exact snapshot before a deliberate restore."), + mcp.WithString("task_id", mcp.Description("The task ID whose plan history to list. Defaults to your current task when omitted; pass another task's ID to target it directly.")), + mcp.WithInteger("before_revision_number", mcp.Min(0), mcp.Description("Optional exclusive revision-number cursor from the previous response; zero starts at the newest revision.")), + mcp.WithInteger("limit", mcp.Min(1), mcp.Max(service.MaxPlanRevisionPageLimit), mcp.Description("Optional page size. Defaults to 20 and cannot exceed 100.")), + ), + s.wrapHandler("list_task_plan_revisions_kandev", s.listTaskPlanRevisionsHandler()), + ) + s.mcpServer.AddTool( + mcp.NewTool("get_task_plan_revision_kandev", + mcp.WithDescription("Fetch one exact task-plan revision, including its title and unchanged content plus a revision_version for conditional restore. The revision must belong to the addressed task."), + mcp.WithString("task_id", mcp.Description("The task ID that owns the revision. Defaults to your current task when omitted; pass another task's ID to target it directly.")), + mcp.WithString("revision_id", mcp.Required(), mcp.Description("The revision ID returned by list_task_plan_revisions_kandev.")), + ), + s.wrapHandler("get_task_plan_revision_kandev", s.getTaskPlanRevisionHandler()), + ) + s.mcpServer.AddTool( + mcp.NewTool("restore_task_plan_revision_kandev", + mcp.WithDescription("Restore one identified task-plan revision as a new agent-attributed revision. Requires the current plan version and the selected revision_version. It rejects intervening edits or changed source content, preserves the source and current history, and reports already_current without writing."), + mcp.WithString("task_id", mcp.Description("The task ID whose plan to restore. Defaults to your current task when omitted; pass another task's ID to target it directly.")), + mcp.WithString("revision_id", mcp.Required(), mcp.Description("The task-scoped revision to restore.")), + mcp.WithString("expected_version", mcp.Required(), mcp.Description("The current plan version returned by get_task_plan_kandev or a successful write.")), + mcp.WithString("expected_revision_version", mcp.Required(), mcp.Description("The revision_version returned by get_task_plan_revision_kandev for the selected source.")), + ), + s.wrapHandler("restore_task_plan_revision_kandev", s.restoreTaskPlanRevisionHandler()), + ) s.mcpServer.AddTool( mcp.NewTool("delete_task_plan_kandev", mcp.WithDescription("Delete a task plan. task_id selects the task whose plan to delete: your own task by default, or another task's ID to delete that task's plan (allowed only within your reach — same workspace / task tree; a task outside it is rejected, never silently redirected to your own)."), diff --git a/apps/backend/internal/mcp/server/server_test.go b/apps/backend/internal/mcp/server/server_test.go index 9066e2df82..2abde47ba9 100644 --- a/apps/backend/internal/mcp/server/server_test.go +++ b/apps/backend/internal/mcp/server/server_test.go @@ -807,7 +807,7 @@ drained: // as in TestServerModeTask_ToolCount and // TestRegisterTools_LoggedCountMatchesRegisteredTools (list_task_sessions_test.go), // which pin the per-mode registration rather than this SetProviders rebuild. - require.Len(t, tools, 37, "final registry should contain the complete GitLab-only task tool set") + require.Len(t, tools, 41, "final registry should contain the complete GitLab-only task tool set") assert.Contains(t, tools, "get_task_change_requests_kandev") assert.Contains(t, tools, "manage_task_change_request_kandev") assert.Contains(t, tools, "update_task_change_request_automation_kandev") @@ -984,7 +984,7 @@ func TestServerModeTask_ToolCount(t *testing.T) { // 20 kanban (incl. delete + archive task + stop_task + spawn_session + // list_task_sessions) + 1 add_branch_to_task + // 1 add_workspace_sources + 1 update_repository_base_branch + - // 1 step_complete (ADR 0015) + 1 interaction + 4 plan + 3 walkthrough + + // 1 step_complete (ADR 0015) + 1 interaction + 8 plan + 3 walkthrough + // 1 publish_review_findings + 1 related-tasks + 1 diagnostic bundle // + 2 task-dependency (add/remove) + 3 neutral task change-request tools + // 1 neutral outcome tool + 1 rich-output. The exact count below guards @@ -999,7 +999,7 @@ func TestServerModeTask_ToolCount(t *testing.T) { assert.Contains(t, tools, "add_task_dependency_kandev", "dependency edges must be manageable in task mode") assert.Contains(t, tools, "remove_task_dependency_kandev") assert.Contains(t, tools, "show_rich_output_kandev", "native rich output must be registered in task mode") - assert.Equal(t, 38, len(tools)) + assert.Equal(t, 42, len(tools)) } func TestServerStepCompleteTool_TaskAndOfficeOnlyAndDiscoverable(t *testing.T) { @@ -1068,6 +1068,10 @@ func TestServerModeOffice_RegistersCorrectTools(t *testing.T) { assert.Contains(t, tools, "create_task_plan_kandev") assert.Contains(t, tools, "get_task_plan_kandev") assert.Contains(t, tools, "update_task_plan_kandev") + assert.Contains(t, tools, "edit_task_plan_kandev") + assert.Contains(t, tools, "list_task_plan_revisions_kandev") + assert.Contains(t, tools, "get_task_plan_revision_kandev") + assert.Contains(t, tools, "restore_task_plan_revision_kandev") assert.Contains(t, tools, "delete_task_plan_kandev") // Office mode should have interaction tools @@ -1115,12 +1119,12 @@ 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 step_complete (ADR 0015) = 11. + // 8 plan + 1 interaction + 1 related-tasks + 3 task-documents + // + 1 rich-output + 1 step_complete (ADR 0015) = 15. // (delegate_task_kandev retired in favour of `agentctl kandev task create …`). // (list_task_comments_kandev retired in favour of `agentctl kandev comment list …`). assert.Contains(t, tools, "step_complete_kandev", "office mode must register the ADR 0015 completion signal") - assert.Equal(t, 11, len(tools)) + assert.Equal(t, 15, len(tools)) } func TestServerModeOffice_DisableAskQuestion(t *testing.T) { @@ -1137,10 +1141,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 step_complete (ADR 0015) = 10 + // 8 plan + 1 related-tasks + 3 task-documents + 1 rich-output + // + 1 step_complete (ADR 0015) = 14 // (no ask_user_question, no delegate, no list_task_comments) - assert.Equal(t, 10, len(tools)) + assert.Equal(t, 14, 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 dfcd1a135f..679546595a 100644 --- a/apps/backend/internal/mcp/server/sysprompt_sync_test.go +++ b/apps/backend/internal/mcp/server/sysprompt_sync_test.go @@ -274,7 +274,7 @@ func TestSyspromptToolNames_ExactlyMatchMCPOfficeMode(t *testing.T) { assert.Equal(t, registered, referenced, "Office first-turn context must advertise exactly the ModeOffice tool inventory") - assert.Len(t, registered, 11, "Office MCP must expose the replacement 11-tool catalog") + assert.Len(t, registered, 15, "Office MCP must expose the complete 15-tool catalog") } func TestDecisionMCPTransportIsAbsentFromOfficeAndKanban(t *testing.T) { diff --git a/apps/backend/internal/mcp/server/task_plan_append_mode_test.go b/apps/backend/internal/mcp/server/task_plan_append_mode_test.go index 7bab34709c..8acfa97898 100644 --- a/apps/backend/internal/mcp/server/task_plan_append_mode_test.go +++ b/apps/backend/internal/mcp/server/task_plan_append_mode_test.go @@ -64,9 +64,12 @@ func TestUpdateTaskPlanKandev_ToolDescription_CoversAC006(t *testing.T) { assert.Contains(t, desc, "only an addition") }) - t.Run("006.2 retains the destructive warning scoped to replace", func(t *testing.T) { - assert.Contains(t, desc, "replace mode") - assert.Contains(t, desc, "silently delete everything else") + t.Run("006.2 describes guarded replace correction", func(t *testing.T) { + assert.Contains(t, desc, "Replace mode") + assert.Contains(t, desc, "expected_version") + assert.Contains(t, desc, "rejected before mutation") + assert.Contains(t, desc, "edit_task_plan_kandev") + assert.NotContains(t, desc, "silently delete everything else") }) t.Run("006.3 states the blank-line separator", func(t *testing.T) { diff --git a/apps/backend/internal/mcp/server/task_plan_recovery.go b/apps/backend/internal/mcp/server/task_plan_recovery.go new file mode 100644 index 0000000000..fe808c05b5 --- /dev/null +++ b/apps/backend/internal/mcp/server/task_plan_recovery.go @@ -0,0 +1,119 @@ +package mcp + +import ( + "context" + "encoding/json" + "fmt" + + ws "github.com/kandev/kandev/pkg/websocket" + "github.com/mark3labs/mcp-go/mcp" + "github.com/mark3labs/mcp-go/server" +) + +func (s *Server) listTaskPlanRevisionsHandler() server.ToolHandlerFunc { + return func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { + taskID, err := s.resolveTaskID(req) + if err != nil { + return mcp.NewToolResultError("task_id is required"), nil + } + payload := map[string]interface{}{ + "task_id": taskID, + } + if args := req.GetArguments(); args["before_revision_number"] != nil { + payload["before_revision_number"] = req.GetInt("before_revision_number", 0) + } + if args := req.GetArguments(); args["limit"] != nil { + payload["limit"] = req.GetInt("limit", 0) + } + var result map[string]interface{} + if err := s.backend.RequestPayload(ctx, ws.ActionMCPListTaskPlanRevisions, payload, &result); err != nil { + return mcp.NewToolResultError(planToolError(err)), nil + } + data, marshalErr := json.MarshalIndent(result, "", " ") + if marshalErr != nil { + return mcp.NewToolResultError("failed to format task plan revision metadata"), nil + } + return mcp.NewToolResultText(string(data)), nil + } +} + +func (s *Server) getTaskPlanRevisionHandler() server.ToolHandlerFunc { + return func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { + taskID, err := s.resolveTaskID(req) + if err != nil { + return mcp.NewToolResultError("task_id is required"), nil + } + revisionID, err := req.RequireString("revision_id") + if err != nil { + return mcp.NewToolResultError("revision_id is required"), nil + } + var result map[string]interface{} + if err := s.backend.RequestPayload(ctx, ws.ActionMCPGetTaskPlanRevision, map[string]string{ + "task_id": taskID, "revision_id": revisionID, + }, &result); err != nil { + return mcp.NewToolResultError(planToolError(err)), nil + } + content, ok := result["content"].(string) + if !ok { + data, marshalErr := json.MarshalIndent(result, "", " ") + if marshalErr != nil { + return mcp.NewToolResultError("failed to format task plan revision"), nil + } + return mcp.NewToolResultText(string(data)), nil + } + metadata := make(map[string]interface{}, len(result)) + for key, value := range result { + if key != "content" { + metadata[key] = value + } + } + metadata["content_bytes"] = len(content) + data, marshalErr := json.MarshalIndent(metadata, "", " ") + if marshalErr != nil { + return mcp.NewToolResultError("failed to format task plan revision metadata"), nil + } + return &mcp.CallToolResult{Content: []mcp.Content{ + mcp.NewTextContent("Revision metadata:\n" + string(data)), + mcp.NewTextContent(content), + }}, nil + } +} + +func (s *Server) restoreTaskPlanRevisionHandler() server.ToolHandlerFunc { + return func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { + taskID, err := s.resolveTaskID(req) + if err != nil { + return mcp.NewToolResultError("task_id is required"), nil + } + revisionID, err := req.RequireString("revision_id") + if err != nil { + return mcp.NewToolResultError("revision_id is required"), nil + } + expectedVersion, err := req.RequireString("expected_version") + if err != nil { + return mcp.NewToolResultError("expected_version is required"), nil + } + expectedRevisionVersion, err := req.RequireString("expected_revision_version") + if err != nil { + return mcp.NewToolResultError("expected_revision_version is required"), nil + } + var result map[string]interface{} + if err := s.backend.RequestPayload(ctx, ws.ActionMCPRestoreTaskPlanRevision, map[string]string{ + "task_id": taskID, "revision_id": revisionID, + "expected_version": expectedVersion, "expected_revision_version": expectedRevisionVersion, + }, &result); err != nil { + return mcp.NewToolResultError(planToolError(err)), nil + } + status := stringField(result, "status") + if status == "" { + status = "restored" + } + ack := fmt.Sprintf( + "Plan %s successfully: task_id=%s, revision_id=%s, revision_number=%v, version=%s, %v bytes.", + status, stringField(result, "task_id"), stringField(result, "revision_id"), + result["revision_number"], stringField(result, "version"), result["content_bytes"], + ) + ack += " Plan content is omitted from this response; read it back with get_task_plan_kandev if needed." + return mcp.NewToolResultText(ack), nil + } +} diff --git a/apps/backend/internal/mcp/server/task_plan_recovery_test.go b/apps/backend/internal/mcp/server/task_plan_recovery_test.go new file mode 100644 index 0000000000..17f1b2e3c2 --- /dev/null +++ b/apps/backend/internal/mcp/server/task_plan_recovery_test.go @@ -0,0 +1,77 @@ +package mcp + +import ( + "encoding/json" + "testing" + + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPlanReadReturnsMetadataAndExactContentBlocks(t *testing.T) { + content := "# Plan\r\n\n- [ ] keep exact spacing\r\n" + backend := &testBackend{response: map[string]interface{}{ + "task_id": "task-A", "title": "Plan", "content": content, + "version": "write-v1", "updated_at": "2026-09-17T10:00:00Z", + }} + s := newTaskModeServer(t, backend, "task-A") + result := callTool(t, s, "get_task_plan_kandev", map[string]interface{}{}) + require.False(t, result.IsError) + require.Len(t, result.Content, 2) + metadata, ok := result.Content[0].(mcp.TextContent) + require.True(t, ok) + assert.Contains(t, metadata.Text, "write-v1") + assert.NotContains(t, metadata.Text, content) + body, ok := result.Content[1].(mcp.TextContent) + require.True(t, ok) + assert.Equal(t, content, body.Text) +} + +func TestPlanRevisionToolsBridgeAllArguments(t *testing.T) { + backend := &testBackend{response: map[string]interface{}{ + "task_id": "task-A", "revisions": []interface{}{}, + }} + s := newTaskModeServer(t, backend, "task-A") + + result := callTool(t, s, "list_task_plan_revisions_kandev", map[string]interface{}{ + "before_revision_number": 7, "limit": 4, + }) + require.False(t, result.IsError) + payload, ok := backend.lastPayload.(map[string]interface{}) + require.True(t, ok) + assert.Equal(t, "task-A", payload["task_id"]) + assert.Equal(t, 7, payload["before_revision_number"]) + assert.Equal(t, 4, payload["limit"]) + + backend.response = map[string]interface{}{ + "task_id": "task-A", "revision_id": "rev-1", "content": "exact", + "revision_version": "rev-v1", + } + result = callTool(t, s, "get_task_plan_revision_kandev", map[string]interface{}{ + "revision_id": "rev-1", + }) + require.False(t, result.IsError) + require.Len(t, result.Content, 2) + body, ok := result.Content[1].(mcp.TextContent) + require.True(t, ok) + assert.Equal(t, "exact", body.Text) + + backend.response = map[string]interface{}{ + "task_id": "task-A", "status": "restored", "revision_id": "rev-2", + "revision_number": 2, "version": "write-v2", "content_bytes": 5, + } + result = callTool(t, s, "restore_task_plan_revision_kandev", map[string]interface{}{ + "revision_id": "rev-1", "expected_version": "write-v1", + "expected_revision_version": "rev-v1", + }) + require.False(t, result.IsError) + text := result.Content[0].(mcp.TextContent).Text + assert.Contains(t, text, "write-v2") + assert.Contains(t, text, "rev-2") + assert.NotContains(t, text, "exact") + assert.Equal(t, "mcp.restore_task_plan_revision", backend.lastAction) + encoded, err := json.Marshal(backend.lastPayload) + require.NoError(t, err) + assert.Contains(t, string(encoded), "expected_revision_version") +} diff --git a/apps/backend/internal/mcp/server/task_plan_safe_edits_integration_test.go b/apps/backend/internal/mcp/server/task_plan_safe_edits_integration_test.go new file mode 100644 index 0000000000..a53c87d623 --- /dev/null +++ b/apps/backend/internal/mcp/server/task_plan_safe_edits_integration_test.go @@ -0,0 +1,347 @@ +package mcp + +import ( + "context" + "encoding/json" + "errors" + "path/filepath" + "reflect" + "strings" + "testing" + "time" + + "github.com/jmoiron/sqlx" + "github.com/kandev/kandev/internal/db" + "github.com/kandev/kandev/internal/events/bus" + backendmcp "github.com/kandev/kandev/internal/mcp/handlers" + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository/sqlite" + "github.com/kandev/kandev/internal/task/service" + v1 "github.com/kandev/kandev/pkg/api/v1" + ws "github.com/kandev/kandev/pkg/websocket" + "github.com/mark3labs/mcp-go/mcp" + "github.com/stretchr/testify/require" +) + +type recoveryIntegrationEventBus struct { + bus.EventBus + events []*bus.Event +} + +func (b *recoveryIntegrationEventBus) Publish(_ context.Context, _ string, event *bus.Event) error { + b.events = append(b.events, event) + return nil +} + +func (b *recoveryIntegrationEventBus) ClearEvents() { + b.events = nil +} + +func TestPlanSafeEditsMCPJourney(t *testing.T) { + log := newTestLogger(t) + conn, err := db.OpenSQLite(filepath.Join(t.TempDir(), "plan-safe-edits.db")) + require.NoError(t, err) + database := sqlx.NewDb(conn, "sqlite3") + t.Cleanup(func() { require.NoError(t, database.Close()) }) + repo, err := sqlite.NewWithDB(database, database, log) + require.NoError(t, err) + seedSafeEditsIntegrationTask(t, repo) + + planService := service.NewPlanService(repo, nil, log, 0) + backendHandlers := backendmcp.NewHandlers( + nil, nil, nil, nil, nil, nil, nil, nil, planService, nil, nil, nil, log, + ) + dispatcher := ws.NewDispatcher() + backendHandlers.RegisterHandlers(dispatcher) + backend := NewDispatcherBackendClient(dispatcher, log) + server := New(backend, "integration-session", "task-plan-safe-integration", 10005, log, "", false, ModeTask) + + original := "# Review plan\n\n" + strings.Repeat("- [ ] preserve review detail\n", 120) + created := callTool(t, server, "create_task_plan_kandev", map[string]interface{}{ + "content": original, + "title": "Review plan", + }) + require.False(t, created.IsError) + initialVersion := planVersionFromRead(t, callTool(t, server, "get_task_plan_kandev", nil)) + require.NotEmpty(t, initialVersion) + + appended := callTool(t, server, "update_task_plan_kandev", map[string]interface{}{ + "mode": "append", "content": "## Checklist\n\n- [ ] checkbox", + }) + require.False(t, appended.IsError) + currentVersion := planVersionFromRead(t, callTool(t, server, "get_task_plan_kandev", nil)) + require.NotEqual(t, initialVersion, currentVersion) + + rejected := callTool(t, server, "update_task_plan_kandev", map[string]interface{}{ + "content": "- [ ] checkbox", + "expected_version": currentVersion, + }) + require.True(t, rejected.IsError) + rejectedText := firstText(t, rejected) + require.Contains(t, rejectedText, "plan_truncation_rejected") + require.Contains(t, rejectedText, "write_applied=false") + require.Contains(t, rejectedText, "next_action=") + require.NotContains(t, strings.ToLower(rejectedText), "stop") + + edited := callTool(t, server, "edit_task_plan_kandev", map[string]interface{}{ + "expected_version": currentVersion, + "old_text": "- [ ] checkbox", + "new_text": "- [x] checkbox", + }) + require.False(t, edited.IsError) + currentVersion = planVersionFromRead(t, callTool(t, server, "get_task_plan_kandev", nil)) + readAfterEdit := callTool(t, server, "get_task_plan_kandev", nil) + require.Equal(t, original[:len(original)-1]+"\n\n## Checklist\n\n- [x] checkbox", planContentFromRead(t, readAfterEdit)) + + deleted := callTool(t, server, "edit_task_plan_kandev", map[string]interface{}{ + "expected_version": currentVersion, + "old_text": "- [x] checkbox", + "new_text": "", + }) + require.False(t, deleted.IsError) + currentVersion = planVersionFromRead(t, callTool(t, server, "get_task_plan_kandev", nil)) + readAfterDelete := callTool(t, server, "get_task_plan_kandev", nil) + require.NotContains(t, planContentFromRead(t, readAfterDelete), "- [x] checkbox") + + listed := callTool(t, server, "list_task_plan_revisions_kandev", map[string]interface{}{"limit": 10}) + require.False(t, listed.IsError) + var listPayload struct { + Revisions []struct { + RevisionID string `json:"revision_id"` + RevisionNumber int `json:"revision_number"` + } `json:"revisions"` + } + require.NoError(t, json.Unmarshal([]byte(firstText(t, listed)), &listPayload)) + require.GreaterOrEqual(t, len(listPayload.Revisions), 3) + sourceID := listPayload.Revisions[len(listPayload.Revisions)-1].RevisionID + + sourceRead := callTool(t, server, "get_task_plan_revision_kandev", map[string]interface{}{ + "revision_id": sourceID, + }) + require.False(t, sourceRead.IsError) + sourceVersion, sourceContent := revisionReadValues(t, sourceRead) + require.Equal(t, original, sourceContent) + + restored := callTool(t, server, "restore_task_plan_revision_kandev", map[string]interface{}{ + "revision_id": sourceID, + "expected_version": currentVersion, + "expected_revision_version": sourceVersion, + }) + require.False(t, restored.IsError) + require.Contains(t, firstText(t, restored), "Plan restored successfully") + require.Equal(t, original, planContentFromRead(t, callTool(t, server, "get_task_plan_kandev", nil))) +} + +// TestPlanRecoveryMCPEnforcesTaskScopeAndAuthorization covers +// AC-TASKS-PLAN-SAFE-004.1, AC-TASKS-PLAN-SAFE-004.3, AC-TASKS-PLAN-SAFE-004.5, +// and AC-TASKS-PLAN-SAFE-004.6 through the real MCP dispatcher. History reads +// authorize before lookup, and a revision from another task is rejected even +// when the addressed task itself is authorized. +func TestPlanRecoveryMCPEnforcesTaskScopeAndAuthorization(t *testing.T) { + log := newTestLogger(t) + conn, err := db.OpenSQLite(filepath.Join(t.TempDir(), "plan-recovery-scope.db")) + require.NoError(t, err) + database := sqlx.NewDb(conn, "sqlite3") + t.Cleanup(func() { require.NoError(t, database.Close()) }) + repo, err := sqlite.NewWithDB(database, database, log) + require.NoError(t, err) + seedSafeEditsIntegrationTask(t, repo) + seedSafeEditsIntegrationTaskWithID(t, repo, "task-plan-safe-foreign") + + eventBus := &recoveryIntegrationEventBus{} + planService := service.NewPlanService(repo, eventBus, log, 0) + ctx := context.Background() + current, err := planService.CreatePlan(ctx, service.CreatePlanRequest{ + TaskID: "task-plan-safe-integration", Title: "Current", Content: "CURRENT-BODY", + CreatedBy: "agent", ForceNewRevision: true, + }) + require.NoError(t, err) + foreign, err := planService.CreatePlan(ctx, service.CreatePlanRequest{ + TaskID: "task-plan-safe-foreign", Title: "Foreign", Content: "FOREIGN-SECRET", + CreatedBy: "agent", ForceNewRevision: true, + }) + require.NoError(t, err) + currentRevision, err := planService.GetLatestRevision(ctx, "task-plan-safe-integration") + require.NoError(t, err) + foreignRevision, err := planService.GetLatestRevision(ctx, "task-plan-safe-foreign") + require.NoError(t, err) + foreignRevisionVersion := service.PlanRevisionVersion(foreignRevision) + eventBus.ClearEvents() + + planService.SetTaskAuthorizer(func(_ context.Context, taskID string) error { + if taskID != "task-plan-safe-integration" { + return errors.New("task access denied") + } + return nil + }) + backendHandlers := backendmcp.NewHandlers( + nil, nil, nil, nil, nil, nil, nil, nil, planService, nil, nil, nil, log, + ) + dispatcher := ws.NewDispatcher() + backendHandlers.RegisterHandlers(dispatcher) + backend := NewDispatcherBackendClient(dispatcher, log) + server := New(backend, "integration-session", "task-plan-safe-integration", 10005, log, "", false, ModeTask) + + listed := callTool(t, server, "list_task_plan_revisions_kandev", nil) + require.False(t, listed.IsError) + readCurrent := callTool(t, server, "get_task_plan_revision_kandev", map[string]interface{}{ + "revision_id": currentRevision.ID, + }) + require.False(t, readCurrent.IsError) + _, currentContent := revisionReadValues(t, readCurrent) + require.Equal(t, "CURRENT-BODY", currentContent) + + currentSourceVersion := service.PlanRevisionVersion(currentRevision) + alreadyCurrent := callTool(t, server, "restore_task_plan_revision_kandev", map[string]interface{}{ + "revision_id": currentRevision.ID, + "expected_version": current.Plan.WriteVersion, + "expected_revision_version": currentSourceVersion, + }) + require.False(t, alreadyCurrent.IsError) + require.Contains(t, firstText(t, alreadyCurrent), "already_current") + + beforeCurrent, err := repo.GetTaskPlan(ctx, "task-plan-safe-integration") + require.NoError(t, err) + beforeCurrentHistory, err := repo.ListTaskPlanRevisions(ctx, "task-plan-safe-integration", 0) + require.NoError(t, err) + beforeForeign, err := repo.GetTaskPlan(ctx, "task-plan-safe-foreign") + require.NoError(t, err) + beforeForeignHistory, err := repo.ListTaskPlanRevisions(ctx, "task-plan-safe-foreign", 0) + require.NoError(t, err) + eventsBeforeRejections := len(eventBus.events) + + // The current task is authorized, but the selected revision belongs to a + // different task. The response must not expose its body or metadata. + crossTaskRead := callTool(t, server, "get_task_plan_revision_kandev", map[string]interface{}{ + "task_id": current.Plan.TaskID, "revision_id": foreignRevision.ID, + }) + assertNoForeignRevisionDetails(t, crossTaskRead, foreignRevision, foreign.Plan) + + crossTaskRestore := callTool(t, server, "restore_task_plan_revision_kandev", map[string]interface{}{ + "task_id": current.Plan.TaskID, "revision_id": foreignRevision.ID, + "expected_version": current.Plan.WriteVersion, "expected_revision_version": foreignRevisionVersion, + }) + assertNoForeignRevisionDetails(t, crossTaskRestore, foreignRevision, foreign.Plan) + + // An unauthorized task is rejected before list, read, or restore lookup. + deniedList := callTool(t, server, "list_task_plan_revisions_kandev", map[string]interface{}{ + "task_id": "task-plan-safe-foreign", + }) + assertNoForeignRevisionDetails(t, deniedList, foreignRevision, foreign.Plan) + + deniedRead := callTool(t, server, "get_task_plan_revision_kandev", map[string]interface{}{ + "task_id": "task-plan-safe-foreign", "revision_id": foreignRevision.ID, + }) + assertNoForeignRevisionDetails(t, deniedRead, foreignRevision, foreign.Plan) + + deniedRestore := callTool(t, server, "restore_task_plan_revision_kandev", map[string]interface{}{ + "task_id": "task-plan-safe-foreign", "revision_id": foreignRevision.ID, + "expected_version": foreign.Plan.WriteVersion, "expected_revision_version": foreignRevisionVersion, + }) + assertNoForeignRevisionDetails(t, deniedRestore, foreignRevision, foreign.Plan) + + afterCurrent, err := repo.GetTaskPlan(ctx, "task-plan-safe-integration") + require.NoError(t, err) + require.True(t, reflect.DeepEqual(afterCurrent, beforeCurrent), "cross-task rejections changed current HEAD") + afterCurrentHistory, err := repo.ListTaskPlanRevisions(ctx, "task-plan-safe-integration", 0) + require.NoError(t, err) + require.True(t, reflect.DeepEqual(afterCurrentHistory, beforeCurrentHistory), "cross-task rejections changed current history") + afterForeign, err := repo.GetTaskPlan(ctx, "task-plan-safe-foreign") + require.NoError(t, err) + require.True(t, reflect.DeepEqual(afterForeign, beforeForeign), "unauthorized requests changed foreign HEAD") + afterForeignHistory, err := repo.ListTaskPlanRevisions(ctx, "task-plan-safe-foreign", 0) + require.NoError(t, err) + require.True(t, reflect.DeepEqual(afterForeignHistory, beforeForeignHistory), "unauthorized requests changed foreign history") + require.Equal(t, eventsBeforeRejections, len(eventBus.events), "rejected recovery calls published events") +} + +func seedSafeEditsIntegrationTask(t *testing.T, repo *sqlite.Repository) { + t.Helper() + ctx := context.Background() + require.NoError(t, repo.CreateWorkspace(ctx, &models.Workspace{ID: "ws-plan-safe-integration", Name: "Plan safety"})) + require.NoError(t, repo.CreateWorkflow(ctx, &models.Workflow{ + ID: "wf-plan-safe-integration", WorkspaceID: "ws-plan-safe-integration", Name: "Plan safety workflow", + })) + seedSafeEditsIntegrationTaskWithID(t, repo, "task-plan-safe-integration") +} + +func seedSafeEditsIntegrationTaskWithID(t *testing.T, repo *sqlite.Repository, taskID string) { + t.Helper() + ctx := context.Background() + now := time.Now().UTC() + require.NoError(t, repo.CreateTask(ctx, &models.Task{ + ID: taskID, WorkspaceID: "ws-plan-safe-integration", + WorkflowID: "wf-plan-safe-integration", Title: "Plan safety task", + State: v1.TaskStateCreated, Priority: "medium", CreatedAt: now, UpdatedAt: now, + })) +} + +func firstText(t *testing.T, result *mcp.CallToolResult) string { + t.Helper() + require.NotEmpty(t, result.Content) + text, ok := result.Content[0].(mcp.TextContent) + require.True(t, ok) + return text.Text +} + +func allText(t *testing.T, result *mcp.CallToolResult) string { + t.Helper() + var out strings.Builder + for _, content := range result.Content { + text, ok := content.(mcp.TextContent) + if ok { + out.WriteString(text.Text) + } + } + return out.String() +} + +func assertNoForeignRevisionDetails( + t *testing.T, + result *mcp.CallToolResult, + revision *models.TaskPlanRevision, + plan *models.TaskPlan, +) { + t.Helper() + require.True(t, result.IsError) + text := allText(t, result) + for _, secret := range []string{ + revision.ID, revision.Title, revision.Content, service.PlanRevisionVersion(revision), + plan.WriteVersion, + } { + require.NotContains(t, text, secret) + } +} + +func planVersionFromRead(t *testing.T, result *mcp.CallToolResult) string { + t.Helper() + require.False(t, result.IsError) + require.Len(t, result.Content, 2) + var metadata map[string]interface{} + text := firstText(t, result) + require.NoError(t, json.Unmarshal([]byte(strings.TrimPrefix(text, "Plan metadata:\n")), &metadata)) + version, ok := metadata["version"].(string) + require.True(t, ok) + return version +} + +func planContentFromRead(t *testing.T, result *mcp.CallToolResult) string { + t.Helper() + require.False(t, result.IsError) + require.Len(t, result.Content, 2) + text, ok := result.Content[1].(mcp.TextContent) + require.True(t, ok) + return text.Text +} + +func revisionReadValues(t *testing.T, result *mcp.CallToolResult) (version, content string) { + t.Helper() + require.Len(t, result.Content, 2) + metadataText := result.Content[0].(mcp.TextContent).Text + var metadata map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(strings.TrimPrefix(metadataText, "Revision metadata:\n")), &metadata)) + version, ok := metadata["revision_version"].(string) + require.True(t, ok) + content = result.Content[1].(mcp.TextContent).Text + return version, content +} diff --git a/apps/backend/internal/task/models/models.go b/apps/backend/internal/task/models/models.go index 8e16755088..aa372a31cb 100644 --- a/apps/backend/internal/task/models/models.go +++ b/apps/backend/internal/task/models/models.go @@ -2543,11 +2543,14 @@ func (r *TaskEnvironmentRepo) ToAPI() map[string]interface{} { // TaskPlan represents a plan associated with a task type TaskPlan struct { - ID string `json:"id"` - TaskID string `json:"task_id"` - Title string `json:"title"` - Content string `json:"content"` - CreatedBy string `json:"created_by"` // "agent" or "user" + ID string `json:"id"` + TaskID string `json:"task_id"` + Title string `json:"title"` + Content string `json:"content"` + CreatedBy string `json:"created_by"` // "agent" or "user" + // WriteVersion changes on every committed title/content write. It is an + // internal optimistic-concurrency token and is not part of browser DTOs. + WriteVersion string `json:"-"` CreatedAt time.Time `json:"created_at"` UpdatedAt time.Time `json:"updated_at"` CommentsRevision int64 `json:"comments_revision"` @@ -2587,11 +2590,14 @@ type TaskPlanCommentRef struct { // TaskPlanRevision is one immutable snapshot in the revision history of a task plan. // Revisions are the source of truth for history; TaskPlan stores the latest revision's content as HEAD. type TaskPlanRevision struct { - ID string `json:"id"` - TaskID string `json:"task_id"` - RevisionNumber int `json:"revision_number"` - Title string `json:"title"` - Content string `json:"content"` + ID string `json:"id"` + TaskID string `json:"task_id"` + RevisionNumber int `json:"revision_number"` + Title string `json:"title"` + Content string `json:"content"` + // ContentBytes is populated by bounded metadata reads. Full revision reads + // leave it zero because callers can derive the size from Content. + ContentBytes int `json:"-"` AuthorKind string `json:"author_kind"` // "agent" | "user" AuthorName string `json:"author_name"` // display snapshot (agent profile name or user identifier) RevertOfRevisionID *string `json:"revert_of_revision_id,omitempty"` diff --git a/apps/backend/internal/task/planws/errors.go b/apps/backend/internal/task/planws/errors.go index 3eeba5af68..ee0e70d28d 100644 --- a/apps/backend/internal/task/planws/errors.go +++ b/apps/backend/internal/task/planws/errors.go @@ -18,6 +18,45 @@ import ( ws "github.com/kandev/kandev/pkg/websocket" ) +func safePlanErrorResponse(msg *ws.Message, err error) (*ws.Message, bool, error) { + var safety *service.PlanSafetyError + if !errors.As(err, &safety) { + return nil, false, nil + } + details := map[string]interface{}{ + "reason": safety.Code, + "operation": msg.Action, + "task_id": safety.TaskID, + "write_applied": false, + "next_action": safety.NextAction, + } + if safety.CurrentVersion != "" { + details["current_version"] = safety.CurrentVersion + } + if safety.CurrentRevisionVersion != "" { + details["current_revision_version"] = safety.CurrentRevisionVersion + } + if safety.CurrentRevisionNumber > 0 { + details["current_revision_number"] = safety.CurrentRevisionNumber + } + if safety.ReplacedRunes > 0 { + details["replaced_runes"] = safety.ReplacedRunes + details["new_runes"] = safety.NewRunes + } + code := ws.ErrorCodeInternalError + switch safety.Code { + case service.PlanErrorVersionRequired, service.PlanErrorTruncationRejected, + service.PlanErrorAppendTruncationFlag, + service.PlanErrorContentRequired, service.PlanErrorEditTextRequired, service.PlanErrorEditNotFound, + service.PlanErrorEditAmbiguous, service.PlanErrorRevisionVersionRequired: + code = ws.ErrorCodeValidation + case service.PlanErrorVersionConflict, service.PlanErrorRevisionChanged: + code = ws.ErrorCodeConflict + } + response, responseErr := ws.NewError(msg.ID, msg.Action, code, safety.Message, details) + return response, true, responseErr +} + // TaskIDRequest is the payload for the plan actions keyed only by task, so the // wire shape is declared once rather than re-spelled per surface. type TaskIDRequest struct { @@ -35,25 +74,27 @@ type mapping struct { // The task-plan error vocabulary. Messages are the ones the frontend and the // MCP tools already receive, so editing one now moves both surfaces together. var ( - taskIDRequired = mapping{service.ErrTaskIDRequired, ws.ErrorCodeValidation, "task_id is required"} - taskNotFound = mapping{repository.ErrTaskNotFound, ws.ErrorCodeNotFound, "Task not found"} - sessionIDRequired = mapping{service.ErrSessionIDRequired, ws.ErrorCodeValidation, "session_id is required"} - sessionTaskMismatch = mapping{service.ErrSessionTaskMismatch, ws.ErrorCodeValidation, "Session does not belong to task"} - planNotFound = mapping{service.ErrTaskPlanNotFound, ws.ErrorCodeNotFound, "Task plan not found"} - revisionIDRequired = mapping{service.ErrRevisionIDRequired, ws.ErrorCodeValidation, "revision_id is required"} - revisionNotFound = mapping{service.ErrRevisionNotFound, ws.ErrorCodeNotFound, "Revision not found"} - revisionTaskMismatch = mapping{service.ErrRevisionTaskMismatch, ws.ErrorCodeValidation, "Revision does not belong to task"} - planIDRequired = mapping{service.ErrPlanIDRequired, ws.ErrorCodeValidation, "plan_id is required"} - commentIDRequired = mapping{service.ErrPlanCommentIDRequired, ws.ErrorCodeValidation, "comment id is required"} - commentIDInvalid = mapping{service.ErrPlanCommentIDInvalid, ws.ErrorCodeValidation, "comment id must be a UUID"} - commentBodyRequired = mapping{service.ErrPlanCommentBodyRequired, ws.ErrorCodeValidation, "comment body is required"} - commentBodyTooLarge = mapping{service.ErrPlanCommentBodyTooLarge, ws.ErrorCodeValidation, "plan comment body is too large"} - commentTextTooLarge = mapping{service.ErrPlanCommentTextTooLarge, ws.ErrorCodeValidation, "plan comment selected text is too large"} - commentLimitExceeded = mapping{service.ErrPlanCommentLimitExceeded, ws.ErrorCodeValidation, "task plan comment collection is too large"} - commentVersionNeeded = mapping{service.ErrPlanCommentVersionNeeded, ws.ErrorCodeValidation, "expected_version must be positive"} - commentAnchorInvalid = mapping{service.ErrPlanCommentAnchorInvalid, ws.ErrorCodeValidation, "plan comment anchor is invalid"} - planCommentsChanged = mapping{service.ErrTaskPlanCommentsChanged, ws.ErrorCodePlanCommentsChanged, "Task plan comments changed"} - contentRequired = mapping{service.ErrContentRequired, ws.ErrorCodeValidation, "content is required"} + taskIDRequired = mapping{service.ErrTaskIDRequired, ws.ErrorCodeValidation, "task_id is required"} + taskNotFound = mapping{repository.ErrTaskNotFound, ws.ErrorCodeNotFound, "Task not found"} + sessionIDRequired = mapping{service.ErrSessionIDRequired, ws.ErrorCodeValidation, "session_id is required"} + sessionTaskMismatch = mapping{service.ErrSessionTaskMismatch, ws.ErrorCodeValidation, "Session does not belong to task"} + planNotFound = mapping{service.ErrTaskPlanNotFound, ws.ErrorCodeNotFound, "Task plan not found"} + revisionIDRequired = mapping{service.ErrRevisionIDRequired, ws.ErrorCodeValidation, "revision_id is required"} + revisionNotFound = mapping{service.ErrRevisionNotFound, ws.ErrorCodeNotFound, "Revision not found"} + revisionTaskMismatch = mapping{service.ErrRevisionTaskMismatch, ws.ErrorCodeValidation, "Revision does not belong to task"} + planIDRequired = mapping{service.ErrPlanIDRequired, ws.ErrorCodeValidation, "plan_id is required"} + commentIDRequired = mapping{service.ErrPlanCommentIDRequired, ws.ErrorCodeValidation, "comment id is required"} + commentIDInvalid = mapping{service.ErrPlanCommentIDInvalid, ws.ErrorCodeValidation, "comment id must be a UUID"} + commentBodyRequired = mapping{service.ErrPlanCommentBodyRequired, ws.ErrorCodeValidation, "comment body is required"} + commentBodyTooLarge = mapping{service.ErrPlanCommentBodyTooLarge, ws.ErrorCodeValidation, "plan comment body is too large"} + commentTextTooLarge = mapping{service.ErrPlanCommentTextTooLarge, ws.ErrorCodeValidation, "plan comment selected text is too large"} + commentLimitExceeded = mapping{service.ErrPlanCommentLimitExceeded, ws.ErrorCodeValidation, "task plan comment collection is too large"} + commentVersionNeeded = mapping{service.ErrPlanCommentVersionNeeded, ws.ErrorCodeValidation, "expected_version must be positive"} + commentAnchorInvalid = mapping{service.ErrPlanCommentAnchorInvalid, ws.ErrorCodeValidation, "plan comment anchor is invalid"} + planCommentsChanged = mapping{service.ErrTaskPlanCommentsChanged, ws.ErrorCodePlanCommentsChanged, "Task plan comments changed"} + contentRequired = mapping{service.ErrContentRequired, ws.ErrorCodeValidation, "content is required"} + revisionCursorInvalid = mapping{service.ErrPlanRevisionCursorInvalid, ws.ErrorCodeValidation, "before_revision_number must be zero or a positive revision number"} + revisionLimitInvalid = mapping{service.ErrPlanRevisionLimitInvalid, ws.ErrorCodeValidation, "limit must be between 1 and 100"} // appendFragmentWhitespaceOnly maps the whitespace-only fragment error. appendFragmentWhitespaceOnly = mapping{ service.ErrPlanAppendFragmentWhitespaceOnly, ws.ErrorCodeValidation, @@ -80,6 +121,8 @@ var ( revisionNotFound, revisionTaskMismatch, contentRequired, + revisionCursorInvalid, + revisionLimitInvalid, appendFragmentWhitespaceOnly, planContentUnreadable, } @@ -113,9 +156,11 @@ func contentTooLargeResponse(msg *ws.Message, err error) (*ws.Message, bool, err } out, mapErr := ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, sizeErr.Error(), map[string]interface{}{ - "reason": "plan_content_too_large", - "limit": sizeErr.Limit, - "submitted": sizeErr.Submitted, + "reason": "plan_content_too_large", + "limit": sizeErr.Limit, + "submitted": sizeErr.Submitted, + "write_applied": false, + "next_action": "Reduce the submitted content below the limit and submit it again.", }) return out, true, mapErr } @@ -154,6 +199,9 @@ func PlanCommentError(msg *ws.Message, err error, snapshot interface{}) (*ws.Mes // repository's ErrTaskNotFound is different: it means the write target task // did not exist and is safe to report as a not-found response. func CreateError(msg *ws.Message, err error) (*ws.Message, error) { + if out, matched, mapErr := safePlanErrorResponse(msg, err); matched { + return out, mapErr + } if out, matched, mapErr := contentTooLargeResponse(msg, err); matched { return out, mapErr } @@ -169,6 +217,9 @@ func GetError(msg *ws.Message, err error) (*ws.Message, error) { // UpdateError maps a PlanService.UpdatePlan failure. func UpdateError(msg *ws.Message, err error) (*ws.Message, error) { + if out, matched, mapErr := safePlanErrorResponse(msg, err); matched { + return out, mapErr + } if out, matched, mapErr := contentTooLargeResponse(msg, err); matched { return out, mapErr } @@ -178,6 +229,20 @@ func UpdateError(msg *ws.Message, err error) (*ws.Message, error) { }) } +// EditError maps the exact-edit service operation. It keeps the same safety +// details as create/update while retaining ordinary plan lookup errors. +func EditError(msg *ws.Message, err error) (*ws.Message, error) { + if out, matched, mapErr := safePlanErrorResponse(msg, err); matched { + return out, mapErr + } + if out, matched, mapErr := contentTooLargeResponse(msg, err); matched { + return out, mapErr + } + return errorResponse(msg, err, "Failed to edit task plan: "+err.Error(), []mapping{ + taskIDRequired, taskNotFound, planNotFound, contentRequired, + }) +} + // DeleteError maps a PlanService.DeletePlan failure. func DeleteError(msg *ws.Message, err error) (*ws.Message, error) { return errorResponse(msg, err, "Failed to delete task plan: "+err.Error(), []mapping{taskIDRequired, planNotFound}) @@ -187,5 +252,8 @@ func DeleteError(msg *ws.Message, err error) (*ws.Message, error) { // vocabulary — implementation-started, revert, and the revision reads. fallback // is the action's own summary; err.Error() is appended to it. func Error(msg *ws.Message, err error, fallback string) (*ws.Message, error) { + if out, matched, mapErr := safePlanErrorResponse(msg, err); matched { + return out, mapErr + } return errorResponse(msg, err, fallback+": "+err.Error(), allMappings) } diff --git a/apps/backend/internal/task/planws/errors_test.go b/apps/backend/internal/task/planws/errors_test.go index d6cb7713a3..0891bf08bb 100644 --- a/apps/backend/internal/task/planws/errors_test.go +++ b/apps/backend/internal/task/planws/errors_test.go @@ -137,6 +137,24 @@ func TestErrorCoversFullVocabulary(t *testing.T) { }) } +func TestSafePlanAppendTruncationFlagMapsToValidation(t *testing.T) { + out, mapErr := UpdateError(request(), &service.PlanSafetyError{ + Code: service.PlanErrorAppendTruncationFlag, + Message: "append truncation flag is not applicable", + NextAction: "retry the append without the flag", + }) + payload := decode(t, out, mapErr) + if payload.Code != ws.ErrorCodeValidation { + t.Fatalf("code = %q, want %q", payload.Code, ws.ErrorCodeValidation) + } + if payload.Details["reason"] != service.PlanErrorAppendTruncationFlag { + t.Fatalf("reason = %v, want %q", payload.Details["reason"], service.PlanErrorAppendTruncationFlag) + } + if payload.Details["write_applied"] != false { + t.Fatalf("write_applied = %v, want false", payload.Details["write_applied"]) + } +} + // TestSentinelsMatchThroughWrapping guards the mappers against a service that // starts annotating its errors: errors.Is, not equality, decides the code. func TestSentinelsMatchThroughWrapping(t *testing.T) { diff --git a/apps/backend/internal/task/repository/sqlite/base_migrations.go b/apps/backend/internal/task/repository/sqlite/base_migrations.go index d69bc4a174..40522f6aba 100644 --- a/apps/backend/internal/task/repository/sqlite/base_migrations.go +++ b/apps/backend/internal/task/repository/sqlite/base_migrations.go @@ -198,6 +198,12 @@ func (r *Repository) runMigrations() error { r.migrate.Apply("task_plans.implementation_started_session_id", `ALTER TABLE task_plans ADD COLUMN implementation_started_session_id TEXT`) r.migrate.Apply("task_plans.implementation_started_by", `ALTER TABLE task_plans ADD COLUMN implementation_started_by TEXT`) _ = r.migrate.Apply("task_plans.comments_revision", `ALTER TABLE task_plans ADD COLUMN comments_revision INTEGER NOT NULL DEFAULT 0`) + if err := r.migrate.Apply("task_plans.write_version", `ALTER TABLE task_plans ADD COLUMN write_version TEXT NOT NULL DEFAULT ''`); err != nil { + return err + } + if err := r.backfillTaskPlanWriteVersions(); err != nil { + return err + } // Authoritative per-message change signal (chat render-perf). SQLite forbids a // non-constant default on ADD COLUMN, so the column is added nullable and @@ -418,6 +424,44 @@ func (r *Repository) runMigrations() error { return nil } + +// backfillTaskPlanWriteVersions assigns a fresh opaque token to legacy HEAD +// rows. The empty-value predicate makes startup replay and an interrupted +// backfill safe without changing an already assigned version. +func (r *Repository) backfillTaskPlanWriteVersions() error { + ctx := r.migrationContext() + rows, err := r.db.QueryxContext(ctx, r.db.Rebind(` + SELECT id FROM task_plans + WHERE COALESCE(write_version, '') = '' + `)) + if err != nil { + return fmt.Errorf("list plans missing write versions: %w", err) + } + defer func() { _ = rows.Close() }() + + var planIDs []string + for rows.Next() { + var id string + if err := rows.Scan(&id); err != nil { + return fmt.Errorf("scan plan missing write version: %w", err) + } + planIDs = append(planIDs, id) + } + if err := rows.Err(); err != nil { + return fmt.Errorf("iterate plans missing write versions: %w", err) + } + for _, id := range planIDs { + if _, err := r.db.ExecContext(ctx, r.db.Rebind(` + UPDATE task_plans + SET write_version = ? + WHERE id = ? AND COALESCE(write_version, '') = '' + `), uuid.NewString(), id); err != nil { + return fmt.Errorf("backfill plan write version %s: %w", id, err) + } + } + return nil +} + func (r *Repository) backfillTaskSessionQueueIncarnations() error { ctx := r.migrationContext() rows, err := r.db.QueryxContext(ctx, `SELECT id FROM task_sessions WHERE queue_incarnation_id = ''`) diff --git a/apps/backend/internal/task/repository/sqlite/base_schema.go b/apps/backend/internal/task/repository/sqlite/base_schema.go index 83879615b5..0a44253e98 100644 --- a/apps/backend/internal/task/repository/sqlite/base_schema.go +++ b/apps/backend/internal/task/repository/sqlite/base_schema.go @@ -719,6 +719,7 @@ func (r *Repository) initPlansSchema() error { created_by TEXT NOT NULL DEFAULT 'agent', created_at TIMESTAMP NOT NULL, updated_at TIMESTAMP NOT NULL, + write_version TEXT NOT NULL DEFAULT '', comments_revision INTEGER NOT NULL DEFAULT 0, implementation_started_at TIMESTAMP, implementation_started_session_id TEXT, diff --git a/apps/backend/internal/task/repository/sqlite/document_plan_postgres_test.go b/apps/backend/internal/task/repository/sqlite/document_plan_postgres_test.go index a84dcc0621..09efc11c36 100644 --- a/apps/backend/internal/task/repository/sqlite/document_plan_postgres_test.go +++ b/apps/backend/internal/task/repository/sqlite/document_plan_postgres_test.go @@ -141,6 +141,10 @@ func TestPostgresWritePlanRevisionUpsertAndImplementationMarker(t *testing.T) { if first.RevisionNumber != 1 { t.Fatalf("RevisionNumber = %d, want 1", first.RevisionNumber) } + if head.WriteVersion == "" { + t.Fatal("first plan write did not assign a write version") + } + firstVersion := head.WriteVersion // The nullable implementation marker: COALESCE + CASE WHEN over TIMESTAMP. marked, err := repo.MarkTaskPlanImplementationStarted(ctx, "task-plan-pg", "session-pg", "jcfs") @@ -150,6 +154,9 @@ func TestPostgresWritePlanRevisionUpsertAndImplementationMarker(t *testing.T) { if marked.ImplementationStartedAt == nil { t.Fatal("ImplementationStartedAt = nil, want the marker set") } + if marked.WriteVersion != head.WriteVersion { + t.Fatalf("marker write version = %q, want %q unchanged", marked.WriteVersion, head.WriteVersion) + } startedAt := *marked.ImplementationStartedAt if marked.ImplementationStartedSessionID == nil || *marked.ImplementationStartedSessionID != "session-pg" { t.Errorf("ImplementationStartedSessionID = %v, want session-pg", marked.ImplementationStartedSessionID) @@ -178,6 +185,9 @@ func TestPostgresWritePlanRevisionUpsertAndImplementationMarker(t *testing.T) { if second.RevisionNumber != 2 { t.Errorf("second RevisionNumber = %d, want 2", second.RevisionNumber) } + if head.WriteVersion == "" || head.WriteVersion == firstVersion { + t.Fatalf("second plan write version = %q, want a fresh token after %q", head.WriteVersion, firstVersion) + } if got := countRows(t, repo, `SELECT COUNT(1) FROM task_plans WHERE task_id = ?`, "task-plan-pg"); got != 1 { t.Errorf("HEAD rows = %d, want 1 (ON CONFLICT must update, not insert)", got) } @@ -271,7 +281,17 @@ func TestPostgresPlanRevisionWorkflowColumnsReplayMigration(t *testing.T) { t.Fatalf("drop legacy column %s: %v", column, err) } } + if _, err := db.Exec(`ALTER TABLE task_plans DROP COLUMN write_version`); err != nil { + t.Fatalf("drop legacy task plan write_version: %v", err) + } now := time.Now().UTC() + if _, err := db.Exec(db.Rebind(` + INSERT INTO task_plans + (id, task_id, title, content, created_by, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?) + `), "legacy-plan-pg", "task-plan-replay-pg", "Plan", "legacy plan", "agent", now, now); err != nil { + t.Fatalf("insert legacy task plan: %v", err) + } if _, err := db.Exec(db.Rebind(` INSERT INTO task_plan_revisions (id, task_id, revision_number, title, content, author_kind, author_name, revert_of_revision_id, created_at, updated_at) @@ -286,6 +306,13 @@ func TestPostgresPlanRevisionWorkflowColumnsReplayMigration(t *testing.T) { if err := repo.runMigrations(); err != nil { t.Fatalf("replay legacy plan migrations: %v", err) } + var writeVersion string + if err := db.QueryRow(db.Rebind(`SELECT write_version FROM task_plans WHERE task_id = ?`), "task-plan-replay-pg").Scan(&writeVersion); err != nil { + t.Fatalf("read migrated task plan write version: %v", err) + } + if writeVersion == "" { + t.Fatal("legacy task plan write version was not backfilled") + } var stepID, stepName, stepColor string if err := db.QueryRow(db.Rebind(` diff --git a/apps/backend/internal/task/repository/sqlite/plan.go b/apps/backend/internal/task/repository/sqlite/plan.go index 553f4b1c5f..f140609253 100644 --- a/apps/backend/internal/task/repository/sqlite/plan.go +++ b/apps/backend/internal/task/repository/sqlite/plan.go @@ -9,6 +9,7 @@ import ( "github.com/google/uuid" "github.com/jmoiron/sqlx" internaldb "github.com/kandev/kandev/internal/db" + "github.com/kandev/kandev/internal/db/dialect" "github.com/kandev/kandev/internal/task/models" ) @@ -20,7 +21,7 @@ const revisionSelectCols = `id, task_id, revision_number, title, content, author // and is the fallback for unknown values when persisting plan history rows. const authorKindAgent = "agent" -const planSelectCols = `id, task_id, title, content, created_by, created_at, updated_at, comments_revision, implementation_started_at, implementation_started_session_id, implementation_started_by` +const planSelectCols = `id, task_id, title, content, created_by, created_at, updated_at, write_version, comments_revision, implementation_started_at, implementation_started_session_id, implementation_started_by` // CreateTaskPlan creates a new task plan. func (r *Repository) CreateTaskPlan(ctx context.Context, plan *models.TaskPlan) error { @@ -37,11 +38,15 @@ func (r *Repository) CreateTaskPlan(ctx context.Context, plan *models.TaskPlan) if plan.CreatedBy == "" { plan.CreatedBy = authorKindAgent } + writeVersion := uuid.NewString() _, err := r.db.ExecContext(ctx, r.db.Rebind(` - INSERT INTO task_plans (id, task_id, title, content, created_by, created_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?) - `), plan.ID, plan.TaskID, plan.Title, plan.Content, plan.CreatedBy, plan.CreatedAt, plan.UpdatedAt) + INSERT INTO task_plans (id, task_id, title, content, created_by, created_at, updated_at, write_version) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + `), plan.ID, plan.TaskID, plan.Title, plan.Content, plan.CreatedBy, plan.CreatedAt, plan.UpdatedAt, writeVersion) + if err == nil { + plan.WriteVersion = writeVersion + } return err } @@ -59,12 +64,13 @@ func (r *Repository) GetTaskPlan(ctx context.Context, taskID string) (*models.Ta // UpdateTaskPlan updates an existing task plan. func (r *Repository) UpdateTaskPlan(ctx context.Context, plan *models.TaskPlan) error { - plan.UpdatedAt = time.Now().UTC() + updatedAt := time.Now().UTC() + writeVersion := uuid.NewString() result, err := r.db.ExecContext(ctx, r.db.Rebind(` - UPDATE task_plans SET title = ?, content = ?, created_by = ?, updated_at = ? + UPDATE task_plans SET title = ?, content = ?, created_by = ?, updated_at = ?, write_version = ? WHERE task_id = ? - `), plan.Title, plan.Content, plan.CreatedBy, plan.UpdatedAt, plan.TaskID) + `), plan.Title, plan.Content, plan.CreatedBy, updatedAt, writeVersion, plan.TaskID) if err != nil { return fmt.Errorf("failed to update task plan: %w", err) } @@ -73,6 +79,8 @@ func (r *Repository) UpdateTaskPlan(ctx context.Context, plan *models.TaskPlan) if rows == 0 { return fmt.Errorf("task plan not found for task: %s", plan.TaskID) } + plan.UpdatedAt = updatedAt + plan.WriteVersion = writeVersion return nil } @@ -229,6 +237,67 @@ func (r *Repository) ListTaskPlanRevisions(ctx context.Context, taskID string, l return out, nil } +// ListTaskPlanRevisionMetadata returns bounded newest-first revision metadata +// without loading revision content. beforeRevisionNumber is an exclusive, +// positive cursor; a non-positive value starts at the newest revision. +func (r *Repository) ListTaskPlanRevisionMetadata( + ctx context.Context, taskID string, beforeRevisionNumber, limit int, +) ([]*models.TaskPlanRevision, error) { + contentBytes := dialect.ByteLength(r.db.DriverName(), "content") + query := `SELECT id, task_id, revision_number, title, ` + contentBytes + `, author_kind, author_name, revert_of_revision_id, workflow_step_id, workflow_step_name, workflow_step_color, created_at, updated_at FROM task_plan_revisions WHERE task_id = ?` + args := []interface{}{taskID} + if beforeRevisionNumber > 0 { + query += ` AND revision_number < ?` + args = append(args, beforeRevisionNumber) + } + query += ` ORDER BY revision_number DESC` + if limit > 0 { + query += sqlLimitClause + args = append(args, limit) + } + rows, err := r.ro.QueryContext(ctx, r.ro.Rebind(query), args...) + if err != nil { + return nil, fmt.Errorf("failed to list task plan revision metadata: %w", err) + } + defer func() { _ = rows.Close() }() + + var out []*models.TaskPlanRevision + for rows.Next() { + rev := &models.TaskPlanRevision{} + var contentBytes int64 + var revertOf sql.NullString + if err := rows.Scan( + &rev.ID, &rev.TaskID, &rev.RevisionNumber, &rev.Title, &contentBytes, + &rev.AuthorKind, &rev.AuthorName, &revertOf, + &rev.WorkflowStepID, &rev.WorkflowStepName, &rev.WorkflowStepColor, + &rev.CreatedAt, &rev.UpdatedAt, + ); err != nil { + return nil, fmt.Errorf("failed to scan task plan revision metadata: %w", err) + } + rev.ContentBytes = int(contentBytes) + if revertOf.Valid { + v := revertOf.String + rev.RevertOfRevisionID = &v + } + out = append(out, rev) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("iterate task plan revision metadata: %w", err) + } + return out, nil +} + +// GetTaskPlanRevisionForTask fetches a revision only when both its ID and +// owning task match. This prevents cross-task revision identifiers from +// becoming an information disclosure through agent recovery tools. +func (r *Repository) GetTaskPlanRevisionForTask( + ctx context.Context, taskID, revisionID string, +) (*models.TaskPlanRevision, error) { + return r.scanRevisionRow(r.ro.QueryRowContext(ctx, r.ro.Rebind( + `SELECT `+revisionSelectCols+` FROM task_plan_revisions WHERE task_id = ? AND id = ?`, + ), taskID, revisionID)) +} + // NextTaskPlanRevisionNumber returns max(revision_number)+1 for a task, or 1 if none exist. // // Note: prefer WritePlanRevision when writing a new revision — it computes the next number @@ -284,7 +353,8 @@ func (r *Repository) WritePlanRevision( return err } } - if err := upsertPlanHead(ctx, tx, r.db, head, now, preserveTitle, preserveCreatedBy); err != nil { + writeVersion, err := upsertPlanHead(ctx, tx, r.db, head, now, preserveTitle, preserveCreatedBy) + if err != nil { return err } rev.Title = head.Title @@ -297,7 +367,11 @@ func (r *Repository) WritePlanRevision( return err } } - return tx.Commit() + if err := tx.Commit(); err != nil { + return err + } + head.WriteVersion = writeVersion + return nil } func scanPlanRow(row *sql.Row) (*models.TaskPlan, error) { @@ -313,6 +387,7 @@ func scanPlanRow(row *sql.Row) (*models.TaskPlan, error) { &plan.CreatedBy, &plan.CreatedAt, &plan.UpdatedAt, + &plan.WriteVersion, &plan.CommentsRevision, &startedAt, &sessionID, @@ -343,7 +418,7 @@ func upsertPlanHead( head *models.TaskPlan, now time.Time, preserveTitle, preserveCreatedBy bool, -) error { +) (string, error) { if head.ID == "" { head.ID = uuid.New().String() } @@ -357,27 +432,29 @@ func upsertPlanHead( head.CreatedAt = now } head.UpdatedAt = now + writeVersion := uuid.NewString() if _, err := tx.ExecContext(ctx, db.Rebind(` - INSERT INTO task_plans (id, task_id, title, content, created_by, created_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?) + INSERT INTO task_plans (id, task_id, title, content, created_by, created_at, updated_at, write_version) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) ON CONFLICT(task_id) DO UPDATE SET title = CASE WHEN ? THEN task_plans.title ELSE excluded.title END, content = excluded.content, created_by = CASE WHEN ? THEN task_plans.created_by ELSE excluded.created_by END, - updated_at = excluded.updated_at - `), head.ID, head.TaskID, head.Title, head.Content, head.CreatedBy, head.CreatedAt, head.UpdatedAt, + updated_at = excluded.updated_at, + write_version = excluded.write_version + `), head.ID, head.TaskID, head.Title, head.Content, head.CreatedBy, head.CreatedAt, head.UpdatedAt, writeVersion, preserveTitle, preserveCreatedBy); err != nil { if internaldb.IsForeignKeyViolation(err) { - return fmt.Errorf("upsert task plan head for task %s: %w", head.TaskID, ErrTaskNotFound) + return "", fmt.Errorf("upsert task plan head for task %s: %w", head.TaskID, ErrTaskNotFound) } - return fmt.Errorf("upsert task plan head: %w", err) + return "", fmt.Errorf("upsert task plan head: %w", err) } if preserveTitle || preserveCreatedBy { var storedTitle, storedCreatedBy string if err := tx.QueryRowContext(ctx, db.Rebind(` SELECT title, created_by FROM task_plans WHERE task_id = ? `), head.TaskID).Scan(&storedTitle, &storedCreatedBy); err != nil { - return fmt.Errorf("read preserved task plan metadata: %w", err) + return "", fmt.Errorf("read preserved task plan metadata: %w", err) } if preserveTitle { head.Title = storedTitle @@ -386,7 +463,7 @@ func upsertPlanHead( head.CreatedBy = storedCreatedBy } } - return nil + return writeVersion, nil } func mergeRevisionInTx(ctx context.Context, tx *sqlx.Tx, db *sqlx.DB, rev *models.TaskPlanRevision, latestID string, now time.Time) error { diff --git a/apps/backend/internal/task/repository/sqlite/plan_version_test.go b/apps/backend/internal/task/repository/sqlite/plan_version_test.go new file mode 100644 index 0000000000..74547fc42e --- /dev/null +++ b/apps/backend/internal/task/repository/sqlite/plan_version_test.go @@ -0,0 +1,300 @@ +package sqlite + +import ( + "context" + "path/filepath" + "testing" + + "github.com/jmoiron/sqlx" + "github.com/kandev/kandev/internal/db" + "github.com/kandev/kandev/internal/task/models" +) + +func TestTaskPlanWriteVersionIsPersisted(t *testing.T) { + repo := newRepoForEntityTests(t) + ctx := context.Background() + seedTaskForDocs(t, repo, "task-plan-version") + + plan := &models.TaskPlan{ + ID: "plan-version", TaskID: "task-plan-version", Title: "Plan", Content: "first", + } + if err := repo.CreateTaskPlan(ctx, plan); err != nil { + t.Fatalf("CreateTaskPlan: %v", err) + } + first := plan.WriteVersion + if first == "" { + t.Fatal("CreateTaskPlan did not assign a write version") + } + + got, err := repo.GetTaskPlan(ctx, plan.TaskID) + if err != nil { + t.Fatalf("GetTaskPlan: %v", err) + } + if gotVersion := got.WriteVersion; gotVersion != first { + t.Fatalf("persisted write version = %q, want %q", gotVersion, first) + } + + plan.Content = "second" + if err := repo.UpdateTaskPlan(ctx, plan); err != nil { + t.Fatalf("UpdateTaskPlan: %v", err) + } + second := plan.WriteVersion + if second == "" || second == first { + t.Fatalf("UpdateTaskPlan write version = %q, want a new non-empty version after %q", second, first) + } +} + +func TestTaskPlanWriteVersionChangesWhenRevisionCoalesces(t *testing.T) { + repo := newRepoForEntityTests(t) + ctx := context.Background() + seedTaskForDocs(t, repo, "task-plan-version-coalesce") + + head := &models.TaskPlan{ID: "plan-version-coalesce", TaskID: "task-plan-version-coalesce", Content: "first"} + firstRevision := &models.TaskPlanRevision{ + TaskID: head.TaskID, Title: "Plan", Content: "first", AuthorKind: "agent", AuthorName: "Agent", + } + if err := repo.WritePlanRevision(ctx, head, firstRevision, nil, false, false); err != nil { + t.Fatalf("WritePlanRevision(first): %v", err) + } + firstVersion := head.WriteVersion + + head.Content = "second" + secondRevision := &models.TaskPlanRevision{ + TaskID: head.TaskID, Title: "Plan", Content: "second", AuthorKind: "agent", AuthorName: "Agent", + } + if err := repo.WritePlanRevision(ctx, head, secondRevision, &firstRevision.ID, false, false); err != nil { + t.Fatalf("WritePlanRevision(coalesce): %v", err) + } + if head.WriteVersion == firstVersion || head.WriteVersion == "" { + t.Fatalf("coalesced write version = %q, want a new non-empty version after %q", head.WriteVersion, firstVersion) + } + if secondRevision.ID != firstRevision.ID { + t.Fatalf("coalesced revision ID = %q, want %q", secondRevision.ID, firstRevision.ID) + } +} + +func TestTaskPlanWriteVersionSurvivesMarkerAndCommentWrites(t *testing.T) { + repo := newRepoForEntityTests(t) + ctx := context.Background() + seedTaskForDocs(t, repo, "task-plan-version-markers") + + plan := &models.TaskPlan{ID: "plan-version-markers", TaskID: "task-plan-version-markers", Content: "body"} + if err := repo.CreateTaskPlan(ctx, plan); err != nil { + t.Fatalf("CreateTaskPlan: %v", err) + } + version := plan.WriteVersion + if _, err := repo.MarkTaskPlanImplementationStarted(ctx, plan.TaskID, "session-version", "user"); err != nil { + t.Fatalf("MarkTaskPlanImplementationStarted: %v", err) + } + comment := &models.TaskPlanComment{ + ID: "comment-version", TaskID: plan.TaskID, PlanID: plan.ID, Body: "review", SelectedText: "body", + AnchorTo: len("body"), + } + if _, err := repo.CreateTaskPlanComment(ctx, comment); err != nil { + t.Fatalf("CreateTaskPlanComment: %v", err) + } + got, err := repo.GetTaskPlan(ctx, plan.TaskID) + if err != nil { + t.Fatalf("GetTaskPlan: %v", err) + } + if got.WriteVersion != version { + t.Fatalf("marker/comment write version = %q, want %q", got.WriteVersion, version) + } +} + +func TestTaskPlanWriteVersionMigrationBackfillsAndReplays(t *testing.T) { + repo := newRepoForEntityTests(t) + ctx := context.Background() + seedTaskForDocs(t, repo, "task-plan-version-migration") + plan := &models.TaskPlan{ + ID: "plan-version-migration", TaskID: "task-plan-version-migration", Content: "legacy body", + } + if err := repo.CreateTaskPlan(ctx, plan); err != nil { + t.Fatalf("CreateTaskPlan: %v", err) + } + if _, err := repo.db.ExecContext(ctx, `ALTER TABLE task_plans DROP COLUMN write_version`); err != nil { + t.Fatalf("drop write_version for legacy fixture: %v", err) + } + if err := repo.runMigrations(); err != nil { + t.Fatalf("runMigrations(backfill): %v", err) + } + migrated, err := repo.GetTaskPlan(ctx, plan.TaskID) + if err != nil { + t.Fatalf("GetTaskPlan after migration: %v", err) + } + if migrated.WriteVersion == "" { + t.Fatal("migration left legacy plan without a write version") + } + version := migrated.WriteVersion + if err := repo.runMigrations(); err != nil { + t.Fatalf("runMigrations(replay): %v", err) + } + replayed, err := repo.GetTaskPlan(ctx, plan.TaskID) + if err != nil { + t.Fatalf("GetTaskPlan after replay: %v", err) + } + if replayed.WriteVersion != version { + t.Fatalf("replayed write version = %q, want %q", replayed.WriteVersion, version) + } +} + +func TestTaskPlanWriteVersionRollbackLeavesHeadUnchanged(t *testing.T) { + repo := newRepoForEntityTests(t) + ctx := context.Background() + seedTaskForDocs(t, repo, "task-plan-version-rollback") + + head := &models.TaskPlan{ID: "plan-version-rollback", TaskID: "task-plan-version-rollback", Content: "before"} + firstRevision := &models.TaskPlanRevision{ + TaskID: head.TaskID, Title: "Plan", Content: "before", AuthorKind: "agent", AuthorName: "Agent", + } + if err := repo.WritePlanRevision(ctx, head, firstRevision, nil, false, false); err != nil { + t.Fatalf("WritePlanRevision(first): %v", err) + } + firstVersion := head.WriteVersion + missingRevisionID := "missing-plan-revision" + head.Content = "after" + err := repo.WritePlanRevision(ctx, head, &models.TaskPlanRevision{ + TaskID: head.TaskID, Title: "Plan", Content: "after", AuthorKind: "agent", AuthorName: "Agent", + }, &missingRevisionID, false, false) + if err == nil { + t.Fatal("WritePlanRevision accepted a missing coalesce target") + } + got, err := repo.GetTaskPlan(ctx, head.TaskID) + if err != nil { + t.Fatalf("GetTaskPlan after rollback: %v", err) + } + if got.Content != "before" || got.WriteVersion != firstVersion { + t.Fatalf("HEAD after rollback = content %q/version %q, want before/%q", got.Content, got.WriteVersion, firstVersion) + } +} + +func TestTaskPlanWriteVersionDeleteRecreateIsFresh(t *testing.T) { + repo := newRepoForEntityTests(t) + ctx := context.Background() + seedTaskForDocs(t, repo, "task-plan-version-recreate") + + first := &models.TaskPlan{ID: "plan-version-recreate-1", TaskID: "task-plan-version-recreate", Content: "first"} + if err := repo.CreateTaskPlan(ctx, first); err != nil { + t.Fatalf("CreateTaskPlan(first): %v", err) + } + if err := repo.DeleteTaskPlan(ctx, first.TaskID); err != nil { + t.Fatalf("DeleteTaskPlan: %v", err) + } + second := &models.TaskPlan{ID: "plan-version-recreate-2", TaskID: first.TaskID, Content: "second"} + if err := repo.CreateTaskPlan(ctx, second); err != nil { + t.Fatalf("CreateTaskPlan(second): %v", err) + } + if second.WriteVersion == "" || second.WriteVersion == first.WriteVersion { + t.Fatalf("recreated write version = %q, want a fresh token after %q", second.WriteVersion, first.WriteVersion) + } +} + +func TestTaskPlanWriteVersionSurvivesRestart(t *testing.T) { + ctx := context.Background() + dbPath := filepath.Join(t.TempDir(), "plan-version-restart.db") + + firstConn, err := db.OpenSQLite(dbPath) + if err != nil { + t.Fatalf("open first database: %v", err) + } + firstDB := sqlx.NewDb(firstConn, "sqlite3") + firstRepo, err := NewWithDB(firstDB, firstDB, nil) + if err != nil { + _ = firstDB.Close() + t.Fatalf("create first repository: %v", err) + } + seedWorkspace(t, firstRepo, "workspace-plan-version-restart") + if err := firstRepo.CreateTask(ctx, &models.Task{ + ID: "task-plan-version-restart", WorkspaceID: "workspace-plan-version-restart", Title: "Restart task", + }); err != nil { + _ = firstDB.Close() + t.Fatalf("CreateTask: %v", err) + } + plan := &models.TaskPlan{ + ID: "plan-version-restart", TaskID: "task-plan-version-restart", Content: "persistent body", + } + if err := firstRepo.CreateTaskPlan(ctx, plan); err != nil { + _ = firstDB.Close() + t.Fatalf("CreateTaskPlan: %v", err) + } + version := plan.WriteVersion + if err := firstDB.Close(); err != nil { + t.Fatalf("close first database: %v", err) + } + + secondConn, err := db.OpenSQLite(dbPath) + if err != nil { + t.Fatalf("open second database: %v", err) + } + secondDB := sqlx.NewDb(secondConn, "sqlite3") + t.Cleanup(func() { _ = secondDB.Close() }) + secondRepo, err := NewWithDB(secondDB, secondDB, nil) + if err != nil { + t.Fatalf("create second repository: %v", err) + } + got, err := secondRepo.GetTaskPlan(ctx, plan.TaskID) + if err != nil { + t.Fatalf("GetTaskPlan after restart: %v", err) + } + if got.WriteVersion != version || got.Content != plan.Content { + t.Fatalf("plan after restart = content %q/version %q, want %q/%q", got.Content, got.WriteVersion, plan.Content, version) + } +} + +func TestTaskPlanRevisionMetadataIsBoundedAndTaskScoped(t *testing.T) { + repo := newRepoForEntityTests(t) + ctx := context.Background() + seedTaskForDocs(t, repo, "task-plan-metadata") + seedTaskForDocs(t, repo, "task-plan-metadata-other") + + head := &models.TaskPlan{ + ID: "plan-metadata", TaskID: "task-plan-metadata", Title: "Plan", Content: "first 内容", + } + first := &models.TaskPlanRevision{ + TaskID: head.TaskID, Title: head.Title, Content: head.Content, AuthorKind: "agent", AuthorName: "Agent", + } + if err := repo.WritePlanRevision(ctx, head, first, nil, false, false); err != nil { + t.Fatalf("WritePlanRevision(first): %v", err) + } + head.Content = "第二 body" + second := &models.TaskPlanRevision{ + TaskID: head.TaskID, Title: head.Title, Content: head.Content, AuthorKind: "agent", AuthorName: "Agent", + } + if err := repo.WritePlanRevision(ctx, head, second, nil, false, false); err != nil { + t.Fatalf("WritePlanRevision(second): %v", err) + } + + page, err := repo.ListTaskPlanRevisionMetadata(ctx, head.TaskID, 0, 1) + if err != nil { + t.Fatalf("ListTaskPlanRevisionMetadata(first page): %v", err) + } + if len(page) != 1 || page[0].Content != "" || page[0].ContentBytes != len(second.Content) { + t.Fatalf("metadata page = %+v, want one content-free row with byte size %d", page, len(second.Content)) + } + if page[0].RevisionNumber != second.RevisionNumber { + t.Fatalf("metadata revision number = %d, want %d", page[0].RevisionNumber, second.RevisionNumber) + } + + older, err := repo.ListTaskPlanRevisionMetadata(ctx, head.TaskID, page[0].RevisionNumber, 1) + if err != nil { + t.Fatalf("ListTaskPlanRevisionMetadata(cursor): %v", err) + } + if len(older) != 1 || older[0].ID != first.ID { + t.Fatalf("cursor page = %+v, want first revision %q", older, first.ID) + } + + foreign, err := repo.GetTaskPlanRevisionForTask(ctx, "task-plan-metadata-other", second.ID) + if err != nil { + t.Fatalf("GetTaskPlanRevisionForTask(foreign): %v", err) + } + if foreign != nil { + t.Fatalf("foreign task received revision: %+v", foreign) + } + got, err := repo.GetTaskPlanRevisionForTask(ctx, head.TaskID, second.ID) + if err != nil { + t.Fatalf("GetTaskPlanRevisionForTask(owner): %v", err) + } + if got == nil || got.Content != second.Content { + t.Fatalf("owner revision = %+v, want exact content %q", got, second.Content) + } +} diff --git a/apps/backend/internal/task/service/plan_agent_recovery_test.go b/apps/backend/internal/task/service/plan_agent_recovery_test.go new file mode 100644 index 0000000000..ebbc0c4cb7 --- /dev/null +++ b/apps/backend/internal/task/service/plan_agent_recovery_test.go @@ -0,0 +1,529 @@ +package service + +import ( + "context" + "errors" + "reflect" + "strings" + "sync" + "testing" + "time" + + "github.com/kandev/kandev/internal/task/models" +) + +func TestPlanAgentRecovery(t *testing.T) { + svc, _, repo := createTestPlanService(t) + ctx := context.Background() + taskID := "task-plan-agent-recovery" + seedTask(t, ctx, repo, taskID) + first, err := svc.CreatePlan(ctx, CreatePlanRequest{ + TaskID: taskID, Title: "Original", Content: "complete plan", + }) + if err != nil { + t.Fatalf("CreatePlan(first): %v", err) + } + damaged, err := svc.CreatePlan(ctx, CreatePlanRequest{ + TaskID: taskID, Title: "Damaged", Content: "short fragment", ForceNewRevision: true, + }) + if err != nil { + t.Fatalf("CreatePlan(damaged): %v", err) + } + history, err := svc.ListRevisions(ctx, taskID) + if err != nil { + t.Fatalf("ListRevisions: %v", err) + } + if len(history) != 2 { + t.Fatalf("history length = %d, want 2", len(history)) + } + var source *models.TaskPlanRevision + for _, revision := range history { + if revision.Content == first.Plan.Content { + source = revision + } + } + if source == nil { + t.Fatal("could not identify the preserved source revision") + } + _, sourceVersion, err := svc.GetAgentPlanRevision(ctx, taskID, source.ID) + if err != nil { + t.Fatalf("GetAgentPlanRevision: %v", err) + } + + restored, err := svc.RestoreAgentPlanRevision(ctx, RestorePlanRequest{ + TaskID: taskID, RevisionID: source.ID, + ExpectedVersion: damaged.Plan.WriteVersion, ExpectedRevisionVersion: sourceVersion, + }) + if err != nil { + t.Fatalf("RestoreAgentPlanRevision: %v", err) + } + if restored.AlreadyCurrent || restored.Plan.Content != first.Plan.Content || restored.Plan.Title != first.Plan.Title { + t.Fatalf("restore result = %#v, want original snapshot", restored) + } + if restored.Plan.WriteVersion == damaged.Plan.WriteVersion { + t.Fatal("restore did not rotate the HEAD version") + } + if restored.Revision.RevertOfRevisionID == nil || *restored.Revision.RevertOfRevisionID != source.ID { + t.Fatalf("restore revision source = %v, want %q", restored.Revision.RevertOfRevisionID, source.ID) + } + if restored.Revision.AuthorKind != createdByAgent { + t.Fatalf("restore author kind = %q, want agent", restored.Revision.AuthorKind) + } + + finalHistory, err := svc.ListRevisions(ctx, taskID) + if err != nil { + t.Fatalf("ListRevisions after restore: %v", err) + } + if len(finalHistory) != 3 || finalHistory[1].Content != damaged.Plan.Content || finalHistory[2].Content != source.Content { + t.Fatalf("history after restore = %#v, want damaged and source preserved", finalHistory) + } +} + +func TestPlanAgentRecoveryRequiresBothStableVersions(t *testing.T) { + svc, _, repo := createTestPlanService(t) + ctx := context.Background() + taskID := "task-plan-agent-recovery-versions" + seedTask(t, ctx, repo, taskID) + first, err := svc.CreatePlan(ctx, CreatePlanRequest{TaskID: taskID, Content: "first"}) + if err != nil { + t.Fatalf("CreatePlan(first): %v", err) + } + second, err := svc.CreatePlan(ctx, CreatePlanRequest{TaskID: taskID, Content: "second", ForceNewRevision: true}) + if err != nil { + t.Fatalf("CreatePlan(second): %v", err) + } + revision, err := svc.GetLatestRevision(ctx, taskID) + if err != nil { + t.Fatalf("GetLatestRevision: %v", err) + } + _, revisionVersion, err := svc.GetAgentPlanRevision(ctx, taskID, revision.ID) + if err != nil { + t.Fatalf("GetAgentPlanRevision: %v", err) + } + + cases := []struct { + name string + req RestorePlanRequest + code string + }{ + { + name: "missing head version", code: PlanErrorVersionRequired, + req: RestorePlanRequest{TaskID: taskID, RevisionID: revision.ID, ExpectedRevisionVersion: revisionVersion}, + }, + { + name: "missing source version", code: PlanErrorRevisionVersionRequired, + req: RestorePlanRequest{TaskID: taskID, RevisionID: revision.ID, ExpectedVersion: second.Plan.WriteVersion}, + }, + { + name: "stale head version", code: PlanErrorVersionConflict, + req: RestorePlanRequest{TaskID: taskID, RevisionID: revision.ID, ExpectedVersion: first.Plan.WriteVersion, ExpectedRevisionVersion: revisionVersion}, + }, + { + name: "stale source version", code: PlanErrorRevisionChanged, + req: RestorePlanRequest{TaskID: taskID, RevisionID: revision.ID, ExpectedVersion: second.Plan.WriteVersion, ExpectedRevisionVersion: "stale"}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := svc.RestoreAgentPlanRevision(ctx, tc.req) + var safety *PlanSafetyError + if !errors.As(err, &safety) || safety.Code != tc.code { + t.Fatalf("error = %T %v, want safety code %q", err, err, tc.code) + } + }) + } + got, err := svc.GetPlan(ctx, taskID) + if err != nil || got.Content != "second" || got.WriteVersion != second.Plan.WriteVersion { + t.Fatalf("version failures changed plan = %#v, %v", got, err) + } +} + +// TestPlanAgentRecoveryRejectsCoalescedSourceSnapshot covers +// AC-TASKS-PLAN-SAFE-004.2 and AC-TASKS-PLAN-SAFE-005.1: a source token that +// was valid before coalescing must not restore the source after its bytes +// changed in place. +func TestPlanAgentRecoveryRejectsCoalescedSourceSnapshot(t *testing.T) { + svc, eventBus, repo := createTestPlanService(t) + ctx := context.Background() + taskID := "task-plan-agent-recovery-coalesced-source" + seedTask(t, ctx, repo, taskID) + + _, err := svc.CreatePlan(ctx, CreatePlanRequest{TaskID: taskID, Content: "source-v1"}) + if err != nil { + t.Fatalf("CreatePlan(first): %v", err) + } + firstRevision, err := svc.GetLatestRevision(ctx, taskID) + if err != nil { + t.Fatalf("GetLatestRevision(first): %v", err) + } + source, sourceVersion, err := svc.GetAgentPlanRevision(ctx, taskID, firstRevision.ID) + if err != nil { + t.Fatalf("GetAgentPlanRevision(source): %v", err) + } + + _, err = svc.CreatePlan(ctx, CreatePlanRequest{TaskID: taskID, Content: "source-v2"}) + if err != nil { + t.Fatalf("CreatePlan(coalesced source): %v", err) + } + coalescedRevision, err := svc.GetLatestRevision(ctx, taskID) + if err != nil { + t.Fatalf("GetLatestRevision(coalesced source): %v", err) + } + if coalescedRevision.ID != source.ID || coalescedRevision.RevisionNumber != source.RevisionNumber || coalescedRevision.Content != "source-v2" { + t.Fatalf("coalesced revision = %#v, want source ID/number with updated bytes", coalescedRevision) + } + mutatedSource, mutatedSourceVersion, err := svc.GetAgentPlanRevision(ctx, taskID, source.ID) + if err != nil { + t.Fatalf("GetAgentPlanRevision(mutated source): %v", err) + } + if mutatedSourceVersion == sourceVersion || mutatedSource.Content != "source-v2" { + t.Fatalf("source snapshot did not change in place: content=%q version=%q", mutatedSource.Content, mutatedSourceVersion) + } + + damaged, err := svc.CreatePlan(ctx, CreatePlanRequest{ + TaskID: taskID, Content: "damaged", ForceNewRevision: true, + }) + if err != nil { + t.Fatalf("CreatePlan(damaged): %v", err) + } + beforePlan, err := svc.GetPlan(ctx, taskID) + if err != nil { + t.Fatalf("GetPlan before restore: %v", err) + } + beforeHistory, err := svc.ListRevisions(ctx, taskID) + if err != nil { + t.Fatalf("ListRevisions before restore: %v", err) + } + eventBus.ClearEvents() + + _, err = svc.RestoreAgentPlanRevision(ctx, RestorePlanRequest{ + TaskID: taskID, RevisionID: source.ID, + ExpectedVersion: damaged.Plan.WriteVersion, ExpectedRevisionVersion: sourceVersion, + }) + var safety *PlanSafetyError + if !errors.As(err, &safety) || safety.Code != PlanErrorRevisionChanged { + t.Fatalf("restore error = %T %v, want %q", err, err, PlanErrorRevisionChanged) + } + if safety.CurrentRevisionVersion != mutatedSourceVersion { + t.Fatalf("error source version = %q, want current %q", safety.CurrentRevisionVersion, mutatedSourceVersion) + } + + gotPlan, err := svc.GetPlan(ctx, taskID) + if err != nil { + t.Fatalf("GetPlan after rejected restore: %v", err) + } + if !reflect.DeepEqual(gotPlan, beforePlan) { + t.Fatalf("plan changed after rejected restore: got %#v, want %#v", gotPlan, beforePlan) + } + gotHistory, err := svc.ListRevisions(ctx, taskID) + if err != nil { + t.Fatalf("ListRevisions after rejected restore: %v", err) + } + if !reflect.DeepEqual(gotHistory, beforeHistory) { + t.Fatalf("history changed after rejected restore: got %#v, want %#v", gotHistory, beforeHistory) + } + if published := eventBus.GetPublishedEvents(); len(published) != 0 { + t.Fatalf("rejected restore published %d events", len(published)) + } +} + +// TestPlanAgentRecoveryRestoresOversizedHistoryAndPreservesPlanMetadata covers +// AC-TASKS-PLAN-SAFE-002.5 and AC-TASKS-PLAN-SAFE-004.7: historical content is +// exempt from the current-write size limit, while comments and implementation +// markers survive the restore. +func TestPlanAgentRecoveryRestoresOversizedHistoryAndPreservesPlanMetadata(t *testing.T) { + svc, eventBus, repo := createTestPlanService(t) + ctx := context.Background() + taskID := "task-plan-agent-recovery-oversized" + seedTask(t, ctx, repo, taskID) + largeContent := strings.Repeat("x", MaxPlanContentBytes+1) + head := &models.TaskPlan{ + ID: taskID + "-plan", TaskID: taskID, Title: "Historical source", + Content: largeContent, CreatedBy: createdByAgent, + } + sourceRevision := &models.TaskPlanRevision{ + ID: taskID + "-source", TaskID: taskID, RevisionNumber: 1, + Title: "Historical source", Content: largeContent, + AuthorKind: createdByAgent, AuthorName: "Agent", + } + if err := repo.WritePlanRevision(ctx, head, sourceRevision, nil, false, false); err != nil { + t.Fatalf("WritePlanRevision(source): %v", err) + } + if len(sourceRevision.Content) <= MaxPlanContentBytes { + t.Fatal("test source did not exceed the current plan size limit") + } + if _, err := repo.MarkTaskPlanImplementationStarted(ctx, taskID, "session-preserve", "user"); err != nil { + t.Fatalf("MarkTaskPlanImplementationStarted: %v", err) + } + comment := &models.TaskPlanComment{ + ID: taskID + "-comment", TaskID: taskID, PlanID: head.ID, + Body: "Keep this review note", SelectedText: "historical", + AnchorFrom: 0, AnchorTo: len("historical"), + } + if _, err := repo.CreateTaskPlanComment(ctx, comment); err != nil { + t.Fatalf("CreateTaskPlanComment: %v", err) + } + + damaged, err := svc.CreatePlan(ctx, CreatePlanRequest{ + TaskID: taskID, Title: "Current", Content: "current", ForceNewRevision: true, + }) + if err != nil { + t.Fatalf("CreatePlan(damaged): %v", err) + } + beforeRestore, err := svc.GetPlan(ctx, taskID) + if err != nil { + t.Fatalf("GetPlan before restore: %v", err) + } + if beforeRestore.ImplementationStartedAt == nil || beforeRestore.ImplementationStartedSessionID == nil || beforeRestore.ImplementationStartedBy == nil { + t.Fatalf("metadata before restore = %#v, want implementation marker", beforeRestore) + } + startedAt := *beforeRestore.ImplementationStartedAt + startedSessionID := *beforeRestore.ImplementationStartedSessionID + startedBy := *beforeRestore.ImplementationStartedBy + beforeComments, err := repo.ListTaskPlanComments(ctx, taskID) + if err != nil { + t.Fatalf("ListTaskPlanComments before restore: %v", err) + } + beforeHistory, err := svc.ListRevisions(ctx, taskID) + if err != nil { + t.Fatalf("ListRevisions before restore: %v", err) + } + _, sourceVersion, err := svc.GetAgentPlanRevision(ctx, taskID, sourceRevision.ID) + if err != nil { + t.Fatalf("GetAgentPlanRevision(source): %v", err) + } + eventBus.ClearEvents() + + result, err := svc.RestoreAgentPlanRevision(ctx, RestorePlanRequest{ + TaskID: taskID, RevisionID: sourceRevision.ID, + ExpectedVersion: damaged.Plan.WriteVersion, ExpectedRevisionVersion: sourceVersion, + }) + if err != nil { + t.Fatalf("RestoreAgentPlanRevision(oversized source): %v", err) + } + if result.Plan == nil || result.Plan.Title != sourceRevision.Title || result.Plan.Content != largeContent { + t.Fatalf("restore result = %#v, want the oversized historical source", result) + } + if result.Plan.WriteVersion == damaged.Plan.WriteVersion { + t.Fatal("restore did not rotate the HEAD version") + } + if result.Plan.ImplementationStartedAt == nil || !result.Plan.ImplementationStartedAt.Equal(startedAt) { + t.Fatalf("restored marker time = %v, want %v", result.Plan.ImplementationStartedAt, startedAt) + } + if result.Plan.ImplementationStartedSessionID == nil || *result.Plan.ImplementationStartedSessionID != startedSessionID { + t.Fatalf("restored session marker = %v, want %s", result.Plan.ImplementationStartedSessionID, startedSessionID) + } + if result.Plan.ImplementationStartedBy == nil || *result.Plan.ImplementationStartedBy != startedBy { + t.Fatalf("restored marker actor = %v, want %s", result.Plan.ImplementationStartedBy, startedBy) + } + if result.Plan.CommentsRevision != beforeRestore.CommentsRevision { + t.Fatalf("restored comments revision = %d, want %d", result.Plan.CommentsRevision, beforeRestore.CommentsRevision) + } + afterComments, err := repo.ListTaskPlanComments(ctx, taskID) + if err != nil { + t.Fatalf("ListTaskPlanComments after restore: %v", err) + } + if !reflect.DeepEqual(afterComments, beforeComments) { + t.Fatalf("comments changed after restore: got %#v, want %#v", afterComments, beforeComments) + } + afterHistory, err := svc.ListRevisions(ctx, taskID) + if err != nil { + t.Fatalf("ListRevisions after restore: %v", err) + } + if len(afterHistory) != len(beforeHistory)+1 || afterHistory[len(afterHistory)-1].Content != largeContent { + t.Fatalf("history after restore = %#v, want the oversized source plus one revert", afterHistory) + } + if published := eventBus.GetPublishedEvents(); len(published) == 0 { + t.Fatal("successful restore did not publish plan events") + } +} + +// TestPlanAgentRecoveryConflictsWithInterveningBrowserWrite covers +// AC-TASKS-PLAN-SAFE-002.4, AC-TASKS-PLAN-SAFE-004.2, and +// AC-TASKS-PLAN-SAFE-005.4: a browser write admitted while an agent restore is +// waiting must win, and the restore must not overwrite it. +func TestPlanAgentRecoveryConflictsWithInterveningBrowserWrite(t *testing.T) { + svc, eventBus, repo := createTestPlanService(t) + ctx := context.Background() + taskID := "task-plan-agent-recovery-browser-race" + seedTask(t, ctx, repo, taskID) + + _, err := svc.CreatePlan(ctx, CreatePlanRequest{TaskID: taskID, Content: "source"}) + if err != nil { + t.Fatalf("CreatePlan(source): %v", err) + } + source, sourceVersion, err := svc.GetAgentPlanRevision(ctx, taskID, mustGetLatestRevisionID(t, svc, ctx, taskID)) + if err != nil { + t.Fatalf("GetAgentPlanRevision(source): %v", err) + } + _ = source + damaged, err := svc.CreatePlan(ctx, CreatePlanRequest{ + TaskID: taskID, Content: "agent damage", ForceNewRevision: true, + }) + if err != nil { + t.Fatalf("CreatePlan(damaged): %v", err) + } + + authorizationEntered := make(chan struct{}) + releaseAuthorization := make(chan struct{}) + var firstAuthorization sync.Once + svc.SetTaskAuthorizer(func(context.Context, string) error { + blocked := false + firstAuthorization.Do(func() { + close(authorizationEntered) + blocked = true + }) + if blocked { + <-releaseAuthorization + } + return nil + }) + + type restoreOutcome struct { + result RestorePlanResult + err error + } + restoreDone := make(chan restoreOutcome, 1) + go func() { + result, restoreErr := svc.RestoreAgentPlanRevision(ctx, RestorePlanRequest{ + TaskID: taskID, RevisionID: source.ID, + ExpectedVersion: damaged.Plan.WriteVersion, ExpectedRevisionVersion: sourceVersion, + }) + restoreDone <- restoreOutcome{result: result, err: restoreErr} + }() + select { + case <-authorizationEntered: + case <-time.After(time.Second): + t.Fatal("restore did not reach authorization barrier") + } + + browser, err := svc.UpdatePlan(ctx, UpdatePlanRequest{ + TaskID: taskID, Title: "Browser", Content: "browser edit", CreatedBy: createdByUser, + }) + if err != nil { + t.Fatalf("UpdatePlan(browser): %v", err) + } + beforeRestorePlan, err := svc.GetPlan(ctx, taskID) + if err != nil { + t.Fatalf("GetPlan before releasing restore: %v", err) + } + if beforeRestorePlan.Content != browser.Plan.Content || beforeRestorePlan.WriteVersion == damaged.Plan.WriteVersion { + t.Fatalf("browser write = %#v, stored = %#v", browser.Plan, beforeRestorePlan) + } + beforeRestoreHistory, err := svc.ListRevisions(ctx, taskID) + if err != nil { + t.Fatalf("ListRevisions before releasing restore: %v", err) + } + eventsAfterBrowser := len(eventBus.GetPublishedEvents()) + if eventsAfterBrowser == 0 { + t.Fatal("browser write did not publish an event") + } + + close(releaseAuthorization) + var outcome restoreOutcome + select { + case outcome = <-restoreDone: + case <-time.After(time.Second): + t.Fatal("restore did not finish after authorization barrier release") + } + var safety *PlanSafetyError + if !errors.As(outcome.err, &safety) || safety.Code != PlanErrorVersionConflict { + t.Fatalf("restore error = %T %v, want %q", outcome.err, outcome.err, PlanErrorVersionConflict) + } + if outcome.result.Plan != nil { + t.Fatalf("rejected restore returned a plan: %#v", outcome.result.Plan) + } + + gotPlan, err := svc.GetPlan(ctx, taskID) + if err != nil { + t.Fatalf("GetPlan after rejected restore: %v", err) + } + if !reflect.DeepEqual(gotPlan, beforeRestorePlan) { + t.Fatalf("plan changed after rejected restore: got %#v, want %#v", gotPlan, beforeRestorePlan) + } + gotHistory, err := svc.ListRevisions(ctx, taskID) + if err != nil { + t.Fatalf("ListRevisions after rejected restore: %v", err) + } + if !reflect.DeepEqual(gotHistory, beforeRestoreHistory) { + t.Fatalf("history changed after rejected restore: got %#v, want %#v", gotHistory, beforeRestoreHistory) + } + if published := len(eventBus.GetPublishedEvents()); published != eventsAfterBrowser { + t.Fatalf("rejected restore published events: before=%d after=%d", eventsAfterBrowser, published) + } +} + +func mustGetLatestRevisionID(t *testing.T, svc *PlanService, ctx context.Context, taskID string) string { + t.Helper() + revision, err := svc.GetLatestRevision(ctx, taskID) + if err != nil { + t.Fatalf("GetLatestRevision: %v", err) + } + return revision.ID +} + +func TestPlanAgentRecoveryAlreadyCurrentDoesNotWrite(t *testing.T) { + svc, _, repo := createTestPlanService(t) + ctx := context.Background() + taskID := "task-plan-agent-recovery-current" + seedTask(t, ctx, repo, taskID) + created, err := svc.CreatePlan(ctx, CreatePlanRequest{TaskID: taskID, Content: "current"}) + if err != nil { + t.Fatalf("CreatePlan: %v", err) + } + latest, err := svc.GetLatestRevision(ctx, taskID) + if err != nil { + t.Fatalf("GetLatestRevision: %v", err) + } + _, revisionVersion, err := svc.GetAgentPlanRevision(ctx, taskID, latest.ID) + if err != nil { + t.Fatalf("GetAgentPlanRevision: %v", err) + } + result, err := svc.RestoreAgentPlanRevision(ctx, RestorePlanRequest{ + TaskID: taskID, RevisionID: latest.ID, + ExpectedVersion: created.Plan.WriteVersion, ExpectedRevisionVersion: revisionVersion, + }) + if err != nil { + t.Fatalf("RestoreAgentPlanRevision: %v", err) + } + if !result.AlreadyCurrent || result.Plan.WriteVersion != created.Plan.WriteVersion { + t.Fatalf("already-current result = %#v, want unchanged version", result) + } + history, err := svc.ListRevisions(ctx, taskID) + if err != nil || len(history) != 1 { + t.Fatalf("already-current history = %d, %v; want one row", len(history), err) + } +} + +func TestPlanAgentRevisionMetadataIsBoundedAndContentFree(t *testing.T) { + svc, _, repo := createTestPlanService(t) + ctx := context.Background() + taskID := "task-plan-agent-recovery-metadata" + seedTask(t, ctx, repo, taskID) + for i := 0; i < 3; i++ { + if _, err := svc.CreatePlan(ctx, CreatePlanRequest{ + TaskID: taskID, Content: strings.Repeat(string(rune('a'+i)), 10), ForceNewRevision: true, + }); err != nil { + t.Fatalf("CreatePlan(%d): %v", i, err) + } + } + page, err := svc.ListAgentPlanRevisionMetadata(ctx, taskID, 0, 2) + if err != nil { + t.Fatalf("ListAgentPlanRevisionMetadata: %v", err) + } + if len(page.Revisions) != 2 || page.NextBeforeRevisionNum == 0 { + t.Fatalf("page = %#v, want two rows and a cursor", page) + } + for _, revision := range page.Revisions { + if revision.Content != "" || revision.ContentBytes != 10 { + t.Fatalf("metadata revision = %#v, want no content and byte size 10", revision) + } + } + next, err := svc.ListAgentPlanRevisionMetadata(ctx, taskID, page.NextBeforeRevisionNum, 2) + if err != nil { + t.Fatalf("next metadata page: %v", err) + } + if len(next.Revisions) != 1 || next.Revisions[0].RevisionNumber >= page.NextBeforeRevisionNum { + t.Fatalf("next page = %#v, want the older revision", next) + } +} diff --git a/apps/backend/internal/task/service/plan_exact_edit.go b/apps/backend/internal/task/service/plan_exact_edit.go new file mode 100644 index 0000000000..c6fc904256 --- /dev/null +++ b/apps/backend/internal/task/service/plan_exact_edit.go @@ -0,0 +1,123 @@ +package service + +import ( + "context" + "strings" +) + +// ExactPlanEditRequest describes one agent edit of one exact text span. +// NewText may be empty when the caller intends to delete the unique match. +type ExactPlanEditRequest struct { + TaskID string + ExpectedVersion string + OldText string + NewText string + AllowTruncation bool +} + +// EditPlan replaces one unique exact occurrence in the current plan. The +// complete read, match check, version guard, and write stay inside the task's +// plan lock so a successful edit applies to the snapshot it inspected. +func (s *PlanService) EditPlan(ctx context.Context, req ExactPlanEditRequest) (PlanWriteResult, error) { + if req.TaskID == "" { + return PlanWriteResult{}, ErrTaskIDRequired + } + if err := s.authorize(ctx, req.TaskID); err != nil { + return PlanWriteResult{}, err + } + if req.OldText == "" { + return PlanWriteResult{}, newPlanSafetyError( + PlanErrorEditTextRequired, ErrPlanEditTextRequired, req.TaskID, + "Plan was not changed because old_text must not be empty.", + "Read the current plan and retry with one non-empty exact fragment.", + ) + } + + release := s.locks.acquire(req.TaskID) + defer release() + readCtx := context.WithoutCancel(ctx) + head, state, headErr := s.readPlanHead(readCtx, req.TaskID) + if state == planHeadUnknown { + return PlanWriteResult{}, s.headUnavailableError(req.TaskID, headErr) + } + if state == planHeadAbsent { + return PlanWriteResult{}, ErrTaskPlanNotFound + } + if err := s.requireCurrentVersion(CreatePlanRequest{ + TaskID: req.TaskID, ExpectedVersion: req.ExpectedVersion, + }, head); err != nil { + return PlanWriteResult{}, err + } + + count, offset := countOverlappingOccurrences(head.Content, req.OldText) + if count == 0 { + err := newPlanSafetyError( + PlanErrorEditNotFound, ErrPlanEditNotFound, req.TaskID, + "Plan was not changed because old_text was not found in the current plan.", + "Read the current plan and retry with an exact fragment from that snapshot.", + ) + err.CurrentVersion = head.WriteVersion + return PlanWriteResult{}, err + } + if count > 1 { + err := newPlanSafetyError( + PlanErrorEditAmbiguous, ErrPlanEditAmbiguous, req.TaskID, + "Plan was not changed because old_text occurs more than once in the current plan.", + "Read the current plan and retry with a longer exact fragment that occurs once.", + ) + err.CurrentVersion = head.WriteVersion + return PlanWriteResult{}, err + } + + content := head.Content[:offset] + req.NewText + head.Content[offset+len(req.OldText):] + if content == "" { + return PlanWriteResult{}, s.emptyAgentPlanError(req.TaskID) + } + if err := checkPlanContentSize(content); err != nil { + return PlanWriteResult{}, err + } + + // upsertPlan performs the same read and version check again while the lock + // is held. The expected version makes the exact span check fail closed if a + // writer outside this service instance changed the row in between reads. + result, err := s.upsertPlan(ctx, CreatePlanRequest{ + TaskID: req.TaskID, + Title: head.Title, + Content: content, + CreatedBy: createdByAgent, + AuthorKind: createdByAgent, + AgentWrite: true, + ExpectedVersion: req.ExpectedVersion, + AllowTruncation: req.AllowTruncation, + EvaluateTruncation: true, + Mode: PlanWriteModeReplace, + }, true) + if err != nil { + return PlanWriteResult{}, err + } + + release() + s.publishPlanEvent(ctx, result.eventType, result.result.Plan) + s.publishRevisionEvent(ctx, result.rev, result.coalesced) + return result.result, nil +} + +// countOverlappingOccurrences counts literal byte spans. Advancing one byte +// after a match is intentional: strings such as "aaa" in "aaaa" have two +// overlapping matches and are not safe to edit as one unique fragment. +func countOverlappingOccurrences(content, fragment string) (count, firstOffset int) { + firstOffset = -1 + for start := 0; start <= len(content)-len(fragment); { + offset := strings.Index(content[start:], fragment) + if offset < 0 { + break + } + offset += start + if firstOffset < 0 { + firstOffset = offset + } + count++ + start = offset + 1 + } + return count, firstOffset +} diff --git a/apps/backend/internal/task/service/plan_exact_edit_test.go b/apps/backend/internal/task/service/plan_exact_edit_test.go new file mode 100644 index 0000000000..cc272cce67 --- /dev/null +++ b/apps/backend/internal/task/service/plan_exact_edit_test.go @@ -0,0 +1,193 @@ +package service + +import ( + "context" + "errors" + "strings" + "testing" +) + +func TestPlanExactEdit(t *testing.T) { + svc, _, repo := createTestPlanService(t) + ctx := context.Background() + taskID := "task-plan-exact-edit" + seedTask(t, ctx, repo, taskID) + created, err := svc.CreatePlan(ctx, CreatePlanRequest{ + TaskID: taskID, + Title: "Checklist", + Content: "before\r\n- [ ] preserve this\r\n- [ ] change this\r\nafter", + }) + if err != nil { + t.Fatalf("CreatePlan: %v", err) + } + + updated, err := svc.EditPlan(ctx, ExactPlanEditRequest{ + TaskID: taskID, + ExpectedVersion: created.Plan.WriteVersion, + OldText: "- [ ] change this", + NewText: "- [x] change this", + }) + if err != nil { + t.Fatalf("EditPlan: %v", err) + } + want := "before\r\n- [ ] preserve this\r\n- [x] change this\r\nafter" + if updated.Plan.Content != want { + t.Fatalf("edited content = %q, want %q", updated.Plan.Content, want) + } + if updated.Plan.Title != created.Plan.Title { + t.Fatalf("edited title = %q, want %q", updated.Plan.Title, created.Plan.Title) + } + if updated.Plan.WriteVersion == created.Plan.WriteVersion { + t.Fatal("exact edit did not rotate the write version") + } + + history, err := svc.ListRevisions(ctx, taskID) + if err != nil { + t.Fatalf("ListRevisions: %v", err) + } + if len(history) != 1 || history[0].Content != want { + t.Fatalf("history = %#v, want the edited snapshot", history) + } +} + +func TestPlanExactEditRejectsInvalidMatchWithoutMutation(t *testing.T) { + svc, _, repo := createTestPlanService(t) + ctx := context.Background() + taskID := "task-plan-exact-invalid" + seedTask(t, ctx, repo, taskID) + created, err := svc.CreatePlan(ctx, CreatePlanRequest{TaskID: taskID, Content: "aaa"}) + if err != nil { + t.Fatalf("CreatePlan: %v", err) + } + + cases := []struct { + name string + oldText string + code string + }{ + {name: "empty", oldText: "", code: PlanErrorEditTextRequired}, + {name: "missing", oldText: "not present", code: PlanErrorEditNotFound}, + {name: "overlapping", oldText: "aa", code: PlanErrorEditAmbiguous}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := svc.EditPlan(ctx, ExactPlanEditRequest{ + TaskID: taskID, ExpectedVersion: created.Plan.WriteVersion, + OldText: tc.oldText, NewText: "replacement", + }) + if err == nil { + t.Fatal("EditPlan succeeded for invalid match") + } + var safety *PlanSafetyError + if !errors.As(err, &safety) || safety.Code != tc.code { + t.Fatalf("error = %T %v, want safety code %q", err, err, tc.code) + } + got, getErr := svc.GetPlan(ctx, taskID) + if getErr != nil || got.Content != created.Plan.Content || got.WriteVersion != created.Plan.WriteVersion { + t.Fatalf("rejected edit changed plan: %#v, %v", got, getErr) + } + }) + } + + history, err := svc.ListRevisions(ctx, taskID) + if err != nil { + t.Fatalf("ListRevisions: %v", err) + } + if len(history) != 1 { + t.Fatalf("rejected edits added history rows: %d", len(history)) + } +} + +func TestPlanExactEditRequiresCurrentVersion(t *testing.T) { + svc, _, repo := createTestPlanService(t) + ctx := context.Background() + taskID := "task-plan-exact-version" + seedTask(t, ctx, repo, taskID) + created, err := svc.CreatePlan(ctx, CreatePlanRequest{TaskID: taskID, Content: "before"}) + if err != nil { + t.Fatalf("CreatePlan: %v", err) + } + + for _, expected := range []string{"", "stale"} { + _, err := svc.EditPlan(ctx, ExactPlanEditRequest{ + TaskID: taskID, ExpectedVersion: expected, + OldText: "before", NewText: "after", + }) + if err == nil { + t.Fatalf("EditPlan with expected version %q succeeded", expected) + } + var safety *PlanSafetyError + if !errors.As(err, &safety) { + t.Fatalf("error = %T %v, want PlanSafetyError", err, err) + } + wantCode := PlanErrorVersionRequired + if expected != "" { + wantCode = PlanErrorVersionConflict + } + if safety.Code != wantCode { + t.Fatalf("safety code = %q, want %q", safety.Code, wantCode) + } + } + got, err := svc.GetPlan(ctx, taskID) + if err != nil || got.Content != "before" || got.WriteVersion != created.Plan.WriteVersion { + t.Fatalf("plan after version failures = %#v, %v", got, err) + } +} + +func TestPlanExactEditAllowsDeletionAndRejectsFinalEmpty(t *testing.T) { + svc, _, repo := createTestPlanService(t) + ctx := context.Background() + taskID := "task-plan-exact-delete" + seedTask(t, ctx, repo, taskID) + created, err := svc.CreatePlan(ctx, CreatePlanRequest{TaskID: taskID, Content: "keep\nremove"}) + if err != nil { + t.Fatalf("CreatePlan: %v", err) + } + updated, err := svc.EditPlan(ctx, ExactPlanEditRequest{ + TaskID: taskID, ExpectedVersion: created.Plan.WriteVersion, + OldText: "remove", NewText: "", + }) + if err != nil { + t.Fatalf("delete edit: %v", err) + } + if updated.Plan.Content != "keep\n" { + t.Fatalf("content after deletion = %q, want %q", updated.Plan.Content, "keep\\n") + } + + _, err = svc.EditPlan(ctx, ExactPlanEditRequest{ + TaskID: taskID, ExpectedVersion: updated.Plan.WriteVersion, + OldText: "keep\n", NewText: "", + }) + if !errors.Is(err, ErrContentRequired) { + t.Fatalf("final-empty edit error = %v, want ErrContentRequired", err) + } + got, err := svc.GetPlan(ctx, taskID) + if err != nil || got.Content != "keep\n" || got.WriteVersion != updated.Plan.WriteVersion { + t.Fatalf("final-empty rejection changed plan: %#v, %v", got, err) + } +} + +func TestPlanExactEditCountsOverlappingUnicodeMatches(t *testing.T) { + svc, _, repo := createTestPlanService(t) + ctx := context.Background() + taskID := "task-plan-exact-unicode" + seedTask(t, ctx, repo, taskID) + created, err := svc.CreatePlan(ctx, CreatePlanRequest{ + TaskID: taskID, Content: "前 aa 前 aa 后", + }) + if err != nil { + t.Fatalf("CreatePlan: %v", err) + } + _, err = svc.EditPlan(ctx, ExactPlanEditRequest{ + TaskID: taskID, ExpectedVersion: created.Plan.WriteVersion, + OldText: "aa", NewText: "長", + }) + var safety *PlanSafetyError + if !errors.As(err, &safety) || safety.Code != PlanErrorEditAmbiguous { + t.Fatalf("unicode ambiguous edit error = %T %v, want %q", err, err, PlanErrorEditAmbiguous) + } + + if !strings.Contains(created.Plan.Content, "前") { + t.Fatal("test fixture lost its Unicode content") + } +} diff --git a/apps/backend/internal/task/service/plan_revision_recovery.go b/apps/backend/internal/task/service/plan_revision_recovery.go new file mode 100644 index 0000000000..8ae8b4f5a7 --- /dev/null +++ b/apps/backend/internal/task/service/plan_revision_recovery.go @@ -0,0 +1,302 @@ +package service + +import ( + "context" + "errors" + + "github.com/kandev/kandev/internal/events" + "github.com/kandev/kandev/internal/task/models" + "go.uber.org/zap" +) + +const ( + // DefaultPlanRevisionPageLimit is the default number of metadata rows an + // agent receives from one history-list call. + DefaultPlanRevisionPageLimit = 20 + // MaxPlanRevisionPageLimit bounds one agent history-list call. + MaxPlanRevisionPageLimit = 100 +) + +var ( + ErrPlanRevisionCursorInvalid = errors.New("revision cursor must be positive") + ErrPlanRevisionLimitInvalid = errors.New("revision page limit is invalid") +) + +// PlanRevisionMetadataPage is a bounded, content-free history page. +type PlanRevisionMetadataPage struct { + Revisions []*models.TaskPlanRevision + NextBeforeRevisionNum int +} + +type planRevisionMetadataRepository interface { + ListTaskPlanRevisionMetadata(context.Context, string, int, int) ([]*models.TaskPlanRevision, error) +} + +type taskScopedPlanRevisionRepository interface { + GetTaskPlanRevisionForTask(context.Context, string, string) (*models.TaskPlanRevision, error) +} + +// ListAgentPlanRevisionMetadata returns newest-first, bounded history without +// loading revision bodies on the production repository path. +func (s *PlanService) ListAgentPlanRevisionMetadata( + ctx context.Context, taskID string, beforeRevisionNumber, limit int, +) (PlanRevisionMetadataPage, error) { + if taskID == "" { + return PlanRevisionMetadataPage{}, ErrTaskIDRequired + } + if err := s.authorize(ctx, taskID); err != nil { + return PlanRevisionMetadataPage{}, err + } + limit, err := normalizeRevisionPage(beforeRevisionNumber, limit) + if err != nil { + return PlanRevisionMetadataPage{}, err + } + + readLimit := limit + 1 + var revisions []*models.TaskPlanRevision + if repo, ok := s.repo.(planRevisionMetadataRepository); ok { + revisions, err = repo.ListTaskPlanRevisionMetadata(ctx, taskID, beforeRevisionNumber, readLimit) + } else { + // Small test doubles may only implement the original repository surface. + // The production SQLite repository always takes the bounded query above. + revisions, err = s.repo.ListTaskPlanRevisions(ctx, taskID, 0) + revisions = filterRevisionPage(revisions, beforeRevisionNumber, readLimit) + } + if err != nil { + return PlanRevisionMetadataPage{}, s.revisionUnavailableError(taskID, err) + } + + page := PlanRevisionMetadataPage{Revisions: revisions} + if len(revisions) > limit { + page.Revisions = revisions[:limit] + page.NextBeforeRevisionNum = page.Revisions[len(page.Revisions)-1].RevisionNumber + } + for _, revision := range page.Revisions { + if revision.ContentBytes == 0 && revision.Content != "" { + revision.ContentBytes = len(revision.Content) + } + } + return page, nil +} + +// GetAgentPlanRevision reads an exact task-scoped revision and its mutable +// snapshot token. Authorization happens before the identifier lookup. +func (s *PlanService) GetAgentPlanRevision( + ctx context.Context, taskID, revisionID string, +) (*models.TaskPlanRevision, string, error) { + if taskID == "" { + return nil, "", ErrTaskIDRequired + } + if err := s.authorize(ctx, taskID); err != nil { + return nil, "", err + } + if revisionID == "" { + return nil, "", ErrRevisionIDRequired + } + + revision, err := s.getTaskScopedRevision(ctx, taskID, revisionID) + if err != nil { + return nil, "", s.revisionUnavailableError(taskID, err) + } + if revision == nil { + return nil, "", ErrRevisionNotFound + } + return revision, PlanRevisionVersion(revision), nil +} + +// RestoreAgentPlanRevision conditionally restores one task-scoped revision. +// It creates a new agent revision and preserves the selected source row. +func (s *PlanService) RestoreAgentPlanRevision( + ctx context.Context, req RestorePlanRequest, +) (RestorePlanResult, error) { + if req.TaskID == "" { + return RestorePlanResult{}, ErrTaskIDRequired + } + if err := s.authorize(ctx, req.TaskID); err != nil { + return RestorePlanResult{}, err + } + if req.RevisionID == "" { + return RestorePlanResult{}, ErrRevisionIDRequired + } + + release := s.locks.acquire(req.TaskID) + defer release() + readCtx := context.WithoutCancel(ctx) + prepared, err := s.prepareRestore(readCtx, req) + if err != nil { + return RestorePlanResult{}, err + } + head, source := prepared.head, prepared.source + if head.Title == source.Title && head.Content == source.Content { + return RestorePlanResult{ + Plan: head, Revision: source, AlreadyCurrent: true, + }, nil + } + + plan := &models.TaskPlan{ + ID: head.ID, + TaskID: req.TaskID, + Title: source.Title, + Content: source.Content, + CreatedBy: createdByAgent, + CreatedAt: head.CreatedAt, + CommentsRevision: head.CommentsRevision, + ImplementationStartedAt: head.ImplementationStartedAt, + ImplementationStartedSessionID: head.ImplementationStartedSessionID, + ImplementationStartedBy: head.ImplementationStartedBy, + } + authorName := s.resolveAgentDisplayName(readCtx, req.TaskID) + if authorName == "" { + authorName = defaultAgentAuthorFallback + } + sourceID := source.ID + revision := &models.TaskPlanRevision{ + TaskID: req.TaskID, + Title: source.Title, + Content: source.Content, + AuthorKind: createdByAgent, + AuthorName: authorName, + RevertOfRevisionID: &sourceID, + } + revision.WorkflowStepID, revision.WorkflowStepName, revision.WorkflowStepColor = + s.currentWorkflowStepStamp(readCtx, req.TaskID) + if err := s.repo.WritePlanRevision(ctx, plan, revision, nil, false, false); err != nil { + s.logPlanWriteError(req.TaskID, err) + return RestorePlanResult{}, err + } + + saved, readErr := s.repo.GetTaskPlan(ctx, req.TaskID) + if readErr != nil || saved == nil { + saved = plan + } + release() + s.publishPlanEvent(ctx, events.TaskPlanUpdated, saved) + s.publishRevisionEvent(ctx, revision, false) + s.publishReverted(ctx, revision) + return RestorePlanResult{Plan: saved, Revision: revision}, nil +} + +type restorePreparation struct { + head *models.TaskPlan + source *models.TaskPlanRevision +} + +func (s *PlanService) prepareRestore( + ctx context.Context, req RestorePlanRequest, +) (restorePreparation, error) { + head, state, headErr := s.readPlanHead(ctx, req.TaskID) + if state == planHeadUnknown { + return restorePreparation{}, s.headUnavailableError(req.TaskID, headErr) + } + if state == planHeadAbsent { + return restorePreparation{}, ErrTaskPlanNotFound + } + if err := s.requireCurrentVersion(CreatePlanRequest{ + TaskID: req.TaskID, ExpectedVersion: req.ExpectedVersion, + }, head); err != nil { + return restorePreparation{}, err + } + if req.ExpectedRevisionVersion == "" { + return restorePreparation{}, newPlanSafetyError( + PlanErrorRevisionVersionRequired, ErrPlanRevisionVersionRequired, req.TaskID, + "Plan was not changed because expected_revision_version is required.", + "Fetch the selected revision again and retry with its revision_version and the current plan version.", + ) + } + + source, err := s.getTaskScopedRevision(ctx, req.TaskID, req.RevisionID) + if err != nil { + return restorePreparation{}, s.revisionUnavailableError(req.TaskID, err) + } + if source == nil { + return restorePreparation{}, ErrRevisionNotFound + } + sourceVersion := PlanRevisionVersion(source) + if sourceVersion != req.ExpectedRevisionVersion { + safety := newPlanSafetyError( + PlanErrorRevisionChanged, ErrPlanRevisionChanged, req.TaskID, + "Plan was not changed because the selected revision changed after it was read.", + "Fetch the selected revision again before deciding whether to restore it.", + ) + safety.CurrentVersion = head.WriteVersion + safety.CurrentRevisionVersion = sourceVersion + return restorePreparation{}, safety + } + + latest, latestState, latestErr := s.readLatestRevisionDetailed(ctx, req.TaskID) + if err := s.verifyPlanHistory(head, latest, latestState, latestErr); err != nil { + if safety, ok := err.(*PlanSafetyError); ok { + safety.CurrentVersion = head.WriteVersion + } + return restorePreparation{}, err + } + return restorePreparation{head: head, source: source}, nil +} + +// RestorePlanRequest carries both sides of the conditional restore guard. +type RestorePlanRequest struct { + TaskID string + RevisionID string + ExpectedVersion string + ExpectedRevisionVersion string +} + +// RestorePlanResult reports the committed HEAD and source revision. No write +// occurs when AlreadyCurrent is true. +type RestorePlanResult struct { + Plan *models.TaskPlan + Revision *models.TaskPlanRevision + AlreadyCurrent bool +} + +func normalizeRevisionPage(beforeRevisionNumber, limit int) (int, error) { + if beforeRevisionNumber < 0 { + return 0, ErrPlanRevisionCursorInvalid + } + if limit == 0 { + limit = DefaultPlanRevisionPageLimit + } + if limit < 1 || limit > MaxPlanRevisionPageLimit { + return 0, ErrPlanRevisionLimitInvalid + } + return limit, nil +} + +func filterRevisionPage(revisions []*models.TaskPlanRevision, before, limit int) []*models.TaskPlanRevision { + filtered := make([]*models.TaskPlanRevision, 0, len(revisions)) + for _, revision := range revisions { + if before > 0 && revision.RevisionNumber >= before { + continue + } + filtered = append(filtered, revision) + if len(filtered) == limit { + break + } + } + return filtered +} + +func (s *PlanService) getTaskScopedRevision(ctx context.Context, taskID, revisionID string) (*models.TaskPlanRevision, error) { + if repo, ok := s.repo.(taskScopedPlanRevisionRepository); ok { + return repo.GetTaskPlanRevisionForTask(ctx, taskID, revisionID) + } + revision, err := s.repo.GetTaskPlanRevision(ctx, revisionID) + if err != nil || revision == nil { + return revision, err + } + if revision.TaskID != taskID { + return nil, nil + } + return revision, nil +} + +func (s *PlanService) revisionUnavailableError(taskID string, cause error) error { + if cause != nil { + s.logger.Warn("agent plan revision read failed", zap.String("task_id", taskID), zap.Error(cause)) + } + return newPlanSafetyError( + PlanErrorRevisionUnavailable, ErrPlanRevisionUnavailable, taskID, + "Plan was not changed because the selected plan revision could not be read.", + "Read the revision again and retry only after the read succeeds.", + ) +} diff --git a/apps/backend/internal/task/service/plan_revision_version.go b/apps/backend/internal/task/service/plan_revision_version.go new file mode 100644 index 0000000000..52a6517255 --- /dev/null +++ b/apps/backend/internal/task/service/plan_revision_version.go @@ -0,0 +1,37 @@ +package service + +import ( + "crypto/sha256" + "encoding/binary" + "encoding/hex" + "time" + + "github.com/kandev/kandev/internal/task/models" +) + +// PlanRevisionVersion returns the opaque snapshot token used by agent restore +// calls. It changes when coalescing changes any source field included here. +func PlanRevisionVersion(revision *models.TaskPlanRevision) string { + if revision == nil { + return "" + } + var canonical []byte + for _, value := range []string{ + revision.ID, + revision.TaskID, + revision.Title, + revision.Content, + revision.UpdatedAt.UTC().Format(time.RFC3339Nano), + } { + canonical = appendLengthPrefixed(canonical, value) + } + digest := sha256.Sum256(canonical) + return hex.EncodeToString(digest[:]) +} + +func appendLengthPrefixed(dst []byte, value string) []byte { + var length [binary.MaxVarintLen64]byte + n := binary.PutUvarint(length[:], uint64(len(value))) + dst = append(dst, length[:n]...) + return append(dst, value...) +} diff --git a/apps/backend/internal/task/service/plan_safe_edits.go b/apps/backend/internal/task/service/plan_safe_edits.go new file mode 100644 index 0000000000..29fe5a0841 --- /dev/null +++ b/apps/backend/internal/task/service/plan_safe_edits.go @@ -0,0 +1,221 @@ +package service + +import ( + "context" + "errors" + + "github.com/kandev/kandev/internal/task/models" + "go.uber.org/zap" +) + +const ( + PlanErrorVersionRequired = "plan_version_required" + PlanErrorVersionConflict = "plan_version_conflict" + PlanErrorHeadUnavailable = "plan_head_unavailable" + PlanErrorTruncationRejected = "plan_truncation_rejected" + PlanErrorAppendTruncationFlag = "plan_append_truncation_not_applicable" + PlanErrorHistoryUnavailable = "plan_history_unavailable" + PlanErrorContentRequired = "plan_content_required" + PlanErrorEditTextRequired = "plan_edit_text_required" + PlanErrorEditNotFound = "plan_edit_not_found" + PlanErrorEditAmbiguous = "plan_edit_ambiguous" + PlanErrorRevisionChanged = "plan_revision_changed" + PlanErrorRevisionUnavailable = "plan_revision_unavailable" + PlanErrorRevisionVersionRequired = "plan_revision_version_required" + PlanErrorAlreadyCurrent = "plan_already_current" +) + +var ( + ErrPlanVersionRequired = errors.New("plan write version is required") + ErrPlanVersionConflict = errors.New("plan write version is stale") + ErrPlanHeadUnavailable = errors.New("current task plan could not be read") + ErrPlanTruncationRejected = errors.New("suspicious plan reduction was rejected") + ErrPlanAppendTruncationFlag = errors.New("allow_truncation is not applicable to append") + ErrPlanHistoryUnavailable = errors.New("plan revision history could not be verified") + ErrPlanEditTextRequired = errors.New("old plan edit text is required") + ErrPlanEditNotFound = errors.New("plan edit text was not found") + ErrPlanEditAmbiguous = errors.New("plan edit text is ambiguous") + ErrPlanRevisionChanged = errors.New("plan revision changed") + ErrPlanRevisionUnavailable = errors.New("plan revision could not be read") + ErrPlanRevisionVersionRequired = errors.New("plan revision version is required") +) + +// PlanSafetyError is the structured, no-write failure returned by agent plan +// tools. The message and Code are stable; the details are safe to expose only +// after the service has authorized the target task. +type PlanSafetyError struct { + Code string + TaskID string + CurrentVersion string + CurrentRevisionVersion string + CurrentRevisionNumber int + ReplacedRunes int + NewRunes int + Message string + NextAction string + sentinel error +} + +func (e *PlanSafetyError) Error() string { return e.Message } + +func (e *PlanSafetyError) Unwrap() error { return e.sentinel } + +func newPlanSafetyError(code string, sentinel error, taskID, message, nextAction string) *PlanSafetyError { + return &PlanSafetyError{ + Code: code, TaskID: taskID, Message: message, NextAction: nextAction, + sentinel: sentinel, + } +} + +func (s *PlanService) readLatestRevisionDetailed( + ctx context.Context, taskID string, +) (*models.TaskPlanRevision, planRevisionState, error) { + rev, err := s.repo.GetLatestTaskPlanRevision(ctx, taskID) + if err != nil { + return nil, planRevisionUnknown, err + } + if rev == nil { + return nil, planRevisionAbsent, nil + } + return rev, planRevisionFound, nil +} + +// guardAgentPlanWrite validates a replacement against the HEAD snapshot that +// upsertPlan already read while holding the task lock. +func (s *PlanService) guardAgentPlanWrite( + req CreatePlanRequest, + head *models.TaskPlan, + headState planHeadState, + headErr error, + latest *models.TaskPlanRevision, + latestState planRevisionState, + latestErr error, +) (CreatePlanRequest, error) { + if !req.AgentWrite { + return req, nil + } + if headState == planHeadUnknown { + return req, s.headUnavailableError(req.TaskID, headErr) + } + if headState == planHeadAbsent { + if req.ExpectedVersion != "" { + return req, s.versionConflictError(req.TaskID, "") + } + return req, nil + } + if req.Mode == PlanWriteModeAppend { + return req, s.guardAgentAppend(req, head) + } + if err := s.requireCurrentVersion(req, head); err != nil { + return req, err + } + if !req.EvaluateTruncation || !planTruncationDetected(head.Content, req.Content) { + return req, nil + } + if !req.AllowTruncation { + return req, s.truncationError(req.TaskID, head, req.Content) + } + if err := s.verifyPlanHistory(head, latest, latestState, latestErr); err != nil { + return req, err + } + // An acknowledged reduction must be a new revision, so it cannot merge + // over the snapshot that provides recovery. + req.ForceNewRevision = true + req.EvaluateTruncation = false + return req, nil +} + +func (s *PlanService) guardAgentAppend(req CreatePlanRequest, head *models.TaskPlan) error { + if req.AllowTruncation { + return s.appendTruncationFlagError(req.TaskID, head.WriteVersion) + } + if req.ExpectedVersion == "" { + return nil + } + return s.requireCurrentVersion(req, head) +} + +func (s *PlanService) requireCurrentVersion(req CreatePlanRequest, head *models.TaskPlan) error { + if req.ExpectedVersion == "" { + return newPlanSafetyError( + PlanErrorVersionRequired, ErrPlanVersionRequired, req.TaskID, + "Plan was not changed because expected_version is required for an existing plan.", + "Read the current plan with get_task_plan_kandev and retry with its version.", + ) + } + if req.ExpectedVersion != head.WriteVersion { + return s.versionConflictError(req.TaskID, head.WriteVersion) + } + return nil +} + +func (s *PlanService) emptyAgentPlanError(taskID string) error { + return newPlanSafetyError( + PlanErrorContentRequired, ErrContentRequired, taskID, + "Plan was not changed because the operation would leave the document empty.", + "Provide a non-empty replacement or use an exact edit that leaves plan content.", + ) +} + +func (s *PlanService) versionConflictError(taskID, currentVersion string) error { + err := newPlanSafetyError( + PlanErrorVersionConflict, ErrPlanVersionConflict, taskID, + "Plan was not changed because expected_version does not match the current plan.", + "Read the current plan with get_task_plan_kandev, apply the intended change to that snapshot, and retry.", + ) + err.CurrentVersion = currentVersion + return err +} + +func (s *PlanService) headUnavailableError(taskID string, cause error) error { + message := "Plan was not changed because the current plan could not be read." + if cause != nil { + s.logger.Warn("agent plan safety read failed", zap.String("task_id", taskID), zap.Error(cause)) + } + return newPlanSafetyError( + PlanErrorHeadUnavailable, ErrPlanHeadUnavailable, taskID, message, + "Read the plan again and retry after the read succeeds.", + ) +} + +func (s *PlanService) truncationError(taskID string, head *models.TaskPlan, content string) error { + err := newPlanSafetyError( + PlanErrorTruncationRejected, ErrPlanTruncationRejected, taskID, + "Plan was not changed because the replacement removes most of the existing document.", + "Retry with the complete current document, use edit_task_plan_kandev for a local change, or retry this replacement with the matching expected_version and allow_truncation=true if the reduction is intentional.", + ) + err.CurrentVersion = head.WriteVersion + err.ReplacedRunes = models.PlanContentLength(head.Content) + err.NewRunes = models.PlanContentLength(content) + return err +} + +func (s *PlanService) appendTruncationFlagError(taskID, currentVersion string) error { + err := newPlanSafetyError( + PlanErrorAppendTruncationFlag, ErrPlanAppendTruncationFlag, taskID, + "Plan was not changed because allow_truncation applies only to replacement and exact-edit operations.", + "Retry the append without allow_truncation, or use edit_task_plan_kandev for a local change that removes content.", + ) + err.CurrentVersion = currentVersion + return err +} + +func (s *PlanService) verifyPlanHistory( + head *models.TaskPlan, + latest *models.TaskPlanRevision, + state planRevisionState, + readErr error, +) error { + if state != planRevisionFound || latest == nil || latest.Title != head.Title || latest.Content != head.Content { + if readErr != nil { + s.logger.Warn("agent plan history read failed", + zap.String("task_id", head.TaskID), zap.Error(readErr)) + } + return newPlanSafetyError( + PlanErrorHistoryUnavailable, ErrPlanHistoryUnavailable, head.TaskID, + "Plan was not changed because its previous content could not be verified in revision history.", + "Read the current plan and revision history, then retry only after the preserved snapshot is available.", + ) + } + return nil +} diff --git a/apps/backend/internal/task/service/plan_safe_edits_test.go b/apps/backend/internal/task/service/plan_safe_edits_test.go new file mode 100644 index 0000000000..1cfce41fb5 --- /dev/null +++ b/apps/backend/internal/task/service/plan_safe_edits_test.go @@ -0,0 +1,309 @@ +package service + +import ( + "context" + "errors" + "strings" + "sync" + "testing" +) + +func TestAgentReplaceRejectsSuspiciousReductionBeforeWrite(t *testing.T) { + svc, _, repo := createTestPlanService(t) + ctx := context.Background() + seedTask(t, ctx, repo, "task-plan-safe-replace") + large := strings.Repeat("x", 4000) + if _, err := svc.CreatePlan(ctx, CreatePlanRequest{TaskID: "task-plan-safe-replace", Content: large}); err != nil { + t.Fatalf("CreatePlan: %v", err) + } + + if _, err := svc.UpdatePlan(ctx, UpdatePlanRequest{ + TaskID: "task-plan-safe-replace", + Content: strings.Repeat("y", 800), + CreatedBy: createdByAgent, + EvaluateTruncation: true, + Mode: PlanWriteModeReplace, + AgentWrite: true, + }); err == nil { + t.Fatal("agent replacement accepted a suspicious reduction without a version and acknowledgement") + } +} + +func TestAgentReplaceRequiresAndChecksCurrentVersion(t *testing.T) { + svc, _, repo := createTestPlanService(t) + ctx := context.Background() + seedTask(t, ctx, repo, "task-plan-safe-version") + created, err := svc.CreatePlan(ctx, CreatePlanRequest{TaskID: "task-plan-safe-version", Content: "before"}) + if err != nil { + t.Fatalf("CreatePlan: %v", err) + } + + cases := map[string]struct { + expected string + code string + }{ + "missing version": {expected: "", code: PlanErrorVersionRequired}, + "stale version": {expected: "stale-version", code: PlanErrorVersionConflict}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + _, err := svc.UpdatePlan(ctx, UpdatePlanRequest{ + TaskID: "task-plan-safe-version", Content: "after", CreatedBy: createdByAgent, + AgentWrite: true, ExpectedVersion: tc.expected, Mode: PlanWriteModeReplace, + }) + if err == nil { + t.Fatal("agent replacement succeeded without the current version") + } + var safety *PlanSafetyError + if !errors.As(err, &safety) { + t.Fatalf("error = %T %v, want PlanSafetyError", err, err) + } + if safety.Code != tc.code { + t.Fatalf("safety code = %q, want %q", safety.Code, tc.code) + } + if plan, getErr := svc.GetPlan(ctx, "task-plan-safe-version"); getErr != nil || plan.Content != "before" { + t.Fatalf("plan after rejected replacement = %q, %v; want before", plan.Content, getErr) + } + }) + } + if created.Plan.WriteVersion == "" { + t.Fatal("initial write did not return a version") + } +} + +func TestAgentReplaceCanAcknowledgeReductionAfterHistoryCheck(t *testing.T) { + svc, _, repo := createTestPlanService(t) + ctx := context.Background() + seedTask(t, ctx, repo, "task-plan-safe-ack") + large := strings.Repeat("x", 4000) + created, err := svc.CreatePlan(ctx, CreatePlanRequest{TaskID: "task-plan-safe-ack", Content: large}) + if err != nil { + t.Fatalf("CreatePlan: %v", err) + } + small := strings.Repeat("y", 800) + updated, err := svc.UpdatePlan(ctx, UpdatePlanRequest{ + TaskID: "task-plan-safe-ack", Content: small, CreatedBy: createdByAgent, + AgentWrite: true, ExpectedVersion: created.Plan.WriteVersion, + AllowTruncation: true, EvaluateTruncation: true, Mode: PlanWriteModeReplace, + }) + if err != nil { + t.Fatalf("acknowledged replacement: %v", err) + } + if updated.Plan.Content != small || updated.Plan.WriteVersion == created.Plan.WriteVersion { + t.Fatalf("updated plan = content %q/version %q, want new acknowledged content/version", updated.Plan.Content, updated.Plan.WriteVersion) + } + revisions, err := svc.ListRevisions(ctx, "task-plan-safe-ack") + if err != nil { + t.Fatalf("ListRevisions: %v", err) + } + if len(revisions) != 2 || revisions[1].Content != large { + t.Fatalf("revisions = %d, contents %q/%q; want separate full predecessor and replacement", len(revisions), revisions[0].Content, revisions[1].Content) + } +} + +func TestAgentAppendKeepsOptionalVersion(t *testing.T) { + svc, _, repo := createTestPlanService(t) + ctx := context.Background() + seedTask(t, ctx, repo, "task-plan-safe-append") + if _, err := svc.CreatePlan(ctx, CreatePlanRequest{TaskID: "task-plan-safe-append", Content: "before"}); err != nil { + t.Fatalf("CreatePlan: %v", err) + } + updated, err := svc.UpdatePlan(ctx, UpdatePlanRequest{ + TaskID: "task-plan-safe-append", Content: "after", CreatedBy: createdByAgent, + AgentWrite: true, Mode: PlanWriteModeAppend, + }) + if err != nil { + t.Fatalf("append without expected version: %v", err) + } + if updated.Plan.Content != "before\n\nafter" { + t.Fatalf("appended content = %q, want composed plan", updated.Plan.Content) + } +} + +func TestAgentAppendRejectsTruncationAcknowledgementWithoutReductionDetails(t *testing.T) { + svc, eventBus, repo := createTestPlanService(t) + ctx := context.Background() + const taskID = "task-plan-safe-append-truncation-flag" + seedTask(t, ctx, repo, taskID) + created, err := svc.CreatePlan(ctx, CreatePlanRequest{TaskID: taskID, Content: "before"}) + if err != nil { + t.Fatalf("CreatePlan: %v", err) + } + eventBus.ClearEvents() + + beforePlan, err := repo.GetTaskPlan(ctx, taskID) + if err != nil { + t.Fatalf("GetTaskPlan before rejected append: %v", err) + } + beforeHistory, err := repo.ListTaskPlanRevisions(ctx, taskID, 0) + if err != nil { + t.Fatalf("ListTaskPlanRevisions before rejected append: %v", err) + } + + _, err = svc.UpdatePlan(ctx, UpdatePlanRequest{ + TaskID: taskID, Content: "after", CreatedBy: createdByAgent, + AgentWrite: true, Mode: PlanWriteModeAppend, + ExpectedVersion: created.Plan.WriteVersion, AllowTruncation: true, + }) + if err == nil { + t.Fatal("append accepted allow_truncation, which is not applicable to append") + } + var safety *PlanSafetyError + if !errors.As(err, &safety) { + t.Fatalf("error = %T %v, want PlanSafetyError", err, err) + } + if !errors.Is(err, ErrPlanAppendTruncationFlag) || safety.Code != PlanErrorAppendTruncationFlag { + t.Fatalf("error = %v/code %q, want %v/%q", err, safety.Code, ErrPlanAppendTruncationFlag, PlanErrorAppendTruncationFlag) + } + if safety.ReplacedRunes != 0 || safety.NewRunes != 0 { + t.Fatalf("reduction details = %d/%d, want zero for append validation", safety.ReplacedRunes, safety.NewRunes) + } + message := strings.ToLower(safety.Message + " " + safety.NextAction) + if strings.Contains(message, "allow_truncation=true") || strings.Contains(message, "replacement removes") { + t.Fatalf("append-specific error contains replacement guidance: %q", message) + } + + afterPlan, err := repo.GetTaskPlan(ctx, taskID) + if err != nil { + t.Fatalf("GetTaskPlan after rejected append: %v", err) + } + if afterPlan.Content != beforePlan.Content || afterPlan.WriteVersion != beforePlan.WriteVersion { + t.Fatalf("plan after rejected append = %q/%q, want %q/%q", afterPlan.Content, afterPlan.WriteVersion, beforePlan.Content, beforePlan.WriteVersion) + } + afterHistory, err := repo.ListTaskPlanRevisions(ctx, taskID, 0) + if err != nil { + t.Fatalf("ListTaskPlanRevisions after rejected append: %v", err) + } + if len(afterHistory) != len(beforeHistory) { + t.Fatalf("history length after rejected append = %d, want %d", len(afterHistory), len(beforeHistory)) + } + if len(eventBus.GetPublishedEvents()) != 0 { + t.Fatalf("rejected append published %d event(s), want none", len(eventBus.GetPublishedEvents())) + } +} + +func TestAgentInitialCreationRaceGuardsSecondWriter(t *testing.T) { + svc, _, repo := createTestPlanService(t) + ctx := context.Background() + taskID := "task-plan-safe-create-race" + seedTask(t, ctx, repo, taskID) + + var mu sync.Mutex + entered := 0 + ready := make(chan struct{}) + svc.SetTaskAuthorizer(func(context.Context, string) error { + mu.Lock() + entered++ + if entered == 2 { + close(ready) + } + mu.Unlock() + <-ready + return nil + }) + + type attempt struct { + result PlanWriteResult + err error + } + results := make(chan attempt, 2) + for _, content := range []string{"first", "second"} { + go func(content string) { + result, err := svc.CreatePlan(ctx, CreatePlanRequest{ + TaskID: taskID, Content: content, AgentWrite: true, + }) + results <- attempt{result: result, err: err} + }(content) + } + + var success attempt + conflicts := 0 + for i := 0; i < 2; i++ { + out := <-results + if out.err == nil { + success = out + continue + } + var safety *PlanSafetyError + if !errors.As(out.err, &safety) || safety.Code != PlanErrorVersionRequired { + t.Fatalf("failed concurrent create = %T %v, want a version-required rejection", out.err, out.err) + } + conflicts++ + } + if success.result.Plan == nil || success.result.Plan.WriteVersion == "" || conflicts != 1 { + t.Fatalf("concurrent creates = success %#v/rejections %d, want one success and one rejection", success.result, conflicts) + } + plan, err := svc.GetPlan(ctx, taskID) + if err != nil { + t.Fatalf("GetPlan: %v", err) + } + if plan.Content != success.result.Plan.Content || plan.WriteVersion != success.result.Plan.WriteVersion { + t.Fatalf("stored plan = %q/%q, want winning create %q/%q", plan.Content, plan.WriteVersion, success.result.Plan.Content, success.result.Plan.WriteVersion) + } +} + +func TestAgentReplacementWithSameVersionAdmitsOneConcurrentWriter(t *testing.T) { + svc, _, repo := createTestPlanService(t) + ctx := context.Background() + taskID := "task-plan-safe-update-race" + seedTask(t, ctx, repo, taskID) + created, err := svc.CreatePlan(ctx, CreatePlanRequest{TaskID: taskID, Content: "before"}) + if err != nil { + t.Fatalf("CreatePlan: %v", err) + } + + var mu sync.Mutex + entered := 0 + ready := make(chan struct{}) + svc.SetTaskAuthorizer(func(context.Context, string) error { + mu.Lock() + entered++ + if entered == 2 { + close(ready) + } + mu.Unlock() + <-ready + return nil + }) + + type attempt struct { + result PlanWriteResult + err error + } + results := make(chan attempt, 2) + for _, content := range []string{"first update", "second update"} { + go func(content string) { + result, err := svc.UpdatePlan(ctx, UpdatePlanRequest{ + TaskID: taskID, Content: content, CreatedBy: createdByAgent, + AgentWrite: true, ExpectedVersion: created.Plan.WriteVersion, + Mode: PlanWriteModeReplace, + }) + results <- attempt{result: result, err: err} + }(content) + } + + var success attempt + conflicts := 0 + for i := 0; i < 2; i++ { + out := <-results + if out.err == nil { + success = out + continue + } + var safety *PlanSafetyError + if !errors.As(out.err, &safety) || safety.Code != PlanErrorVersionConflict { + t.Fatalf("failed concurrent update = %T %v, want a version conflict", out.err, out.err) + } + conflicts++ + } + if success.result.Plan == nil || success.result.Plan.WriteVersion == created.Plan.WriteVersion || conflicts != 1 { + t.Fatalf("concurrent updates = success %#v/conflicts %d, want one fresh success and one conflict", success.result, conflicts) + } + plan, err := svc.GetPlan(ctx, taskID) + if err != nil { + t.Fatalf("GetPlan: %v", err) + } + if plan.Content != success.result.Plan.Content || plan.WriteVersion != success.result.Plan.WriteVersion { + t.Fatalf("stored plan = %q/%q, want winning update %q/%q", plan.Content, plan.WriteVersion, success.result.Plan.Content, success.result.Plan.WriteVersion) + } +} diff --git a/apps/backend/internal/task/service/plan_service.go b/apps/backend/internal/task/service/plan_service.go index 41ea1395e6..92765d7a2c 100644 --- a/apps/backend/internal/task/service/plan_service.go +++ b/apps/backend/internal/task/service/plan_service.go @@ -175,6 +175,11 @@ type CreatePlanRequest struct { // is true, so this field is inert on the CreatePlan path regardless of // what a caller sets it to. Mode PlanWriteMode + // AgentWrite enables conditional admission for the MCP plan surface. It + // is set by the trusted handler, not decoded from browser payloads. + AgentWrite bool + ExpectedVersion string + AllowTruncation bool } // PlanWriteResult is returned by CreatePlan/UpdatePlan. Plan is always @@ -230,14 +235,8 @@ const ( ) func (s *PlanService) readLatestRevision(ctx context.Context, taskID string) (*models.TaskPlanRevision, planRevisionState) { - rev, err := s.repo.GetLatestTaskPlanRevision(ctx, taskID) - if err != nil { - return nil, planRevisionUnknown - } - if rev == nil { - return nil, planRevisionAbsent - } - return rev, planRevisionFound + rev, state, _ := s.readLatestRevisionDetailed(ctx, taskID) + return rev, state } // CreatePlan upserts a plan and appends or coalesces a revision. @@ -248,6 +247,9 @@ func (s *PlanService) CreatePlan(ctx context.Context, req CreatePlanRequest) (Pl if err := s.validatePlanWrite(ctx, req.TaskID, req.Content); err != nil { return PlanWriteResult{}, err } + if req.AgentWrite && req.Content == "" { + return PlanWriteResult{}, s.emptyAgentPlanError(req.TaskID) + } release := s.locks.acquire(req.TaskID) defer release() outcome, err := s.upsertPlan(ctx, req, false) @@ -271,6 +273,9 @@ type UpdatePlanRequest struct { ForceNewRevision bool EvaluateTruncation bool Mode PlanWriteMode + AgentWrite bool + ExpectedVersion string + AllowTruncation bool } // UpdatePlan updates an existing plan (errors if missing). @@ -299,6 +304,9 @@ func (s *PlanService) UpdatePlan(ctx context.Context, req UpdatePlanRequest) (Pl } } else { if req.Mode == PlanWriteModeReplace && req.Content == "" { + if req.AgentWrite { + return PlanWriteResult{}, s.emptyAgentPlanError(req.TaskID) + } return PlanWriteResult{}, ErrContentRequired } if err := s.checkContentSize(ctx, req.TaskID, req.Content); err != nil { @@ -365,6 +373,13 @@ func (s *PlanService) upsertPlan(ctx context.Context, req CreatePlanRequest, req req.EvaluateTruncation = false } + latest, latestState, latestErr := s.readLatestRevisionDetailed(readCtx, req.TaskID) + var err error + req, err = s.guardAgentPlanWrite(req, headPlan, headState, headErr, latest, latestState, latestErr) + if err != nil { + return planWriteOutcome{}, err + } + req, preserveTitle, preserveCreatedBy := s.resolveHeadFallbacks(ctx, req, headPlan, headState, requireExistingHead) title := req.Title @@ -390,7 +405,6 @@ func (s *PlanService) upsertPlan(ctx context.Context, req CreatePlanRequest, req plan.CommentsRevision = headPlan.CommentsRevision } - latest, latestState := s.readLatestRevision(readCtx, req.TaskID) rb := s.buildRevision(readCtx, req, headPlan, headState, latest, authorKind, authorName, title) if err := s.repo.WritePlanRevision(ctx, plan, rb.rev, rb.coalesceID, preserveTitle, preserveCreatedBy); err != nil { @@ -657,6 +671,21 @@ func (s *PlanService) GetPlan(ctx context.Context, taskID string) (*models.TaskP return s.repo.GetTaskPlan(ctx, taskID) } +// GetPlanSnapshot reads one task-plan row while holding the same per-task +// lock used by plan writers. The repository query returns title, content, and +// write version from that row, so agent callers receive one coherent snapshot. +func (s *PlanService) GetPlanSnapshot(ctx context.Context, taskID string) (*models.TaskPlan, error) { + if taskID == "" { + return nil, ErrTaskIDRequired + } + if err := s.authorize(ctx, taskID); err != nil { + return nil, err + } + release := s.locks.acquire(taskID) + defer release() + return s.repo.GetTaskPlan(ctx, taskID) +} + type MarkImplementationStartedRequest struct { TaskID string SessionID string diff --git a/apps/backend/internal/task/service/plan_service_logging_test.go b/apps/backend/internal/task/service/plan_service_logging_test.go index 0a7a6396c3..afea718fcf 100644 --- a/apps/backend/internal/task/service/plan_service_logging_test.go +++ b/apps/backend/internal/task/service/plan_service_logging_test.go @@ -3,6 +3,7 @@ package service import ( "context" "errors" + "strings" "testing" "github.com/kandev/kandev/internal/task/models" @@ -130,6 +131,50 @@ func TestPlanServiceRevertOtherWriteErrorLogSeverity(t *testing.T) { } } +func TestPlanServiceHistoryReadErrorIsLogged(t *testing.T) { + svc, _, repo := createTestPlanService(t) + ctx := context.Background() + const taskID = "task-plan-history-read-log" + seedTask(t, ctx, repo, taskID) + created, err := svc.CreatePlan(ctx, CreatePlanRequest{ + TaskID: taskID, Content: strings.Repeat("x", planTruncationMinPriorChars+100), + }) + if err != nil { + t.Fatalf("CreatePlan: %v", err) + } + injected := errors.New("revision history unavailable") + svc.repo = &planLatestRevisionReadErrorRepo{planRepo: repo, err: injected} + log, logs := newObservedServiceLogger(t) + svc.logger = log + + _, err = svc.UpdatePlan(ctx, UpdatePlanRequest{ + TaskID: taskID, Content: "short", CreatedBy: createdByAgent, + AgentWrite: true, ExpectedVersion: created.Plan.WriteVersion, + AllowTruncation: true, EvaluateTruncation: true, Mode: PlanWriteModeReplace, + }) + if err == nil { + t.Fatal("acknowledged replacement succeeded despite unavailable revision history") + } + var safety *PlanSafetyError + if !errors.As(err, &safety) || safety.Code != PlanErrorHistoryUnavailable { + t.Fatalf("error = %T %v, want history-unavailable PlanSafetyError", err, err) + } + entries := logs.FilterMessage("agent plan history read failed").All() + if len(entries) != 1 { + t.Fatalf("history-read log count = %d, want 1", len(entries)) + } + if entries[0].Level != zapcore.WarnLevel { + t.Fatalf("history-read log level = %s, want warn", entries[0].Level) + } + fields := entries[0].ContextMap() + if fields["task_id"] != taskID { + t.Fatalf("history-read task_id = %v, want %q", fields["task_id"], taskID) + } + if !strings.Contains(fields["error"].(string), injected.Error()) { + t.Fatalf("history-read error = %v, want %q", fields["error"], injected) + } +} + type planWriteErrorRepo struct { planRepo err error @@ -145,3 +190,12 @@ func (r *planWriteErrorRepo) WritePlanRevision( ) error { return r.err } + +type planLatestRevisionReadErrorRepo struct { + planRepo + err error +} + +func (r *planLatestRevisionReadErrorRepo) GetLatestTaskPlanRevision(context.Context, string) (*models.TaskPlanRevision, error) { + return nil, r.err +} diff --git a/apps/backend/pkg/websocket/actions.go b/apps/backend/pkg/websocket/actions.go index e7b4bc5ad9..349375c0fe 100644 --- a/apps/backend/pkg/websocket/actions.go +++ b/apps/backend/pkg/websocket/actions.go @@ -474,6 +474,10 @@ const ( ActionMCPCreateTaskPlan = "mcp.create_task_plan" ActionMCPGetTaskPlan = "mcp.get_task_plan" ActionMCPUpdateTaskPlan = "mcp.update_task_plan" + ActionMCPEditTaskPlan = "mcp.edit_task_plan" + ActionMCPListTaskPlanRevisions = "mcp.list_task_plan_revisions" + ActionMCPGetTaskPlanRevision = "mcp.get_task_plan_revision" + ActionMCPRestoreTaskPlanRevision = "mcp.restore_task_plan_revision" ActionMCPDeleteTaskPlan = "mcp.delete_task_plan" ActionMCPShowWalkthrough = "mcp.show_walkthrough" ActionMCPGetWalkthrough = "mcp.get_walkthrough" diff --git a/apps/web/e2e/tests/layout/plan-panel-indicator.spec.ts b/apps/web/e2e/tests/layout/plan-panel-indicator.spec.ts index 44a055aaf7..21d3b4bc23 100644 --- a/apps/web/e2e/tests/layout/plan-panel-indicator.spec.ts +++ b/apps/web/e2e/tests/layout/plan-panel-indicator.spec.ts @@ -15,7 +15,8 @@ const CREATE_PLAN_SCRIPT = [ const UPDATE_PLAN_SCRIPT = [ 'e2e:thinking("updating plan")', "e2e:delay(100)", - 'e2e:mcp:kandev:update_task_plan_kandev({"task_id":"{task_id}","content":"## Updated\\n\\nStep one\\nStep two"})', + 'e2e:mcp:kandev:get_task_plan_kandev({"task_id":"{task_id}"})', + 'e2e:mcp:kandev:update_task_plan_kandev({"task_id":"{task_id}","content":"## Updated\\n\\nStep one\\nStep two","expected_version":"{plan_version}"})', "e2e:delay(100)", 'e2e:message("plan updated")', ].join("\n"); diff --git a/apps/web/e2e/tests/task/mobile-plan-restore.spec.ts b/apps/web/e2e/tests/task/mobile-plan-restore.spec.ts index ea5deb58b3..006f61fbc3 100644 --- a/apps/web/e2e/tests/task/mobile-plan-restore.spec.ts +++ b/apps/web/e2e/tests/task/mobile-plan-restore.spec.ts @@ -6,7 +6,7 @@ import { SessionPage } from "../../pages/session-page"; function mcpWrite(content: string): string { const escaped = JSON.stringify(content).slice(1, -1); - return `e2e:mcp:kandev:create_task_plan_kandev({"task_id":"{task_id}","content":"${escaped}"})`; + return `e2e:mcp:kandev:create_task_plan_kandev({"task_id":"{task_id}","content":"${escaped}","expected_version":"{plan_version}"})`; } function planRevisionScript(): string { diff --git a/apps/web/e2e/tests/task/plan-checkpointing.spec.ts b/apps/web/e2e/tests/task/plan-checkpointing.spec.ts index 30f96cd21a..7f2106659a 100644 --- a/apps/web/e2e/tests/task/plan-checkpointing.spec.ts +++ b/apps/web/e2e/tests/task/plan-checkpointing.spec.ts @@ -117,7 +117,7 @@ async function expectRevisionCount(session: SessionPage, count: number, timeout function mcpWrite(content: string): string { // JSON-string-escape content: handles backslashes, quotes, and control chars. const escaped = JSON.stringify(content).slice(1, -1); - return `e2e:mcp:kandev:create_task_plan_kandev({"task_id":"{task_id}","content":"${escaped}"})`; + return `e2e:mcp:kandev:create_task_plan_kandev({"task_id":"{task_id}","content":"${escaped}","expected_version":"{plan_version}"})`; } /** Build a mock-agent script that chains plan writes with inter-write delays. */ diff --git a/docs/decisions/2026-09-16-conditional-agent-plan-writes.md b/docs/decisions/2026-09-16-conditional-agent-plan-writes.md new file mode 100644 index 0000000000..9e03cda8b9 --- /dev/null +++ b/docs/decisions/2026-09-16-conditional-agent-plan-writes.md @@ -0,0 +1,39 @@ +# ADR-2026-09-16-conditional-agent-plan-writes: Conditional agent plan writes + +**Status:** accepted +**Date:** 2026-09-16 +**Area:** protocol + +## Context + +An agent replaced a detailed plan with its progress checklist. +Kandev preserved history but warned only after the destructive write. +The warning required user recovery because agent tools exposed no history reads or restores. +History coalescing also means that a revision number does not identify one immutable content state. + +## Decision + +Agent replacement requires a durable edit version. Suspicious reductions require explicit acknowledgement before storage. +The backend changes the version on every content/title write, including browser writes and history coalescing. +Exact edits and revision recovery use the same conditional-write boundary. +Agent results provide correction guidance without requiring a task stop when no mutation occurred. +Recovery never silently replaces intervening user work. + +This decision is implemented by the +[safe agent plan edits package](../plans/plan-safe-edits/plan.md). + +## Consequences + +Existing agent replacement calls without a version receive corrective errors. +Browser interactions retain their current contract, while browser saves invalidate stale agent versions. +A durable version field requires a replayable migration and coverage for both database dialects. +Agent history tools add recovery capability without adding automatic session orchestration. + +## Alternatives Considered + +- Stronger warnings alone retain the proven failure mode because mutation precedes the warning. +- Revision numbers alone miss in-place coalescing. Timestamps can also collide or change for unrelated metadata. +- Content hashes alone cannot distinguish a later return to identical content from the state the agent originally read. +- Automatic rollback after every reduction can erase intentional edits and race with another writer. +- Mandatory user approval for every edit prevents autonomous correction of routine progress updates. +- A Markdown section parser requires heading identity rules. Unique exact-text replacement meets the current checklist-edit need with fewer semantics. diff --git a/docs/plans/plan-safe-edits/plan.md b/docs/plans/plan-safe-edits/plan.md new file mode 100644 index 0000000000..f481d14907 --- /dev/null +++ b/docs/plans/plan-safe-edits/plan.md @@ -0,0 +1,140 @@ +--- +created: 2026-09-16 +status: complete +requirements: + - REQ-TASKS-PLAN-SAFE-001 + - REQ-TASKS-PLAN-SAFE-002 + - REQ-TASKS-PLAN-SAFE-003 + - REQ-TASKS-PLAN-SAFE-004 + - REQ-TASKS-PLAN-SAFE-005 +system_design: + - ../../specs/tasks/system-design/plan-safe-edits.md +legacy_specs: [] +--- + +# Implementation Plan: Safe agent plan edits + +## Overview + +Prevent accidental agent plan replacement and provide safe correction within the same turn. +Implement durable versions first, then guarded replacement, exact edits, and revision recovery. +The package contains four sequential work orders. All four are implemented in +this checkout. + +The incident involved task `8207ac52-d02a-428c-a04f-ce34dd50f26f`. +At 2026-09-16T20:30:04Z, an explicit replacement reduced its plan from 16,332 to 807 characters. +Revision 3 remained intact. Reproduce this shape with synthetic data, not the live task. + +## Scope + +### In scope + +- Versioned agent replacements through both create and update tools. +- Pre-commit truncation rejection with explicit acknowledgement for intentional reductions. +- One exact fragment edit per call. +- Bounded history listing, exact revision reads, and conditional restore. +- Corrective MCP results, profile exposure, documentation, and contract reconciliation. + +### Out of scope + +- Browser layout or interaction changes, model switching, and automatic session restart. +- Markdown parsing, fuzzy edits, append deduplication, and multi-document migration. +- Automatic repair of unrelated history divergence. +- Changing or restoring the incident task. + +## Technical approach + +Use the existing `PlanService` lock across state checks and writes. +Add `task_plans.write_version`, with UUID rotation on every content/title write. +Keep the version independent from coalesced revision identity and metadata-only updates. + +Agent replacement requires `expected_version` against existing HEAD. +Suspicious reductions need `allow_truncation=true` plus verified history preservation. +The backend returns rejection before committing, with `write_applied=false` and a correction action. + +Add `edit_task_plan_kandev` with one exact unique match. +Add revision list, read, and restore tools through the existing guarded dispatcher. +Restore compares both HEAD version and the selected revision snapshot. +The browser revert path retains its current behavior. + +Use the implemented [design](../../specs/tasks/system-design/plan-safe-edits.md). +Update the affected older contracts at implementation time through its transition matrix. +This package adds no rendered UI, so it has no UI preview or browser E2E requirement. + +## Tests + +The table records the implemented test entry points and their evidence. + +| Acceptance criteria | Evidence | +| --- | --- | +| 002.1–002.3, 002.5 | `repository/sqlite/plan_version_test.go`: persistence, coalescing, migration replay, restart, rollback, delete/recreate, and marker/comment cases | +| 001.1–001.6, 002.4 | `service/plan_safe_edits_test.go` and `internal/mcp/handlers/task_plan_guard_test.go`: guarded replacement, concurrent creation/update, unavailable HEAD/history, and no-mutation cases | +| 003.1–003.5 | `service/plan_exact_edit_test.go`: `TestPlanExactEdit`; MCP schema/forwarding tests, including raw omission/null rejection for `new_text` | +| 004.1–004.7 | `service/plan_agent_recovery_test.go`: `TestPlanAgentRecovery`, coalesced-source invalidation, deterministic browser-write conflict, oversized-history restore, and marker/comment preservation; bounded repository history tests; `mcp/server/task_plan_safe_edits_integration_test.go`: authorized, cross-task, and unauthorized list/read/restore coverage | +| 005.1–005.7 | `mcp/server/task_plan_safe_edits_integration_test.go`: `TestPlanSafeEditsMCPJourney`; description, error, and profile tests | + +All abbreviated IDs use `AC-TASKS-PLAN-SAFE-`. +Tests must assert unchanged title/content/history/version and no mutation events on rejection. +Use deterministic barriers for concurrency rather than timing-dependent goroutine races. +Keep existing append, size-limit, browser, authorization, and marker regressions. + +## End-to-end evidence + +Task 04 owns a Go integration test through actual MCP dispatch, backend handlers, service, and SQLite. +A test bridge can replace transport delivery, but cannot stub plan responses or policy. +The journey reads a long plan, appends a checklist, rejects its fragment replacement, and corrects a checkbox with exact edit. +It then exercises revision read and conditional restoration. +The agent-visible result must provide correction steps without a stop instruction. +A browser or a live LLM is not needed to prove this agent-tool contract. + +## Work orders + +- [x] [Task 01: Persist edit versions](task-01-edit-versions.md) (done) +- [x] [Task 02: Guard agent replacement](task-02-guarded-replacement.md) (done) +- [x] [Task 03: Add exact fragment edits](task-03-exact-edits.md) (done) +- [x] [Task 04: Add revision recovery and integration evidence](task-04-revision-recovery.md) (done) + +Execution order: 01 → 02 → 03 → 04. All work orders are complete. + +## Verification results + +Implementation verification completed on 2026-09-17. + +Checks on 2026-09-17: + +- `go test ./internal/task/repository/... ./internal/task/service ./internal/task/handlers ./internal/task/planws ./internal/mcp/handlers ./internal/mcp/server -count=1`: passed. +- The same affected backend packages passed with `go test -race`. +- Review follow-up regressions passed: raw MCP `new_text` omission/null rejection, coalesced-source invalidation, deterministic browser-write conflict, oversized-history restore with metadata preservation, and MCP authorization/task-scope recovery coverage. +- `go test -race ./internal/persistence/storeconformance -count=1`: passed. +- `make lint` and `make build` from `apps/backend`: passed. +- `go run ./cmd/sqlguard ./internal`: passed. +- Public documentation tests and validator: passed; 46 published pages validated. +- `python3 scripts/list-docs.py validate`: passed, 282 decisions and 971 specifications. +- `python3 scripts/lint-spec-files.test.py`: passed, 36 tests. +- `python3 scripts/lint-spec-files.py --all`: passed. +- `git diff --check`: passed. +- PostgreSQL tests remain environment-gated and were skipped because `KANDEV_TEST_POSTGRES_DSN` was not set. + +The incident task was not restored or modified. Changes remain uncommitted for review. + +## Risks + +- Schema changes must cover SQLite upgrades and PostgreSQL parity. +- Cached tool schemas can lack new fields. Existing replacement calls fail safely and require refreshed discovery. +- A revision can change during coalescing. Restore needs its snapshot token as well as the HEAD token. +- The guard detects large reductions, not all semantic omissions. Exact edits reduce ordinary progress-update risk. +- Browser saves remain unconditional under their existing contract. This package protects agent writes against intervening browser saves. +- Legacy error and guard tests encode post-write warnings. Reconcile their requirements before changing their assertions. +- Do not claim PostgreSQL checks passed when the DSN-gated tests skipped. +- Human conflict resolution remains necessary when intent or concurrent edits cannot be reconciled safely. + +## Sources + +- [Requirements](../../specs/tasks/requirements/plan-safe-edits.md), REQ-TASKS-PLAN-SAFE-001 through 005. +- [Design](../../specs/tasks/system-design/plan-safe-edits.md). +- [Decision](../../decisions/2026-09-16-conditional-agent-plan-writes.md). +- [Existing consistency contract](../../specs/tasks/requirements/plan-write-consistency.md). +- [Existing append contract](../../specs/tasks/requirements/plan-write-append-mode.md). + +The existing plan-write specifications contain no linked companion implementation package. +The older backend diagnostic work order classifies missing-task errors and remains outside this change. diff --git a/docs/plans/plan-safe-edits/task-01-edit-versions.md b/docs/plans/plan-safe-edits/task-01-edit-versions.md new file mode 100644 index 0000000000..db386089ff --- /dev/null +++ b/docs/plans/plan-safe-edits/task-01-edit-versions.md @@ -0,0 +1,105 @@ +--- +id: "01-edit-versions" +title: "Persist edit versions" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-TASKS-PLAN-SAFE-002 +acceptance_criteria: + - AC-TASKS-PLAN-SAFE-002.1 + - AC-TASKS-PLAN-SAFE-002.2 + - AC-TASKS-PLAN-SAFE-002.3 + - AC-TASKS-PLAN-SAFE-002.5 +system_design: + - ../../specs/tasks/system-design/plan-safe-edits.md +--- + +# Task 01: Persist edit versions + +## Summary + +Persist a version for each committed plan edit and expose coherent agent reads. Preserve history coalescing, browser payloads, comments, and implementation markers. + +## In scope + +- Add the version migration, backfill, model field, scans, and rotation to every production content/title repository writer. +- Add service snapshot reads and MCP read metadata without changing exact content. +- Update repository interface test doubles and upgrade fixtures where required. Cover committed-result fallback after a post-write read fails. + +## Out of scope + +Replacement admission, exact edits, revision tools, and rendered UI. + +## Acceptance + +- Versions survive restart and change for identical writes, browser writes, coalescing, and delete/recreate. +- Migration replay and rollback preserve existing data. Marker/comment updates do not change the version. +- Agent reads return exact content with its matching version. No generated browser types or UI controls are required. + +## TDD and evidence + +Seed a plan, coalesce another write into the same revision, and assert the version changes. Add migration replay, restart, rollback, and marker-only cases. Extend PostgreSQL coverage using the existing isolated-database helper. + +Test entry points and acceptance mappings are in [the manifest](plan.md#tests). +Update existing assertions only where the design explicitly changes their contract. + +## Verification + +Run from the repository root. Set `KANDEV_TEST_POSTGRES_DSN` to an isolated test database for PostgreSQL coverage. +Record a missing DSN as skipped coverage, never as a pass. +The normal repository command includes the environment-gated PostgreSQL tests. + +```bash +(cd apps/backend && go test ./internal/task/repository/... ./internal/task/service ./internal/mcp/handlers ./internal/mcp/server -count=1) +(cd apps/backend && go test -race ./internal/persistence/storeconformance -count=1) +(cd apps/backend && go run ./cmd/sqlguard ./internal) +git diff --check +``` + +## Files changed + +- `apps/backend/internal/task/models/models.go` +- `apps/backend/internal/task/repository/sqlite/base_migrations.go` +- `apps/backend/internal/task/repository/sqlite/plan.go` +- `apps/backend/internal/task/repository/sqlite/plan_version_test.go (new)` +- `apps/backend/internal/task/repository/sqlite/document_plan_postgres_test.go` +- `apps/backend/internal/task/service/plan_service.go` +- `apps/backend/internal/mcp/handlers/handlers.go` +- `apps/backend/internal/mcp/server/handlers.go` +- `apps/backend/internal/persistence/storeconformance/upgrade_test.go and testdata/upgrades/` + +The implementation also updates the shared MCP read/write bridge and related +server and contract tests. + +## Dependencies + +None. + +## Risks + +A revision ID is not a write version. Do not use timestamps or reset a numeric counter on recreation. + +## Parallelism + +`sequential` + +## Inputs + +- [Requirements](../../specs/tasks/requirements/plan-safe-edits.md). +- [System design](../../specs/tasks/system-design/plan-safe-edits.md). +- [Decision](../../decisions/2026-09-16-conditional-agent-plan-writes.md). +- Existing `plan_service_concurrency_test.go`, `task_plan_guard_test.go`, and `task_plan_append_mode_test.go`. +- Backend guidance and the TDD backend-testing reference. + +## Results + +Implemented durable `task_plans.write_version` storage with replayable SQLite +and PostgreSQL-compatible migration/backfill behavior. Repository content/title +writes rotate the opaque version, while marker and comment writes preserve it. +Agent reads now return exact content with the matching version, and metadata +history reads use a bounded byte-count query. + +Repository, service, MCP handler, and MCP server tests passed. The PostgreSQL +checks are environment-gated and were not run without a test DSN. diff --git a/docs/plans/plan-safe-edits/task-02-guarded-replacement.md b/docs/plans/plan-safe-edits/task-02-guarded-replacement.md new file mode 100644 index 0000000000..8fb6d7d296 --- /dev/null +++ b/docs/plans/plan-safe-edits/task-02-guarded-replacement.md @@ -0,0 +1,124 @@ +--- +id: "02-guarded-replacement" +title: "Guard agent replacement" +status: done +wave: 2 +depends_on: + - "01-edit-versions" +plan: "plan.md" +requirements: + - REQ-TASKS-PLAN-SAFE-001 + - REQ-TASKS-PLAN-SAFE-002 + - REQ-TASKS-PLAN-SAFE-005 +acceptance_criteria: + - AC-TASKS-PLAN-SAFE-001.1 + - AC-TASKS-PLAN-SAFE-001.2 + - AC-TASKS-PLAN-SAFE-001.3 + - AC-TASKS-PLAN-SAFE-001.4 + - AC-TASKS-PLAN-SAFE-001.5 + - AC-TASKS-PLAN-SAFE-001.6 + - AC-TASKS-PLAN-SAFE-002.4 + - AC-TASKS-PLAN-SAFE-005.1 + - AC-TASKS-PLAN-SAFE-005.2 + - AC-TASKS-PLAN-SAFE-005.5 + - AC-TASKS-PLAN-SAFE-005.6 +system_design: + - ../../specs/tasks/system-design/plan-safe-edits.md +--- + +# Task 02: Guard agent replacement + +## Summary + +Reject stale and suspicious agent replacements before mutation. Return corrective errors and versioned write acknowledgements through the actual MCP path. + +## In scope + +- Add expected_version and allow_truncation to create/update schemas, forwarding, typed requests, and service admission. +- Cover the create-upsert alternative and initial-create race. Enforce checks inside the existing plan lock. +- Map typed errors through planws and the MCP bridge without discarding reason data. +- Replace post-write stop warnings on agent paths. Reconcile the consistency, append-agent-text, and size-limit clauses named by the design. + +## Out of scope + +Exact-edit and revision tools, browser admission changes, and automatic conflict reconciliation. + +## Acceptance + +- A 16,332-character plan cannot become an 807-character fragment without a matching version and explicit acknowledgement. +- Every rejected operation leaves plan/history/markers unchanged and emits no mutation event. A subsequent valid write succeeds. +- Schemas and bridges preserve both new fields. Compact success responses expose the committed version, and errors provide correction steps. + +## TDD and evidence + +Reproduce the incident through handlers. Assert no writes on missing/stale versions, shrink rejection, failed HEAD reads, and unavailable preservation history. Cover exact threshold boundaries and Unicode. Force a concurrent browser save before agent admission and assert conflict. + +Test entry points and acceptance mappings are in [the manifest](plan.md#tests). +Update existing assertions only where the design explicitly changes their contract. + +## Verification + +Run from the repository root. Set `KANDEV_TEST_POSTGRES_DSN` to an isolated test database for PostgreSQL coverage. +Record a missing DSN as skipped coverage, never as a pass. + + +```bash +(cd apps/backend && go test -race ./internal/task/service ./internal/task/planws ./internal/mcp/handlers ./internal/mcp/server -count=1) +python3 scripts/list-docs.py validate +python3 scripts/lint-spec-files.py --all +git diff --check +``` + +## Files changed + +- `apps/backend/internal/task/service/plan_service.go` +- `apps/backend/internal/task/service/plan_truncation.go` +- `apps/backend/internal/task/service/plan_safe_edits_test.go (new)` +- `apps/backend/internal/mcp/handlers/handlers.go` +- `apps/backend/internal/mcp/handlers/task_plan_guard.go and task_plan_guard_test.go` +- `apps/backend/internal/mcp/server/server.go` +- `apps/backend/internal/mcp/server/handlers.go` +- `apps/backend/internal/mcp/server/compact_results_test.go` +- `apps/backend/internal/mcp/server/task_plan_append_mode_test.go` +- `apps/backend/internal/task/planws/errors.go` +- `docs/specs/tasks/requirements/plan-write-consistency.md` +- `docs/specs/tasks/requirements/plan-write-append-mode.md` +- `docs/specs/tasks/requirements/plan-content-size-limit.md` +- `docs/specs/tasks/system-design/plan-write-consistency.md` +- `docs/specs/tasks/system-design/plan-write-append-mode.md` +- `docs/specs/tasks/system-design/plan-write-append-mode-agent-text.md` + +The implementation also updates the task-plan error contract, Office prompt, +server catalog, and affected compatibility tests. + +## Dependencies + +Task 01: Persist edit versions. + +## Risks + +Default replacement remains a whole-document operation. New preconditions must apply even when the caller omits mode or uses create. + +## Parallelism + +`sequential` + +## Inputs + +- [Requirements](../../specs/tasks/requirements/plan-safe-edits.md). +- [System design](../../specs/tasks/system-design/plan-safe-edits.md). +- [Decision](../../decisions/2026-09-16-conditional-agent-plan-writes.md). +- Existing `plan_service_concurrency_test.go`, `task_plan_guard_test.go`, and `task_plan_append_mode_test.go`. +- Backend guidance and the TDD backend-testing reference. + +## Results + +Implemented trusted agent admission for create and update replacements. Existing +plans require a matching `expected_version`; suspicious reductions require +explicit acknowledgement and verified history before a separate revision is +written. Rejections carry stable reason data, no-write status, and correction +guidance through the WebSocket and MCP layers. + +Added deterministic concurrent-create and concurrent-update coverage, including +unreadable HEAD/history cases and Unicode reduction detection. The affected +backend tests and lint checks passed. diff --git a/docs/plans/plan-safe-edits/task-03-exact-edits.md b/docs/plans/plan-safe-edits/task-03-exact-edits.md new file mode 100644 index 0000000000..5c1605a774 --- /dev/null +++ b/docs/plans/plan-safe-edits/task-03-exact-edits.md @@ -0,0 +1,110 @@ +--- +id: "03-exact-edits" +title: "Add exact fragment edits" +status: done +wave: 3 +depends_on: + - "02-guarded-replacement" +plan: "plan.md" +requirements: + - REQ-TASKS-PLAN-SAFE-003 + - REQ-TASKS-PLAN-SAFE-005 +acceptance_criteria: + - AC-TASKS-PLAN-SAFE-003.1 + - AC-TASKS-PLAN-SAFE-003.2 + - AC-TASKS-PLAN-SAFE-003.3 + - AC-TASKS-PLAN-SAFE-003.4 + - AC-TASKS-PLAN-SAFE-003.5 + - AC-TASKS-PLAN-SAFE-005.1 + - AC-TASKS-PLAN-SAFE-005.2 + - AC-TASKS-PLAN-SAFE-005.5 + - AC-TASKS-PLAN-SAFE-005.6 + - AC-TASKS-PLAN-SAFE-005.7 +system_design: + - ../../specs/tasks/system-design/plan-safe-edits.md +--- + +# Task 03: Add exact fragment edits + +## Summary + +Add a small exact-text editing tool for routine checklist changes. Reuse conditional admission and avoid a Markdown parser. + +## In scope + +- Register edit_task_plan_kandev and its guarded backend action with version, old_text, new_text, and optional truncation acknowledgement. +- Compose inside the existing task lock, require one exact occurrence including overlap checks, and preserve surrounding bytes. +- Reuse final-content validation, limits, history, attribution, and compact responses. Preserve append behavior with optional expected_version. + +## Out of scope + +Fuzzy matching, heading identity, edit batches, automatic token refresh, and revision recovery. + +## Acceptance + +- One checkbox edit changes only the requested span and preserves review sections, title, and exact surrounding bytes. +- Empty old text, ambiguous/absent matches, stale tokens, oversized composition, and unacknowledged large reductions reject without mutation. +- The tool appears only on existing plan-write surfaces. Invalid arguments cannot become replacement or append operations. + +## TDD and evidence + +Add checkbox, Unicode, CRLF, overlap, deletion, missing new_text, identical replacement, final-empty, and size-boundary cases. Assert untouched bytes around the span. Test schema-to-service propagation and excluded profiles. + +Test entry points and acceptance mappings are in [the manifest](plan.md#tests). +Update existing assertions only where the design explicitly changes their contract. + +## Verification + +Run from the repository root. Set `KANDEV_TEST_POSTGRES_DSN` to an isolated test database for PostgreSQL coverage. +Record a missing DSN as skipped coverage, never as a pass. + + +```bash +(cd apps/backend && go test -race ./internal/task/service ./internal/task/planws ./internal/mcp/handlers ./internal/mcp/server -count=1) +git diff --check +``` + +## Files changed + +- `apps/backend/internal/task/service/plan_exact_edit.go (new)` +- `apps/backend/internal/task/service/plan_exact_edit_test.go (new)` +- `apps/backend/internal/mcp/handlers/task_plan_edit.go (new)` +- `apps/backend/internal/mcp/handlers/handlers.go` +- `apps/backend/internal/mcp/server/task_plan_edit.go (new)` +- `apps/backend/internal/mcp/server/server.go` +- `apps/backend/pkg/websocket/actions.go` +- `apps/backend/internal/mcp/server/task_plan_append_mode_test.go` + +The implementation also updates the WebSocket action catalog and append-mode +compatibility coverage. + +## Dependencies + +Task 02: Guard agent replacement. + +## Risks + +Non-overlapping string counts can miss ambiguous overlapping matches. No whitespace normalization is allowed. + +## Parallelism + +`sequential` + +## Inputs + +- [Requirements](../../specs/tasks/requirements/plan-safe-edits.md). +- [System design](../../specs/tasks/system-design/plan-safe-edits.md). +- [Decision](../../decisions/2026-09-16-conditional-agent-plan-writes.md). +- Existing `plan_service_concurrency_test.go`, `task_plan_guard_test.go`, and `task_plan_append_mode_test.go`. +- Backend guidance and the TDD backend-testing reference. + +## Results + +Implemented `edit_task_plan_kandev` with exact byte-span replacement, overlap +detection, version checks, size validation, truncation protection, deletion, +and compact acknowledgements. Surrounding content and line endings remain +unchanged. Append keeps its existing composition and accepts an optional +version guard. + +Checkbox, CRLF, Unicode, ambiguous, missing, deletion, final-empty, size, and +MCP forwarding cases passed. diff --git a/docs/plans/plan-safe-edits/task-04-revision-recovery.md b/docs/plans/plan-safe-edits/task-04-revision-recovery.md new file mode 100644 index 0000000000..67ffe0f505 --- /dev/null +++ b/docs/plans/plan-safe-edits/task-04-revision-recovery.md @@ -0,0 +1,141 @@ +--- +id: "04-revision-recovery" +title: "Add revision recovery and integration evidence" +status: done +wave: 4 +depends_on: + - "03-exact-edits" +plan: "plan.md" +requirements: + - REQ-TASKS-PLAN-SAFE-004 + - REQ-TASKS-PLAN-SAFE-005 +acceptance_criteria: + - AC-TASKS-PLAN-SAFE-004.1 + - AC-TASKS-PLAN-SAFE-004.2 + - AC-TASKS-PLAN-SAFE-004.3 + - AC-TASKS-PLAN-SAFE-004.4 + - AC-TASKS-PLAN-SAFE-004.5 + - AC-TASKS-PLAN-SAFE-004.6 + - AC-TASKS-PLAN-SAFE-004.7 + - AC-TASKS-PLAN-SAFE-005.1 + - AC-TASKS-PLAN-SAFE-005.2 + - AC-TASKS-PLAN-SAFE-005.3 + - AC-TASKS-PLAN-SAFE-005.4 + - AC-TASKS-PLAN-SAFE-005.5 + - AC-TASKS-PLAN-SAFE-005.6 + - AC-TASKS-PLAN-SAFE-005.7 +system_design: + - ../../specs/tasks/system-design/plan-safe-edits.md +--- + +# Task 04: Add revision recovery and integration evidence + +## Summary + +Expose bounded history reads and conditional agent restoration. Complete deterministic MCP journey coverage and publish recovery guidance with the implementation. + +## In scope + +- Add metadata pagination and task-scoped revision reads without loading all revision bodies. +- Add snapshot tokens and conditional restore through shared revert internals, preserving browser behavior and agent attribution. +- Register all tools and actions through existing profile and guarded-dispatch paths. +- Build the real MCP-to-service journey regression and synchronize tool instructions, public documentation, and affected contract statuses. + +## Out of scope + +Automatic incident restoration, automatic session restart, revision retention changes, and live LLM evaluations. + +## Acceptance + +- List/read/restore work through MCP with bounded metadata pages, exact content, correct task scope, and agent attribution. +- Source coalescing, destination edits, unknown history, missing plans, and unauthorized targets reject without mutation. Valid restore preserves markers and previous history. +- The incident journey rejects the bad replacement and then completes a valid exact edit. Guidance does not require a user stop for safe correction. + +## TDD and evidence + +Test descending cursor pagination without gaps, cross-task IDs, mutable source snapshots, stale destination versions, oversized historical restore, and repeat-after-lost-response conflicts. Use deterministic concurrency barriers. The integration bridge must call actual handlers and service methods, not canned backend responses. + +Test entry points and acceptance mappings are in [the manifest](plan.md#tests). +Update existing assertions only where the design explicitly changes their contract. + +## Verification + +Run from the repository root. Set `KANDEV_TEST_POSTGRES_DSN` to an isolated test database for PostgreSQL coverage. +Record a missing DSN as skipped coverage, never as a pass. +The normal repository command includes the environment-gated PostgreSQL tests. + +```bash +(cd apps/backend && go test -race ./internal/task/repository/... ./internal/task/service ./internal/task/handlers ./internal/task/planws ./internal/mcp/handlers ./internal/mcp/server -count=1) +(cd apps/backend && go test ./internal/mcp/server -run '^TestPlanSafeEditsMCPJourney$' -count=1 -v) +(cd apps/backend && go run ./cmd/sqlguard ./internal) +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 changed + +- `apps/backend/internal/task/repository/sqlite/plan.go` +- `apps/backend/internal/task/service/plan_service.go` +- `apps/backend/internal/task/service/plan_agent_recovery.go (new)` +- `apps/backend/internal/task/service/plan_agent_recovery_test.go (new)` +- `apps/backend/internal/mcp/handlers/task_plan_recovery.go (new)` +- `apps/backend/internal/mcp/handlers/handlers.go` +- `apps/backend/internal/mcp/server/task_plan_recovery.go (new)` +- `apps/backend/internal/mcp/server/server.go` +- `apps/backend/internal/mcp/server/task_plan_safe_edits_integration_test.go (new)` +- `apps/backend/pkg/websocket/actions.go` +- `docs/public/tasks-and-workflows.md` +- `apps/backend/AGENTS.md` +- `docs/specs/tasks/requirements/plan-safe-edits.md` +- `docs/specs/tasks/system-design/plan-safe-edits.md` + +The implementation also updates public documentation, repository guidance, +coverage metadata, and the Office MCP prompt. + +## Dependencies + +Task 03: Add exact fragment edits. + +## Risks + +Existing GetRevision reads by ID before task authorization, and RevertPlan stamps the user author kind. Do not expose either unchanged as the new agent contract. + +## Parallelism + +`sequential` + +## Inputs + +- [Requirements](../../specs/tasks/requirements/plan-safe-edits.md). +- [System design](../../specs/tasks/system-design/plan-safe-edits.md). +- [Decision](../../decisions/2026-09-16-conditional-agent-plan-writes.md). +- Existing `plan_service_concurrency_test.go`, `task_plan_guard_test.go`, and `task_plan_append_mode_test.go`. +- Backend guidance and the TDD backend-testing reference. + +## Results + +Implemented bounded revision metadata pagination, exact task-scoped revision +reads, snapshot tokens, and conditional agent restore. Restore preserves the +source and prior history, records agent attribution and the source reference, +and reports `already_current` without writing. All four plan recovery tools are +available on task and Office plan surfaces through the guarded dispatcher. + +Added a real MCP-to-service-to-SQLite journey covering append, rejected fragment +replacement, exact correction, revision read, and conditional restore. Public +docs and repository guidance now describe the safe contract. Full affected +tests, race checks, lint, build, SQL guard, and documentation validation passed. + +Review follow-up coverage now verifies that a previously valid source token is +invalidated by in-place revision coalescing, that a browser write admitted +during a restore wins deterministically, and that rejected restores leave HEAD, +version, history, and events unchanged. It also covers oversized historical +restore with comment and implementation-marker preservation, raw MCP omission +and `null` handling for `new_text`, and authorized, cross-task, and +unauthorized history reads and restores without foreign revision disclosure. +The MCP journey also covers an explicit empty `new_text` deletion, while the +agent write guard has dedicated append-flag validation and logs underlying +revision-history read failures before returning its no-write safety error. diff --git a/docs/public/agent-communication.md b/docs/public/agent-communication.md index 9e688af0bf..83ba588a8f 100644 --- a/docs/public/agent-communication.md +++ b/docs/public/agent-communication.md @@ -258,6 +258,10 @@ These tools complement cross-task communication for common coordination patterns | `move_task_kandev` | Hand off a task to a workflow step with optional one-time entry options for the receiving agent | | `create_task_plan_kandev` | Record an agreed implementation plan (both tasks can create/update their own plans) | | `get_task_plan_kandev` | Read a task's plan; useful before messaging to share a structured proposal | +| `edit_task_plan_kandev` | Apply one exact, unique text edit to a reachable task plan | +| `list_task_plan_revisions_kandev` | List bounded metadata for a reachable task plan's history | +| `get_task_plan_revision_kandev` | Read one exact revision before a conditional restore | +| `restore_task_plan_revision_kandev` | Restore a revision after checking current and source versions | | `step_complete_kandev` | Signal that the current workflow step is done (task-mode only) | | `ask_user_question_kandev` | Escalate to a human when agent negotiation cannot resolve a question | diff --git a/docs/public/automation-and-mcp.md b/docs/public/automation-and-mcp.md index d45d3ada9c..75722617d5 100644 --- a/docs/public/automation-and-mcp.md +++ b/docs/public/automation-and-mcp.md @@ -291,6 +291,56 @@ Task tools use normal client discovery. When `step_complete_kandev` is required `create_task_kandev` advertises `prompt` for instructions delivered to a newly started agent. Older callers may still send `description` when `prompt` is absent, but sending both is an error; the compatibility name is intentionally omitted from the advertised schema. +### Protect task plan writes + +Task plans are shared documents. Agent writes use an opaque `version` to detect +an intervening write. The version changes after every title or content write, +including a write that coalesces into an existing history revision. Comment and +implementation-marker updates do not change it. + +Use `get_task_plan_kandev` before a whole-document replacement. Pass its +`version` as `expected_version` to `create_task_plan_kandev` or to +`update_task_plan_kandev` in `replace` mode. A new plan does not need an +expected version. A stale version rejects the write before Kandev changes the +plan or publishes a plan event. + +Kandev rejects a suspiciously smaller replacement before storage. The error +includes a stable reason, the current version, and a correction. Use +`edit_task_plan_kandev` for a local change. It replaces one exact, unique +`old_text` match and preserves all other bytes, including line endings. An +empty `new_text` deletes the match. Ambiguous, missing, or stale edits do not +write anything. + +Set `allow_truncation=true` only for an intentional large reduction. The +request still needs the current `expected_version`. Kandev checks that plan +history is available, keeps the previous snapshot, and writes the reduction as +a new revision. + +Use `update_task_plan_kandev` with `mode="append"` to add a section without +reading the plan first. Kandev reads the stored plan and adds one blank line +before the fragment. Append is not idempotent. A repeated call adds the +fragment again. `expected_version` is optional for append mode, but a supplied +stale version is rejected. + +### Recover a plan revision + +Use these task-scoped tools when a plan needs inspection or recovery: + +- `list_task_plan_revisions_kandev` returns newest-first metadata without + revision content. It returns 20 rows by default and accepts a cursor and a + maximum page size of 100. +- `get_task_plan_revision_kandev` returns one exact title and content, plus its + `revision_version` snapshot token. +- `restore_task_plan_revision_kandev` restores the selected snapshot as a new + agent-attributed revision. Pass both the current plan `expected_version` and + the selected `expected_revision_version`. + +The restore rejects a changed current plan or a changed source revision. It +preserves the source history and reports `already_current` without writing when +the selected snapshot already matches the current plan. These tools accept +another task ID only when that task is within the caller's reachable workspace +or task tree. + ### Native rich output Task and Office agents can call `show_rich_output_kandev` when a workspace file, @@ -565,7 +615,7 @@ A task session currently registers these tool groups: | Board lookups and task lifecycle | List workspaces, workflows, workflow steps, tasks, agents, and executor profiles; create, update, move, archive, or delete tasks; halt all live work on a direct child. This mode does not mutate workflows, profiles, or executors. | | Coordination | Message a task or targeted session, spawn a named session on the current or another same-workspace task, and read task conversation. See [Agent Communication](agent-communication.md) for delivery semantics, bidirectional reply patterns, and a worked example. | | User interaction | Ask a structured question when the current agent/session supports it. | -| Plans | Create, get, update, and delete the current task plan. | +| Plans | Create, get, update, and delete the current task plan; apply exact edits; list, read, and restore revisions. | | Walkthroughs | Show, get, and delete the task's code walkthrough. | | Relationships and workspace sources | List related tasks, add a mixed repository/folder source batch to an idle task, use the legacy one-branch tool, and change a repository's diff base. | | Workflow signal | Signal step completion when an auto-advance step explicitly requires that signal. | @@ -647,12 +697,13 @@ Office runs use a smaller MCP surface than regular task-mode sessions. The built - `ask_user_question_kandev`; - `create_task_plan_kandev`, `get_task_plan_kandev`, `update_task_plan_kandev`, and `delete_task_plan_kandev`; +- `edit_task_plan_kandev`, `list_task_plan_revisions_kandev`, `get_task_plan_revision_kandev`, and `restore_task_plan_revision_kandev`; - `list_related_tasks_kandev`; - `list_task_documents_kandev`, `get_task_document_kandev`, and `write_task_document_kandev`; - `show_rich_output_kandev`; - `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 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. +These tools cover human questions, the current task plan, plan edits and recovery, related-task discovery, task documents, 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/public/coverage.json b/docs/public/coverage.json index 4d5c469f88..a83f9b3078 100644 --- a/docs/public/coverage.json +++ b/docs/public/coverage.json @@ -260,11 +260,19 @@ ], "sources": [ "apps/backend/internal/mcp/handlers/review.go", + "apps/backend/internal/mcp/handlers/task_plan_edit.go", + "apps/backend/internal/mcp/handlers/task_plan_recovery.go", + "apps/backend/internal/task/service/plan_exact_edit.go", + "apps/backend/internal/task/service/plan_revision_recovery.go", "apps/web/components/review/review-dialog.tsx", "apps/web/components/review/walkthrough-overlay.tsx" ], "tests": [ "apps/backend/internal/mcp/handlers/review_test.go", + "apps/backend/internal/mcp/handlers/task_plan_safe_edits_test.go", + "apps/backend/internal/mcp/server/task_plan_safe_edits_integration_test.go", + "apps/backend/internal/task/service/plan_exact_edit_test.go", + "apps/backend/internal/task/service/plan_agent_recovery_test.go", "apps/web/components/review/review-dialog.build-files.test.ts", "apps/web/components/review/walkthrough-overlay.test.tsx" ], @@ -272,9 +280,13 @@ "create_task_plan_kandev", "delete_task_plan_kandev", "delete_walkthrough_kandev", + "edit_task_plan_kandev", "get_task_plan_kandev", + "get_task_plan_revision_kandev", "get_walkthrough_kandev", + "list_task_plan_revisions_kandev", "publish_review_findings_kandev", + "restore_task_plan_revision_kandev", "show_walkthrough_kandev", "update_task_plan_kandev" ] diff --git a/docs/public/tasks-and-workflows.md b/docs/public/tasks-and-workflows.md index b653e7d11b..f6d63b1f80 100644 --- a/docs/public/tasks-and-workflows.md +++ b/docs/public/tasks-and-workflows.md @@ -723,7 +723,30 @@ and primary session are eligible, even if other feedback is still being restored. A recovered comment must finish its own browser-draft cleanup before it can be run. -Agents use `create_task_plan_kandev`, `get_task_plan_kandev`, `update_task_plan_kandev`, and `delete_task_plan_kandev`. Human edits are therefore visible to the next agent that reads the plan. A plan records intent; verify that code and review still match it. +Agents use `create_task_plan_kandev`, `get_task_plan_kandev`, `update_task_plan_kandev`, and `delete_task_plan_kandev`. Human edits are therefore visible to the next agent that reads the plan. A plan records intent; verify that code and review still match it. For safe agent corrections, see [Protect task plan writes](automation-and-mcp.md#protect-task-plan-writes). + +### Protect agent plan writes + +Every agent plan read returns an opaque `version`. The version changes after a +title or content write. Comment and implementation-marker changes do not change +the version. + +Use `expected_version` for a whole-document replacement. Kandev rejects the +write when the stored version differs. The rejection happens before Kandev +changes the title, content, history, or events. + +Kandev also rejects a replacement that looks like accidental truncation. Use +`edit_task_plan_kandev` for a local text change. Set `allow_truncation` only +when the reduction is intentional and the current version matches. Kandev +keeps the previous snapshot in plan history. + +Use `update_task_plan_kandev` with `mode="append"` to add a section without +reading the plan first. The server adds one blank line before the new section. +Append is not idempotent, so a repeated call adds the section again. + +If an agent loses a write response, read the plan before retrying. Use the +returned version as the next `expected_version`. Do not repeat a whole-document +replacement with an old version. ## Arrange task panels diff --git a/docs/specs/tasks/requirements/plan-safe-edits.md b/docs/specs/tasks/requirements/plan-safe-edits.md new file mode 100644 index 0000000000..abb683d4cd --- /dev/null +++ b/docs/specs/tasks/requirements/plan-safe-edits.md @@ -0,0 +1,104 @@ +--- +status: active +system: tasks +created: 2026-09-16 +owners: + - kandev +--- + +# Safe agent plan edits Requirements + +## Overview + +Agents must correct routine plan-edit mistakes without losing task context or requiring a user to restore history. +The tasks system owns this contract because it owns the shared plan and its history. +The contract applies to agent plan tools. Browser editing retains its current interaction contract. + +## Terminology + +- **Edit version:** an opaque value that identifies one committed plan state. It changes even when history combines consecutive writes. +- **Suspicious reduction:** replacement of at least 2,000 Unicode code points with fewer than half that number. +- **Exact edit:** replacement of one uniquely matching text fragment within the current plan. +- **Restore:** a new history entry that copies a selected revision. It does not erase history. + +## Requirements + +### REQ-TASKS-PLAN-SAFE-001: Reject accidental replacement before storage + +#### Acceptance criteria + +- **AC-TASKS-PLAN-SAFE-001.1:** An agent replacement shall require the current edit version when a plan already exists, including replacement through the create tool. +- **AC-TASKS-PLAN-SAFE-001.2:** A missing or stale edit version shall reject the replacement without changing content, title, history, or implementation state. +- **AC-TASKS-PLAN-SAFE-001.3:** A suspicious reduction shall require explicit truncation acknowledgement and a matching edit version. Otherwise, the plan shall remain unchanged. +- **AC-TASKS-PLAN-SAFE-001.4:** An acknowledged reduction shall preserve the preceding content in history and create a separate revision. Unknown history shall reject the write. +- **AC-TASKS-PLAN-SAFE-001.5:** An unreadable current plan shall reject an agent replacement. The result shall distinguish this condition from an absent plan. +- **AC-TASKS-PLAN-SAFE-001.6:** Initial creation shall work without an edit version. A concurrent creation shall not turn that request into an unchecked replacement. + +### REQ-TASKS-PLAN-SAFE-002: Detect intervening edits + +#### Acceptance criteria + +- **AC-TASKS-PLAN-SAFE-002.1:** A plan read shall return exact content and its edit version from the same committed state. +- **AC-TASKS-PLAN-SAFE-002.2:** Every successful content or title write shall produce a new edit version, including identical writes, history coalescing, and browser writes. +- **AC-TASKS-PLAN-SAFE-002.3:** Versions shall survive restarts and remain distinct after plan deletion and recreation. A rejected write shall not change the version. +- **AC-TASKS-PLAN-SAFE-002.4:** Concurrent agent edits with one expected version shall admit at most one write. Other callers shall receive a conflict with no mutation. +- **AC-TASKS-PLAN-SAFE-002.5:** Plan comments and implementation-start markers shall remain intact after edits and restores. Marker-only changes shall not invalidate an edit version. + +### REQ-TASKS-PLAN-SAFE-003: Edit one fragment without resending the plan + +#### Acceptance criteria + +- **AC-TASKS-PLAN-SAFE-003.1:** An exact edit shall require the current edit version and a nonempty fragment that occurs exactly once. +- **AC-TASKS-PLAN-SAFE-003.2:** A successful exact edit shall change only that fragment. It shall preserve every other byte and the title. +- **AC-TASKS-PLAN-SAFE-003.3:** Missing, ambiguous, or stale matches shall reject the edit without mutation. No fuzzy matching or whole-document fallback shall occur. +- **AC-TASKS-PLAN-SAFE-003.4:** Exact edits shall obey the composed-content size limit and suspicious-reduction protection. An empty replacement fragment shall permit deletion. +- **AC-TASKS-PLAN-SAFE-003.5:** Append shall retain its existing composition and retry rules. An optional edit version shall reject a stale append when supplied. `allow_truncation=true` shall be rejected as inapplicable without reduction details. + +### REQ-TASKS-PLAN-SAFE-004: Read and restore exact revisions through agent tools + +#### Acceptance criteria + +- **AC-TASKS-PLAN-SAFE-004.1:** An authorized agent shall list revision metadata in bounded pages and fetch a selected revision's exact title and content. +- **AC-TASKS-PLAN-SAFE-004.2:** A restore shall require the current edit version and the selected revision's snapshot version. A changed source or destination shall reject the restore. +- **AC-TASKS-PLAN-SAFE-004.3:** Restore shall accept only a revision belonging to the addressed task. It shall create a separate, agent-attributed revision with a source reference. +- **AC-TASKS-PLAN-SAFE-004.4:** A restore shall preserve the previous HEAD and source revision. Missing or unverifiable history shall reject the request without mutation. +- **AC-TASKS-PLAN-SAFE-004.5:** Agent restore shall require an existing plan. Missing plans and revisions shall return distinct outcomes without creating replacement content. +- **AC-TASKS-PLAN-SAFE-004.6:** Unauthorized requests shall reveal no plan content, revision metadata, or version tokens. Tools shall retain existing task reach restrictions. +- **AC-TASKS-PLAN-SAFE-004.7:** Stored oversized revisions shall remain readable and restorable under the existing historical-content exemption. + +### REQ-TASKS-PLAN-SAFE-005: Guide correction without a forced task stop + +#### Acceptance criteria + +- **AC-TASKS-PLAN-SAFE-005.1:** Rejections shall carry a stable reason, an explicit no-write result, and a correction action. They shall not report lost content. +- **AC-TASKS-PLAN-SAFE-005.2:** Tool guidance shall direct routine correction and continuation. It shall forbid reconstruction from memory and automatic truncation overrides merely to clear an error. +- **AC-TASKS-PLAN-SAFE-005.3:** Recovery guidance shall require an identified source and the absence of intervening edits before automatic repair of the agent's own overwrite. +- **AC-TASKS-PLAN-SAFE-005.4:** A conflict shall direct a fresh read and reconciliation. Unresolved intent or conflicting user changes shall require user input, not a blind restore. +- **AC-TASKS-PLAN-SAFE-005.5:** Successful write acknowledgements shall report the new edit version without echoing the full plan. Explicit reads shall preserve exact content. +- **AC-TASKS-PLAN-SAFE-005.6:** A lost response shall not justify a blind append or a refreshed-token retry. The agent shall inspect current state before another mutation. +- **AC-TASKS-PLAN-SAFE-005.7:** Agent recovery tools shall be available on each existing surface that permits plan writes. Read-only or excluded surfaces shall gain no write authority. + +## Relationship to existing contracts + +This is the successor to the agent safety policy in +[plan write consistency](plan-write-consistency.md) and +[append mode](plan-write-append-mode.md). The affected agent clauses now use +this contract; the older documents remain the traceability source. +The [design](../system-design/plan-safe-edits.md#contract-transition) identifies each replaced clause. + +The implementation is complete. Agent plan tools use this contract; browser +editing keeps its existing interaction and payload behavior. +Append composition, authorization, history serialization, and the +[size limit](plan-content-size-limit.md) retain their owners. + +## Out of scope + +- Browser conflict controls, layout changes, and browser truncation policy. +- Automatic task restart, model changes, or guarantees that an LLM always continues. +- Markdown heading parsers, fuzzy patches, multi-edit batches, or automatic plan summarization. +- Append deduplication, revision retention changes, and multi-document migration. +- Restoration of the incident task during this planning work. + +## Implementation Plans + +- [Safe agent plan edits](../../../plans/plan-safe-edits/plan.md) diff --git a/docs/specs/tasks/requirements/plan-write-append-mode.md b/docs/specs/tasks/requirements/plan-write-append-mode.md index f0d47ccf56..5a51328054 100644 --- a/docs/specs/tasks/requirements/plan-write-append-mode.md +++ b/docs/specs/tasks/requirements/plan-write-append-mode.md @@ -8,16 +8,16 @@ owners: # Task plan append-mode write Requirements +## Implemented agent safety extension + +The implemented [safe agent edits contract](plan-safe-edits.md) adds conditional +replacement, edits, and revision recovery. [Transition matrix](../system-design/plan-safe-edits.md#contract-transition) +identifies clauses superseded for agent writes. + ## Overview -`update_task_plan_kandev` replaces the whole plan document. There is no partial write at -any layer, so a caller adding a section must read the whole plan and send it back with -the addition attached. One write therefore costs the size of the entire plan, and a run -costs the square of the number of writes in it. - -The plan is also a task's only durable memory across a context reset, so it grows -monotonically for as long as the task runs. On this instance the largest stored revision -is 158,393 characters, so the two effects compound in production, not in theory. +Whole-document replacement requires the caller to resend existing content for +each addition. Append avoids that repeated input and the risk of omitted sections. This capability adds a second write mode in which the caller submits only the fragment it wants to add and the system composes the stored document. It changes what a caller @@ -274,10 +274,10 @@ of a list. after the write rather than the size of the submitted fragment. - **AC-TASKS-PLAN-APPEND-006.7:** The truncation warning shall not state or imply that no partial update or append mode exists. -- **AC-TASKS-PLAN-APPEND-006.8:** The truncation warning shall name `append` as the way - to add a section without resubmitting the whole document, while continuing to state - where the pre-write content lives, that the MCP plan tools cannot fetch it, and that - the caller must not rewrite the plan from memory. +- **AC-TASKS-PLAN-APPEND-006.8:** A legacy truncation warning shall name `append` as + the way to add a section without resubmitting the whole document. When the current + MCP surface exposes revision tools, it shall direct the caller to list and fetch + the task-scoped revision, and it shall not direct the caller to rewrite from memory. - **AC-TASKS-PLAN-APPEND-006.9:** No agent-facing text describing a plan write shall state or imply that no partial update or append mode exists, or direct a caller who wants to add a section to read the plan and resubmit it. This binds every tool and @@ -331,5 +331,6 @@ would let a small fragment commit an oversized document, defeating an existing l a store for it — a separate contract. - **A mode on `create_task_plan_kandev` or on the browser write path.** Fixed by AC-TASKS-PLAN-APPEND-005.2 and AC-TASKS-PLAN-APPEND-005.3. -- **Reading a past revision through MCP.** Unchanged: no plan tool returns one. +- **Reading a past revision through MCP.** Owned by the implemented safe agent plan + edits contract. Its task-scoped revision tools do not change browser behavior. - **Prepend, or insertion at any position other than the end.** diff --git a/docs/specs/tasks/requirements/plan-write-consistency.md b/docs/specs/tasks/requirements/plan-write-consistency.md index ef48666beb..da132bf0e2 100644 --- a/docs/specs/tasks/requirements/plan-write-consistency.md +++ b/docs/specs/tasks/requirements/plan-write-consistency.md @@ -8,17 +8,19 @@ owners: # Task plan write consistency Requirements +## Implemented agent safety successor + +The implemented [safe agent edits contract](plan-safe-edits.md) adds preventive +rejection and conditional recovery for agent writes. Its +[transition matrix](../system-design/plan-safe-edits.md#contract-transition) +identifies the affected clauses and preserves browser behavior. + ## Overview -A task plan write reads the current content and latest revision. It decides if the -write truncates content and if it can coalesce. These reads and the commit are not -serialized, so another writer can change the state before the commit. - -This capability defines what a plan write must guarantee about the state it reports -on and the state it commits against. The `tasks` system owns it because the durable -artifact is the plan and its revision history, not the MCP surface that exposes the -warning. The missing-task boundary is owned by `REQ-TASKS-DOCUMENTS-001` and is -unchanged here. +A task plan write reads current content and history before it decides truncation +and coalescing. This contract serializes those decisions with the commit. +The tasks system owns the durable plan and its history. +The missing-task boundary remains under `REQ-TASKS-DOCUMENTS-001`. ## Terminology diff --git a/docs/specs/tasks/system-design/plan-safe-edits.md b/docs/specs/tasks/system-design/plan-safe-edits.md new file mode 100644 index 0000000000..665f7388e1 --- /dev/null +++ b/docs/specs/tasks/system-design/plan-safe-edits.md @@ -0,0 +1,250 @@ +--- +status: current +system: tasks +created: 2026-09-16 +requirements: + - REQ-TASKS-PLAN-SAFE-001 + - REQ-TASKS-PLAN-SAFE-002 + - REQ-TASKS-PLAN-SAFE-003 + - REQ-TASKS-PLAN-SAFE-004 + - REQ-TASKS-PLAN-SAFE-005 +owners: + - kandev +--- + +# Safe agent plan edits System Design + +## Purpose and evidence + +The September 16 incident replaced 16,332 characters with an 807-character checklist. +Raw ACP and backend records both contained an explicit `mode="replace"` request. +The server saved the request, preserved revision 3, and instructed the agent to stop. +Compaction preceded the request. The evidence does not establish that compaction caused it. + +This design changes agent admission and recovery within the tasks system. +It reuses `PlanService`, `planLockTable`, `WritePlanRevision`, and the existing history tables. +It adds no browser controls and no automatic session continuation mechanism. + +## Requirement mapping + +| Requirement | Design section | +| --- | --- | +| `REQ-TASKS-PLAN-SAFE-001` | Admission and history preservation | +| `REQ-TASKS-PLAN-SAFE-002` | Edit versions | +| `REQ-TASKS-PLAN-SAFE-003` | Exact edit | +| `REQ-TASKS-PLAN-SAFE-004` | Revision recovery | +| `REQ-TASKS-PLAN-SAFE-005` | Agent results and continuation | + +## Components + +- `internal/task/service` owns authorization, state reads, version comparison, composition, truncation policy, and recovery eligibility. +- `internal/task/repository/sqlite` owns durable versions and atomic HEAD/history writes for SQLite and PostgreSQL. +- `internal/mcp/handlers` selects the agent policy and maps typed outcomes through `internal/task/planws`. +- `internal/mcp/server` owns tool schemas, forwarding, capability exposure, and compact agent results. +- `pkg/websocket` owns added action names. Existing browser actions keep their payloads and interaction behavior. + +The service and repository methods below are implemented. Existing method names +identify the integration points that remain shared with browser plan behavior. + +## Edit versions + +Add `task_plans.write_version TEXT NOT NULL DEFAULT ''` through an idempotent migration. +Backfill empty values with unique UUIDs through a portable Go migration loop. +The migration runs before reads become available and does not change content, history, or timestamps. +Add the field to `models.TaskPlan` and repository scans. Expose it in an MCP-specific response wrapper as `version`. +The browser DTO does not need a new public field. + +Generate a fresh UUID for each successful content/title write in the repository transaction. +Cover `CreateTaskPlan`, `UpdateTaskPlan`, and `WritePlanRevision`/`upsertPlanHead`, including browser and revert callers. +Generate versions for identical-content writes too. Rollback must roll back the version with HEAD and history. +Do not derive versions from timestamps, revision numbers, or content hashes. +Coalescing mutates an existing revision, and content can return to a previous value. +Deletion followed by recreation receives a fresh UUID. Marker and comment operations preserve the existing version. + +Add a coherent service read for agent plan snapshots. +Read HEAD and its version together under the existing per-task lock, after authorization. +Return exact content, title, task ID, version, and current revision metadata when verified against HEAD. +Do not label a mismatched latest revision as the current snapshot. + +Version comparison and the write occur within the same existing service critical section. +The supported concurrency boundary remains one backend process with its shared `PlanService` instance. +This package does not introduce cross-process plan writers or a second lock table. +Repository PostgreSQL transactions retain the task locking used by `WritePlanRevision`. + +## Admission and history preservation + +Extend agent create/update requests with optional schema fields `expected_version` and `allow_truncation` (default false). +They remain schema-optional because first creation and ordinary append need no version. +The service requires a nonempty, matching version for replacement of an existing plan. +The create tool remains an upsert only when that precondition passes. This closes the alternative overwrite path. +If no plan exists, creation without a token succeeds. A supplied stale token must not create a new plan. + +The MCP adapter selects a trusted agent policy. A caller cannot disable it through a payload field. +Browser writes retain their existing admission policy but always advance the durable version. +Agent admission fails closed on an unreadable HEAD. Browser read-failure behavior remains under its current contract. + +Validation order for new agent writes: + +1. Validate argument types and mode names without accessing task state. +2. Authorize the addressed task. +3. Validate required content and standalone replacement size under existing rules. +4. Acquire the task lock and read HEAD through the existing tri-state read. +5. Reject missing state or read failure, as applicable to the requested operation. +6. Enforce the version precondition. +7. Compose append or exact-edit content and enforce its final byte size. +8. Evaluate suspicious reduction and explicit acknowledgement. +9. Verify preservation, commit HEAD/history/version, release, and publish existing events. + +Reuse `planTruncationDetected`: prior length at least 2,000 Unicode code points and retained length strictly below 50 percent. +Never detect truncation from an append fragment alone. +An unacknowledged reduction returns `plan_truncation_rejected` before any mutation. +An acknowledged reduction requires a latest revision whose title/content match HEAD. +An unreadable or divergent history returns `plan_history_unavailable` without mutation. +Force a new revision for an acknowledged reduction. Do not coalesce away the preserved predecessor. +This choice avoids a speculative repair mechanism for historically divergent HEAD/history pairs. + +Every rejection preserves title, content, version, revision timestamps/counts, comments, and implementation markers. +It emits no plan or revision mutation event. A later valid write must still acquire the lock and succeed. + +## Exact edit + +Add `edit_task_plan_kandev(task_id?, expected_version, old_text, new_text, allow_truncation?)`. +Use a dedicated tool instead of extending append-mode parsing or introducing Markdown section identities. +`old_text` must be nonempty. `new_text` is required but can be empty. +Both values represent exact UTF-8 text. There is no whitespace normalization. + +Within the task lock, count all literal occurrences, including overlapping occurrences. +Zero occurrences return `plan_edit_not_found`. More than one returns `plan_edit_ambiguous`. +Replace the unique byte span and preserve all surrounding bytes and the title. +Use the same version, final-size, truncation, revision, attribution, and event path as an agent update. +Reject a final empty plan under the existing agent empty-content rule. +An identical fragment replacement is an ordinary successful write with a new version. + +The tool describes checklist updates as its primary example. +New sections use `update_task_plan_kandev(mode="append")`. +Append retains its separator and non-idempotence rules. It checks `expected_version` only when supplied. +Reject `allow_truncation=true` on append as inapplicable instead of suggesting that append can remove content. + +## Revision recovery + +Add these task-scoped tools: + +| Tool | Arguments beyond optional `task_id` | Result | +| --- | --- | --- | +| `list_task_plan_revisions_kandev` | `before_revision_number?`, `limit?` | Metadata page, no content, next cursor | +| `get_task_plan_revision_kandev` | `revision_id` | Exact title/content and `revision_version` | +| `restore_task_plan_revision_kandev` | `revision_id`, `expected_revision_version`, `expected_version` | Compact write acknowledgement | + +List defaults to 20 entries and permits 1 through 100. +Use descending revision numbers and an exclusive positive integer cursor. +Add a bounded repository metadata query. Do not load all content with `ListRevisions` and discard it afterward. +Metadata includes ID, number, author, timestamps, revert source, and byte size. +It does not claim immutable snapshot identity. The caller fetches the selected revision before restore. + +`revision_version` is an opaque SHA-256 digest of canonical length-prefixed fields: +revision ID, task ID, title, content, and canonical `updated_at`. +It detects changes to the selected snapshot during coalescing. It is not an authorization credential. +Fetch selected revisions with a task-scoped query after task authorization. +An ID from another task returns not found without revealing that other task. + +Use the same atomic `WritePlanRevision` and event sequence as the existing revert +implementation. Keep agent preflight separate because it holds the non-reentrant task +lock; do not call `RevertPlan` from another method that already holds that lock. +The agent branch requires existing, readable HEAD, matching `expected_version`, and matching source `revision_version`. +It also verifies that the latest revision preserves current HEAD before restoring. +Both source and destination checks occur under the task lock before the write. +Copy the source title/content exactly. Write a new revision with `revert_of_revision_id` and agent attribution from trusted session context. +Preserve current implementation markers and comments. The browser branch retains user attribution and its existing absent-HEAD behavior. +Restoration retains the existing oversized-history exemption and needs no truncation override. +An already-current target can return `already_current` with no mutation when title/content and both expected versions match. + +## Agent results and continuation + +Use MCP error results for rejected writes. Preserve structured reason data through the WebSocket bridge. +Render a concise text equivalent for agents that consume only text blocks. +Implemented fields include `code`, `operation`, `task_id`, `write_applied=false`, +`next_action`, and authorized current-version/count metadata. +Never include plan content in an error or a successful write acknowledgement. + +| Reason | Correction guidance | +| --- | --- | +| `plan_version_required` | Read the current plan, then submit the intended edit with its version. | +| `plan_version_conflict` | Read and reconcile current content. Do not retry the old body with a new token. | +| `plan_truncation_rejected` | Plan unchanged. Use exact edit or append. Acknowledge only a deliberate reduction within the user's request. | +| `plan_append_truncation_not_applicable` | Plan unchanged. Retry append without `allow_truncation`; use exact edit for a local change that removes content. | +| `plan_edit_not_found` / `plan_edit_ambiguous` | Read current content and select a unique exact fragment. | +| `plan_revision_changed` | Fetch the selected revision again before deciding whether to restore it. | +| `plan_history_unavailable` / read failure | Do not invent recovery data or recreate the plan. Report the unresolved storage condition if it persists. | + +`get_task_plan_kandev` must stop discarding metadata in `getTaskPlanHandler`. +Return a metadata text block plus the unchanged content text block, with equivalent structured fields where supported. +`planWriteAck` returns task ID, title, stored byte length, and the committed version. +The returned version comes from the committed write, including the post-write read-failure fallback. +Schema and forwarding tests must prove that each new field reaches its service consumer. + +Guidance permits correction within the same turn. No server response requests a task stop for a rejected write that changed nothing. +The server does not automatically relaunch, prompt, or complete the session. +LLM continuation is guidance, not a deterministic product guarantee. + +Automatic restore guidance applies only to an agent's identified accidental write. +The agent needs the successful write's returned version and an exact, identified recovery source. +If current version differs, it must reconcile instead of replacing intervening work. +Legacy incidents without that evidence require review of current state and explicit restoration intent. +A list order or the label “previous revision” alone is not enough to select a recovery target. + +A transport error does not establish whether a write committed. +After a lost response, the agent reads current state before deciding its next action. +An old expected token rejects a repeated edit/restore after success. Append remains non-idempotent without a supplied version. +No retry loop automatically refreshes a version and resends an old body. + +## Security and exposure + +Register each tool wherever `registerPlanTools` currently provides plan writes, including applicable Office task surfaces. +Preserve external, configuration, and automation exclusions according to the current profile contracts. +Register backend actions with the guarded MCP dispatcher and existing task-reach checks. +Tool-supplied task IDs never replace trusted session identity for authorization or attribution. +The selected revision must belong to the addressed task, even when the caller can access both tasks. + +## Migration and observability + +Use replayable migrations for both database dialects and update the required-store upgrade fixture when schema history changes. +The token backfill does not rewrite documents or create revisions. +Old clients lacking tokens receive a corrective rejection for replacements of existing plans. +There is no compatibility bypass. Existing initial-create and tokenless append calls continue to work. +The existing runtime registers these schemas on agent startup. Cached clients can refresh discovery or resume with current tools. + +Use bounded structured logs for rejected operations: task/session correlation, operation, reason, and character counts. +Do not add plan content, replacement fragments, or revision bodies to logs. +An error result distinguishes rejection from storage failure and uncertain transport completion. +This package adds no metrics endpoint or feature toggle. + +## Contract transition + +The following changes apply only when this package is implemented: + +- Consistency `001.1` through `001.5` and `001.7` become pre-write rejection or acknowledged-write preservation on agent paths. +- Consistency `001.6` and `001.9` no longer permit an unreadable HEAD on agent replacement. Browser behavior remains unchanged. +- Consistency `003.4`, `004.1`, `004.2`, `005.1`, and `005.2` gain the agent preconditions and MCP metadata specified here. +- Append `005.1` no longer freezes agent replacement admission. Append `006.2` and `006.8` use preventive guidance and available revision tools. +- Append `006.9` retains its append guidance but describes create-overwrite preconditions. New exact-edit tooling does not change append parsing. +- Append `001.7` keeps existing mode/authorization precedence, with new optional version checks at the defined state-check stage. +- Size-limit `001.8` remains subject to the new agent preconditions. Historical restore exemptions remain unchanged. + +The implementation must update affected clauses and tests, rather than keeping two active contradictory policies. +Existing companion designs remain the source for browser behavior, append composition, size limits, and serialization. +No linked companion implementation package was found in those plan-write specifications at planning time. + +## Verification strategy + +Use repository migration and rollback tests, deterministic service concurrency tests, and real MCP-to-service integration tests. +The incident regression seeds a long plan, appends a checklist, attempts fragment replacement, then corrects one checkbox through exact edit. +Assert unchanged content/history after rejection, preserved review sections after correction, and no forced workflow transition. +Recovery tests cover exact restoration, mutable revision targets, intervening browser edits, authorization, and unchanged markers/comments. +The work orders define the exact commands and test locations. The implementation +and verification are recorded in the completed work orders. + +## Related decisions and implementation + +- [Conditional agent plan writes](../../../decisions/2026-09-16-conditional-agent-plan-writes.md) +- [Implementation-start marker](../../../decisions/0033-durable-plan-implementation-start.md) +- [Implementation package](../../../plans/plan-safe-edits/plan.md) diff --git a/docs/specs/tasks/system-design/plan-write-append-mode-agent-text.md b/docs/specs/tasks/system-design/plan-write-append-mode-agent-text.md index 98aa385d37..6c2ff01982 100644 --- a/docs/specs/tasks/system-design/plan-write-append-mode-agent-text.md +++ b/docs/specs/tasks/system-design/plan-write-append-mode-agent-text.md @@ -10,6 +10,12 @@ owners: # Task plan append-mode agent text System Design +## Implemented recovery extension + +The implemented [safe agent edits design](plan-safe-edits.md) replaces post-write stop +guidance with preventive rejection and revision recovery tools. The implemented +contract transition applies to agent writes. Existing append guidance remains. + ## Purpose and boundaries This design owns `REQ-TASKS-PLAN-APPEND-006`: every agent-facing string that describes a @@ -88,14 +94,10 @@ it back toward the replace path that caused the loss. `AC-TASKS-PLAN-APPEND-006. `006.8` require that sentence replaced by one naming `append` as the way to add a section without resubmitting the document. -Everything else in that warning must survive, for reasons recorded in its doc comment -rather than obvious: it deliberately does not tell the caller to "recover" the content -through an MCP tool, because no plan tool reads a past revision and -`get_task_plan_kandev` would hand back the truncated document; it names where the prior -content lives; and it tells the caller not to rewrite from memory. Naming `append` -strengthens that advice rather than competing with it — the caller is told what to do -instead of what just failed. Keep the rune counts, the percentage, the two branches on -whether the prior revision number is known, and the response field. +The legacy warning remains only for compatibility with non-guarded callers. Current +agent writes reject suspicious reductions before storage and expose task-scoped +revision list/read tools. Keep the rune counts, percentage, append guidance, and +the instruction not to rewrite from memory. This is Go string content, not localized UI copy, so the i18n rules do not reach it. `AC-TASKS-PLAN-APPEND-005.1` excludes this wording from the replace-mode freeze @@ -103,11 +105,11 @@ precisely so the two criteria do not contradict each other. ### Acknowledgement -The acknowledgement needs **no change**, verified rather than assumed. `planWriteAck` +The acknowledgement also reports the committed edit version. `planWriteAck` in `internal/mcp/server/handlers.go` already prefers the stored `content` length over what the call sent, renders it as bytes, and already appends a line stating that plan -content is omitted and should be read back with `get_task_plan_kandev`. That is exactly -`AC-TASKS-PLAN-APPEND-006.6`, satisfied by existing behavior on both modes. +content is omitted and should be read back with `get_task_plan_kandev`. Append keeps +the stored-size behavior from `AC-TASKS-PLAN-APPEND-006.6`. Build must not add plan content to that acknowledgement. Doing so would return the accumulated document on every append and give back the response-leg saving this diff --git a/docs/specs/tasks/system-design/plan-write-consistency.md b/docs/specs/tasks/system-design/plan-write-consistency.md index 7f8bc37676..3e6edced6f 100644 --- a/docs/specs/tasks/system-design/plan-write-consistency.md +++ b/docs/specs/tasks/system-design/plan-write-consistency.md @@ -14,6 +14,12 @@ owners: # Task plan write consistency System Design +## Implemented agent safety successor + +The implemented [safe agent edits design](plan-safe-edits.md) adds durable edit versions +and preventive agent admission. Its contract transition names the changes to +this design. Existing serialization and browser behavior remain dependencies. + ## Purpose and boundaries This design closes the window between a plan read and its commit. It does not @@ -528,12 +534,8 @@ one whose HEAD read is unknown proceeds; and a revert to the latest revision forced to interleave with a same-author coalescing write commits the target content, not the content that write merged in. -## Implementation note: starting state +## Implementation state -The parent card calls a related change "already implemented and merged". It is not -merged: the work sits only on `feature/plan-truncation-guar-qd5`, which has been -rebuilt and extended since, so identify it by subject rather than by commit id. On -`origin/main` `evaluatePlanWriteGuard` still calls `PlanService.ListRevisions`, there -is no `GetLatestRevision`, and the false `ListRevisions` doc comment remains. It touches `task_plan_guard.go` and -`plan_service.go`, which this work also touches: confirm its merge state and rebase -rather than reimplement. +As of 2026-09-16, the service owns truncation detection and the per-task lock. +`GetLatestRevision` exists. Historical starting-state descriptions in this design +explain the original change, not the current checkout.