Merge PR #3896: fix(task): respect task.maxConcurrency + task.maxRecursionDepth across spawn paths (@roboomp)

# Conflicts: # packages/coding-agent/src/eval/__tests__/agent-bridge.test.ts # packages/coding-agent/src/eval/agent-bridge.ts
2026-07-01 22:07:28 +02:00
parent 5ca07b1759 1b9c6be129
commit 1cb8608a58
6 changed files with 155 additions and 8 deletions
@@ -159,6 +159,9 @@
 ### Fixed

 - Fixed `/copy code` and `/copy cmd` being treated as normal prompts instead of copying the latest code or command block. ([#3893](https://github.com/can1357/oh-my-pi/issues/3893))
+### 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

@@ -219,6 +219,48 @@ describe("runEvalAgent", () => {
 		expect(runSpy.mock.calls[0]?.[0].agent.name).toBe("reviewer");
 	});

+	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));
@@ -234,7 +276,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 },
@@ -25,7 +25,7 @@ import {
 } from "../task/isolation-runner";
 import { AgentOutputManager } from "../task/output-manager";
 import { resolveSpawnPolicy } from "../task/spawn-policy";
-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";
@@ -37,7 +37,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_LABEL = "EvalAgent";
@@ -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}).`,
 		);
 	}
 }
@@ -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}}
 {{#if ircEnabled}}- **Steering delivery:** Parent-to-subagent IRC is delivered immediately as steering; subagents blocked in `job poll` / `irc wait` do not need to poll separately for it.{{/if}}
 - **Role matching:** Assign each subagent a specific `role` (e.g. "Security Reviewer", "DB Migrator"). Do not spawn generic workers.
@@ -501,8 +501,10 @@ export class TaskTool implements AgentTool<TaskToolSchemaInstance, TaskToolDetai
 	readonly #blockedAgent: string | undefined;
 	/**
 	 * One semaphore per TaskTool instance (i.e. per session): bounds concurrent
-	 * subagents across parallel `task` calls within the session. Sized from
-	 * `task.maxConcurrency` at first use; later setting changes do not resize it.
+	 * subagents across parallel `task` calls within the session. Resized in
+	 * place from `task.maxConcurrency` on every acquire so a mid-session
+	 * settings change (UI toggle, `/settings`) takes effect on the next spawn,
+	 * rather than baking in whatever the cap was when the first spawn ran.
 	 */
 	#spawnSemaphore: Semaphore | undefined;

@@ -545,7 +547,12 @@ export class TaskTool implements AgentTool<TaskToolSchemaInstance, TaskToolDetai
 	}

 	#getSpawnSemaphore(): Semaphore {
-		this.#spawnSemaphore ??= new Semaphore(this.session.settings.get("task.maxConcurrency"));
+		const max = this.session.settings.get("task.maxConcurrency");
+		if (this.#spawnSemaphore) {
+			this.#spawnSemaphore.resize(max);
+		} else {
+			this.#spawnSemaphore = new Semaphore(max);
+		}
 		return this.#spawnSemaphore;
 	}

@@ -273,4 +273,74 @@ describe("task spawn routing", () => {
 			]);
 		});
 	}
+
+	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<string, Deferred>();
+		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");
+	});
 });