From 1b9c6be12999ec0ba88bfcbd45d7506b80b94d7e Mon Sep 17 00:00:00 2001 From: roboomp Date: Tue, 30 Jun 2026 11:37:58 +0000 Subject: [PATCH] fix(task): respect task.maxConcurrency + task.maxRecursionDepth across spawn paths Three independent paths bypassed the user's subagent caps: 1. TaskTool.#getSpawnSemaphore sized the spawn semaphore from task.maxConcurrency only on first use and never re-read the setting, so lowering the cap mid-session left every later spawn running against the old ceiling. Resize the live semaphore against the current setting on each acquire. 2. The task tool prompt threaded MAX_CONCURRENCY through to the template but never rendered it. A model with task.maxConcurrency=1 could still emit oversized tasks[] batches that registered immediately and piled up behind the semaphore. Render a 'Concurrency cap' directive in task.md whenever the setting is bounded. 3. The eval agent() bridge's assertDepthAllowed gated only against the hardcoded EVAL_AGENT_MAX_DEPTH=3 and ignored task.maxRecursionDepth, so a user-tightened recursion limit (0='None', 1='Single') still let cell-spawned subagents recurse to depth 3. Mirror the task tool's canSpawnAtDepth gate, clamped by the hard ceiling. Fixes #3895 --- packages/coding-agent/CHANGELOG.md | 4 ++ .../src/eval/__tests__/agent-bridge.test.ts | 56 ++++++++++++++- .../coding-agent/src/eval/agent-bridge.ts | 18 +++-- .../coding-agent/src/prompts/tools/task.md | 3 + packages/coding-agent/src/task/index.ts | 13 +++- .../coding-agent/test/task/task-spawn.test.ts | 70 +++++++++++++++++++ 6 files changed, 156 insertions(+), 8 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 46831be05..fbe85b50f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed `task.maxConcurrency` and `task.maxRecursionDepth` being silently exceeded by sub-spawn paths. (1) The session-scoped spawn semaphore was sized from the setting only on first use and never re-read, so lowering the cap mid-session left every later spawn running against the old ceiling — `TaskTool.#getSpawnSemaphore` now `resize()`s the live semaphore against the current setting on every acquire. (2) The task tool description never surfaced the cap to the model, so a model handed `task.maxConcurrency=1` could still emit oversized `tasks[]` batches that piled up behind the semaphore; `task.md` now renders a `Concurrency cap` directive whenever the setting is bounded. (3) The eval `agent()` bridge gated only against the hardcoded `EVAL_AGENT_MAX_DEPTH=3` ceiling and ignored `task.maxRecursionDepth`, so a user-tightened recursion limit (e.g. `0`/"None", `1`/"Single") still let cell-spawned subagents recurse up to depth 3; the bridge now mirrors the task tool's `canSpawnAtDepth` gate, clamped by the hard ceiling. ([#3895](https://github.com/can1357/oh-my-pi/issues/3895)) + ## [16.2.8] - 2026-06-30 ### Added diff --git a/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts b/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts index 30c4678c1..fa3588131 100644 --- a/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts +++ b/packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts @@ -205,6 +205,48 @@ describe("runEvalAgent", () => { expect(runSpy).not.toHaveBeenCalled(); }); + it("honors task.maxRecursionDepth on top of the hard eval ceiling", async () => { + mockAgents(); + const runSpy = vi.spyOn(taskExecutor, "runSubprocess").mockImplementation(async options => singleResult(options)); + + // task.maxRecursionDepth=0 means "no spawning at all" — even depth 0 (the + // top-level agent) must be blocked, matching canSpawnAtDepth(). + await expect( + runEvalAgent( + { prompt: "hello" }, + { + session: makeSession({ + settings: Settings.isolated({ + "async.enabled": false, + "task.isolation.mode": "none", + "task.maxRecursionDepth": 0, + }), + }), + }, + ), + ).rejects.toThrow("maximum depth is 0"); + + // task.maxRecursionDepth=1 ("Single") lets the top spawn but a depth-1 + // subagent cannot spawn further — even though the hard ceiling is 3. + await expect( + runEvalAgent( + { prompt: "hello" }, + { + session: makeSession({ + depth: 1, + settings: Settings.isolated({ + "async.enabled": false, + "task.isolation.mode": "none", + "task.maxRecursionDepth": 1, + }), + }), + }, + ), + ).rejects.toThrow("maximum depth is 1"); + + expect(runSpy).not.toHaveBeenCalled(); + }); + it("throws instead of spawning from plan mode", async () => { mockAgents(); const runSpy = vi.spyOn(taskExecutor, "runSubprocess").mockImplementation(async options => singleResult(options)); @@ -220,7 +262,19 @@ describe("runEvalAgent", () => { const runSpy = vi.spyOn(taskExecutor, "runSubprocess").mockImplementation(async options => singleResult(options)); const abortController = new AbortController(); const schema = { type: "object", properties: { ok: { type: "boolean" } } }; - const session = makeSession({ depth: 2, activeModel: "p/current", modelString: "p/fallback" }); + const session = makeSession({ + depth: 2, + activeModel: "p/current", + modelString: "p/fallback", + settings: Settings.isolated({ + "async.enabled": false, + "task.isolation.mode": "none", + "task.enableLsp": true, + // Default task.maxRecursionDepth is 2, which would now (correctly) + // block depth=2 — widen it so the test still exercises depth=2. + "task.maxRecursionDepth": -1, + }), + }); await runEvalAgent( { prompt: " hello ", label: "My Agent", model: "p/override", schema }, diff --git a/packages/coding-agent/src/eval/agent-bridge.ts b/packages/coding-agent/src/eval/agent-bridge.ts index 301709281..a87add315 100644 --- a/packages/coding-agent/src/eval/agent-bridge.ts +++ b/packages/coding-agent/src/eval/agent-bridge.ts @@ -24,7 +24,7 @@ import { runIsolatedSubprocess, } from "../task/isolation-runner"; import { AgentOutputManager } from "../task/output-manager"; -import type { AgentDefinition, AgentProgress, SingleResult } from "../task/types"; +import { type AgentDefinition, type AgentProgress, canSpawnAtDepth, type SingleResult } from "../task/types"; import { type NestedRepoPatch, parseIsolationMode } from "../task/worktree"; import type { ToolSession } from "../tools"; import { ToolError } from "../tools/tool-errors"; @@ -36,7 +36,11 @@ import "../tools/review"; /** Synthetic bridge name reserved for the `agent()` helper across both runtimes. */ export const EVAL_AGENT_BRIDGE_NAME = "__agent__"; -/** Hard recursion limit for eval-driven subagents. */ +/** + * Hard recursion ceiling for eval-driven subagents. The user setting + * `task.maxRecursionDepth` is honored on top of this — whichever is tighter + * wins, so a maintainer-friendly cap can't get raised by a user setting. + */ export const EVAL_AGENT_MAX_DEPTH = 3; const DEFAULT_AGENT_TYPE = "task"; @@ -131,9 +135,15 @@ function parseAgentArgs(args: unknown): EvalAgentArgs { function assertDepthAllowed(session: ToolSession): void { const taskDepth = session.taskDepth ?? 0; - if (taskDepth >= EVAL_AGENT_MAX_DEPTH) { + // Honor the user's `task.maxRecursionDepth` (mirroring the task tool's gate + // in tools/index.ts) but never above the hard ceiling. `< 0` means + // "Unlimited" in the same schema `canSpawnAtDepth` reads, so it falls back + // to the hard ceiling instead of going past it. + const settingMax = session.settings.get("task.maxRecursionDepth") ?? 2; + const effectiveMax = settingMax < 0 ? EVAL_AGENT_MAX_DEPTH : Math.min(settingMax, EVAL_AGENT_MAX_DEPTH); + if (!canSpawnAtDepth(effectiveMax, taskDepth)) { throw new ToolError( - `agent() cannot spawn another agent at task depth ${taskDepth}; maximum depth is ${EVAL_AGENT_MAX_DEPTH}.`, + `agent() cannot spawn another agent at task depth ${taskDepth}; maximum depth is ${effectiveMax} (task.maxRecursionDepth=${settingMax}, hard ceiling=${EVAL_AGENT_MAX_DEPTH}).`, ); } } diff --git a/packages/coding-agent/src/prompts/tools/task.md b/packages/coding-agent/src/prompts/tools/task.md index 4b192a888..b141e9a78 100644 --- a/packages/coding-agent/src/prompts/tools/task.md +++ b/packages/coding-agent/src/prompts/tools/task.md @@ -4,6 +4,9 @@ Execution blocks your turn: the call only returns once the work is completely fi # Delegation Strategy - **Maximize parallelism:** Break work into the widest possible {{#if batchEnabled}}array of `tasks[]`{{else}}set of parallel `task` calls{{/if}}. NEVER serialize work that can run concurrently. Tasks touching different files or independent refactors should run in parallel; agents resolve their own file collisions live. +{{#when MAX_CONCURRENCY ">" 0}} +- **Concurrency cap:** At most {{pluralize MAX_CONCURRENCY "subagent" "subagents"}} run at once in this session — anything beyond that just queues, so a {{#if batchEnabled}}`tasks[]` batch{{else}}set of parallel `task` calls{{/if}} larger than {{MAX_CONCURRENCY}} only delays results. Keep the fan-out at or under the cap. +{{/when}} - **Sequence only when necessary:** The only reason to run A before B is if B strictly requires A's output to function (e.g., a core API contract or schema migration). {{#if ircEnabled}}If the missing piece is small, run them in parallel and have B ask A via `irc`!{{/if}} - **Role matching:** Assign each subagent a specific `role` (e.g. "Security Reviewer", "DB Migrator"). Do not spawn generic workers. - **No overhead:** Each assignment MUST instruct its agent to skip formatters, linters, and project-wide test suites. You will run those once at the end. diff --git a/packages/coding-agent/src/task/index.ts b/packages/coding-agent/src/task/index.ts index d61208f65..a12f7677c 100644 --- a/packages/coding-agent/src/task/index.ts +++ b/packages/coding-agent/src/task/index.ts @@ -498,8 +498,10 @@ export class TaskTool implements AgentTool { ]); }); } + + it("re-reads task.maxConcurrency on each spawn so a mid-session change applies on the next acquire", async () => { + vi.spyOn(discoveryModule, "discoverAgents").mockResolvedValue({ + agents: [taskAgent], + projectAgentsDir: null, + }); + const started: string[] = []; + const gates = new Map(); + vi.spyOn(executorModule, "runSubprocess").mockImplementation(async options => { + const id = options.id ?? "?"; + started.push(id); + const gate = deferred(); + gates.set(id, gate); + await gate.promise; + return makeResult(id); + }); + + const manager = createManager(); + const settings = Settings.isolated({ "task.maxConcurrency": 4 }); + const tool = await TaskTool.create({ + cwd: "/tmp", + hasUI: false, + settings, + getSessionFile: () => null, + getSessionSpawns: () => "*", + asyncJobManager: manager, + } as unknown as ToolSession); + + // Prime the semaphore at the initial high cap. + const first = await tool.execute("tc-1", { agent: "task", id: "First", assignment: "Work A." } as TaskParams); + await pollUntil(() => started.length === 1); + + // Tighten the cap mid-session. The next spawn MUST see the new ceiling. + settings.override("task.maxConcurrency", 1); + const second = await tool.execute("tc-2", { agent: "task", id: "Second", assignment: "Work B." } as TaskParams); + const secondJob = manager.getJob(second.details!.async!.jobId)!; + + // First is still running (and holding the only slot under the new cap), + // so Second is parked at the semaphore — queued, not running. + await Bun.sleep(20); + expect(started).toEqual(["First"]); + expect(secondJob.queued).toBe(true); + + // Releasing First admits Second. + gates.get("First")!.resolve(); + await manager.getJob(first.details!.async!.jobId)!.promise; + await pollUntil(() => started.length === 2); + expect(started).toEqual(["First", "Second"]); + + gates.get("Second")!.resolve(); + await secondJob.promise; + }); + + it("surfaces task.maxConcurrency in the tool description so the model can self-throttle", async () => { + vi.spyOn(discoveryModule, "discoverAgents").mockResolvedValue({ + agents: [taskAgent], + projectAgentsDir: null, + }); + + const cappedTool = await TaskTool.create(createSession({ settings: { "task.maxConcurrency": 1 } })); + expect(cappedTool.description).toContain("At most 1 subagent"); + expect(cappedTool.description).toContain("Concurrency cap"); + + const fanoutTool = await TaskTool.create(createSession({ settings: { "task.maxConcurrency": 4 } })); + expect(fanoutTool.description).toContain("At most 4 subagents"); + + // `0` = Unlimited in the settings UI; the prompt must NOT advertise a cap. + const unboundedTool = await TaskTool.create(createSession({ settings: { "task.maxConcurrency": 0 } })); + expect(unboundedTool.description).not.toContain("Concurrency cap"); + }); });