Skip to content

Commit 17b95cf

Browse files
committed
Route update_plan onto the real manage_tasks handler
createUpdatePlanProxy called runTool("manage_tasks", ...), but runTool forwards only to posixTools, which has no manage_tasks handler — every update_plan call errored with "unknown tool: manage_tasks". Give createCodexToolProxies its own runManageTasks callback and wire each mount site (tools.ts, subagent/run.ts) to the same manage_tasks logic their stringTool handler already uses. Also fix requireOk's hardcoded "apply_patch failed" label to use the actual label argument. Tests: dropped the manage_tasks special case from the apply_patch mock recorder (posixTools has no such handler, so it now falls through to the accurate "unknown tool" branch), and added coverage that dispatches through the real parseManageTasksArgs/applyManageTasks pair and through the real createAgentToolset mount with unstubbed posixTools — both would have caught the dead dispatch.
1 parent d9b7a5b commit 17b95cf

5 files changed

Lines changed: 192 additions & 51 deletions

File tree

‎src/agent/codex-tool-mount.test.ts‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,40 @@ describe("Codex apply_patch mount", () => {
8585
await toolset.dispose();
8686
});
8787

88+
test("update_plan dispatches through the real mount without hitting posixTools", async () => {
89+
// Unstubbed createPosixTools (real temp dir): update_plan used to call
90+
// runTool("manage_tasks", ...), which forwards onto posixTools.run and
91+
// fails with "unknown tool: manage_tasks" — posixTools has no
92+
// manage_tasks handler. This exercises the real createAgentToolset mount
93+
// (src/agent/tools.ts) end to end, not a mock recorder, so it would have
94+
// caught that dead dispatch.
95+
const cwd = mkdtempSync(join(tmpdir(), "corbits-codex-mount-"));
96+
const { createAgentToolset } = await import("./tools.js");
97+
const permissionGate = {
98+
check: async () => ({ allowed: true }),
99+
getSkipPermissions: () => false,
100+
} as never;
101+
102+
const toolset = await createAgentToolset({
103+
cwd,
104+
permissionGate,
105+
onOperatorGate: async () => ({ kind: "option", index: 0 }),
106+
isCodex: true,
107+
});
108+
const result = await toolset.dynamicRunner.run(
109+
{
110+
id: "call-1",
111+
name: "update_plan",
112+
arguments: {
113+
plan: [{ step: "Do the thing", status: "in_progress" }],
114+
},
115+
},
116+
new AbortController().signal,
117+
);
118+
expect(result.isError).toBeFalsy();
119+
await toolset.dispose();
120+
});
121+
88122
test("IMPLEMENT_TOOLS and DOCS_TOOLS include apply_patch; CORE_TOOL_NAMES does not", () => {
89123
expect(IMPLEMENT_TOOLS).toContain("apply_patch");
90124
expect(DOCS_TOOLS).toContain("apply_patch");
@@ -95,6 +129,7 @@ describe("Codex apply_patch mount", () => {
95129
const proxies = createCodexToolProxies({
96130
isCodex: true,
97131
runTool: async () => ({ content: "ok" }),
132+
runManageTasks: async () => ({ content: "ok" }),
98133
});
99134
expect(proxies.map((t) => t.definition.name)).toEqual([
100135
"apply_patch",
@@ -120,6 +155,7 @@ describe("Codex apply_patch mount", () => {
120155
const proxies = createCodexToolProxies({
121156
isCodex: true,
122157
runTool: async () => ({ content: "ok" }),
158+
runManageTasks: async () => ({ content: "ok" }),
123159
allowDelete: allowDeleteFromCapabilities(docsCapabilities),
124160
allowShell: allowShellFromCapabilities(docsCapabilities),
125161
});
@@ -138,6 +174,7 @@ describe("Codex apply_patch mount", () => {
138174
const proxies = createCodexToolProxies({
139175
isCodex: false,
140176
runTool: async () => ({ content: "ok" }),
177+
runManageTasks: async () => ({ content: "ok" }),
141178
allowDelete: allowDeleteFromCapabilities({ mode: "allow", tools: IMPLEMENT_TOOLS }),
142179
allowShell: allowShellFromCapabilities({ mode: "allow", tools: IMPLEMENT_TOOLS }),
143180
});

‎src/agent/codex-tool-proxies.test.ts‎

Lines changed: 97 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,19 @@ import {
66
allowDeleteFromCapabilities,
77
allowShellFromCapabilities,
88
createCodexToolProxies,
9+
type CodexRunManageTasks,
910
type CodexRunTool,
1011
} from "./codex-tool-proxies.js";
1112
import { DOCS_TOOLS, IMPLEMENT_TOOLS } from "./directors/tool-sets.js";
13+
import { applyManageTasks, parseManageTasksArgs, type Task } from "./tasks.js";
1214

1315
type Call = { name: string; args: Record<string, unknown> };
1416

17+
// `manage_tasks` is deliberately NOT a branch here: the real posixTools
18+
// registry runTool forwards to has no manage_tasks handler (only
19+
// read_file/write_file/run_shell/edit_file/search_files/grep + the
20+
// delete_file plugin), so an unrecognized name falling through to the
21+
// `unknown tool` branch is the accurate stand-in for that registry.
1522
function makeRecorder(initial: Record<string, string> = {}): {
1623
calls: Call[];
1724
files: Map<string, string>;
@@ -42,14 +49,39 @@ function makeRecorder(initial: Record<string, string> = {}): {
4249
if (name === "run_shell") {
4350
return { content: `ran: ${JSON.stringify(args)}` };
4451
}
45-
if (name === "manage_tasks") {
46-
return { content: "Tasks updated." };
47-
}
4852
return { content: `unknown tool: ${name}`, isError: true };
4953
};
5054
return { calls, files, runTool };
5155
}
5256

57+
const unusedManageTasks: CodexRunManageTasks = async () => ({ content: "unused" });
58+
59+
// A real manage_tasks dispatch: parses with the actual arktype schema and
60+
// mutates a real Task[] with the actual applyManageTasks reducer from
61+
// tasks.ts — the same two functions the manage_tasks stringTool handlers in
62+
// src/agent/tools.ts and src/subagent/run.ts call. No mock recorder involved.
63+
function makeRealManageTasks(): {
64+
calls: Record<string, unknown>[];
65+
getTasks: () => Task[];
66+
runManageTasks: CodexRunManageTasks;
67+
} {
68+
let tasks: Task[] = [];
69+
const calls: Record<string, unknown>[] = [];
70+
const runManageTasks: CodexRunManageTasks = async (rawArgs) => {
71+
calls.push(rawArgs);
72+
const parsed = parseManageTasksArgs(rawArgs);
73+
if (parsed === null) {
74+
return {
75+
content: "Error: manage_tasks requires action ('create' or 'update').",
76+
isError: true,
77+
};
78+
}
79+
tasks = applyManageTasks(tasks, parsed);
80+
return { content: "Tasks updated." };
81+
};
82+
return { calls, getTasks: () => tasks, runManageTasks };
83+
}
84+
5385
async function invokeApplyPatch(tools: AgentTool[], input: string) {
5486
const runner = createToolRunner(tools);
5587
return runner.run(
@@ -68,6 +100,7 @@ describe("createCodexToolProxies", () => {
68100
const tools = createCodexToolProxies({
69101
isCodex: false,
70102
runTool: async () => ({ content: "unused" }),
103+
runManageTasks: unusedManageTasks,
71104
});
72105
expect(tools).toEqual([]);
73106
});
@@ -76,6 +109,7 @@ describe("createCodexToolProxies", () => {
76109
const tools = createCodexToolProxies({
77110
isCodex: true,
78111
runTool: async () => ({ content: "unused" }),
112+
runManageTasks: unusedManageTasks,
79113
});
80114
expect(tools.map((t) => t.definition.name)).toEqual(["apply_patch", "shell", "update_plan"]);
81115
expect(tools.every((t) => t.kind === "string")).toBe(true);
@@ -86,7 +120,7 @@ describe("createCodexToolProxies", () => {
86120

87121
test("add forwards write_file with Codex trailing newline", async () => {
88122
const { calls, files, runTool } = makeRecorder();
89-
const tools = createCodexToolProxies({ isCodex: true, runTool });
123+
const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks });
90124
const result = await invokeApplyPatch(
91125
tools,
92126
`*** Begin Patch
@@ -109,7 +143,7 @@ describe("createCodexToolProxies", () => {
109143

110144
test("delete forwards delete_file", async () => {
111145
const { calls, files, runTool } = makeRecorder({ "obsolete.txt": "gone" });
112-
const tools = createCodexToolProxies({ isCodex: true, runTool });
146+
const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks });
113147
const result = await invokeApplyPatch(
114148
tools,
115149
`*** Begin Patch
@@ -125,7 +159,7 @@ describe("createCodexToolProxies", () => {
125159

126160
test("allowDelete false refuses Delete without calling delete_file", async () => {
127161
const { calls, files, runTool } = makeRecorder({ "obsolete.txt": "gone" });
128-
const tools = createCodexToolProxies({ isCodex: true, runTool, allowDelete: false });
162+
const tools = createCodexToolProxies({ isCodex: true, runTool, allowDelete: false, runManageTasks: unusedManageTasks });
129163
const result = await invokeApplyPatch(
130164
tools,
131165
`*** Begin Patch
@@ -145,7 +179,7 @@ describe("createCodexToolProxies", () => {
145179
print("Hi")
146180
`;
147181
const { calls, files, runTool } = makeRecorder({ "src/app.py": original });
148-
const tools = createCodexToolProxies({ isCodex: true, runTool, allowDelete: false });
182+
const tools = createCodexToolProxies({ isCodex: true, runTool, allowDelete: false, runManageTasks: unusedManageTasks });
149183
const result = await invokeApplyPatch(
150184
tools,
151185
`*** Begin Patch
@@ -169,7 +203,7 @@ print("Hi")
169203
print("Hi")
170204
`;
171205
const { calls, files, runTool } = makeRecorder({ "src/app.py": original });
172-
const tools = createCodexToolProxies({ isCodex: true, runTool, allowDelete: false });
206+
const tools = createCodexToolProxies({ isCodex: true, runTool, allowDelete: false, runManageTasks: unusedManageTasks });
173207
const result = await invokeApplyPatch(
174208
tools,
175209
`*** Begin Patch
@@ -193,7 +227,7 @@ print("Hi")
193227
print("bye")
194228
`;
195229
const { calls, files, runTool } = makeRecorder({ "src/app.py": original });
196-
const tools = createCodexToolProxies({ isCodex: true, runTool });
230+
const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks });
197231
const result = await invokeApplyPatch(
198232
tools,
199233
`*** Begin Patch
@@ -219,7 +253,7 @@ print("bye")
219253
print("Hi")
220254
`;
221255
const { calls, files, runTool } = makeRecorder({ "src/app.py": original });
222-
const tools = createCodexToolProxies({ isCodex: true, runTool });
256+
const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks });
223257
const result = await invokeApplyPatch(
224258
tools,
225259
`*** Begin Patch
@@ -250,7 +284,7 @@ print("Hello, world!")
250284
"src/app.py": "old\n",
251285
"obsolete.txt": "x",
252286
});
253-
const tools = createCodexToolProxies({ isCodex: true, runTool });
287+
const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks });
254288
const result = await invokeApplyPatch(
255289
tools,
256290
`*** Begin Patch
@@ -278,7 +312,7 @@ print("Hello, world!")
278312

279313
test("parse failure surfaces as tool error (isError)", async () => {
280314
const { calls, runTool } = makeRecorder();
281-
const tools = createCodexToolProxies({ isCodex: true, runTool });
315+
const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks });
282316
const result = await invokeApplyPatch(
283317
tools,
284318
`*** Add File: a.txt
@@ -295,6 +329,7 @@ print("Hello, world!")
295329
const tools = createCodexToolProxies({
296330
isCodex: true,
297331
runTool: async () => ({ content: "unused" }),
332+
runManageTasks: unusedManageTasks,
298333
});
299334
const runner = createToolRunner(tools);
300335
const result = await runner.run(
@@ -307,7 +342,7 @@ print("Hello, world!")
307342

308343
test("runTool isError aborts the patch with isError", async () => {
309344
const { runTool } = makeRecorder();
310-
const tools = createCodexToolProxies({ isCodex: true, runTool });
345+
const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks });
311346
const result = await invokeApplyPatch(
312347
tools,
313348
`*** Begin Patch
@@ -326,29 +361,29 @@ print("Hello, world!")
326361
describe("shell proxy", () => {
327362
test("string command forwards to run_shell", async () => {
328363
const { calls, runTool } = makeRecorder();
329-
const tools = createCodexToolProxies({ isCodex: true, runTool });
364+
const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks });
330365
const result = await invokeTool(tools, "shell", { command: "ls -la" });
331366
expect(result.isError).toBeFalsy();
332367
expect(calls).toEqual([{ name: "run_shell", args: { command: "ls -la" } }]);
333368
});
334369

335370
test("bash -lc argv triple unwraps to the script", async () => {
336371
const { calls, runTool } = makeRecorder();
337-
const tools = createCodexToolProxies({ isCodex: true, runTool });
372+
const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks });
338373
await invokeTool(tools, "shell", { command: ["bash", "-lc", "echo 'hi there'"] });
339374
expect(calls).toEqual([{ name: "run_shell", args: { command: "echo 'hi there'" } }]);
340375
});
341376

342377
test("other argv arrays are shell-quoted and joined", async () => {
343378
const { calls, runTool } = makeRecorder();
344-
const tools = createCodexToolProxies({ isCodex: true, runTool });
379+
const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks });
345380
await invokeTool(tools, "shell", { command: ["echo", "hello world"] });
346381
expect(calls).toEqual([{ name: "run_shell", args: { command: "echo 'hello world'" } }]);
347382
});
348383

349384
test("workdir and timeout_ms translate to cwd and timeout", async () => {
350385
const { calls, runTool } = makeRecorder();
351-
const tools = createCodexToolProxies({ isCodex: true, runTool });
386+
const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks });
352387
await invokeTool(tools, "shell", {
353388
command: "pwd",
354389
workdir: "/tmp/work",
@@ -361,7 +396,7 @@ describe("shell proxy", () => {
361396

362397
test("missing command surfaces as tool error", async () => {
363398
const { calls, runTool } = makeRecorder();
364-
const tools = createCodexToolProxies({ isCodex: true, runTool });
399+
const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks });
365400
const result = await invokeTool(tools, "shell", {});
366401
expect(result.isError).toBe(true);
367402
expect(result.content).toMatch(/command/);
@@ -370,7 +405,7 @@ describe("shell proxy", () => {
370405

371406
test("allowShell false refuses without calling run_shell", async () => {
372407
const { calls, runTool } = makeRecorder();
373-
const tools = createCodexToolProxies({ isCodex: true, runTool, allowShell: false });
408+
const tools = createCodexToolProxies({ isCodex: true, runTool, allowShell: false, runManageTasks: unusedManageTasks });
374409
const result = await invokeTool(tools, "shell", { command: "ls" });
375410
expect(result.isError).toBe(true);
376411
expect(result.content).toMatch(/not allowed/);
@@ -379,17 +414,31 @@ describe("shell proxy", () => {
379414

380415
test("run_shell isError propagates as tool error", async () => {
381416
const runTool: CodexRunTool = async () => ({ content: "boom", isError: true });
382-
const tools = createCodexToolProxies({ isCodex: true, runTool });
417+
const tools = createCodexToolProxies({ isCodex: true, runTool, runManageTasks: unusedManageTasks });
383418
const result = await invokeTool(tools, "shell", { command: "ls" });
384419
expect(result.isError).toBe(true);
385420
expect(result.content).toMatch(/boom/);
386421
});
387422
});
388423

389424
describe("update_plan proxy", () => {
390-
test("maps plan steps onto manage_tasks(action=create)", async () => {
391-
const { calls, runTool } = makeRecorder();
392-
const tools = createCodexToolProxies({ isCodex: true, runTool });
425+
// Real dispatch, not a mock recorder: runManageTasks here is
426+
// makeRealManageTasks, which parses with the real parseManageTasksArgs and
427+
// mutates a real Task[] with the real applyManageTasks reducer from
428+
// tasks.ts — the same two functions the manage_tasks stringTool handlers
429+
// wire up in src/agent/tools.ts and src/subagent/run.ts. This is what would
430+
// have caught the dead-dispatch bug: routing update_plan through `runTool`
431+
// (which only reaches posixTools, with no manage_tasks handler) fails with
432+
// "unknown tool: manage_tasks" the instant this real dispatch is invoked,
433+
// even though the old mock recorder's `if (name === "manage_tasks")`
434+
// special case made every existing test pass.
435+
test("maps plan steps onto manage_tasks(action=create) and actually mutates the task list", async () => {
436+
const { calls, getTasks, runManageTasks } = makeRealManageTasks();
437+
const tools = createCodexToolProxies({
438+
isCodex: true,
439+
runTool: async () => ({ content: "unused" }),
440+
runManageTasks,
441+
});
393442
const result = await invokeTool(tools, "update_plan", {
394443
explanation: "getting started",
395444
plan: [
@@ -401,22 +450,30 @@ describe("update_plan proxy", () => {
401450
expect(result.isError).toBeFalsy();
402451
expect(calls).toEqual([
403452
{
404-
name: "manage_tasks",
405-
args: {
406-
action: "create",
407-
tasks: [
408-
{ id: "p1", title: "Read the file", status: "done" },
409-
{ id: "p2", title: "Write the fix", status: "doing" },
410-
{ id: "p3", title: "Run tests", status: "todo" },
411-
],
412-
},
453+
action: "create",
454+
tasks: [
455+
{ id: "p1", title: "Read the file", status: "done" },
456+
{ id: "p2", title: "Write the fix", status: "doing" },
457+
{ id: "p3", title: "Run tests", status: "todo" },
458+
],
413459
},
414460
]);
461+
// The real Task[] state, produced by the real applyManageTasks reducer —
462+
// proof the dispatch reaches an actual task store, not just a recorded call.
463+
expect(getTasks()).toEqual([
464+
{ id: "p1", title: "Read the file", status: "done" },
465+
{ id: "p2", title: "Write the fix", status: "doing" },
466+
{ id: "p3", title: "Run tests", status: "todo" },
467+
]);
415468
});
416469

417470
test("malformed plan surfaces as tool error", async () => {
418-
const { calls, runTool } = makeRecorder();
419-
const tools = createCodexToolProxies({ isCodex: true, runTool });
471+
const { calls, runManageTasks } = makeRealManageTasks();
472+
const tools = createCodexToolProxies({
473+
isCodex: true,
474+
runTool: async () => ({ content: "unused" }),
475+
runManageTasks,
476+
});
420477
const result = await invokeTool(tools, "update_plan", {
421478
plan: [{ step: "no status here" }],
422479
});
@@ -426,8 +483,12 @@ describe("update_plan proxy", () => {
426483
});
427484

428485
test("missing plan surfaces as tool error", async () => {
429-
const { calls, runTool } = makeRecorder();
430-
const tools = createCodexToolProxies({ isCodex: true, runTool });
486+
const { calls, runManageTasks } = makeRealManageTasks();
487+
const tools = createCodexToolProxies({
488+
isCodex: true,
489+
runTool: async () => ({ content: "unused" }),
490+
runManageTasks,
491+
});
431492
const result = await invokeTool(tools, "update_plan", {});
432493
expect(result.isError).toBe(true);
433494
expect(calls).toEqual([]);

0 commit comments

Comments
 (0)