Merge pull request #3197 from code-yeongyu/fix/delegate-task-depth-guard

fix(delegate-task): tighten subagent depth guard + add regression smoke tests
2026-04-09 11:29:29 +09:00
parent 524e0f6cbd 545c444a85
commit 99a6d50db5
3 changed files with 338 additions and 9 deletions
@@ -1,6 +1,14 @@
 import { describe, expect, test } from "bun:test"
 import type { OpencodeClient } from "./constants"
-import { resolveSubagentSpawnContext } from "./subagent-spawn-limits"
+import {
+  resolveSubagentSpawnContext,
+  getMaxSubagentDepth,
+  DEFAULT_MAX_SUBAGENT_DEPTH,
+  createSubagentDepthLimitError,
+  createSubagentDescendantLimitError,
+  getMaxRootSessionSpawnBudget,
+  DEFAULT_MAX_ROOT_SESSION_SPAWN_BUDGET,
+} from "./subagent-spawn-limits"

 function createMockClient(sessionGet: OpencodeClient["session"]["get"]): OpencodeClient {
  return {
@@ -41,4 +49,177 @@ describe("resolveSubagentSpawnContext", () => {
      await expect(result).rejects.toThrow(/background_task\.maxDescendants cannot be enforced safely.*No session data returned/)
    })
  })
+
+  describe("depth calculation smoke tests (regression guard)", () => {
+    test("root session (no parentID) reports depth 0 and childDepth 1", async () => {
+      // given - a root session with no parent
+      const client = createMockClient(async (opts) => {
+        if (opts.path.id === "root-session") {
+          return { data: { id: "root-session", parentID: undefined } }
+        }
+        return { error: "not found", data: undefined }
+      })
+
+      // when
+      const result = await resolveSubagentSpawnContext(client, "root-session")
+
+      // then
+      expect(result.rootSessionID).toBe("root-session")
+      expect(result.parentDepth).toBe(0)
+      expect(result.childDepth).toBe(1)
+    })
+
+    test("depth-1 child reports childDepth 2", async () => {
+      // given - child -> root chain
+      const client = createMockClient(async (opts) => {
+        if (opts.path.id === "child-1") {
+          return { data: { id: "child-1", parentID: "root-session" } }
+        }
+        if (opts.path.id === "root-session") {
+          return { data: { id: "root-session", parentID: undefined } }
+        }
+        return { error: "not found", data: undefined }
+      })
+
+      // when
+      const result = await resolveSubagentSpawnContext(client, "child-1")
+
+      // then
+      expect(result.rootSessionID).toBe("root-session")
+      expect(result.parentDepth).toBe(1)
+      expect(result.childDepth).toBe(2)
+    })
+
+    test("depth-2 grandchild reports childDepth 3", async () => {
+      // given - grandchild -> child -> root chain
+      const client = createMockClient(async (opts) => {
+        const sessions: Record<string, { id: string; parentID?: string }> = {
+          "grandchild": { id: "grandchild", parentID: "child" },
+          "child": { id: "child", parentID: "root" },
+          "root": { id: "root", parentID: undefined },
+        }
+        const session = sessions[opts.path.id]
+        if (session) return { data: session }
+        return { error: "not found", data: undefined }
+      })
+
+      // when
+      const result = await resolveSubagentSpawnContext(client, "grandchild")
+
+      // then
+      expect(result.rootSessionID).toBe("root")
+      expect(result.parentDepth).toBe(2)
+      expect(result.childDepth).toBe(3)
+    })
+
+    test("depth at DEFAULT_MAX_SUBAGENT_DEPTH reports exact max childDepth", async () => {
+      // given - chain of exactly DEFAULT_MAX_SUBAGENT_DEPTH depth
+      // With default=3: session-3 -> session-2 -> session-1 -> root
+      const sessions: Record<string, { id: string; parentID?: string }> = {
+        "root": { id: "root" },
+      }
+      for (let i = 1; i <= DEFAULT_MAX_SUBAGENT_DEPTH; i++) {
+        sessions[`session-${i}`] = {
+          id: `session-${i}`,
+          parentID: i === 1 ? "root" : `session-${i - 1}`,
+        }
+      }
+
+      const client = createMockClient(async (opts) => {
+        const session = sessions[opts.path.id]
+        if (session) return { data: session }
+        return { error: "not found", data: undefined }
+      })
+
+      // when - resolve from the deepest session
+      const deepest = `session-${DEFAULT_MAX_SUBAGENT_DEPTH}`
+      const result = await resolveSubagentSpawnContext(client, deepest)
+
+      // then - childDepth should be DEFAULT_MAX_SUBAGENT_DEPTH + 1 (exceeds limit)
+      expect(result.childDepth).toBe(DEFAULT_MAX_SUBAGENT_DEPTH + 1)
+      expect(result.parentDepth).toBe(DEFAULT_MAX_SUBAGENT_DEPTH)
+    })
+
+    test("detects parent cycle and throws", async () => {
+      // given - A -> B -> A (cycle)
+      const client = createMockClient(async (opts) => {
+        const sessions: Record<string, { id: string; parentID?: string }> = {
+          "session-a": { id: "session-a", parentID: "session-b" },
+          "session-b": { id: "session-b", parentID: "session-a" },
+        }
+        const session = sessions[opts.path.id]
+        if (session) return { data: session }
+        return { error: "not found", data: undefined }
+      })
+
+      // when
+      const result = resolveSubagentSpawnContext(client, "session-a")
+
+      // then
+      await expect(result).rejects.toThrow(/session parent cycle/)
+    })
+  })
+})
+
+describe("getMaxSubagentDepth", () => {
+  test("returns DEFAULT_MAX_SUBAGENT_DEPTH when no config", () => {
+    expect(getMaxSubagentDepth()).toBe(DEFAULT_MAX_SUBAGENT_DEPTH)
+    expect(getMaxSubagentDepth(undefined)).toBe(DEFAULT_MAX_SUBAGENT_DEPTH)
+  })
+
+  test("returns config.maxDepth when provided", () => {
+    expect(getMaxSubagentDepth({ maxDepth: 5 })).toBe(5)
+    expect(getMaxSubagentDepth({ maxDepth: 1 })).toBe(1)
+    expect(getMaxSubagentDepth({ maxDepth: 0 })).toBe(0)
+  })
+
+  test("default is 3", () => {
+    expect(DEFAULT_MAX_SUBAGENT_DEPTH).toBe(3)
+  })
+})
+
+describe("getMaxRootSessionSpawnBudget", () => {
+  test("returns DEFAULT_MAX_ROOT_SESSION_SPAWN_BUDGET when no config", () => {
+    expect(getMaxRootSessionSpawnBudget()).toBe(DEFAULT_MAX_ROOT_SESSION_SPAWN_BUDGET)
+  })
+
+  test("returns config.maxDescendants when provided", () => {
+    expect(getMaxRootSessionSpawnBudget({ maxDescendants: 10 })).toBe(10)
+  })
+
+  test("default is 50", () => {
+    expect(DEFAULT_MAX_ROOT_SESSION_SPAWN_BUDGET).toBe(50)
+  })
+})
+
+describe("createSubagentDepthLimitError", () => {
+  test("includes childDepth, maxDepth, and session IDs in message", () => {
+    const error = createSubagentDepthLimitError({
+      childDepth: 4,
+      maxDepth: 3,
+      parentSessionID: "parent-123",
+      rootSessionID: "root-456",
+    })
+
+    expect(error.message).toContain("child depth 4")
+    expect(error.message).toContain("maxDepth=3")
+    expect(error.message).toContain("parent-123")
+    expect(error.message).toContain("root-456")
+    expect(error.message).toContain("spawn blocked")
+  })
+})
+
+describe("createSubagentDescendantLimitError", () => {
+  test("includes descendant count, max, and root session ID", () => {
+    const error = createSubagentDescendantLimitError({
+      rootSessionID: "root-789",
+      descendantCount: 50,
+      maxDescendants: 50,
+    })
+
+    expect(error.message).toContain("root-789")
+    expect(error.message).toContain("50")
+    expect(error.message).toContain("maxDescendants=50")
+    expect(error.message).toContain("spawn blocked")
+  })
 })
