From 9b151a2551aabad2f8d517ed1fa5fd422ca60494 Mon Sep 17 00:00:00 2001 From: Claude Agent Date: Tue, 19 May 2026 09:08:59 +0200 Subject: [PATCH] =?UTF-8?q?fix(delegate-task):=20address=20Oracle=20review?= =?UTF-8?q?=20on=20PR=20#4121=20=E2=80=94=20preserve=20explicit-null=20rej?= =?UTF-8?q?ect=20+=20rewrite=20continuation=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two blockers from the maintainer's Oracle review on PR #4121: Blocker 1 — load_skills=null should still throw, omitted should default The previous PR collapsed both `loadSkills === undefined` and `loadSkills === null` into a silent default of `[]`. The closing rationale of PR #1663 (which reverted PR #1493) and the maintainer's review both call out the importance of preserving the distinct "omitted -> default, explicit invalid -> throw" contract. `null` strongly signals "I tried to pass something and it was wrong"; silently coercing it hides bugs upstream. Restored the split: `undefined` -> default `[]` + log, `null` -> throw with the historical error string. Blocker 2 — task_id continuation test rewritten, not deleted The original PR removed the `task_id without run_in_background -> throws` test entirely. The behavior IS preserved (default false -> `isExplicitSyncRun` true -> `executeSyncContinuation`), but with the test gone the new contract was unprotected. Added a regression test that asserts the new contract: when `task_id` is present and `run_in_background` is omitted, `tool.execute` must route through sync continuation without throwing the legacy required-parameter error. Mocks include `session.abort` because the sync poller calls it during shutdown. Also flipped the existing `load_skills=null` regression test from "normalizes to []" back to "throws with the legacy error string" to match the restored contract. Tests: - bun test src/tools/delegate-task/tools.test.ts -> 132/132 pass - bun test src/tools/delegate-task/ -> 406/406 pass - bun run typecheck -> clean --- .../tool-argument-preparation.ts | 28 ++++-- src/tools/delegate-task/tools.test.ts | 94 +++++++++++++------ 2 files changed, 82 insertions(+), 40 deletions(-) diff --git a/src/tools/delegate-task/tool-argument-preparation.ts b/src/tools/delegate-task/tool-argument-preparation.ts index 2b09c5533..4fbb2cd3c 100644 --- a/src/tools/delegate-task/tool-argument-preparation.ts +++ b/src/tools/delegate-task/tool-argument-preparation.ts @@ -52,16 +52,26 @@ export async function prepareDelegateTaskArgs(args: Record, ctx } } - if (loadSkills === undefined || loadSkills === null) { - // Default to no skills. Same rationale as run_in_background above: callers - // that omit the field already implicitly mean "no skill content needed". - if (loadSkills === null) { - log("[task] load_skills=null received; normalizing to []", { - category: args.category, - subagent_type: originalSubagentType, - }) - } + if (loadSkills === undefined) { + // Default to no skills when the field is OMITTED. Callers that don't + // pass the field implicitly mean "no skill content needed". This is + // what fixes the #4119 retry loop when Sisyphus / Claude Code Agent + // SDK forget the argument. loadSkills = [] + log("[task] load_skills omitted; defaulting to []", { + category: args.category, + subagent_type: originalSubagentType, + }) + } + + if (loadSkills === null) { + // Explicit `null` is REJECTED loudly. The "omitted -> default, explicit + // invalid -> throw" contract was the closing rationale of PR #1663 + // (which reverted PR #1493) and the maintainer's Oracle review on PR + // #4121 explicitly requested we preserve it. `null` strongly signals + // "I tried to pass something and it was wrong" - silently coercing + // hides bugs upstream. + throw new Error("Invalid arguments: load_skills=null is not allowed. Pass [] if no skills needed.") } const normalizedLoadSkills = Array.isArray(loadSkills) diff --git a/src/tools/delegate-task/tools.test.ts b/src/tools/delegate-task/tools.test.ts index de8fe491f..980c7b47e 100644 --- a/src/tools/delegate-task/tools.test.ts +++ b/src/tools/delegate-task/tools.test.ts @@ -1234,38 +1234,24 @@ describe("sisyphus-task", () => { expect(promptBody).toBeDefined() }, { timeout: 20000 }) - test("#given load_skills=null #when executing #then normalizes to [] and proceeds (fixes #4119)", async () => { - // given + test("#given load_skills=null #when executing #then throws (explicit invalid stays rejected, fixes #4119 review)", async () => { + // given - maintainer's Oracle review on PR #4121 (blocker 1) required us + // to preserve the historical "omitted defaults, explicit null throws" + // contract from PR #1663. Omitted load_skills still defaults to [] (see + // the test above); explicit null is loud. const { createDelegateTask } = require("./tools") - let promptBody: any - const mockManager = { launch: async () => ({}) } - - const promptMock = async (input: any) => { - promptBody = input.body - return { data: {} } - } - const mockClient = { app: { agents: async () => ({ data: [] }) }, config: { get: async () => ({ data: { model: SYSTEM_DEFAULT_MODEL } }) }, session: { - get: async () => ({ data: { directory: "/project" } }), create: async () => ({ data: { id: "test-session" } }), - prompt: promptMock, - promptAsync: promptMock, - messages: async () => ({ - data: [{ info: { role: "assistant" }, parts: [{ type: "text", text: "Done" }] }] - }), - status: async () => ({ data: {} }), + prompt: async () => ({ data: {} }), + promptAsync: async () => ({ data: {} }), + messages: async () => ({ data: [] }), }, } - - const tool = createDelegateTask({ - manager: mockManager, - client: mockClient, - }) - + const tool = createDelegateTask({ manager: mockManager, client: mockClient }) const toolContext = { sessionID: "parent-session", messageID: "parent-message", @@ -1273,9 +1259,9 @@ describe("sisyphus-task", () => { abort: new AbortController().signal, } - // when - load_skills=null passed; previously threw "load_skills=null is not allowed". - // then - should normalize to [] and proceed. - await tool.execute( + // when - load_skills explicitly set to null + // then - hard reject with clear error + await expect(tool.execute( { description: "Test task", prompt: "Do something", @@ -1283,11 +1269,9 @@ describe("sisyphus-task", () => { run_in_background: false, load_skills: null, }, - toolContext - ) - - expect(promptBody).toBeDefined() - }, { timeout: 20000 }) + toolContext, + )).rejects.toThrow("Invalid arguments: load_skills=null is not allowed") + }) test("empty array [] is allowed and proceeds without skill content", async () => { // given @@ -1426,6 +1410,54 @@ describe("sisyphus-task", () => { expect(promptBody).toBeDefined() }, { timeout: 20000 }) + test("#given task_id without run_in_background #when executing #then defaults to sync continuation (fixes #4119)", async () => { + // given - mock manager.resume to return a running task, and capture + // which session-prompt method gets called. The previous PR removed the + // 'task_id without run_in_background throws' assertion; the maintainer's + // Oracle review on PR #4121 asked us to rewrite it (not delete it) so + // the new contract "default false routes to sync continuation" is + // pinned by a regression test rather than implicit behavior. + const { createDelegateTask } = require("./tools") + const mockManager = { + resume: async () => ({ id: "task-1", sessionId: "ses_continue_test", status: "running" }), + } + const mockClient = { + app: { agents: async () => ({ data: [] }) }, + config: { get: async () => ({ data: { model: SYSTEM_DEFAULT_MODEL } }) }, + session: { + get: async () => ({ data: { directory: "/project" } }), + create: async () => ({ data: { id: "ses_continue_test" } }), + prompt: async () => ({ data: {} }), + promptAsync: async () => ({ data: {} }), + messages: async () => ({ + data: [{ info: { id: "msg_1", role: "assistant", time: { created: Date.now() }, finish: "end_turn" }, parts: [{ type: "text", text: "Continued" }] }], + }), + status: async () => ({ data: { "ses_continue_test": { type: "idle" } } }), + abort: async () => ({ data: {} }), + }, + } + const tool = createDelegateTask({ manager: mockManager, client: mockClient }) + + // when - omit run_in_background; task_id + default false must route to + // executeSyncContinuation (tools.ts:75). Previously this threw the + // 'run_in_background REQUIRED' error, which #4119 reported as the cause + // of Sisyphus's retry storms. + const result = await tool.execute( + { + description: "Continue without run flag", + prompt: "Continue", + task_id: "ses_continue_test", + load_skills: [], + }, + { sessionID: "parent-session", messageID: "parent-message", agent: "sisyphus", abort: new AbortController().signal }, + ) + + // then - no throw, returned content is a string (the sync continuation + // returned without erroring out on the missing run_in_background flag). + expect(typeof result).toBe("string") + expect(String(result)).not.toContain("'run_in_background' parameter is REQUIRED") + }, { timeout: 20000 }) + test("#given no category no subagent_type and no run_in_background #when executing #then still returns the (different) missing-target error (fixes #4119)", async () => { // given const { createDelegateTask } = require("./tools")