From b112604c8c68fd914cb68cefd37891afe419c1f5 Mon Sep 17 00:00:00 2001 From: Matt Wilkinson Date: Wed, 8 Jul 2026 22:32:37 -0400 Subject: [PATCH] fix(todo): thread blocked status through clone, guards, and renderers Carry task.blocker through AgentSession clone so a blocker reason survives storage round-trips; skip non-open tasks when blocking a phase so completed/abandoned work is never reopened; reject targetless block/unblock; render blocked rows distinctly (warning fg + reason) in the tool renderer and keep them visible in the collapsed HUD. Adds regression tests (red-green verified on the clone path). --- .../src/modes/interactive-mode.ts | 4 +- .../coding-agent/src/session/agent-session.ts | 6 +- packages/coding-agent/src/tools/todo.ts | 15 ++++ .../agent-session-todo-blocker-clone.test.ts | 76 +++++++++++++++++++ packages/coding-agent/test/tools/todo.test.ts | 46 +++++++++++ 5 files changed, 145 insertions(+), 2 deletions(-) create mode 100644 packages/coding-agent/test/agent-session-todo-blocker-clone.test.ts diff --git a/packages/coding-agent/src/modes/interactive-mode.ts b/packages/coding-agent/src/modes/interactive-mode.ts index 645aead80..0d5080262 100644 --- a/packages/coding-agent/src/modes/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive-mode.ts @@ -1760,7 +1760,9 @@ export class InteractiveMode implements InteractiveModeContext { // stage's `done/total` makes the hidden count obvious, so there is no // "… more" row; expanded lists every task. const renderTasks = (phase: TodoPhase): string[] => { - const open = phase.tasks.filter(t => t.status === "pending" || t.status === "in_progress"); + const open = phase.tasks.filter( + t => t.status === "pending" || t.status === "in_progress" || t.status === "blocked", + ); const base = expanded ? phase.tasks : open.length > 0 ? open : phase.tasks; const items = expanded ? base : base.slice(0, activeTaskCap); return renderTreeList( diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index f271f21bf..57e7555c1 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -8364,7 +8364,11 @@ export class AgentSession { #cloneTodoPhases(phases: TodoPhase[]): TodoPhase[] { return phases.map(phase => ({ name: phase.name, - tasks: phase.tasks.map(task => ({ content: task.content, status: task.status })), + tasks: phase.tasks.map(task => + task.blocker !== undefined + ? { content: task.content, status: task.status, blocker: task.blocker } + : { content: task.content, status: task.status }, + ), })); } diff --git a/packages/coding-agent/src/tools/todo.ts b/packages/coding-agent/src/tools/todo.ts index 8a416f9a1..126028fc6 100644 --- a/packages/coding-agent/src/tools/todo.ts +++ b/packages/coding-agent/src/tools/todo.ts @@ -390,14 +390,25 @@ function applyEntry(phases: TodoPhase[], entry: TodoOpEntryValue, errors: string return phases; } case "block": { + if (!entry.task && !entry.phase) { + errors.push("block requires a task or phase target"); + return phases; + } const reason = entry.reason?.trim() || undefined; for (const task of getTaskTargets(phases, entry, errors)) { + // Only actionable open work can be blocked: blocking a phase must + // not reopen completed/abandoned tasks or erase finished progress. + if (task.status !== "pending" && task.status !== "in_progress") continue; task.status = "blocked"; task.blocker = reason; } return phases; } case "unblock": { + if (!entry.task && !entry.phase) { + errors.push("unblock requires a task or phase target"); + return phases; + } for (const task of getTaskTargets(phases, entry, errors)) { if (task.status === "blocked") { task.status = "pending"; @@ -808,6 +819,10 @@ function formatTodoLine( return uiTheme.fg("accent", `${prefix}${checkbox.unchecked} ${item.content}`); case "abandoned": return uiTheme.fg("error", `${prefix}${checkbox.unchecked} ${strikethroughText(item.content)}`); + case "blocked": { + const note = item.blocker ? `blocked: ${item.blocker}` : "blocked"; + return uiTheme.fg("warning", `${prefix}${checkbox.unchecked} ${item.content} (${note})`); + } default: return uiTheme.fg("dim", `${prefix}${checkbox.unchecked} ${item.content}`); } diff --git a/packages/coding-agent/test/agent-session-todo-blocker-clone.test.ts b/packages/coding-agent/test/agent-session-todo-blocker-clone.test.ts new file mode 100644 index 000000000..b4b09adce --- /dev/null +++ b/packages/coding-agent/test/agent-session-todo-blocker-clone.test.ts @@ -0,0 +1,76 @@ +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import * as path from "node:path"; +import { Agent } from "@oh-my-pi/pi-agent-core"; +import { getBundledModel } from "@oh-my-pi/pi-catalog/models"; +import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import { AgentSession } from "@oh-my-pi/pi-coding-agent/session/agent-session"; +import { AuthStorage } from "@oh-my-pi/pi-coding-agent/session/auth-storage"; +import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager"; +import { TempDir } from "@oh-my-pi/pi-utils"; + +/** + * Regression coverage: `AgentSession.#cloneTodoPhases` used to clone only + * `{ content, status }`, dropping the `blocker` note on every set/get. That made + * a blocker reason vanish on the first `todo view` or any later op even though it + * appeared in the immediate tool result. The contract: a blocked task's reason + * survives a `setTodoPhases` → `getTodoPhases` round-trip (the same clone every + * storage read/write goes through). + */ +describe("AgentSession todo blocker clone", () => { + let tempDir: TempDir; + let session: AgentSession; + let sessionManager: SessionManager; + let authStorage: AuthStorage; + let modelRegistry: ModelRegistry; + + beforeEach(async () => { + tempDir = TempDir.createSync("@pi-todo-blocker-clone-"); + authStorage = await AuthStorage.create(path.join(tempDir.path(), "testauth.db")); + authStorage.setRuntimeApiKey("anthropic", "test-key"); + modelRegistry = new ModelRegistry(authStorage); + sessionManager = SessionManager.create(tempDir.path(), tempDir.path()); + + const model = getBundledModel("anthropic", "claude-sonnet-4-5"); + if (!model) throw new Error("Expected built-in anthropic model to exist"); + + const agent = new Agent({ + initialState: { model, systemPrompt: ["Test"], tools: [], messages: [] }, + }); + + session = new AgentSession({ + agent, + sessionManager, + settings: Settings.isolated({ "compaction.enabled": false, "todo.enabled": true, "todo.reminders": false }), + modelRegistry, + }); + }); + + afterEach(async () => { + await session.dispose(); + authStorage.close(); + try { + await tempDir.remove(); + } catch {} + }); + + it("preserves a blocker reason across a setTodoPhases/getTodoPhases round-trip", () => { + session.setTodoPhases([ + { + name: "Work", + tasks: [ + { content: "a", status: "blocked", blocker: "waiting on sign-off" }, + { content: "b", status: "pending" }, + ], + }, + ]); + + const roundTripped = session.getTodoPhases(); + const blocked = roundTripped[0]?.tasks.find(task => task.content === "a"); + expect(blocked?.status).toBe("blocked"); + expect(blocked?.blocker).toBe("waiting on sign-off"); + // A task with no blocker must not gain one through the clone. + const open = roundTripped[0]?.tasks.find(task => task.content === "b"); + expect(open?.blocker).toBeUndefined(); + }); +}); diff --git a/packages/coding-agent/test/tools/todo.test.ts b/packages/coding-agent/test/tools/todo.test.ts index dbaa587e2..94ec6de06 100644 --- a/packages/coding-agent/test/tools/todo.test.ts +++ b/packages/coding-agent/test/tools/todo.test.ts @@ -225,6 +225,52 @@ describe("TodoTool operations", () => { expect(result.details?.phases[0]?.tasks[0]?.status).toBe("blocked"); }); + it("blocking a phase leaves completed/abandoned tasks closed", async () => { + const tool = new TodoTool(createSession()); + await tool.execute("call-1", { op: "init", list: [{ phase: "Work", items: ["a", "b", "c"] }] }); + await tool.execute("call-2", { op: "done", task: "a" }); + await tool.execute("call-3", { op: "drop", task: "c" }); + + const result = await tool.execute("call-4", { op: "block", phase: "Work", reason: "waiting on infra" }); + const tasks = result.details?.phases[0]?.tasks ?? []; + const byContent = (content: string) => tasks.find(task => task.content === content); + // Completed/abandoned work is untouched; only the open task becomes blocked. + expect(byContent("a")?.status).toBe("completed"); + expect(byContent("c")?.status).toBe("abandoned"); + expect(byContent("b")?.status).toBe("blocked"); + expect(byContent("b")?.blocker).toBe("waiting on infra"); + // A completed task must never carry a blocker note. + expect(byContent("a")?.blocker).toBeUndefined(); + }); + + it("rejects a block with neither task nor phase target", async () => { + const tool = new TodoTool(createSession()); + await tool.execute("call-1", { op: "init", list: [{ phase: "Work", items: ["a", "b"] }] }); + + const result = await tool.execute("call-2", { op: "block", reason: "oops" }); + expect(result.isError).toBe(true); + const summary = result.content.find(part => part.type === "text"); + if (summary?.type !== "text") throw new Error("Expected text summary from todo"); + expect(summary.text).toContain("block requires a task or phase target"); + // Nothing was blocked — state is unchanged. + const tasks = result.details?.phases[0]?.tasks ?? []; + expect(tasks.every(task => task.status !== "blocked")).toBe(true); + }); + + it("rejects an unblock with neither task nor phase target", async () => { + const tool = new TodoTool(createSession()); + await tool.execute("call-1", { op: "init", list: [{ phase: "Work", items: ["a"] }] }); + await tool.execute("call-2", { op: "block", task: "a", reason: "x" }); + + const result = await tool.execute("call-3", { op: "unblock" }); + expect(result.isError).toBe(true); + const summary = result.content.find(part => part.type === "text"); + if (summary?.type !== "text") throw new Error("Expected text summary from todo"); + expect(summary.text).toContain("unblock requires a task or phase target"); + // The blocked task stays blocked — the targetless unblock was rejected. + expect(result.details?.phases[0]?.tasks[0]?.status).toBe("blocked"); + }); + it("preserves blocked status across the markdown round-trip", () => { const phases: TodoPhase[] = [ {