diff --git a/src/tools/delegate-task/tool-argument-preparation.ts b/src/tools/delegate-task/tool-argument-preparation.ts index f39e7bbee..4fbb2cd3c 100644 --- a/src/tools/delegate-task/tool-argument-preparation.ts +++ b/src/tools/delegate-task/tool-argument-preparation.ts @@ -29,9 +29,17 @@ export async function prepareDelegateTaskArgs(args: Record, ctx title: description, }) - const runInBackground = args.run_in_background + let runInBackground = args.run_in_background if (runInBackground === undefined) { - throw new Error("Invalid arguments: 'run_in_background' parameter is REQUIRED. Specify run_in_background=false for task delegation, or run_in_background=true for parallel exploration.") + // Default to sync delegation. Tool description still nudges the model to be + // explicit, but a missing flag should not fail an otherwise valid call — + // hard-failing here burns turns and silently downgrades parallel work to + // synchronous fallbacks. See issue #4119. + runInBackground = false + log("[task] run_in_background omitted; defaulting to false (sync delegation)", { + category: args.category, + subagent_type: originalSubagentType, + }) } let loadSkills = args.load_skills @@ -45,10 +53,24 @@ export async function prepareDelegateTaskArgs(args: Record, ctx } if (loadSkills === undefined) { - throw new Error("Invalid arguments: 'load_skills' parameter is REQUIRED. Pass [] if no skills needed.") + // 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.") } diff --git a/src/tools/delegate-task/tool-description.ts b/src/tools/delegate-task/tool-description.ts index 1c0bf1dde..f8251ea04 100644 --- a/src/tools/delegate-task/tool-description.ts +++ b/src/tools/delegate-task/tool-description.ts @@ -37,36 +37,36 @@ export function createDelegateTaskPresentation(options: DelegateTaskToolOptions) }).join("\n") const description = `Spawn agent task with category-based or direct agent selection. - + ⚠️ CRITICAL: You MUST provide EITHER category OR subagent_type. Omitting BOTH will FAIL. - + **COMMON MISTAKE (DO NOT DO THIS):** \`\`\` - task(description="...", prompt="...", run_in_background=false) // ❌ FAILS - missing category AND subagent_type + task(description="...", prompt="...") // ❌ FAILS - missing category AND subagent_type \`\`\` - + **CORRECT - Using category:** \`\`\` - task(category="quick", load_skills=[], description="Fix type error", prompt="...", run_in_background=false) + task(category="quick", description="Fix type error", prompt="...") \`\`\` - - **CORRECT - Using subagent_type:** + + **CORRECT - Using subagent_type with parallel exploration:** \`\`\` - task(subagent_type="explore", load_skills=[], description="Find patterns", prompt="...", run_in_background=true) + task(subagent_type="explore", description="Find patterns", prompt="...", run_in_background=true) \`\`\` - + REQUIRED: Provide ONE of: - category: For task delegation (uses Sisyphus-Junior with category-optimized model) - subagent_type: For direct agent invocation (explore, librarian, oracle, etc.) - + **DO NOT provide both.** If category is provided, subagent_type is ignored. - - - load_skills: ALWAYS REQUIRED. Pass [] if no skills needed, or ["skill-1", "skill-2"] for category tasks. + + - load_skills: Optional. Defaults to [] when omitted. Pass ["skill-1", "skill-2"] for skill-specific tasks. - category: Use predefined category → Spawns Sisyphus-Junior with category config Available categories: ${categoryList} - subagent_type: Use specific agent directly (explore, librarian, oracle, metis, momus) - - run_in_background: REQUIRED. true=async (returns a background task ID like \`bg_...\` for \`background_output\`), false=sync (waits). Use background=true ONLY for parallel exploration with 5+ independent queries. + - run_in_background: Optional. Defaults to false (sync, waits). Set true=async (returns a background task ID like \`bg_...\` for \`background_output\`) ONLY for parallel exploration with 5+ independent queries. Sync waits use a 30-minute inactivity window: OpenCode busy/retry/running status resets the window, so this is not a total wall-clock limit. - task_id: Continuation session id (\`ses_...\`) from task metadata. Continues the same subagent session with FULL CONTEXT PRESERVED; not the background task id (\`bg_...\`). - command: The command that triggered this task (optional, for slash command tracking). diff --git a/src/tools/delegate-task/tools.test.ts b/src/tools/delegate-task/tools.test.ts index fb493535c..242a567b6 100644 --- a/src/tools/delegate-task/tools.test.ts +++ b/src/tools/delegate-task/tools.test.ts @@ -1180,19 +1180,30 @@ describe("sisyphus-task", () => { }) describe("skills parameter", () => { - test("skills parameter is required - throws error when not provided", async () => { + test("#given load_skills omitted #when executing #then defaults to [] and proceeds (fixes #4119)", async () => { // given 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: async () => ({ data: {} }), - promptAsync: async () => ({ data: {} }), - messages: async () => ({ data: [] }), + prompt: promptMock, + promptAsync: promptMock, + messages: async () => ({ + data: [{ info: { role: "assistant" }, parts: [{ type: "text", text: "Done" }] }] + }), + status: async () => ({ data: {} }), }, } @@ -1208,9 +1219,9 @@ describe("sisyphus-task", () => { abort: new AbortController().signal, } - // when - skills not provided (undefined) - // then - should throw error about missing skills - await expect(tool.execute( + // when - skills not provided (undefined); previously threw a hard error. + // then - should default to [] and proceed normally. + await tool.execute( { description: "Test task", prompt: "Do something", @@ -1218,49 +1229,48 @@ describe("sisyphus-task", () => { run_in_background: false, }, toolContext - )).rejects.toThrow("Invalid arguments: 'load_skills' parameter is REQUIRED") - }) + ) - test("null skills throws error", async () => { - // given - const { createDelegateTask } = require("./tools") - - const mockManager = { launch: async () => ({}) } - const mockClient = { - app: { agents: async () => ({ data: [] }) }, - config: { get: async () => ({ data: { model: SYSTEM_DEFAULT_MODEL } }) }, - session: { - create: async () => ({ data: { id: "test-session" } }), - prompt: async () => ({ data: {} }), - promptAsync: async () => ({ data: {} }), - messages: async () => ({ data: [] }), - }, - } - - const tool = createDelegateTask({ - manager: mockManager, - client: mockClient, - }) - - const toolContext = { - sessionID: "parent-session", - messageID: "parent-message", - agent: "sisyphus", - abort: new AbortController().signal, - } - - // when - null passed - // then - should throw error about null - await expect(tool.execute( - { - description: "Test task", - prompt: "Do something", - category: "ultrabrain", - run_in_background: false, - load_skills: null, - }, - toolContext - )).rejects.toThrow("Invalid arguments: load_skills=null is not allowed") + expect(promptBody).toBeDefined() + }, { timeout: 20000 }) + + 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") + const mockManager = { launch: async () => ({}) } + const mockClient = { + app: { agents: async () => ({ data: [] }) }, + config: { get: async () => ({ data: { model: SYSTEM_DEFAULT_MODEL } }) }, + session: { + create: async () => ({ data: { id: "test-session" } }), + prompt: async () => ({ data: {} }), + promptAsync: async () => ({ data: {} }), + messages: async () => ({ data: [] }), + }, + } + const tool = createDelegateTask({ manager: mockManager, client: mockClient }) + const toolContext = { + sessionID: "parent-session", + messageID: "parent-message", + agent: "sisyphus", + abort: new AbortController().signal, + } + + // when - load_skills explicitly set to null + // then - hard reject with clear error + await expect(tool.execute( + { + description: "Test task", + prompt: "Do something", + category: "ultrabrain", + run_in_background: false, + load_skills: null, + }, + toolContext, + )).rejects.toThrow("Invalid arguments: load_skills=null is not allowed") }) test("empty array [] is allowed and proceeds without skill content", async () => { @@ -1320,25 +1330,33 @@ describe("sisyphus-task", () => { }) describe("run_in_background parameter", () => { - test("#given category without run_in_background #when executing #then throws required parameter error", async () => { + test("#given category without run_in_background #when executing #then defaults to sync and proceeds (fixes #4119)", async () => { // given const { createDelegateTask } = require("./tools") + let promptBody: any + const promptMock = async (input: any) => { + promptBody = input.body + return { data: {} } + } const mockManager = { launch: async () => ({}) } 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: async () => ({ data: {} }), - promptAsync: async () => ({ data: {} }), - messages: async () => ({ data: [] }), + prompt: promptMock, + promptAsync: promptMock, + messages: async () => ({ + data: [{ info: { role: "assistant" }, parts: [{ type: "text", text: "Done" }] }] + }), + status: async () => ({ data: {} }), }, } const tool = createDelegateTask({ manager: mockManager, client: mockClient }) - // when - // then - await expect(tool.execute( + // when - run_in_background omitted (previously a hard throw) + await tool.execute( { description: "Category without run flag", prompt: "Do something", @@ -1346,28 +1364,39 @@ describe("sisyphus-task", () => { load_skills: [], }, { sessionID: "parent-session", messageID: "parent-message", agent: "sisyphus", abort: new AbortController().signal } - )).rejects.toThrow("Invalid arguments: 'run_in_background' parameter is REQUIRED") - }) + ) - test("#given subagent_type without run_in_background #when executing #then throws required parameter error", async () => { + // then - sync path should run and the session prompt should be sent + expect(promptBody).toBeDefined() + }, { timeout: 20000 }) + + test("#given subagent_type without run_in_background #when executing #then defaults to sync and proceeds (fixes #4119)", async () => { // given const { createDelegateTask } = require("./tools") + let promptBody: any + const promptMock = async (input: any) => { + promptBody = input.body + return { data: {} } + } const mockManager = { launch: async () => ({}) } const mockClient = { app: { agents: async () => ({ data: [{ name: "explore", mode: "subagent" }] }) }, config: { get: async () => ({ data: { model: SYSTEM_DEFAULT_MODEL } }) }, session: { + get: async () => ({ data: { directory: "/project" } }), create: async () => ({ data: { id: "test-session" } }), - prompt: async () => ({ data: {} }), - promptAsync: async () => ({ data: {} }), - messages: async () => ({ data: [] }), + prompt: promptMock, + promptAsync: promptMock, + messages: async () => ({ + data: [{ info: { role: "assistant" }, parts: [{ type: "text", text: "Done" }] }] + }), + status: async () => ({ data: {} }), }, } const tool = createDelegateTask({ manager: mockManager, client: mockClient }) // when - // then - await expect(tool.execute( + await tool.execute( { description: "Subagent without run flag", prompt: "Find patterns", @@ -1375,39 +1404,61 @@ describe("sisyphus-task", () => { load_skills: [], }, { sessionID: "parent-session", messageID: "parent-message", agent: "sisyphus", abort: new AbortController().signal } - )).rejects.toThrow("Invalid arguments: 'run_in_background' parameter is REQUIRED") - }) + ) - test("#given task_id without run_in_background #when executing #then throws required parameter error", async () => { - // given + // then + 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_1", status: "running" }) } + 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: { - create: async () => ({ data: { id: "test-session" } }), + get: async () => ({ data: { directory: "/project" } }), + create: async () => ({ data: { id: "ses_continue_test" } }), prompt: async () => ({ data: {} }), promptAsync: async () => ({ data: {} }), - messages: 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 - // then - await expect(tool.execute( + // 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_existing", + task_id: "ses_continue_test", load_skills: [], }, - { sessionID: "parent-session", messageID: "parent-message", agent: "sisyphus", abort: new AbortController().signal } - )).rejects.toThrow("Invalid arguments: 'run_in_background' parameter is REQUIRED") - }) + { sessionID: "parent-session", messageID: "parent-message", agent: "sisyphus", abort: new AbortController().signal }, + ) - test("#given no category no subagent_type no task_id and no run_in_background #when executing #then throws required parameter error", async () => { + // 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") const mockManager = { launch: async () => ({}) } @@ -1423,16 +1474,18 @@ describe("sisyphus-task", () => { } const tool = createDelegateTask({ manager: mockManager, client: mockClient }) - // when - // then - await expect(tool.execute( + // when - omitting run_in_background no longer throws, but missing category+subagent_type still produces a (returned) error. + const result = await tool.execute( { description: "Missing required args", prompt: "Do something", load_skills: [], }, { sessionID: "parent-session", messageID: "parent-message", agent: "sisyphus", abort: new AbortController().signal } - )).rejects.toThrow("Invalid arguments: 'run_in_background' parameter is REQUIRED") + ) + + // then - the missing-target error remains intact; only the run_in_background gate was removed. + expect(String(result)).toContain("Must provide either category or subagent_type") }) test("#given category without description #when executing #then auto-generates description from prompt", async () => { diff --git a/src/tools/delegate-task/tools.ts b/src/tools/delegate-task/tools.ts index 53336d752..cac8f96a1 100644 --- a/src/tools/delegate-task/tools.ts +++ b/src/tools/delegate-task/tools.ts @@ -21,12 +21,16 @@ export type { SyncSessionCreatedEvent, DelegateTaskToolOptions, BuildSystemConte export { buildSystemContent, buildTaskPrompt } from "./prompt-builder" const delegateTaskArgsSchema = { - load_skills: tool.schema.array(tool.schema.string()).describe("Skill names to inject. REQUIRED - pass [] if no skills needed."), + load_skills: tool.schema + .array(tool.schema.string()) + .optional() + .describe("Skill names to inject. Optional; defaults to [] when omitted. Pass an explicit array (e.g. [\"git-master\"]) for skill-specific tasks."), description: tool.schema.string().optional().describe("Short task description (3-5 words). Auto-generated from prompt if omitted."), prompt: tool.schema.string().describe("Full detailed prompt for the agent"), run_in_background: tool.schema .boolean() - .describe("REQUIRED. true=async (returns background task ID `bg_...` for background_output), false=sync (waits). Use false for task delegation, true ONLY for parallel exploration."), + .optional() + .describe("Optional; defaults to false (sync). true=async (returns background task ID `bg_...` for background_output), false=sync (waits). Use true ONLY for parallel exploration; otherwise omit or pass false for task delegation."), category: tool.schema.string().optional().describe("REQUIRED if subagent_type not provided. Do NOT provide both category and subagent_type."), subagent_type: tool.schema.string().optional().describe("REQUIRED if category not provided. Do NOT provide both category and subagent_type."), task_id: tool.schema