From 98b255dbba168e5b80721fc68097dff08d07d46f Mon Sep 17 00:00:00 2001 From: Henry Date: Mon, 24 Aug 2026 14:24:56 +0000 Subject: [PATCH] fix(mcp): stop task plan writes from silently erasing prior content (#2977) Co-authored-by: nova28 <17953305+nova28@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 Co-authored-by: Carlos Florencio --- .../backend/internal/mcp/handlers/handlers.go | 24 +- .../internal/mcp/handlers/task_plan_guard.go | 166 +++++++ .../mcp/handlers/task_plan_guard_test.go | 451 ++++++++++++++++++ .../mcp/server/compact_results_test.go | 42 ++ apps/backend/internal/mcp/server/handlers.go | 7 +- apps/backend/internal/mcp/server/server.go | 8 +- .../internal/task/service/plan_service.go | 35 +- .../task/service/plan_service_test.go | 34 ++ 8 files changed, 738 insertions(+), 29 deletions(-) create mode 100644 apps/backend/internal/mcp/handlers/task_plan_guard.go create mode 100644 apps/backend/internal/mcp/handlers/task_plan_guard_test.go diff --git a/apps/backend/internal/mcp/handlers/handlers.go b/apps/backend/internal/mcp/handlers/handlers.go index 6c797df0e0..bcd7a05bc3 100644 --- a/apps/backend/internal/mcp/handlers/handlers.go +++ b/apps/backend/internal/mcp/handlers/handlers.go @@ -3873,17 +3873,19 @@ func (h *Handlers) handleCreateTaskPlan(ctx context.Context, msg *ws.Message) (* createdBy = "agent" } + guard := h.evaluatePlanWriteGuard(ctx, req.TaskID, req.Content) plan, err := h.planService.CreatePlan(ctx, service.CreatePlanRequest{ - TaskID: req.TaskID, - Title: req.Title, - Content: req.Content, - CreatedBy: createdBy, + TaskID: req.TaskID, + Title: req.Title, + Content: req.Content, + CreatedBy: createdBy, + ForceNewRevision: guard.forceNewRevision, }) if err != nil { return planws.CreateError(msg, err) } - return ws.NewResponse(msg.ID, msg.Action, dto.TaskPlanFromModel(plan)) + return ws.NewResponse(msg.ID, msg.Action, planWritePayload(dto.TaskPlanFromModel(plan), guard)) } // handleGetTaskPlan retrieves a task plan. @@ -3925,17 +3927,19 @@ func (h *Handlers) handleUpdateTaskPlan(ctx context.Context, msg *ws.Message) (* createdBy = "agent" } + guard := h.evaluatePlanWriteGuard(ctx, req.TaskID, req.Content) plan, err := h.planService.UpdatePlan(ctx, service.UpdatePlanRequest{ - TaskID: req.TaskID, - Title: req.Title, - Content: req.Content, - CreatedBy: createdBy, + TaskID: req.TaskID, + Title: req.Title, + Content: req.Content, + CreatedBy: createdBy, + ForceNewRevision: guard.forceNewRevision, }) if err != nil { return planws.UpdateError(msg, err) } - return ws.NewResponse(msg.ID, msg.Action, dto.TaskPlanFromModel(plan)) + return ws.NewResponse(msg.ID, msg.Action, planWritePayload(dto.TaskPlanFromModel(plan), guard)) } // handleDeleteTaskPlan deletes a task plan. diff --git a/apps/backend/internal/mcp/handlers/task_plan_guard.go b/apps/backend/internal/mcp/handlers/task_plan_guard.go new file mode 100644 index 0000000000..d565085672 --- /dev/null +++ b/apps/backend/internal/mcp/handlers/task_plan_guard.go @@ -0,0 +1,166 @@ +package handlers + +import ( + "context" + "fmt" + "unicode/utf8" + + "github.com/kandev/kandev/internal/task/dto" +) + +const ( + // planTruncationMinPriorChars is the floor below which a shrinking write + // isn't worth flagging. A 300-char stub plan losing half its content is + // not a review-round-costing event; both real WO-38 incidents were on + // 40k+ char plans. + planTruncationMinPriorChars = 2000 + + // planTruncationMaxRetainRatio: this workflow's normal write is an + // append ("appends its own section"), so a plan's content grows + // monotonically across a build. A write that keeps less than half of the + // prior document is anomalous by construction, not a normal edit. Both + // real WO-38 incidents retained 22.8% and 23.9% of the prior plan — this + // line catches both with wide margin. + planTruncationMaxRetainRatio = 0.5 +) + +// planTruncationDetected reports whether newContent looks like an accidental +// destructive truncation of priorContent, rather than a deliberate edit or a +// legitimate (smaller) prune. +// +// Measured in runes, not bytes: len() on a Go string counts UTF-8 bytes, so a +// script change (e.g. an ASCII plan rewritten in CJK) can retain a small +// fraction of the document's characters while retaining most of its bytes — +// silently defeating this guard on exactly the kind of loss it exists to +// catch. Kandev ships zh-cn/zh-hk/zh-tw/pt-pt locales, so non-ASCII plan +// content is not hypothetical. +func planTruncationDetected(priorContent, newContent string) bool { + priorLen := utf8.RuneCountInString(priorContent) + if priorLen < planTruncationMinPriorChars { + return false + } + return float64(utf8.RuneCountInString(newContent)) < float64(priorLen)*planTruncationMaxRetainRatio +} + +// planTruncationWarning renders the agent-facing warning appended to a plan +// write's tool result when planTruncationDetected reports true. It states +// plainly that the write replaced the entire document — update_task_plan_kandev +// and create_task_plan_kandev have no partial-update mode — and names the +// prior revision number when it is known. +// +// priorRevisionNumber of 0 means the prior revision could not be established +// (the revision-history lookup itself failed after truncation was already +// detected off the plan content). Revision numbering starts at 1 +// (NextTaskPlanRevisionNumber), so 0 is never a real revision — in that case +// the "in plan revision N" clause is omitted rather than naming a revision +// that cannot exist. +// +// 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. +func planTruncationWarning(priorContent, newContent string, priorRevisionNumber int) string { + priorLen := utf8.RuneCountInString(priorContent) + newLen := utf8.RuneCountInString(newContent) + // dropped is always >= 0: the only caller, evaluatePlanWriteGuard, invokes this + // after planTruncationDetected has already confirmed newLen < priorLen. + dropped := priorLen - newLen + droppedPct := float64(dropped) / float64(priorLen) * 100 + + preservedIn := "the task's plan revision history" + if priorRevisionNumber > 0 { + preservedIn = fmt.Sprintf("plan revision %d, in the task's plan revision history", priorRevisionNumber) + } + + return fmt.Sprintf( + "WARNING: this write replaced %d chars with %d (dropped %d chars, %.0f%%). "+ + "Plan writes REPLACE THE ENTIRE DOCUMENT — there is no partial update or append "+ + "mode. 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.", + priorLen, newLen, dropped, droppedPct, preservedIn, + ) +} + +// planWriteGuardResult carries the truncation-guard outcome for a plan +// create/update: whether the underlying revision write must be forced to +// append rather than coalesce, and the warning text (if any) to surface in +// the tool response. +type planWriteGuardResult struct { + forceNewRevision bool + warning string + priorRevision int +} + +// evaluatePlanWriteGuard compares a task's current plan content against an +// incoming write and decides whether it looks like an accidental +// whole-document truncation. It covers both create_task_plan_kandev (which +// upserts, so a create over an existing plan is the same destructive write +// through a different door) and update_task_plan_kandev. +// +// A failure to fetch the current plan is non-fatal: the write proceeds +// without a truncation warning, but it forces a new revision so a later +// successful write cannot coalesce into an unknown prior revision. A failure +// to list the revision history is handled differently: +// truncation has already been detected from the plan content at that point, +// so the guard still forces a new revision and still warns — it renders the +// warning without a specific revision number (see planTruncationWarning) +// instead of silently dropping the warning, because clearing +// forceNewRevision here would let this destructive write coalesce into, and +// overwrite, the only surviving copy of the pre-truncation content. +func (h *Handlers) evaluatePlanWriteGuard(ctx context.Context, taskID, newContent string) planWriteGuardResult { + existing, err := h.planService.GetPlan(ctx, taskID) + if err != nil { + return planWriteGuardResult{forceNewRevision: true} + } + if existing == nil { + return planWriteGuardResult{} + } + if !planTruncationDetected(existing.Content, newContent) { + return planWriteGuardResult{} + } + + priorRevisionNumber := 0 + if revisions, revErr := h.planService.ListRevisions(ctx, taskID); revErr == nil && len(revisions) > 0 { + priorRevisionNumber = revisions[0].RevisionNumber + } + + return planWriteGuardResult{ + forceNewRevision: true, + warning: planTruncationWarning(existing.Content, newContent, priorRevisionNumber), + priorRevision: priorRevisionNumber, + } +} + +// planWriteResponse extends the standard plan DTO with a truncation warning +// for the MCP write actions only. 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 planWriteResponse struct { + *dto.TaskPlanDTO + PlanWriteWarning string `json:"plan_write_warning,omitempty"` + PriorRevisionNumber int `json:"prior_revision_number,omitempty"` +} + +// planWritePayload wraps plan in a planWriteResponse when guard carries a +// warning, otherwise returns plan unwrapped so an unaffected write's +// response shape is unchanged. +func planWritePayload(plan *dto.TaskPlanDTO, guard planWriteGuardResult) interface{} { + if guard.warning == "" { + return plan + } + return planWriteResponse{ + TaskPlanDTO: plan, + PlanWriteWarning: guard.warning, + PriorRevisionNumber: guard.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 new file mode 100644 index 0000000000..8045ab8e6b --- /dev/null +++ b/apps/backend/internal/mcp/handlers/task_plan_guard_test.go @@ -0,0 +1,451 @@ +package handlers + +import ( + "context" + "encoding/json" + "errors" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/jmoiron/sqlx" + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/db" + "github.com/kandev/kandev/internal/events/bus" + "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" +) + +func mustMarshalPlanPayload(t *testing.T, v map[string]any) string { + t.Helper() + data, err := json.Marshal(v) + if err != nil { + t.Fatalf("marshal payload: %v", err) + } + return string(data) +} + +func revisionWithNumber(t *testing.T, revisions []*models.TaskPlanRevision, number int) *models.TaskPlanRevision { + t.Helper() + for _, rev := range revisions { + if rev.RevisionNumber == number { + return rev + } + } + t.Fatalf("no revision with number %d among %d revisions", number, len(revisions)) + return nil +} + +// 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) { + h := newMCPPlanTestHandlers(t) + ctx := context.Background() + + large := strings.Repeat("x", 40000) + small := strings.Repeat("y", 10000) // ~25% retained, matching the WO-38 magnitude + + createOut, err := h.handleCreateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPCreateTaskPlan, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanTaskID, + "title": "Ship it", + "content": large, + }))) + if err != nil { + 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) + } + + updateOut, err := h.handleUpdateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPUpdateTaskPlan, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanTaskID, + "content": small, + }))) + 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"]) + } + + 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)) + } + rev1 := revisionWithNumber(t, revisions, 1) + fullRev1, err := h.planService.GetRevision(ctx, rev1.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", + len(fullRev1.Content), len(large)) + } +} + +// TestMCPPlanTruncationGuard_SmallDropsAreQuiet pins the two false-positive +// guards from the threshold: a small plan (under the 2,000-char floor) and a +// modest, legitimate shrink (well above the 50% retain line) must not warn. +func TestMCPPlanTruncationGuard_SmallDropsAreQuiet(t *testing.T) { + h := newMCPPlanTestHandlers(t) + ctx := context.Background() + + t.Run("small plan under the floor", func(t *testing.T) { + _, err := h.handleCreateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPCreateTaskPlan, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanTaskID, + "content": "a short plan", + }))) + if err != nil { + t.Fatalf("handleCreateTaskPlan: %v", err) + } + out, err := h.handleUpdateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPUpdateTaskPlan, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanTaskID, + "content": "x", + }))) + if err != nil { + t.Fatalf("handleUpdateTaskPlan: %v", err) + } + updated := decodeMCPPlanPayload(t, out) + if warning, ok := updated["plan_write_warning"]; ok { + t.Errorf("unexpected warning for a sub-floor plan: %v", warning) + } + }) + + 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, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanlessID, + "content": large, + }))) + if err != nil { + t.Fatalf("handleCreateTaskPlan: %v", err) + } + out, err := h.handleUpdateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPUpdateTaskPlan, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanlessID, + "content": retained, + }))) + if err != nil { + t.Fatalf("handleUpdateTaskPlan: %v", err) + } + updated := decodeMCPPlanPayload(t, out) + if warning, ok := updated["plan_write_warning"]; ok { + t.Errorf("unexpected warning for a legitimate >50%% retain: %v", warning) + } + }) +} + +// TestMCPPlanTruncationGuard_NonASCIIUsesCharacterCount pins Review round 1 +// Finding 1: len() on a Go string counts UTF-8 bytes, not characters. A plan +// rewritten from ASCII into a CJK script can drop 80% of its characters +// while retaining 60% of its bytes, so a byte-counting guard stays silent on +// a loss larger than either real WO-38 incident. This asserts the guard is +// measured in runes: it must fire on this drop even though the byte ratio +// alone would not cross the threshold. +func TestMCPPlanTruncationGuard_NonASCIIUsesCharacterCount(t *testing.T) { + h := newMCPPlanTestHandlers(t) + ctx := context.Background() + + // 4000 ASCII runes == 4000 bytes. + large := strings.Repeat("x", 4000) + // 800 CJK runes, each 3 UTF-8 bytes == 2400 bytes: 20% of characters + // retained, but 60% of bytes retained — the byte ratio alone would not + // cross the 50% line, but the character ratio must. + small := strings.Repeat("好", 800) + + _, 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) + } + + out, err := h.handleUpdateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPUpdateTaskPlan, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanTaskID, + "content": small, + }))) + 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 !strings.Contains(warning, "4000") || !strings.Contains(warning, "800") { + t.Errorf("warning does not name the character counts (4000 -> 800): %q", warning) + } +} + +// TestMCPPlanTruncationGuard_CreateOverExistingPlanWarns pins the deliberate +// 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) { + h := newMCPPlanTestHandlers(t) + ctx := context.Background() + + large := strings.Repeat("x", 40000) + small := strings.Repeat("y", 10000) + + _, err := h.handleCreateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPCreateTaskPlan, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanlessID, + "content": large, + }))) + if err != nil { + t.Fatalf("handleCreateTaskPlan (initial): %v", err) + } + + out, err := h.handleCreateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPCreateTaskPlan, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanlessID, + "content": small, + }))) + 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") + } +} + +// failingRevisionsRepo wraps the real sqlite repository but forces +// ListTaskPlanRevisions to always fail, simulating a transient lookup +// failure (e.g. SQLITE_BUSY) that happens after GetPlan has already +// succeeded. +type failingRevisionsRepo struct { + *sqlite.Repository + failGetPlan bool +} + +func (r *failingRevisionsRepo) ListTaskPlanRevisions(context.Context, string, int) ([]*models.TaskPlanRevision, error) { + return nil, errors.New("simulated revision lookup failure") +} + +func (r *failingRevisionsRepo) GetTaskPlan(ctx context.Context, taskID string) (*models.TaskPlan, error) { + if r.failGetPlan { + r.failGetPlan = false + return nil, errors.New("simulated plan lookup failure") + } + return r.Repository.GetTaskPlan(ctx, taskID) +} + +// newMCPPlanTestHandlersWithFailingRevisionLookup builds Handlers like +// newMCPPlanTestHandlers, but backed by a plan service whose ListRevisions +// call always fails. +func newMCPPlanTestHandlersWithFailingRevisionLookup(t *testing.T) (*Handlers, *failingRevisionsRepo) { + t.Helper() + dbConn, err := db.OpenSQLite(filepath.Join(t.TempDir(), "test.db")) + if err != nil { + t.Fatalf("OpenSQLite: %v", err) + } + sqlxDB := sqlx.NewDb(dbConn, "sqlite3") + t.Cleanup(func() { + if err := sqlxDB.Close(); err != nil { + t.Errorf("close database: %v", err) + } + }) + repo, err := sqlite.NewWithDB(sqlxDB, sqlxDB, nil) + if err != nil { + t.Fatalf("sqlite.NewWithDB: %v", err) + } + t.Cleanup(func() { + if err := repo.Close(); err != nil { + t.Errorf("close repository: %v", err) + } + }) + + log, err := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "json", OutputPath: "stdout"}) + if err != nil { + t.Fatalf("NewLogger: %v", err) + } + eventBus := bus.NewMemoryEventBus(log) + t.Cleanup(func() { eventBus.Close() }) + + ctx := context.Background() + if err := repo.CreateWorkspace(ctx, &models.Workspace{ID: mcpPlanWS, Name: "Plan WS"}); err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + if err := repo.CreateWorkflow(ctx, &models.Workflow{ID: mcpPlanWF, WorkspaceID: mcpPlanWS, Name: "WF"}); err != nil { + t.Fatalf("CreateWorkflow: %v", err) + } + now := time.Now().UTC() + task := &models.Task{ + ID: mcpPlanTaskID, + WorkspaceID: mcpPlanWS, + WorkflowID: mcpPlanWF, + Title: "Plan target", + State: v1.TaskStateCreated, + Priority: "medium", + CreatedAt: now, + UpdatedAt: now, + } + if err := repo.CreateTask(ctx, task); err != nil { + t.Fatalf("CreateTask: %v", err) + } + + wrapped := &failingRevisionsRepo{Repository: repo} + 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 ListRevisions fails after GetPlan has already detected a +// truncating write, evaluatePlanWriteGuard must still force a new revision +// and still warn — silently returning an empty guard here would let the +// coalescing path 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) { + h, _ := newMCPPlanTestHandlersWithFailingRevisionLookup(t) + ctx := context.Background() + + large := strings.Repeat("x", 40000) + small := strings.Repeat("y", 10000) + + _, 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) + } + + // Check the guard directly before the write mutates the stored plan, so + // "existing" below is still the large content. + guard := h.evaluatePlanWriteGuard(ctx, mcpPlanTaskID, small) + if !guard.forceNewRevision { + t.Error("forceNewRevision must stay true even when the revision lookup fails, " + + "otherwise a truncating write can coalesce into the only surviving revision") + } + if guard.warning == "" { + t.Fatal("expected a truncation warning even when the revision lookup fails, got none") + } + if strings.Contains(guard.warning, "revision 0") { + t.Errorf("warning names a nonexistent revision 0 (revision numbering starts at 1): %q", guard.warning) + } + + out, err := h.handleUpdateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPUpdateTaskPlan, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanTaskID, + "content": small, + }))) + 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 strings.Contains(warning, "revision 0") { + t.Errorf("warning names a nonexistent revision 0 (revision numbering starts at 1): %q", warning) + } + 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"]) + } +} + +func TestMCPPlanTruncationGuard_PlanLookupFailurePreservesHistory(t *testing.T) { + h, repo := newMCPPlanTestHandlersWithFailingRevisionLookup(t) + ctx := context.Background() + + large := strings.Repeat("x", 40000) + small := strings.Repeat("y", 10000) + + _, 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) + } + + repo.failGetPlan = true + + out, err := h.handleUpdateTaskPlan(ctx, mcpPlanMsg(t, ws.ActionMCPUpdateTaskPlan, + mustMarshalPlanPayload(t, map[string]any{ + "task_id": mcpPlanTaskID, + "content": small, + }))) + 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) + } + + 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)) + } + rev1 := revisionWithNumber(t, revisions, 1) + fullRev1, err := repo.GetTaskPlanRevision(ctx, rev1.ID) + if err != nil { + t.Fatalf("GetTaskPlanRevision(rev1): %v", err) + } + if fullRev1.Content != large { + t.Errorf("revision 1 content was mutated after a guarded read failure; got len=%d, want len=%d", + len(fullRev1.Content), len(large)) + } +} diff --git a/apps/backend/internal/mcp/server/compact_results_test.go b/apps/backend/internal/mcp/server/compact_results_test.go index 579f79a133..d08be29514 100644 --- a/apps/backend/internal/mcp/server/compact_results_test.go +++ b/apps/backend/internal/mcp/server/compact_results_test.go @@ -81,6 +81,48 @@ func TestPlanAck_ReportsStoredSizeNotSentSize(t *testing.T) { assert.Contains(t, text, "6 bytes") } +// TestUpdateTaskPlan_SurfacesTruncationWarning pins that planWriteAck relays +// a plan_write_warning field from the backend response into the tool result +// text an agent actually reads. The ws handler is the layer that computes +// this warning (internal/mcp/handlers/task_plan_guard.go); this test only +// pins that the ack layer above it doesn't drop the field on the floor. +func TestUpdateTaskPlan_SurfacesTruncationWarning(t *testing.T) { + backend := &testBackend{response: map[string]interface{}{ + "task_id": "task-A", + "title": "Plan", + "content": "y", + "plan_write_warning": "WARNING: this write replaced 40000 chars with 1 (dropped 39999 chars, 100%). Plan revision 3 has the prior content.", + "prior_revision_number": float64(3), + }} + s := newTaskModeServer(t, backend, "task-A") + + text := planText(t, callTool(t, s, "update_task_plan_kandev", map[string]interface{}{ + "content": "y", + })) + + assert.Contains(t, text, "Plan updated successfully") + assert.Contains(t, text, "WARNING") + assert.Contains(t, text, "40000 chars with 1") + assert.Contains(t, text, "revision 3") +} + +// TestUpdateTaskPlan_NoWarningWhenAbsent pins that the ack stays exactly the +// pre-existing shape when the backend reports no truncation. +func TestUpdateTaskPlan_NoWarningWhenAbsent(t *testing.T) { + backend := &testBackend{response: map[string]interface{}{ + "task_id": "task-A", + "title": "Plan", + "content": "step two", + }} + s := newTaskModeServer(t, backend, "task-A") + + text := planText(t, callTool(t, s, "update_task_plan_kandev", map[string]interface{}{ + "content": "step two", + })) + + assert.NotContains(t, text, "WARNING") +} + func TestCreateTask_ResponseOmitsDescriptionEcho(t *testing.T) { backend := &testBackend{response: map[string]interface{}{ "id": "task-new", diff --git a/apps/backend/internal/mcp/server/handlers.go b/apps/backend/internal/mcp/server/handlers.go index d25d5273b8..7ba1c80ae6 100644 --- a/apps/backend/internal/mcp/server/handlers.go +++ b/apps/backend/internal/mcp/server/handlers.go @@ -1028,8 +1028,11 @@ func planWriteAck(action string, result map[string]interface{}, sentContent stri if updatedAt := stringField(result, "updated_at"); updatedAt != "" { ack += ", updated_at=" + updatedAt } - return mcp.NewToolResultText(ack + - ". Plan content is omitted from this response; read it back with get_task_plan_kandev if needed.") + 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 + } + return mcp.NewToolResultText(ack) } func (s *Server) createTaskPlanHandler() server.ToolHandlerFunc { diff --git a/apps/backend/internal/mcp/server/server.go b/apps/backend/internal/mcp/server/server.go index ef60e9d580..690736593f 100644 --- a/apps/backend/internal/mcp/server/server.go +++ b/apps/backend/internal/mcp/server/server.go @@ -1609,9 +1609,9 @@ 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)."), + 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). If a plan already exists for this task, this REPLACES ITS ENTIRE CONTENT — same as update_task_plan_kandev through a different door. Read it first with get_task_plan_kandev if you need to preserve any of it."), 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 plan content in markdown format")), + mcp.WithString("content", mcp.Required(), mcp.Description("The full plan content in markdown format. This REPLACES any existing plan whole — there is no partial update or append mode. To preserve prior content, call get_task_plan_kandev first and include its content plus your additions in this call.")), mcp.WithString("title", mcp.Description("Optional title for the plan (default: 'Plan')")), ), s.wrapHandler("create_task_plan_kandev", s.createTaskPlanHandler()), @@ -1625,9 +1625,9 @@ 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)."), + 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). This REPLACES THE ENTIRE PLAN — there is no partial update, append, or section-patch mode. The correct sequence is: call get_task_plan_kandev, then send this call with the full document (prior content plus your changes), never just the new section."), 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.")), - mcp.WithString("content", mcp.Required(), mcp.Description("The updated plan content in markdown format")), + mcp.WithString("content", mcp.Required(), mcp.Description("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.")), mcp.WithString("title", mcp.Description("Optional new title for the plan")), ), s.wrapHandler("update_task_plan_kandev", s.updateTaskPlanHandler()), diff --git a/apps/backend/internal/task/service/plan_service.go b/apps/backend/internal/task/service/plan_service.go index 96c0bdea46..743b2c2e37 100644 --- a/apps/backend/internal/task/service/plan_service.go +++ b/apps/backend/internal/task/service/plan_service.go @@ -101,6 +101,13 @@ type CreatePlanRequest struct { CreatedBy string // "agent" | "user" AuthorKind string // optional explicit override AuthorName string // optional; display snapshot + // ForceNewRevision skips coalescing for this write even if it would + // otherwise qualify (same author within the coalesce window). Callers set + // this when the write looks like an accidental whole-document truncation: + // coalescing would merge the destructive content into the prior revision + // row in place, destroying the only recovery path. Zero value (false) + // preserves today's coalescing behavior for every existing caller. + ForceNewRevision bool } // CreatePlan upserts a plan and appends or coalesces a revision. @@ -113,12 +120,13 @@ func (s *PlanService) CreatePlan(ctx context.Context, req CreatePlanRequest) (*m // UpdatePlanRequest mirrors CreatePlanRequest; kept as a distinct type for API clarity. type UpdatePlanRequest struct { - TaskID string - Title string - Content string - CreatedBy string - AuthorKind string - AuthorName string + TaskID string + Title string + Content string + CreatedBy string + AuthorKind string + AuthorName string + ForceNewRevision bool } // UpdatePlan updates an existing plan (errors if missing). @@ -145,12 +153,13 @@ func (s *PlanService) UpdatePlan(ctx context.Context, req UpdatePlanRequest) (*m createdBy = existing.CreatedBy } return s.upsertPlan(ctx, CreatePlanRequest{ - TaskID: req.TaskID, - Title: title, - Content: req.Content, - CreatedBy: createdBy, - AuthorKind: req.AuthorKind, - AuthorName: req.AuthorName, + TaskID: req.TaskID, + Title: title, + Content: req.Content, + CreatedBy: createdBy, + AuthorKind: req.AuthorKind, + AuthorName: req.AuthorName, + ForceNewRevision: req.ForceNewRevision, }) } @@ -209,7 +218,7 @@ func (s *PlanService) upsertPlan(ctx context.Context, req CreatePlanRequest) (*m return nil, err } now := time.Now().UTC() - coalesce := s.canCoalesce(latest, authorKind, authorName, now) + coalesce := !req.ForceNewRevision && s.canCoalesce(latest, authorKind, authorName, now) rev := &models.TaskPlanRevision{ TaskID: req.TaskID, diff --git a/apps/backend/internal/task/service/plan_service_test.go b/apps/backend/internal/task/service/plan_service_test.go index ab06c5adbb..b52541c3ef 100644 --- a/apps/backend/internal/task/service/plan_service_test.go +++ b/apps/backend/internal/task/service/plan_service_test.go @@ -439,6 +439,40 @@ func TestPlanService_AppendsWhenWindowExpired(t *testing.T) { } } +func TestPlanService_ForceNewRevisionPreventsCoalesce(t *testing.T) { + svc, _, repo := createTestPlanService(t) + ctx := context.Background() + seedTask(t, ctx, repo, "task-force") + svc.coalesceWindow = 10 * time.Minute // same generous window as the coalesce test + + _, err := svc.CreatePlan(ctx, CreatePlanRequest{ + TaskID: "task-force", Content: "v1", + AuthorKind: "agent", AuthorName: "Claude", + }) + if err != nil { + t.Fatalf("initial create: %v", err) + } + // Same author, same window — would coalesce by default (see + // TestPlanService_CoalescesWithinWindow) but must not when a truncating + // write forces a new revision so the pre-write content survives. + _, err = svc.UpdatePlan(ctx, UpdatePlanRequest{ + TaskID: "task-force", Content: "v2", + AuthorKind: "agent", AuthorName: "Claude", + ForceNewRevision: true, + }) + if err != nil { + t.Fatalf("forced update: %v", err) + } + + list, err := svc.ListRevisions(ctx, "task-force") + if err != nil { + t.Fatalf("ListRevisions: %v", err) + } + if len(list) != 2 { + t.Fatalf("expected 2 separate revisions (forced), got %d", len(list)) + } +} + func TestPlanService_AuthorSwitchBreaksCoalesce(t *testing.T) { svc, _, repo := createTestPlanService(t) ctx := context.Background()