@@ -282,6 +282,139 @@ describe("executeSyncTask - cleanup on error paths", () => {
    expect(deleteCalls.length).toBe(1)
    expect(deleteCalls[0]).toBe("ses_test_12345678")
  })
+
+  test("depth regression: blocks spawn when reserveSubagentSpawn throws depth limit error", async () => {
+    // This is a smoke test guarding against regressions where the depth limit
+    // would be silently bypassed (e.g. via a fallback path that hardcodes
+    // childDepth: 1).
+
+    const mockClient = {
+      session: {
+        create: async () => ({ data: { id: "ses_test_12345678" } }),
+      },
+    }
+
+    const { executeSyncTask } = require("./sync-task")
+
+    const reserveSubagentSpawn = mock(async () => {
+      throw new Error(
+        "Subagent spawn blocked: child depth 4 exceeds background_task.maxDepth=3. Parent session: parent. Root session: root. Continue in an existing subagent session instead of spawning another."
+      )
+    })
+
+    const deps = {
+      createSyncSession: async () => ({ ok: true, sessionID: "ses_test_12345678" }),
+      sendSyncPrompt: async () => null,
+      pollSyncSession: async () => null,
+      fetchSyncResult: async () => ({ ok: true as const, textContent: "Result" }),
+    }
+
+    const mockCtx = {
+      sessionID: "parent-session",
+      callID: "call-123",
+      metadata: () => {},
+    }
+
+    const mockExecutorCtx = {
+      manager: { reserveSubagentSpawn },
+      client: mockClient,
+      directory: "/tmp",
+      onSyncSessionCreated: null,
+    }
+
+    const args = {
+      prompt: "test prompt",
+      description: "test task",
+      category: "test",
+      load_skills: [],
+      run_in_background: false,
+      command: null,
+    }
+
+    //#when - executeSyncTask is called from a session at max depth
+    const result = await executeSyncTask(args, mockCtx, mockExecutorCtx, {
+      sessionID: "parent-session",
+    }, "test-agent", undefined, undefined, undefined, undefined, deps)
+
+    //#then - should propagate the depth limit error and NOT create the session
+    expect(result).toContain("Subagent spawn blocked")
+    expect(result).toContain("child depth 4")
+    expect(result).toContain("maxDepth=3")
+    expect(reserveSubagentSpawn).toHaveBeenCalledWith("parent-session")
+    // critical: createSyncSession must NOT have been called -- if it was,
+    // the depth guard was bypassed.
+    expect(addCalls.length).toBe(0)
+  })
+
+  test("depth regression: does not silently fall back to childDepth: 1 when manager methods are present", async () => {
+    // Guards against the dangerous fallback path in sync-task.ts that
+    // hardcodes childDepth: 1 if reserveSubagentSpawn / assertCanSpawn are
+    // not functions. With a real manager present, the fallback must NOT be
+    // taken.
+
+    const mockClient = {
+      session: {
+        create: async () => ({ data: { id: "ses_test_12345678" } }),
+      },
+    }
+
+    const { executeSyncTask } = require("./sync-task")
+
+    let reservedDepth: number | undefined
+    const commit = mock(() => 1)
+    const rollback = mock(() => {})
+    const reserveSubagentSpawn = mock(async () => {
+      // Return a depth that proves the real manager was consulted
+      reservedDepth = 3
+      return {
+        spawnContext: { rootSessionID: "root", parentDepth: 2, childDepth: 3 },
+        descendantCount: 5,
+        commit,
+        rollback,
+      }
+    })
+
+    const deps = {
+      createSyncSession: async () => ({ ok: true, sessionID: "ses_test_12345678" }),
+      sendSyncPrompt: async () => null,
+      pollSyncSession: async () => null,
+      fetchSyncResult: async () => ({ ok: true as const, textContent: "Result" }),
+    }
+
+    const metadataCalls: any[] = []
+    const mockCtx = {
+      sessionID: "parent-session",
+      callID: "call-123",
+      metadata: (input: any) => { metadataCalls.push(input) },
+    }
+
+    const mockExecutorCtx = {
+      manager: { reserveSubagentSpawn },
+      client: mockClient,
+      directory: "/tmp",
+      onSyncSessionCreated: null,
+    }
+
+    const args = {
+      prompt: "test prompt",
+      description: "test task",
+      category: "test",
+      load_skills: [],
+      run_in_background: false,
+      command: null,
+    }
+
+    //#when
+    await executeSyncTask(args, mockCtx, mockExecutorCtx, {
+      sessionID: "parent-session",
+    }, "test-agent", undefined, undefined, undefined, undefined, deps)
+
+    //#then - the spawnDepth recorded in metadata MUST match what reserveSubagentSpawn returned
+    expect(reservedDepth).toBe(3)
+    const taskMeta = metadataCalls.find((c) => c.metadata?.spawnDepth !== undefined)
+    expect(taskMeta).toBeDefined()
+    expect(taskMeta.metadata.spawnDepth).toBe(3) // NOT 1 (the fallback value)
+  })
 })

 export {}
