From 899c0ef08b1bdf2a4eb569f4fdf4bb213f4c741d Mon Sep 17 00:00:00 2001 From: can1357 Date: Tue, 23 Jun 2026 00:54:54 +0200 Subject: [PATCH] feat: simplified todo tool to single operation interface - Refactored `todo` tool to accept a single operation object instead of an `ops` array. - Implemented parameter normalization to maintain backward compatibility with legacy array-based tool calls. - Updated tool instructions, documentation, and UI rendering components to reflect the new interface. - Added compatibility tests to verify rendering and execution for both legacy and current operation formats. --- docs/tools/todo.md | 38 ++-- packages/coding-agent/CHANGELOG.md | 4 +- .../src/cli/gallery-fixtures/interaction.ts | 15 +- .../coding-agent/src/prompts/tools/todo.md | 2 +- packages/coding-agent/src/tools/todo.ts | 124 ++++++----- .../test/agent-session-eager-todo.test.ts | 8 +- packages/coding-agent/test/tools/todo.test.ts | 193 +++++++----------- packages/collab-web/CHANGELOG.md | 5 +- .../collab-web/src/tool-render/tools/todo.tsx | 14 +- 9 files changed, 182 insertions(+), 221 deletions(-) diff --git a/docs/tools/todo.md b/docs/tools/todo.md index 3abb699f1..c59c6efa8 100644 --- a/docs/tools/todo.md +++ b/docs/tools/todo.md @@ -1,6 +1,6 @@ # todo -> Applies ordered mutations to the session todo list and returns a text summary plus the full phase/task state. +> Applies one mutation to the session todo list and returns a text summary plus the full phase/task state. ## Source - Entry: `packages/coding-agent/src/tools/todo.ts` @@ -14,11 +14,7 @@ ## Inputs -| Field | Type | Required | Description | -| --- | --- | --- | --- | -| `ops` | `TodoOpEntry[]` | Yes | Ordered operations to apply. `minItems: 1`. - -### `TodoOpEntry` +The params object **is** a single op — the discriminator and its fields live at the top level (no `ops` array wrapper). | Op | Required fields | Optional fields | Effect | | --- | --- | --- | --- | @@ -28,9 +24,9 @@ | `drop` | `task` or `phase` or neither | None | Marks the target task, phase, or all tasks `abandoned`. | | `rm` | `task` or `phase` or neither | None | Removes the target task, clears the phase's task list, or clears all task lists. | | `append` | `phase`, `items` | None | Appends new `pending` tasks to a phase; creates the phase if missing. | -| `view` | None | None | Echoes the current list. A call whose ops are all `view` is read-only: no normalization, no state write. | +| `view` | None | None | Echoes the current list. A `view` call is read-only: no normalization, no state write. | -### Fields used inside ops +### Fields | Field | Type | Required | Description | | --- | --- | --- | --- | @@ -46,11 +42,11 @@ The tool returns a single-shot `AgentToolResult`: - `content`: one text part containing the summary from `formatSummary(...)`. - Empty final state with no errors: `Todo list cleared.` (`Todo list is empty.` for a pure-`view` call). - Non-empty final state: remaining-item list, current phase progress, then a per-phase tree. - - If any op produced validation/runtime errors, the summary starts with `Errors: ...` and the result is marked `isError: true`; the whole batch is discarded — the returned and persisted state stay at the pre-call list. + - If the op produced validation/runtime errors, the summary starts with `Errors: ...` and the result is marked `isError: true`; the mutation is discarded — the returned and persisted state stay at the pre-call list. - `details`: - `phases: TodoPhase[]` - `storage: "session" | "memory"` - - `completedTasks?: TodoCompletionTransition[]` when a task changed from non-completed to `completed` during the batch + - `completedTasks?: TodoCompletionTransition[]` when a task changed from non-completed to `completed` during the call `TodoPhase` / `TodoItem` state model: @@ -61,19 +57,19 @@ The TUI renderer (`todoToolRenderer`) merges call and result into one transcript ## Flow 1. `TodoTool.execute(...)` clones the current cached phases from `session.getTodoPhases?.() ?? []` (`packages/coding-agent/src/tools/todo.ts`). -2. `applyParams(...)` walks `params.ops` in order and applies each entry with `applyEntry(...)`. +2. `applyParams(...)` applies the single op (`params`) with `applyEntry(...)`. 3. Each op mutates the working phase array: - `initPhases(...)` rebuilds the list from scratch. - `start` resolves a task by exact `content`, demotes every other `in_progress` task to `pending`, then marks the target `in_progress`. - `done` / `drop` use `getTaskTargets(...)` to target one task, one phase, or every task. - `rm` removes one task, clears one phase's `tasks`, or clears all phases' task arrays. - `appendItems(...)` resolves or creates the target phase and pushes new `pending` tasks unless the same task content already exists anywhere. -4. Missing task/phase references are recorded in an `errors` array by `resolveTaskOrError(...)` / `resolvePhaseOrError(...)`; execution continues through the rest of the batch, but any error discards the batch's mutations at the end. -5. After the full batch, `normalizeInProgressTask(...)` enforces the single-active-task invariant: +4. Missing task/phase references are recorded in an `errors` array by `resolveTaskOrError(...)` / `resolvePhaseOrError(...)`; any error discards the op's mutations at the end. +5. After the op, `normalizeInProgressTask(...)` enforces the single-active-task invariant: - if multiple tasks are `in_progress`, only the first stays active and the rest become `pending`; - if none are `in_progress`, the first `pending` task in phase/task order is auto-promoted to `in_progress`. -6. `execute(...)` stores the updated phases with `session.setTodoPhases?.(...)` only when the batch produced no errors and was not pure-`view`; a failed batch is discarded wholesale (persisting a half-applied batch would make the natural retry hit "already exists"). `storage` is `"session"` when `session.getSessionFile()` exists, else `"memory"`. -7. `getCompletionTransitions(...)` compares the previous and updated phases (skipped for failed or pure-`view` calls); newly completed tasks are returned in `details.completedTasks`. +6. `execute(...)` stores the updated phases with `session.setTodoPhases?.(...)` only when the op produced no errors and was not a `view`; a failed op is discarded (persisting a half-applied mutation would make the natural retry hit "already exists"). `storage` is `"session"` when `session.getSessionFile()` exists, else `"memory"`. +7. `getCompletionTransitions(...)` compares the previous and updated phases (skipped for failed or `view` calls); newly completed tasks are returned in `details.completedTasks`. 8. The agent runtime also watches `todo` tool results in `packages/coding-agent/src/session/agent-session.ts`; successful results refresh cached todos, failed results inject a hidden next-turn reminder telling the model that todo progress is not visible until it retries. 9. The event controller updates the visible todo UI from `result.details.phases` on success, or shows a warning on error (`packages/coding-agent/src/modes/controllers/event-controller.ts`). @@ -87,7 +83,7 @@ The TUI renderer (`todoToolRenderer`) merges call and result into one transcript | `completed` | Can be set back to `in_progress` if targeted | Stays `completed` | Becomes `abandoned` if targeted | Removed | No status change | | `abandoned` | Can be set back to `in_progress` if targeted | Becomes `completed` if targeted | Stays `abandoned` | Removed | No status change | -Normalization then re-applies the single-active-task rule after the full op batch. +Normalization then re-applies the single-active-task rule after the op runs. ### Op targeting rules - `done`, `drop`, `rm`: @@ -118,7 +114,7 @@ The same file also exposes non-tool helpers used by `/todo`: - Session-level auto-clear of `completed`/`abandoned` tasks was removed (the timer mutated canonical phases between tool calls); the TUI todo widget still clears closed entries after `tasks.todoClearDelay` (display-only, `packages/coding-agent/src/modes/interactive-mode.ts`). ## Limits & Caps -- `ops` array: `minItems: 1` (`todoSchema`). +- `init.list`: applies to a single op (`todoSchema`). The params object carries exactly one op. - `init.list[*].items`: `minItems: 1`. - `append.items`: `minItems: 1`. - Renderer collapsed preview: `PREVIEW_LIMITS.COLLAPSED_ITEMS = 8` (`packages/coding-agent/src/tools/render-utils.ts`). @@ -126,7 +122,7 @@ The same file also exposes non-tool helpers used by `/todo`: - Tool execution mode: `concurrency = "exclusive"`, `strict = true`, `loadMode = "discoverable"`. ## Errors -- Ordinary bad op payloads are accumulated as human-readable strings in `errors`; the result is marked `isError: true` and the whole batch is discarded — the returned and persisted state stay at the pre-call list. +- Ordinary bad op payloads are accumulated as human-readable strings in `errors`; the result is marked `isError: true` and the mutation is discarded — the returned and persisted state stay at the pre-call list. - Error strings come from the helpers in `packages/coding-agent/src/tools/todo.ts`, including: - `Missing list for init operation` - `Missing task content` @@ -137,18 +133,18 @@ The same file also exposes non-tool helpers used by `/todo`: - `Missing phase name for append operation` - `Missing items for append operation` - `Task "..." already exists` -- Ops are processed in order and an early error does not stop later ops from being attempted, but any error in the batch discards every mutation the batch made. +- A `todo` call carries a single op; any error in it discards every mutation the op made. - Runtime-level tool failure is handled outside the tool body: `agent-session` injects a hidden reminder and the event controller warns the user that visible progress may be stale. - Idempotency is op-specific: - `init` is a full replacement; replaying the same payload yields the same state. - `start`, `done`, and `drop` are effectively idempotent on an existing target state, but `start` also demotes any other active task. - `rm` is not idempotent for targeted removals: the second call errors because the task or phase is gone. - - `append` is not idempotent: duplicate task content is rejected with `Task "..." already exists`; the whole `append` op validates up front, so a batch with any duplicate appends nothing. + - `append` is not idempotent: duplicate task content is rejected with `Task "..." already exists`; the `append` op validates up front, so an op with any duplicate appends nothing. ## Notes - Task lookup is exact string equality inside the tool. The model-facing prompt says task content and phase names are identifiers and should stay unique; `append` enforces task uniqueness globally, and `init` rejects duplicate phase names and duplicate task contents in its payload. - `findTaskByContent(...)` returns the first matching task across phases. Duplicate task contents make later targeted ops ambiguous. -- `normalizeInProgressTask(...)` runs after the whole batch, not after each op. A single call can intentionally build an intermediate invalid state and rely on final normalization. +- `normalizeInProgressTask(...)` runs once after the op, not mid-op. A single op (e.g. `init`) can build an intermediate invalid state and rely on final normalization. - `storage: "session"` means the session has a session-file backing; it does not mean this tool wrote a durable custom entry. - Reload persistence differs by path: - plain `todo` calls survive in transcript tool-result details; diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index e3392d2ad..06061a941 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,7 +1,6 @@ # Changelog ## [Unreleased] - ### Breaking Changes - Renamed the eval `agent()` helper parameters `agent_type` → `agent` and `return_handle` → `handle` across every workflow runtime (Python, JavaScript, Ruby, Julia), so the names are identical in every language (no camelCase/snake_case split) and the agent-selection parameter matches the `task` tool's `agent`. The `__agent__` eval bridge wire protocol was renamed to match. @@ -12,6 +11,7 @@ ### Changed +- Simplified `todo` tool interface to accept a single operation directly instead of an array of ops - Reinforced routing of fragile, multi-step shell logic to the `eval` tool over `bash`. The system-prompt tool policy, `bash.md`, and `eval.md` now treat loops, conditionals, heredocs, inline `-e`/`-c` scripts, multi-stage pipelines, and quote/JSON escaping as the signal to write an `eval` cell; bash's "compute a fact" carveout is narrowed to single short pipelines, and `eval.md` now actively claims that territory with runtime-templated examples (only enabled backends are advertised). - Made `eval` an essential built-in tool (`loadMode: "essential"`, added to the default essential tool set) so it stays active under `tools.discoveryMode: "all"` instead of being hidden behind `search_tool_bm25`. @@ -12367,4 +12367,4 @@ Initial public release. ## [0.7.6] - 2025-11-13 -Previous releases did not maintain a changelog. +Previous releases did not maintain a changelog. \ No newline at end of file diff --git a/packages/coding-agent/src/cli/gallery-fixtures/interaction.ts b/packages/coding-agent/src/cli/gallery-fixtures/interaction.ts index 34da85a14..7ca065139 100644 --- a/packages/coding-agent/src/cli/gallery-fixtures/interaction.ts +++ b/packages/coding-agent/src/cli/gallery-fixtures/interaction.ts @@ -5,17 +5,14 @@ export const interactionFixtures: Record = { todo: { label: "Todo", streamingArgs: { - ops: [{ op: "init", list: [{ phase: "Foundation", items: ["Scaffold crate"] }] }], + op: "init", + list: [{ phase: "Foundation", items: ["Scaffold crate"] }], }, args: { - ops: [ - { - op: "init", - list: [ - { phase: "Foundation", items: ["Scaffold crate", "Wire workspace"] }, - { phase: "Auth", items: ["Port credential store", "Wire OAuth providers"] }, - ], - }, + op: "init", + list: [ + { phase: "Foundation", items: ["Scaffold crate", "Wire workspace"] }, + { phase: "Auth", items: ["Port credential store", "Wire OAuth providers"] }, ], }, result: { diff --git a/packages/coding-agent/src/prompts/tools/todo.md b/packages/coding-agent/src/prompts/tools/todo.md index 95a52dda2..11fe60f66 100644 --- a/packages/coding-agent/src/prompts/tools/todo.md +++ b/packages/coding-agent/src/prompts/tools/todo.md @@ -1,6 +1,6 @@ **Tasks referenced by verbatim content string, NEVER an auto-generated ID — no "task-1"/"task-N" exists. Pass the content text in the `task` field.** -Manages a phased task list. Pass `ops`: flat array of operations. Next pending task auto-promotes to `in_progress` on each completion. `pending` is a status, not an `op` — leave not-yet-started tasks implicit in `init`/`append`. +Next pending task auto-promotes to `in_progress` on each completion. ## Operations diff --git a/packages/coding-agent/src/tools/todo.ts b/packages/coding-agent/src/tools/todo.ts index 04a343feb..2ae213c3e 100644 --- a/packages/coding-agent/src/tools/todo.ts +++ b/packages/coding-agent/src/tools/todo.ts @@ -52,21 +52,18 @@ const InitListEntry = type({ items: type("string").describe("task content").array().atLeastLength(1).describe("tasks for this phase"), }); -const TodoOpEntry = type({ +const todoSchema = type({ op: TodoOp, "list?": InitListEntry.array().describe("phased task list (init)"), "task?": type("string").describe("task content"), "phase?": type("string").describe("phase name"), "items?": type("string").describe("task content").array().atLeastLength(1).describe("tasks to append"), -}); - -const todoSchema = type({ - ops: TodoOpEntry.array().atLeastLength(1).describe("ordered todo operations"), -}).describe("apply ordered todo operations"); +}).describe("apply a single todo operation"); type TodoParams = TodoSchema; type TodoSchema = typeof todoSchema.infer; -type TodoOpEntryValue = TodoParams["ops"][number]; +/** A single todo op entry (the params object itself). */ +type TodoOpEntryValue = TodoParams; // ============================================================================= // State helpers @@ -402,10 +399,7 @@ function applyEntry(phases: TodoPhase[], entry: TodoOpEntryValue, errors: string function applyParams(phases: TodoPhase[], params: TodoParams): { phases: TodoPhase[]; errors: string[] } { const errors: string[] = []; - let next = phases; - for (const entry of params.ops) { - next = applyEntry(next, entry, errors); - } + const next = applyEntry(phases, params, errors); normalizeInProgressTask(next); return { phases: next, errors }; } @@ -413,9 +407,15 @@ function applyParams(phases: TodoPhase[], params: TodoParams): { phases: TodoPha /** Apply an array of `todo`-style ops to existing phases. Used by /todo slash command. */ export function applyOpsToPhases( currentPhases: TodoPhase[], - ops: TodoParams["ops"], + ops: TodoParams[], ): { phases: TodoPhase[]; errors: string[] } { - return applyParams(clonePhases(currentPhases), { ops }); + const errors: string[] = []; + let next = clonePhases(currentPhases); + for (const op of ops) { + next = applyEntry(next, op, errors); + } + normalizeInProgressTask(next); + return { phases: next, errors }; } // ============================================================================= @@ -572,64 +572,44 @@ export class TodoTool implements AgentTool { { caption: "Initial setup (multi-phase)", call: { - ops: [ - { - op: "init", - list: [ - { phase: "Foundation", items: ["Scaffold crate", "Wire workspace"] }, - { phase: "Auth", items: ["Port credential store", "Wire OAuth providers"] }, - { phase: "Verification", items: ["Run cargo test"] }, - ], - }, + op: "init", + list: [ + { phase: "Foundation", items: ["Scaffold crate", "Wire workspace"] }, + { phase: "Auth", items: ["Port credential store", "Wire OAuth providers"] }, + { phase: "Verification", items: ["Run cargo test"] }, ], }, }, { caption: "View current state (read-only)", - call: { - ops: [{ op: "view" }], - }, + call: { op: "view" }, }, { caption: "Initial setup (single phase)", call: { - ops: [ - { - op: "init", - list: [{ phase: "Implementation", items: ["Apply fix", "Run tests"] }], - }, - ], + op: "init", + list: [{ phase: "Implementation", items: ["Apply fix", "Run tests"] }], }, }, { caption: "Complete one task", - call: { - ops: [{ op: "done", task: "Wire workspace" }], - }, + call: { op: "done", task: "Wire workspace" }, }, { caption: "Complete a whole phase", - call: { - ops: [{ op: "done", phase: "Auth" }], - }, + call: { op: "done", phase: "Auth" }, }, { caption: "Remove all tasks", - call: { - ops: [{ op: "rm" }], - }, + call: { op: "rm" }, }, { caption: "Drop one task", - call: { - ops: [{ op: "drop", task: "Run cargo test" }], - }, + call: { op: "drop", task: "Run cargo test" }, }, { caption: "Append tasks to a phase", - call: { - ops: [{ op: "append", phase: "Auth", items: ["Handle retries", "Run tests"] }], - }, + call: { op: "append", phase: "Auth", items: ["Handle retries", "Run tests"] }, }, ]; readonly loadMode = "discoverable"; @@ -646,7 +626,7 @@ export class TodoTool implements AgentTool { ): Promise> { const previousPhases = clonePhases(this.session.getTodoPhases?.() ?? []); // Pure-view calls are reads: no normalization, no state write. - const readOnly = params.ops.every(entry => entry.op === "view"); + const readOnly = params.op === "view"; const { phases: updated, errors } = readOnly ? { phases: previousPhases, errors: [] as string[] } : applyParams(clonePhases(previousPhases), params); @@ -673,15 +653,32 @@ export class TodoTool implements AgentTool { // TUI Renderer // ============================================================================= -type TodoRenderArgs = { - ops?: Array<{ - op?: string; - task?: string; - phase?: string; - items?: string[]; - }>; +type TodoRenderOp = { + op?: string; + task?: string; + phase?: string; + items?: string[]; }; +/** New single-op shape `{op,...}`; legacy `{ops:[...]}` still seen in old transcripts. */ +type TodoRenderArgs = TodoRenderOp & { + ops?: TodoRenderOp[]; +}; + +/** + * Normalize streaming/legacy render args to a flat op list. Accepts the new + * top-level `{op,...}` shape (returned as a one-element list), the legacy + * `{ops:[...]}` batch from old transcripts/collab-web, and partially-parsed + * streaming deltas (non-array `ops`, non-object entries) without crashing. + */ +function normalizeTodoArg(args: TodoRenderArgs | undefined): TodoRenderOp[] { + if (!args || typeof args !== "object") return []; + if (Array.isArray(args.ops)) { + return args.ops.filter((entry): entry is TodoRenderOp => !!entry && typeof entry === "object"); + } + return typeof args.op === "string" ? [args] : []; +} + // ============================================================================= // Phase numbering (display-only) // ============================================================================= @@ -794,7 +791,7 @@ function computeTouchedPhases( for (const transition of completedTasks) touched.add(transition.phase); // Phases explicitly named by the ops that ran. `init` replaces the whole // list, so the entire plan is fresh and every phase counts as touched. - const ops = Array.isArray(args?.ops) ? args.ops : []; + const ops = normalizeTodoArg(args); for (const op of ops) { if (!op || typeof op !== "object") continue; if (op.op === "init") { @@ -823,18 +820,17 @@ function formatPhaseSummary(phase: TodoPhase, oneBasedIndex: number, uiTheme: Th export const todoToolRenderer = { renderCall(args: TodoRenderArgs, options: RenderResultOptions, uiTheme: Theme): Component { - // `args` here is the raw partially-parsed JSON from the streaming - // tool-call delta and may not satisfy `TodoRenderArgs` at runtime: - // `parseStreamingJson` can hand back `{ ops: "[" }` mid-delta, or - // entries that are `null` / strings before fields stream. Guard - // against non-array `ops` and non-object entries so a malformed - // delta never breaks the TUI render loop (#2005). - const opsList = Array.isArray(args?.ops) ? args.ops : []; + // `args` is the raw partially-parsed JSON from the streaming tool-call + // delta and may not satisfy `TodoRenderArgs` at runtime: + // `parseStreamingJson` can hand back `{ op: 1 }` mid-delta, or a legacy + // `{ ops: "[" }` shape before fields stream. `normalizeTodoArg` guards + // both the new single-op and legacy batch shapes so a malformed delta + // never breaks the TUI render loop (#2005). + const opsList = normalizeTodoArg(args); const ops = opsList.length === 0 ? ["update"] - : opsList.map(entry => { - const e = entry && typeof entry === "object" ? entry : ({} as NonNullable); + : opsList.map(e => { const parts = [e.op ?? "update"]; if (e.task) parts.push(e.task); if (e.phase) parts.push(e.phase); diff --git a/packages/coding-agent/test/agent-session-eager-todo.test.ts b/packages/coding-agent/test/agent-session-eager-todo.test.ts index d5feea1d2..ed8b952fc 100644 --- a/packages/coding-agent/test/agent-session-eager-todo.test.ts +++ b/packages/coding-agent/test/agent-session-eager-todo.test.ts @@ -220,12 +220,8 @@ describe("AgentSession eager todo enforcement", () => { it("initializes todos once, then continues within the same user turn", async () => { scriptedResponses = [ createToolCallAssistantMessage("todo", { - ops: [ - { - op: "init", - list: [{ phase: "List worktrees", items: ["List all git worktrees in the current repository"] }], - }, - ], + op: "init", + list: [{ phase: "List worktrees", items: ["List all git worktrees in the current repository"] }], }), createAssistantMessage("real user turn handled"), ]; diff --git a/packages/coding-agent/test/tools/todo.test.ts b/packages/coding-agent/test/tools/todo.test.ts index 8266667e8..2a6232426 100644 --- a/packages/coding-agent/test/tools/todo.test.ts +++ b/packages/coding-agent/test/tools/todo.test.ts @@ -59,12 +59,8 @@ describe("TodoTool auto-start behavior", () => { it("auto-starts the first task after init", async () => { const tool = new TodoTool(createSession()); const result = await tool.execute("call-1", { - ops: [ - { - op: "init", - list: [{ phase: "Execution", items: ["status", "diagnostics"] }], - }, - ], + op: "init", + list: [{ phase: "Execution", items: ["status", "diagnostics"] }], }); const tasks = result.details?.phases[0]?.tasks ?? []; @@ -79,15 +75,11 @@ describe("TodoTool auto-start behavior", () => { it("auto-promotes the next pending task when current task is completed", async () => { const tool = new TodoTool(createSession()); await tool.execute("call-1", { - ops: [ - { - op: "init", - list: [{ phase: "Execution", items: ["status", "diagnostics"] }], - }, - ], + op: "init", + list: [{ phase: "Execution", items: ["status", "diagnostics"] }], }); - const result = await tool.execute("call-2", { ops: [{ op: "done", task: "status" }] }); + const result = await tool.execute("call-2", { op: "done", task: "status" }); const tasks = result.details?.phases[0]?.tasks ?? []; expect(tasks.map(task => task.status)).toEqual(["completed", "in_progress"]); @@ -96,7 +88,7 @@ describe("TodoTool auto-start behavior", () => { if (summary?.type !== "text") throw new Error("Expected text summary from todo"); expect(summary.text).toContain("Remaining items (1):"); expect(summary.text).toContain("diagnostics [in_progress] (Execution)"); - const completedResult = await tool.execute("call-3", { ops: [{ op: "done", task: "diagnostics" }] }); + const completedResult = await tool.execute("call-3", { op: "done", task: "diagnostics" }); const completedSummary = completedResult.content.find(part => part.type === "text"); if (completedSummary?.type !== "text") { throw new Error("Expected text summary from todo"); @@ -107,10 +99,8 @@ describe("TodoTool auto-start behavior", () => { it("renders completed tasks as checked before revealing strikethrough", async () => { const tool = new TodoTool(createSession()); - await tool.execute("call-1", { - ops: [{ op: "init", list: [{ phase: "Execution", items: ["finish"] }] }], - }); - const result = await tool.execute("call-2", { ops: [{ op: "done", task: "finish" }] }); + await tool.execute("call-1", { op: "init", list: [{ phase: "Execution", items: ["finish"] }] }); + const result = await tool.execute("call-2", { op: "done", task: "finish" }); const options = { expanded: true, isPartial: false, spinnerFrame: 0 }; const component = todoToolRenderer.renderResult(result, options, theme); @@ -124,19 +114,15 @@ it("renders completed tasks as checked before revealing strikethrough", async () expect(revealFrame).toContain("\x1b[9m"); }); -describe("TodoTool ops operations", () => { +describe("TodoTool operations", () => { it("jumps to a specific task out of order", async () => { const tool = new TodoTool(createSession()); await tool.execute("call-1", { - ops: [ - { - op: "init", - list: [{ phase: "Phase A", items: ["first", "second", "third"] }], - }, - ], + op: "init", + list: [{ phase: "Phase A", items: ["first", "second", "third"] }], }); - const result = await tool.execute("call-2", { ops: [{ op: "start", task: "third" }] }); + const result = await tool.execute("call-2", { op: "start", task: "third" }); const tasks = result.details?.phases[0]?.tasks ?? []; expect(tasks.map(task => task.status)).toEqual(["pending", "pending", "in_progress"]); @@ -145,18 +131,14 @@ describe("TodoTool ops operations", () => { it("demotes the current in_progress task when starting another", async () => { const tool = new TodoTool(createSession()); await tool.execute("call-1", { - ops: [ - { - op: "init", - list: [ - { phase: "A", items: ["a1", "a2"] }, - { phase: "B", items: ["b1"] }, - ], - }, + op: "init", + list: [ + { phase: "A", items: ["a1", "a2"] }, + { phase: "B", items: ["b1"] }, ], }); - const result = await tool.execute("call-2", { ops: [{ op: "start", task: "b1" }] }); + const result = await tool.execute("call-2", { op: "start", task: "b1" }); const allTasks = result.details?.phases.flatMap(phase => phase.tasks) ?? []; expect(allTasks.map(task => task.status)).toEqual(["pending", "pending", "in_progress"]); @@ -164,18 +146,12 @@ describe("TodoTool ops operations", () => { it("appends items to an existing phase", async () => { const tool = new TodoTool(createSession()); - await tool.execute("call-1", { - ops: [{ op: "init", list: [{ phase: "Work", items: ["First"] }] }], - }); + await tool.execute("call-1", { op: "init", list: [{ phase: "Work", items: ["First"] }] }); const result = await tool.execute("call-2", { - ops: [ - { - op: "append", - phase: "Work", - items: ["Second"], - }, - ], + op: "append", + phase: "Work", + items: ["Second"], }); const tasks = result.details?.phases[0]?.tasks ?? []; @@ -187,18 +163,12 @@ describe("TodoTool ops operations", () => { it("creates a phase when append targets a missing phase", async () => { const tool = new TodoTool(createSession()); - await tool.execute("call-1", { - ops: [{ op: "init", list: [{ phase: "Work", items: ["First"] }] }], - }); + await tool.execute("call-1", { op: "init", list: [{ phase: "Work", items: ["First"] }] }); const result = await tool.execute("call-2", { - ops: [ - { - op: "append", - phase: "Cleanup", - items: ["Remove dead code"], - }, - ], + op: "append", + phase: "Cleanup", + items: ["Remove dead code"], }); expect(result.details?.phases.map(phase => phase.name)).toEqual(["Work", "Cleanup"]); @@ -208,18 +178,14 @@ describe("TodoTool ops operations", () => { it("marks all tasks in a phase done", async () => { const tool = new TodoTool(createSession()); await tool.execute("call-1", { - ops: [ - { - op: "init", - list: [ - { phase: "Work", items: ["First", "Second"] }, - { phase: "Later", items: ["Third"] }, - ], - }, + op: "init", + list: [ + { phase: "Work", items: ["First", "Second"] }, + { phase: "Later", items: ["Third"] }, ], }); - const result = await tool.execute("call-2", { ops: [{ op: "done", phase: "Work" }] }); + const result = await tool.execute("call-2", { op: "done", phase: "Work" }); const allTasks = result.details?.phases.flatMap(phase => phase.tasks) ?? []; expect(allTasks.map(task => task.status)).toEqual(["completed", "completed", "in_progress"]); }); @@ -227,15 +193,11 @@ describe("TodoTool ops operations", () => { it("removes all tasks when rm omits task and phase", async () => { const tool = new TodoTool(createSession()); await tool.execute("call-1", { - ops: [ - { - op: "init", - list: [{ phase: "Work", items: ["First", "Second"] }], - }, - ], + op: "init", + list: [{ phase: "Work", items: ["First", "Second"] }], }); - const result = await tool.execute("call-2", { ops: [{ op: "rm" }] }); + const result = await tool.execute("call-2", { op: "rm" }); expect(result.details?.phases[0]?.tasks).toEqual([]); const summary = result.content.find(part => part.type === "text"); if (summary?.type !== "text") throw new Error("Expected text summary"); @@ -245,15 +207,11 @@ describe("TodoTool ops operations", () => { it("drops all tasks in a phase", async () => { const tool = new TodoTool(createSession()); await tool.execute("call-1", { - ops: [ - { - op: "init", - list: [{ phase: "Work", items: ["First", "Second"] }], - }, - ], + op: "init", + list: [{ phase: "Work", items: ["First", "Second"] }], }); - const result = await tool.execute("call-2", { ops: [{ op: "drop", phase: "Work" }] }); + const result = await tool.execute("call-2", { op: "drop", phase: "Work" }); const tasks = result.details?.phases[0]?.tasks ?? []; expect(tasks.map(task => task.status)).toEqual(["abandoned", "abandoned"]); }); @@ -270,7 +228,7 @@ describe("TodoTool ops operations", () => { ]); const tool = new TodoTool(session); - const result = await tool.execute("call-1", { ops: [{ op: "view" }] }); + const result = await tool.execute("call-1", { op: "view" }); const tasks = result.details?.phases[0]?.tasks ?? []; expect(tasks.map(task => task.status)).toEqual(["pending", "pending"]); @@ -284,7 +242,7 @@ describe("TodoTool ops operations", () => { it("view on an empty list reports empty, not cleared", async () => { const tool = new TodoTool(createSession()); - const result = await tool.execute("call-1", { ops: [{ op: "view" }] }); + const result = await tool.execute("call-1", { op: "view" }); const summary = result.content.find(part => part.type === "text"); if (summary?.type !== "text") throw new Error("Expected text summary"); expect(summary.text).toContain("Todo list is empty."); @@ -295,9 +253,7 @@ describe("TodoTool ops operations", () => { describe("TodoTool lenient init shapes", () => { it("accepts a flattened init with bare items and no phase", async () => { const tool = new TodoTool(createSession()); - const result = await tool.execute("call-1", { - ops: [{ op: "init", items: ["First", "Second"] }], - }); + const result = await tool.execute("call-1", { op: "init", items: ["First", "Second"] }); expect(result.isError).toBeUndefined(); expect(result.details?.phases.map(phase => phase.name)).toEqual(["Tasks"]); @@ -310,9 +266,7 @@ describe("TodoTool lenient init shapes", () => { it("honors a bare phase on a flattened init", async () => { const tool = new TodoTool(createSession()); - const result = await tool.execute("call-1", { - ops: [{ op: "init", phase: "Cleanup", items: ["Remove dead code"] }], - }); + const result = await tool.execute("call-1", { op: "init", phase: "Cleanup", items: ["Remove dead code"] }); expect(result.isError).toBeUndefined(); expect(result.details?.phases.map(phase => phase.name)).toEqual(["Cleanup"]); @@ -321,7 +275,7 @@ describe("TodoTool lenient init shapes", () => { it("still errors when init has neither list nor items", async () => { const tool = new TodoTool(createSession()); - const result = await tool.execute("call-1", { ops: [{ op: "init" }] }); + const result = await tool.execute("call-1", { op: "init" }); expect(result.isError).toBe(true); const summary = result.content.find(part => part.type === "text"); @@ -443,20 +397,16 @@ describe("todoToolRenderer.renderResult phase collapsing", () => { async function buildThreePhaseAfterDone() { const tool = new TodoTool(createSession()); await tool.execute("init", { - ops: [ - { - op: "init", - list: [ - { phase: "Alpha", items: ["a1", "a2"] }, - { phase: "Beta", items: ["b1", "b2"] }, - { phase: "Gamma", items: ["c1", "c2"] }, - ], - }, + op: "init", + list: [ + { phase: "Alpha", items: ["a1", "a2"] }, + { phase: "Beta", items: ["b1", "b2"] }, + { phase: "Gamma", items: ["c1", "c2"] }, ], }); // `done a1` keeps the active task inside Alpha (auto-promotes a2), leaving // Beta and Gamma untouched by this update. - return tool.execute("done", { ops: [{ op: "done", task: "a1" }] }); + return tool.execute("done", { op: "done", task: "a1" }); } function innerLines(component: Component): string[] { const lines = Bun.stripANSI(component.render(100).join("\n")).split("\n"); @@ -465,7 +415,8 @@ describe("todoToolRenderer.renderResult phase collapsing", () => { it("collapses untouched phases to a one-line summary while expanding the active phase", async () => { const result = await buildThreePhaseAfterDone(); const component = todoToolRenderer.renderResult(result, { expanded: false, isPartial: false }, theme, { - ops: [{ op: "done", task: "a1" }], + op: "done", + task: "a1", }); const rendered = Bun.stripANSI(component.render(100).join("\n")); // Active phase renders its full task list. @@ -493,7 +444,8 @@ describe("todoToolRenderer.renderResult phase collapsing", () => { it("shows every phase fully when manually expanded", async () => { const result = await buildThreePhaseAfterDone(); const component = todoToolRenderer.renderResult(result, { expanded: true, isPartial: false }, theme, { - ops: [{ op: "done", task: "a1" }], + op: "done", + task: "a1", }); const rendered = Bun.stripANSI(component.render(100).join("\n")); expect(rendered).toContain("b1"); @@ -504,7 +456,8 @@ describe("todoToolRenderer.renderResult phase collapsing", () => { it("drops blank separator lines between phases", async () => { const result = await buildThreePhaseAfterDone(); const component = todoToolRenderer.renderResult(result, { expanded: true, isPartial: false }, theme, { - ops: [{ op: "done", task: "a1" }], + op: "done", + task: "a1", }); // No empty body line survives between phases. expect(innerLines(component).every(line => line.length > 0)).toBe(true); @@ -520,27 +473,39 @@ describe("todoToolRenderer.renderCall malformed-args regression (#2005)", () => // retry cascade. const renderOptions = { expanded: false, isPartial: true } as const; - it("does not throw when ops is a streaming-truncated string", () => { - // Mid-stream `partialJson === '{"ops":"[{'` parses into `{ops: "[{"}`. - const args = { ops: '[{"op":"init"' } as unknown as Parameters[0]; + it("does not throw when op is a streaming-truncated number", () => { + // Mid-stream the new flat shape can surface `{ op: 1 }` before the + // discriminator string lands. + const args = { op: 1 } as unknown as Parameters[0]; expect(() => todoToolRenderer.renderCall(args, renderOptions, theme)).not.toThrow(); }); - it("does not throw when ops entries are null", () => { - // `partialParse` of `'{"ops":[null'` can hand back `{ops: [null]}` in - // intermediate states before the entry object opens. - const args = { ops: [null] } as unknown as Parameters[0]; - expect(() => todoToolRenderer.renderCall(args, renderOptions, theme)).not.toThrow(); - }); - - it("does not throw when an entry's items field is a non-array", () => { + it("does not throw when a flat op's items field is a non-array", () => { const args = { - ops: [{ op: "append", phase: "Work", items: "Second" as unknown as string[] }], + op: "append", + phase: "Work", + items: "Second" as unknown as string[], } as unknown as Parameters[0]; expect(() => todoToolRenderer.renderCall(args, renderOptions, theme)).not.toThrow(); }); - it("still renders ops summary metadata for well-formed args", () => { + it("does not throw on the legacy streaming-truncated `ops` string", () => { + // Old transcripts/collab-web still carry `{ ops: "[{" }` mid-stream; + // `normalizeTodoArg` must keep tolerating the legacy batch shape. + const args = { ops: '[{"op":"init"' } as unknown as Parameters[0]; + expect(() => todoToolRenderer.renderCall(args, renderOptions, theme)).not.toThrow(); + }); + + it("renders op summary metadata for a well-formed flat call", () => { + const args = { op: "init", items: ["a", "b", "c"] }; + const component = todoToolRenderer.renderCall(args, renderOptions, theme); + // `Text(text, 0, 0)` from `@oh-my-pi/pi-tui` exposes the content via .render(). + const rendered = Bun.stripANSI(component.render(120).join("\n")); + expect(rendered).toContain("init"); + expect(rendered).toContain("3 items"); + }); + + it("still renders legacy multi-op `ops` arrays from old transcripts", () => { const args = { ops: [ { op: "init", items: ["a", "b", "c"] }, @@ -549,12 +514,10 @@ describe("todoToolRenderer.renderCall malformed-args regression (#2005)", () => ], }; const component = todoToolRenderer.renderCall(args, renderOptions, theme); - // `Text(text, 0, 0)` from `@oh-my-pi/pi-tui` exposes the content via .render(). const rendered = Bun.stripANSI(component.render(120).join("\n")); expect(rendered).toContain("init"); expect(rendered).toContain("3 items"); expect(rendered).toContain("done"); - expect(rendered).toContain("a"); expect(rendered).toContain("append"); expect(rendered).toContain("Cleanup"); expect(rendered).toContain("1 item"); diff --git a/packages/collab-web/CHANGELOG.md b/packages/collab-web/CHANGELOG.md index 503b478a7..7a2d99efd 100644 --- a/packages/collab-web/CHANGELOG.md +++ b/packages/collab-web/CHANGELOG.md @@ -1,6 +1,9 @@ # Changelog ## [Unreleased] +### Fixed + +- Improved compatibility with legacy todo task transcripts ## [16.1.8] - 2026-06-20 @@ -118,4 +121,4 @@ ### Security -- Hardened transcript Markdown rendering by escaping embedded HTML and allowing only safe link schemes +- Hardened transcript Markdown rendering by escaping embedded HTML and allowing only safe link schemes \ No newline at end of file diff --git a/packages/collab-web/src/tool-render/tools/todo.tsx b/packages/collab-web/src/tool-render/tools/todo.tsx index a3614ef00..591e60df9 100644 --- a/packages/collab-web/src/tool-render/tools/todo.tsx +++ b/packages/collab-web/src/tool-render/tools/todo.tsx @@ -43,8 +43,18 @@ function roman(n: number): string { return out; } +/** + * Normalize call args to a flat op list. The current `todo` contract sends a + * single top-level op `{op,...}`; legacy transcripts still carry the batched + * `{ops:[...]}` shape. Non-record entries (streaming deltas) are dropped. + */ +function toOps(args: ToolRenderProps["args"]): unknown[] { + if (Array.isArray(args.ops)) return args.ops; + return typeof args.op === "string" ? [args] : []; +} + function Summary({ args }: ToolRenderProps): ReactNode { - const ops = Array.isArray(args.ops) ? args.ops : []; + const ops = toOps(args); const counts: Record = {}; const order: string[] = []; let firstTask: string | null = null; @@ -128,7 +138,7 @@ function Board({ phases }: { phases: unknown[] }): ReactNode { } function Body({ args, result }: ToolRenderProps): ReactNode { - const ops = Array.isArray(args.ops) ? args.ops : []; + const ops = toOps(args); const rec = detailsRecord(result); const phases = rec && Array.isArray(rec.phases) && !result?.isError ? rec.phases : null; return (