diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index f588ade0e..7e9595b82 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed `todo` calls that omit `op` hard-failing validation ("op must be operation to apply (was missing)"): the tool now validates leniently and infers the op for unambiguous payloads (`list` → `init`, `phase`+`items` → `append`, bare `items` on an empty list → `init`); `op` stays required in the schema, and ambiguous op-less calls surface the schema error as a retryable tool error. + ## [17.1.1] - 2026-07-24 ### Added diff --git a/packages/coding-agent/src/tools/todo.ts b/packages/coding-agent/src/tools/todo.ts index 9d387727d..383d7d8c8 100644 --- a/packages/coding-agent/src/tools/todo.ts +++ b/packages/coding-agent/src/tools/todo.ts @@ -524,7 +524,46 @@ function applyEntry(phases: TodoPhase[], entry: TodoOpEntryValue, errors: string } } -function applyParams(phases: TodoPhase[], params: TodoParams): { phases: TodoPhase[]; errors: string[] } { +/** + * Infer a missing `op` from the raw argument shape. Only unambiguous shapes + * are inferred: + * - `list` → `init` (list is init-only) + * - `items` + `phase` → `append` (lazily creates the phase, so the result + * matches a single-phase init when nothing exists yet) + * - bare `items` with no existing todos → `init` (nothing to overwrite) + * Targeting args alone (`task`/`phase`) map to several ops and stay an error. + */ +function inferTodoOp(args: Record, hasExistingPhases: boolean): TodoOperation | undefined { + if (Array.isArray(args.list) && args.list.length > 0) return "init"; + if (Array.isArray(args.items) && args.items.length > 0) { + if (typeof args.phase === "string" && args.phase) return "append"; + if (!hasExistingPhases) return "init"; + } + return undefined; +} + +/** + * Validate execute-time arguments, repairing an omitted `op`. The tool sets + * `lenientArgValidation`, so the agent loop hands `execute()` the raw + * arguments when schema validation fails; the only failure repaired here is + * a missing `op` alongside an unambiguous payload (models routinely send + * `{list:[...]}` with no op). Anything else returns the schema error text + * for a normal model retry. + */ +function resolveTodoParams(raw: unknown, hasExistingPhases: boolean): TodoOpEntryValue | string { + const direct = todoSchema(raw); + if (!(direct instanceof type.errors)) return direct; + if (isRecord(raw) && raw.op === undefined) { + const inferred = inferTodoOp(raw, hasExistingPhases); + if (inferred) { + const repaired = todoSchema({ ...raw, op: inferred }); + if (!(repaired instanceof type.errors)) return repaired; + } + } + return `Invalid todo arguments: ${direct.summary}`; +} + +function applyParams(phases: TodoPhase[], params: TodoOpEntryValue): { phases: TodoPhase[]; errors: string[] } { const errors: string[] = []; const next = applyEntry(phases, params, errors); normalizeInProgressTask(next); @@ -534,7 +573,7 @@ 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: TodoOpEntryValue[], ): { phases: TodoPhase[]; errors: string[] } { const errors: string[] = []; let next = clonePhases(currentPhases); @@ -731,6 +770,9 @@ export class TodoTool implements AgentTool { readonly parameters = todoSchema; readonly concurrency = "exclusive"; readonly strict = true; + // Raw args reach execute() on schema failure; resolveTodoParams re-validates + // and repairs the one recoverable shape (missing `op`, unambiguous payload). + readonly lenientArgValidation = true; readonly examples: readonly ToolExample[] = [ { @@ -789,11 +831,22 @@ export class TodoTool implements AgentTool { _context?: AgentToolContext, ): Promise> { const previousPhases = clonePhases(this.session.getTodoPhases?.() ?? []); + const storage = this.session.getSessionFile() ? "session" : "memory"; + const resolved = resolveTodoParams(params, previousPhases.length > 0); + if (typeof resolved === "string") { + return { + content: [{ type: "text", text: resolved }], + details: { phases: previousPhases, storage }, + isError: true, + }; + } + const entry = resolved; + const op = entry.op; // Pure-view calls are reads: no normalization, no state write. - const readOnly = params.op === "view"; + const readOnly = op === "view"; const { phases: updated, errors } = readOnly ? { phases: previousPhases, errors: [] as string[] } - : applyParams(clonePhases(previousPhases), params); + : applyParams(clonePhases(previousPhases), entry); // A batch with any error is discarded wholesale: persisting a // half-applied batch makes the natural retry hit "already exists" for // the ops that did land. State and rendered summary stay at previous. @@ -801,8 +854,7 @@ export class TodoTool implements AgentTool { const effective = failed ? previousPhases : updated; const completedTasks = readOnly || failed ? [] : getCompletionTransitions(previousPhases, updated); if (!readOnly && !failed) this.session.setTodoPhases?.(updated); - const storage = this.session.getSessionFile() ? "session" : "memory"; - const details: TodoToolDetails = { op: params.op, phases: effective, storage }; + const details: TodoToolDetails = { op, phases: effective, storage }; if (completedTasks.length > 0) details.completedTasks = completedTasks; return { diff --git a/packages/coding-agent/test/tools/todo.test.ts b/packages/coding-agent/test/tools/todo.test.ts index a233065b0..c0ba9a4e9 100644 --- a/packages/coding-agent/test/tools/todo.test.ts +++ b/packages/coding-agent/test/tools/todo.test.ts @@ -458,6 +458,69 @@ describe("TodoTool lenient init shapes", () => { expect(summary.text).toContain("Missing list for init operation"); }); }); +describe("TodoTool lenient op recovery", () => { + // Regression: models occasionally drop `op` while sending an otherwise + // valid payload (e.g. `{list:[...]}`). The schema keeps `op` required, but + // `lenientArgValidation` routes the raw args to execute(), which infers + // the op for unambiguous shapes instead of failing the call. + it("keeps op required at the schema boundary but lenient at execute", () => { + const tool = new TodoTool(createSession()); + expect(tool.parameters({ list: [{ phase: "Fixes", items: ["One"] }] }) instanceof type.errors).toBe(true); + expect(tool.lenientArgValidation).toBe(true); + }); + + it("infers init from a bare list payload", async () => { + const tool = new TodoTool(createSession()); + const result = await tool.execute("call-1", { + list: [{ phase: "Fixes", items: ["Bytecompiler ordering", "Posix path fd handling"] }], + } as never); + + expect(result.isError).toBeUndefined(); + expect(result.details?.op).toBe("init"); + expect(result.details?.phases.map(phase => phase.name)).toEqual(["Fixes"]); + expect(result.details?.phases[0]?.tasks.map(task => task.content)).toEqual([ + "Bytecompiler ordering", + "Posix path fd handling", + ]); + }); + + it("infers append from phase plus items", async () => { + const tool = new TodoTool(createSession()); + await tool.execute("call-1", { op: "init", list: [{ phase: "Work", items: ["First"] }] }); + + const result = await tool.execute("call-2", { phase: "Work", items: ["Second"] } as never); + + expect(result.isError).toBeUndefined(); + expect(result.details?.op).toBe("append"); + expect(result.details?.phases[0]?.tasks.map(task => task.content)).toEqual(["First", "Second"]); + }); + + it("infers init from bare items only when no todos exist", async () => { + const fresh = new TodoTool(createSession()); + const initialized = await fresh.execute("call-1", { items: ["Only task"] } as never); + expect(initialized.isError).toBeUndefined(); + expect(initialized.details?.op).toBe("init"); + + // With existing todos the same shape is ambiguous (flat init would wipe + // the list; append lacks a phase) and must error instead of guessing. + const populated = new TodoTool( + createSession([{ name: "Work", tasks: [{ content: "First", status: "pending" }] }]), + ); + const ambiguous = await populated.execute("call-2", { items: ["Second"] } as never); + expect(ambiguous.isError).toBe(true); + }); + + it("surfaces the schema error when op is missing and not inferable", async () => { + const tool = new TodoTool(createSession()); + const result = await tool.execute("call-1", { task: "Something" } as never); + + expect(result.isError).toBe(true); + const summary = result.content.find(part => part.type === "text"); + if (summary?.type !== "text") throw new Error("Expected text summary"); + expect(summary.text).toContain("Invalid todo arguments"); + expect(summary.text).toContain("op"); + }); +}); describe("TodoTool empty items tolerance", () => { // Regression: a stray `items: []` on an op that ignores items (here `view`)