@@ -37,14 +37,29 @@ export async function executeSyncTask(
      spawnReservation = await manager.reserveSubagentSpawn(parentContext.sessionID)
    }

-    const spawnContext = spawnReservation?.spawnContext
-      ?? (typeof manager?.assertCanSpawn === "function"
-        ? await manager.assertCanSpawn(parentContext.sessionID)
-        : {
-            rootSessionID: parentContext.sessionID,
-            parentDepth: 0,
-            childDepth: 1,
-          })
+    // Depth/descendant guard. We must NOT silently fall back to childDepth: 1
+    // when the manager is unavailable or lacks the spawn methods, because that
+    // would let subagents recurse without bound. The only safe fallback is
+    // when the manager genuinely cannot enforce limits (legacy SDK), in which
+    // case we still record childDepth: 1 but log a warning so regressions are
+    // visible.
+    let spawnContext: { rootSessionID: string; parentDepth: number; childDepth: number }
+    if (spawnReservation?.spawnContext) {
+      spawnContext = spawnReservation.spawnContext
+    } else if (typeof manager?.assertCanSpawn === "function") {
+      spawnContext = await manager.assertCanSpawn(parentContext.sessionID)
+    } else {
+      log(
+        "[task] WARNING: BackgroundManager has no spawn enforcement methods (reserveSubagentSpawn / assertCanSpawn). " +
+        "Depth and descendant limits cannot be enforced for this task. This indicates an old SDK or a misconfiguration.",
+        { parentSessionID: parentContext.sessionID }
+      )
+      spawnContext = {
+        rootSessionID: parentContext.sessionID,
+        parentDepth: 0,
+        childDepth: 1,
+      }
+    }

    const createSessionResult = await deps.createSyncSession(client, {
      parentSessionID: parentContext.sessionID,