fix(delegate-task): address Oracle review on PR #4121 — preserve explicit-null reject + rewrite continuation test

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

src/tools/delegate-task/tool-argument-preparation.ts

@@ -52,16 +52,26 @@ export async function prepareDelegateTaskArgs(args: Record<string, unknown>, 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)

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")