From 255f535a509749d2be06f803e5602c5b366b373a Mon Sep 17 00:00:00 2001 From: justsisyphus Date: Sat, 17 Jan 2026 20:40:35 +0900 Subject: [PATCH] refactor(delegate-task): use empty array instead of null for skills parameter - Change skills type from string[] | null to string[] - Allow skills=[] for no skills, reject skills=null - Remove emojis from error messages and prompts - Update tests accordingly --- src/tools/delegate-task/constants.ts | 6 +-- src/tools/delegate-task/tools.test.ts | 42 +++++++++----------- src/tools/delegate-task/tools.ts | 57 +++++++++++---------------- src/tools/delegate-task/types.ts | 2 +- 4 files changed, 47 insertions(+), 60 deletions(-) diff --git a/src/tools/delegate-task/constants.ts b/src/tools/delegate-task/constants.ts index 992d0bd4..1d13e085 100644 --- a/src/tools/delegate-task/constants.ts +++ b/src/tools/delegate-task/constants.ts @@ -63,7 +63,7 @@ Approach: -⚠️ THIS CATEGORY USES A LESS CAPABLE MODEL (claude-haiku-4-5). +THIS CATEGORY USES A LESS CAPABLE MODEL (claude-haiku-4-5). The model executing this task has LIMITED reasoning capacity. Your prompt MUST be: @@ -146,7 +146,7 @@ Approach: -⚠️ THIS CATEGORY USES A MID-TIER MODEL (claude-sonnet-4-5). +THIS CATEGORY USES A MID-TIER MODEL (claude-sonnet-4-5). While capable, this model benefits significantly from EXPLICIT instructions. @@ -244,7 +244,7 @@ MUTUALLY EXCLUSIVE: Provide EITHER category OR agent, not both (unless resuming) - agent: Use specific agent directly (e.g., "oracle", "explore") - background: true=async (returns task_id), false=sync (waits for result). Default: false. Use background=true ONLY for parallel exploration with 5+ independent queries. - resume: Session ID to resume (from previous task output). Continues agent with FULL CONTEXT PRESERVED - saves tokens, maintains continuity. -- skills: Array of skill names to prepend to prompt (e.g., ["playwright", "frontend-ui-ux"]). Skills will be resolved and their content prepended with a separator. Empty array [] is NOT allowed - use null if no skills needed. +- skills: Array of skill names to prepend to prompt (e.g., ["playwright", "frontend-ui-ux"]). Use [] (empty array) if no skills needed. **WHEN TO USE resume:** - Task failed/incomplete → resume with "fix: [specific issue]" diff --git a/src/tools/delegate-task/tools.test.ts b/src/tools/delegate-task/tools.test.ts index bae7a6cc..600abc05 100644 --- a/src/tools/delegate-task/tools.test.ts +++ b/src/tools/delegate-task/tools.test.ts @@ -319,7 +319,7 @@ describe("sisyphus-task", () => { prompt: "Do something", category: "ultrabrain", run_in_background: true, - skills: null, + skills: [], }, toolContext ) @@ -334,12 +334,11 @@ describe("sisyphus-task", () => { }) describe("skills parameter", () => { - test("DELEGATE_TASK_DESCRIPTION documents skills parameter with null option", () => { + test("DELEGATE_TASK_DESCRIPTION documents skills parameter with empty array option", () => { // #given / #when / #then expect(DELEGATE_TASK_DESCRIPTION).toContain("skills") expect(DELEGATE_TASK_DESCRIPTION).toContain("Array of skill names") - expect(DELEGATE_TASK_DESCRIPTION).toContain("Empty array [] is NOT allowed") - expect(DELEGATE_TASK_DESCRIPTION).toContain("null if no skills needed") + expect(DELEGATE_TASK_DESCRIPTION).toContain("[] (empty array) if no skills needed") }) test("skills parameter is required - returns error when not provided", async () => { @@ -385,7 +384,7 @@ describe("sisyphus-task", () => { expect(result).toContain("REQUIRED") }) - test("empty array [] returns error with available skills list", async () => { + test("null skills returns error", async () => { // #given const { createDelegateTask } = require("./tools") @@ -412,26 +411,26 @@ describe("sisyphus-task", () => { abort: new AbortController().signal, } - // #when - empty array passed + // #when - null passed const result = await tool.execute( { description: "Test task", prompt: "Do something", category: "ultrabrain", run_in_background: false, - skills: [], + skills: null, }, toolContext ) - // #then - should return error about empty array with guidance - expect(result).toContain("❌") - expect(result).toContain("Empty array []") - expect(result).toContain("not allowed") + // #then - should return error about null + expect(result).toContain("Invalid arguments") expect(result).toContain("skills=null") + expect(result).toContain("not allowed") + expect(result).toContain("skills=[]") }) - test("null skills is allowed and proceeds without skill content", async () => { + test("empty array [] is allowed and proceeds without skill content", async () => { // #given const { createDelegateTask } = require("./tools") let promptBody: any @@ -466,21 +465,20 @@ describe("sisyphus-task", () => { abort: new AbortController().signal, } - // #when - null skills passed + // #when - empty array skills passed await tool.execute( { description: "Test task", prompt: "Do something", category: "ultrabrain", run_in_background: false, - skills: null, + skills: [], }, toolContext ) // #then - should proceed without system content from skills expect(promptBody).toBeDefined() - // system should not contain skill content (only category prompt append if any) }, { timeout: 20000 }) }) @@ -540,7 +538,7 @@ describe("sisyphus-task", () => { prompt: "Continue the task", resume: "ses_resume_test", run_in_background: false, - skills: null, + skills: [], }, toolContext ) @@ -595,7 +593,7 @@ describe("sisyphus-task", () => { prompt: "Continue in background", resume: "ses_bg_resume", run_in_background: true, - skills: null, + skills: [], }, toolContext ) @@ -650,13 +648,12 @@ describe("sisyphus-task", () => { prompt: "Do something", category: "ultrabrain", run_in_background: false, - skills: null, + skills: [], }, toolContext ) // #then - should return detailed error message with args and stack trace - expect(result).toContain("❌") expect(result).toContain("Send prompt failed") expect(result).toContain("JSON Parse error") expect(result).toContain("**Arguments**:") @@ -711,7 +708,7 @@ describe("sisyphus-task", () => { prompt: "Do something", category: "ultrabrain", run_in_background: false, - skills: null, + skills: [], }, toolContext ) @@ -764,13 +761,12 @@ describe("sisyphus-task", () => { prompt: "Do something", category: "ultrabrain", run_in_background: false, - skills: null, + skills: [], }, toolContext ) // #then - should return agent not found error - expect(result).toContain("❌") expect(result).toContain("not found") expect(result).toContain("registered") }) @@ -819,7 +815,7 @@ describe("sisyphus-task", () => { prompt: "test", category: "custom-cat", run_in_background: false, - skills: null + skills: [] }, toolContext) // #then diff --git a/src/tools/delegate-task/tools.ts b/src/tools/delegate-task/tools.ts index 371fc9cb..ee66f3d1 100644 --- a/src/tools/delegate-task/tools.ts +++ b/src/tools/delegate-task/tools.ts @@ -64,7 +64,7 @@ function formatDetailedError(error: unknown, ctx: ErrorContext): string { const stack = error instanceof Error ? error.stack : undefined const lines: string[] = [ - `❌ ${ctx.operation} failed`, + `${ctx.operation} failed`, "", `**Error**: ${message}`, ] @@ -181,37 +181,28 @@ export function createDelegateTask(options: DelegateTaskToolOptions): ToolDefini subagent_type: tool.schema.string().optional().describe("Agent name directly (e.g., 'oracle', 'explore'). Mutually exclusive with category."), run_in_background: tool.schema.boolean().describe("Run in background. MUST be explicitly set. Use false for task delegation, true only for parallel exploration."), resume: tool.schema.string().optional().describe("Session ID to resume - continues previous agent session with full context"), - skills: tool.schema.array(tool.schema.string()).nullable().describe("Array of skill names to prepend to the prompt. Use null if no skills needed. Empty array [] is NOT allowed."), + skills: tool.schema.array(tool.schema.string()).describe("Array of skill names to prepend to the prompt. Use [] (empty array) if no skills needed."), }, async execute(args: DelegateTaskArgs, toolContext) { const ctx = toolContext as ToolContextWithMetadata if (args.run_in_background === undefined) { - return `❌ Invalid arguments: 'run_in_background' parameter is REQUIRED. Use run_in_background=false for task delegation, run_in_background=true only for parallel exploration.` + return `Invalid arguments: 'run_in_background' parameter is REQUIRED. Use run_in_background=false for task delegation, run_in_background=true only for parallel exploration.` } if (args.skills === undefined) { - return `❌ Invalid arguments: 'skills' parameter is REQUIRED. Use skills=null if no skills are needed, or provide an array of skill names.` + return `Invalid arguments: 'skills' parameter is REQUIRED. Use skills=[] if no skills are needed, or provide an array of skill names.` } - if (Array.isArray(args.skills) && args.skills.length === 0) { - const allSkills = await discoverSkills({ includeClaudeCodePaths: true }) - const availableSkillsList = allSkills.map(s => ` - ${s.name}`).slice(0, 15).join("\n") - return `❌ Invalid arguments: Empty array [] is not allowed for 'skills' parameter. - -Use skills=null if this task genuinely requires no specialized skills. -Otherwise, select appropriate skills from available options: - -${availableSkillsList}${allSkills.length > 15 ? `\n ... and ${allSkills.length - 15} more` : ""} - -If you believe no skills are needed, you MUST explicitly explain why to the user before using skills=null.` + if (args.skills === null) { + return `Invalid arguments: skills=null is not allowed. Use skills=[] (empty array) if no skills are needed.` } const runInBackground = args.run_in_background === true let skillContent: string | undefined - if (args.skills !== null && args.skills.length > 0) { + if (args.skills.length > 0) { const { resolved, notFound } = await resolveMultipleSkillsAsync(args.skills, { gitMasterConfig }) if (notFound.length > 0) { const allSkills = await discoverSkills({ includeClaudeCodePaths: true }) const available = allSkills.map(s => s.name).join(", ") - return `❌ Skills not found: ${notFound.join(", ")}. Available: ${available}` + return `Skills not found: ${notFound.join(", ")}. Available: ${available}` } skillContent = Array.from(resolved.values()).join("\n\n") } @@ -334,7 +325,7 @@ Use \`background_output\` with task_id="${task.id}" to check progress.` toastManager.removeTask(taskId) } const errorMessage = promptError instanceof Error ? promptError.message : String(promptError) - return `❌ Failed to send resume prompt: ${errorMessage}\n\nSession ID: ${args.resume}` + return `Failed to send resume prompt: ${errorMessage}\n\nSession ID: ${args.resume}` } // Wait for message stability after prompt completes @@ -372,7 +363,7 @@ Use \`background_output\` with task_id="${task.id}" to check progress.` if (toastManager) { toastManager.removeTask(taskId) } - return `❌ Error fetching result: ${messagesResult.error}\n\nSession ID: ${args.resume}` + return `Error fetching result: ${messagesResult.error}\n\nSession ID: ${args.resume}` } const messages = ((messagesResult as { data?: unknown }).data ?? messagesResult) as Array<{ @@ -390,7 +381,7 @@ Use \`background_output\` with task_id="${task.id}" to check progress.` } if (!lastMessage) { - return `❌ No assistant response found.\n\nSession ID: ${args.resume}` + return `No assistant response found.\n\nSession ID: ${args.resume}` } // Extract text from both "text" and "reasoning" parts (thinking models use "reasoning") @@ -409,11 +400,11 @@ ${textContent || "(No text output)"}` } if (args.category && args.subagent_type) { - return `❌ Invalid arguments: Provide EITHER category OR subagent_type, not both.` + return `Invalid arguments: Provide EITHER category OR subagent_type, not both.` } if (!args.category && !args.subagent_type) { - return `❌ Invalid arguments: Must provide either category or subagent_type.` + return `Invalid arguments: Must provide either category or subagent_type.` } // Fetch OpenCode config at boundary to get system default model @@ -443,7 +434,7 @@ ${textContent || "(No text output)"}` systemDefaultModel, }) if (!resolved) { - return `❌ Unknown category: "${args.category}". Available: ${Object.keys({ ...DEFAULT_CATEGORIES, ...userCategories }).join(", ")}` + return `Unknown category: "${args.category}". Available: ${Object.keys({ ...DEFAULT_CATEGORIES, ...userCategories }).join(", ")}` } // Determine model source by comparing against the actual resolved model @@ -452,11 +443,11 @@ ${textContent || "(No text output)"}` const categoryDefaultModel = DEFAULT_CATEGORIES[args.category]?.model if (!actualModel) { - return `❌ No model configured. Set a model in your OpenCode config, plugin config, or use a category with a default model.` + return `No model configured. Set a model in your OpenCode config, plugin config, or use a category with a default model.` } if (!parseModelString(actualModel)) { - return `❌ Invalid model format "${actualModel}". Expected "provider/model" format (e.g., "anthropic/claude-sonnet-4-5").` + return `Invalid model format "${actualModel}". Expected "provider/model" format (e.g., "anthropic/claude-sonnet-4-5").` } switch (actualModel) { @@ -484,7 +475,7 @@ ${textContent || "(No text output)"}` categoryPromptAppend = resolved.promptAppend || undefined } else { if (!args.subagent_type?.trim()) { - return `❌ Agent name cannot be empty.` + return `Agent name cannot be empty.` } const agentName = args.subagent_type.trim() agentToUse = agentName @@ -501,13 +492,13 @@ ${textContent || "(No text output)"}` if (!callableNames.includes(agentToUse)) { const isPrimaryAgent = agents.some((a) => a.name === agentToUse && a.mode === "primary") if (isPrimaryAgent) { - return `❌ Cannot call primary agent "${agentToUse}" via delegate_task. Primary agents are top-level orchestrators.` + return `Cannot call primary agent "${agentToUse}" via delegate_task. Primary agents are top-level orchestrators.` } const availableAgents = callableNames .sort() .join(", ") - return `❌ Unknown agent: "${agentToUse}". Available agents: ${availableAgents}` + return `Unknown agent: "${agentToUse}". Available agents: ${availableAgents}` } } catch { // If we can't fetch agents, proceed anyway - the session.prompt will fail with a clearer error @@ -527,7 +518,7 @@ ${textContent || "(No text output)"}` parentModel, parentAgent, model: categoryModel, - skills: args.skills ?? undefined, + skills: args.skills.length > 0 ? args.skills : undefined, skillContent: systemContent, }) @@ -576,7 +567,7 @@ System notifies on completion. Use \`background_output\` with task_id="${task.id }) if (createResult.error) { - return `❌ Failed to create session: ${createResult.error}` + return `Failed to create session: ${createResult.error}` } const sessionID = createResult.data.id @@ -591,7 +582,7 @@ System notifies on completion. Use \`background_output\` with task_id="${task.id description: args.description, agent: agentToUse, isBackground: false, - skills: args.skills ?? undefined, + skills: args.skills.length > 0 ? args.skills : undefined, modelInfo, }) } @@ -713,7 +704,7 @@ System notifies on completion. Use \`background_output\` with task_id="${task.id }) if (messagesResult.error) { - return `❌ Error fetching result: ${messagesResult.error}\n\nSession ID: ${sessionID}` + return `Error fetching result: ${messagesResult.error}\n\nSession ID: ${sessionID}` } const messages = ((messagesResult as { data?: unknown }).data ?? messagesResult) as Array<{ @@ -727,7 +718,7 @@ System notifies on completion. Use \`background_output\` with task_id="${task.id const lastMessage = assistantMessages[0] if (!lastMessage) { - return `❌ No assistant response found.\n\nSession ID: ${sessionID}` + return `No assistant response found.\n\nSession ID: ${sessionID}` } // Extract text from both "text" and "reasoning" parts (thinking models use "reasoning") diff --git a/src/tools/delegate-task/types.ts b/src/tools/delegate-task/types.ts index c34be089..f99e68e8 100644 --- a/src/tools/delegate-task/types.ts +++ b/src/tools/delegate-task/types.ts @@ -5,5 +5,5 @@ export interface DelegateTaskArgs { subagent_type?: string run_in_background: boolean resume?: string - skills: string[] | null + skills: string[] }