fix(tools): leniently inferred missing todo op from unambiguous payloads
- Kept op required in the todo schema; lenientArgValidation now routes raw args to execute(), where resolveTodoParams re-validates and repairs an omitted op (list -> init, phase+items -> append, bare items on empty list -> init). - Ambiguous op-less calls surface the schema error as a retryable tool error instead of a hard validation failure.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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<string, unknown>, 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<typeof todoSchema, TodoToolDetails> {
|
||||
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<typeof todoSchema.infer>[] = [
|
||||
{
|
||||
@@ -789,11 +831,22 @@ export class TodoTool implements AgentTool<typeof todoSchema, TodoToolDetails> {
|
||||
_context?: AgentToolContext,
|
||||
): Promise<AgentToolResult<TodoToolDetails>> {
|
||||
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<typeof todoSchema, TodoToolDetails> {
|
||||
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 {
|
||||
|
||||
@@ -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`)
|
||||
|
||||
Reference in New Issue
Block a user