smashimo/oh-my-pi
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(todo): thread blocked status through clone, guards, and renderers

Browse Source 
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).
This commit is contained in:
Matt Wilkinson
2026-07-08 22:32:37 -04:00
parent 792645e707
commit b112604c8c
5 changed files with 145 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/coding-agent/src/modes/interactive-mode.ts
+3 -1
View File
@@ -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(
packages/coding-agent/src/session/agent-session.ts
+5 -1
View File
@@ -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 },
),
}));
}
packages/coding-agent/src/tools/todo.ts
+15
View File
@@ -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}`);
}
packages/coding-agent/test/agent-session-todo-blocker-clone.test.ts
+76
View File
@@ -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();
});
});
packages/coding-agent/test/tools/todo.test.ts
+46
View File
@@ -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[] = [
{