From 1a35547f4ee72bddcd2e2470a670d24c3297ca10 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 4 Apr 2026 02:24:21 +0800 Subject: [PATCH 1/8] feat(core): implement intelligent tool parallelism via Kind-based batching Replace the hard-coded Agent-vs-others split with consecutive batching based on tool Kind. Read-only tools (Read, Search, Fetch, Think) now execute in parallel; mutating tools (Edit, Execute) run sequentially. - Add CONCURRENCY_SAFE_KINDS set to tools.ts - Add partitionToolCalls() for consecutive batch grouping - Add isConcurrencySafe() helper (Agent name + Kind check) - Add runConcurrently() with configurable concurrency cap (QWEN_CODE_MAX_TOOL_CONCURRENCY env var, default 10) - Update MockTool to support custom Kind for testing Co-Authored-By: Claude Opus 4.6 (1M context) --- .../core/src/core/coreToolScheduler.test.ts | 32 +++--- packages/core/src/core/coreToolScheduler.ts | 97 +++++++++++++++---- packages/core/src/test-utils/mock-tool.ts | 3 +- packages/core/src/tools/tools.ts | 8 ++ 4 files changed, 106 insertions(+), 34 deletions(-) diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index 1cbe79a0aed..86e2a530408 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -3119,7 +3119,7 @@ describe('Fire hook functions integration', () => { expect(startIndices.every((i) => i < firstEnd)).toBe(true); }); - it('should run agent tools concurrently while other tools run sequentially', async () => { + it('should run concurrency-safe tools in parallel and unsafe tools sequentially', async () => { const executionLog: string[] = []; const agentTool = new MockTool({ @@ -3138,10 +3138,11 @@ describe('Fire hook functions integration', () => { const readTool = new MockTool({ name: 'read_file', + kind: Kind.Read, execute: async (params) => { const id = (params as { id: string }).id; executionLog.push(`read:start:${id}`); - await new Promise((r) => setTimeout(r, 20)); + await new Promise((r) => setTimeout(r, 50)); executionLog.push(`read:end:${id}`); return { llmContent: `Read ${id} done`, @@ -3163,6 +3164,8 @@ describe('Fire hook functions integration', () => { ); const abortController = new AbortController(); + // All 4 calls are concurrency-safe (read_file=Kind.Read, agent=Agent name) + // so they form one parallel batch and all run concurrently. const requests = [ { callId: '1', @@ -3202,20 +3205,23 @@ describe('Fire hook functions integration', () => { expect(completedCalls).toHaveLength(4); expect(completedCalls.every((c) => c.status === 'success')).toBe(true); - // Non-agent tools should execute sequentially: read:1 finishes before read:2 starts - const read1End = executionLog.indexOf('read:end:1'); - const read2Start = executionLog.indexOf('read:start:2'); - expect(read1End).toBeLessThan(read2Start); - - // Agent tools should execute concurrently: both start before either ends - const agentAStart = executionLog.indexOf('agent:start:A'); - const agentBStart = executionLog.indexOf('agent:start:B'); - const firstAgentEnd = Math.min( + // All 4 tools are concurrency-safe → they should all start + // before any of them finishes (parallel execution). + const allStarts = [ + executionLog.indexOf('read:start:1'), + executionLog.indexOf('agent:start:A'), + executionLog.indexOf('read:start:2'), + executionLog.indexOf('agent:start:B'), + ]; + const firstEnd = Math.min( + executionLog.indexOf('read:end:1'), executionLog.indexOf('agent:end:A'), + executionLog.indexOf('read:end:2'), executionLog.indexOf('agent:end:B'), ); - expect(agentAStart).toBeLessThan(firstAgentEnd); - expect(agentBStart).toBeLessThan(firstAgentEnd); + for (const start of allStarts) { + expect(start).toBeLessThan(firstEnd); + } }); }); }); diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index ca9202621aa..6eb9b6ebd46 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -49,6 +49,7 @@ import type { PartListUnion, } from '@google/genai'; import { ToolNames } from '../tools/tool-names.js'; +import { CONCURRENCY_SAFE_KINDS } from '../tools/tools.js'; import { buildPermissionCheckContext, evaluatePermissionRules, @@ -329,6 +330,44 @@ interface CoreToolSchedulerOptions { chatRecordingService?: ChatRecordingService; } +// ─── Tool Concurrency Helpers ──────────────────────────────── + +interface ToolBatch { + concurrent: boolean; + calls: ScheduledToolCall[]; +} + +/** + * Returns true if a scheduled tool call can safely execute concurrently + * with other safe tools (no side effects, no shared mutable state). + */ +function isConcurrencySafe(call: ScheduledToolCall): boolean { + // Agent tools spawn independent sub-agents with no shared state. + if (call.request.name === ToolNames.AGENT) return true; + return CONCURRENCY_SAFE_KINDS.has(call.tool.kind); +} + +/** + * Partition tool calls into consecutive batches by concurrency safety. + * + * Consecutive safe tools are merged into a single parallel batch. + * Each unsafe tool forms its own sequential batch. + * + * Example: [Read, Read, Edit, Read] → [[Read,Read](parallel), [Edit](seq), [Read](seq)] + */ +function partitionToolCalls(calls: ScheduledToolCall[]): ToolBatch[] { + return calls.reduce((batches, call) => { + const safe = isConcurrencySafe(call); + const lastBatch = batches[batches.length - 1]; + if (safe && lastBatch?.concurrent) { + lastBatch.calls.push(call); + } else { + batches.push({ concurrent: safe, calls: [call] }); + } + return batches; + }, []); +} + export class CoreToolScheduler { private toolRegistry: ToolRegistry; private toolCalls: ToolCall[] = []; @@ -1286,32 +1325,50 @@ export class CoreToolScheduler { if (allCallsFinalOrScheduled) { const callsToExecute = this.toolCalls.filter( - (call) => call.status === 'scheduled', - ); - - // Task tools are safe to run concurrently — they spawn independent - // sub-agents with no shared mutable state. All other tools run - // sequentially in their original order to preserve any implicit - // ordering the model may rely on. - const taskCalls = callsToExecute.filter( - (call) => call.request.name === ToolNames.AGENT, - ); - const otherCalls = callsToExecute.filter( - (call) => call.request.name !== ToolNames.AGENT, + (call): call is ScheduledToolCall => call.status === 'scheduled', ); - const taskPromise = Promise.all( - taskCalls.map((tc) => this.executeSingleToolCall(tc, signal)), - ); + // Partition tool calls into consecutive batches by concurrency safety. + // Consecutive safe tools (Read, Search, Fetch, Think, Agent) are grouped + // into parallel batches; unsafe tools (Edit, Execute, etc.) each form + // their own sequential batch. This preserves ordering semantics while + // allowing read-only tools to execute in parallel. + const batches = partitionToolCalls(callsToExecute); - const othersPromise = (async () => { - for (const toolCall of otherCalls) { - await this.executeSingleToolCall(toolCall, signal); + for (const batch of batches) { + if (batch.concurrent && batch.calls.length > 1) { + await this.runConcurrently(batch.calls, signal); + } else { + for (const call of batch.calls) { + await this.executeSingleToolCall(call, signal); + } } - })(); + } + } + } - await Promise.all([taskPromise, othersPromise]); + /** + * Execute multiple tool calls concurrently with a concurrency cap. + */ + private async runConcurrently( + calls: ScheduledToolCall[], + signal: AbortSignal, + ): Promise { + const maxConcurrency = + parseInt(process.env['QWEN_CODE_MAX_TOOL_CONCURRENCY'] || '', 10) || 10; + const executing = new Set>(); + + for (const call of calls) { + if (signal.aborted) break; + const p = this.executeSingleToolCall(call, signal).then(() => { + executing.delete(p); + }); + executing.add(p); + if (executing.size >= maxConcurrency) { + await Promise.race(executing); + } } + await Promise.all(executing); } private async executeSingleToolCall( diff --git a/packages/core/src/test-utils/mock-tool.ts b/packages/core/src/test-utils/mock-tool.ts index 0e3cf293dc7..6a1f2455588 100644 --- a/packages/core/src/test-utils/mock-tool.ts +++ b/packages/core/src/test-utils/mock-tool.ts @@ -24,6 +24,7 @@ interface MockToolOptions { name: string; displayName?: string; description?: string; + kind?: Kind; canUpdateOutput?: boolean; isOutputMarkdown?: boolean; getDefaultPermission?: () => Promise; @@ -97,7 +98,7 @@ export class MockTool extends BaseDeclarativeTool< options.name, options.displayName ?? options.name, options.description ?? options.name, - Kind.Other, + options.kind ?? Kind.Other, options.params, options.isOutputMarkdown ?? false, options.canUpdateOutput ?? false, diff --git a/packages/core/src/tools/tools.ts b/packages/core/src/tools/tools.ts index c33a3ecbedc..8259123adb2 100644 --- a/packages/core/src/tools/tools.ts +++ b/packages/core/src/tools/tools.ts @@ -736,6 +736,14 @@ export const MUTATOR_KINDS: Kind[] = [ Kind.Execute, ] as const; +/** Tool kinds that are safe to execute concurrently (no side effects). */ +export const CONCURRENCY_SAFE_KINDS: ReadonlySet = new Set([ + Kind.Read, + Kind.Search, + Kind.Fetch, + Kind.Think, +]); + export interface ToolLocation { // Absolute path to the file path: string; From 1aa1a09ff9e0bdd90f7fc72b98206c7e61824030 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 4 Apr 2026 02:48:31 +0800 Subject: [PATCH 2/8] feat(core): add conditional concurrency for shell read-only commands Shell commands detected as read-only (e.g., git log, cat, ls) now run concurrently with other safe tools instead of breaking parallel batches. Uses the existing isShellCommandReadOnly() checker (synchronous, fail-closed). Commands that can't be verified as read-only remain sequential. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/core/src/core/coreToolScheduler.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 6eb9b6ebd46..a94bd1fb298 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -50,6 +50,8 @@ import type { } from '@google/genai'; import { ToolNames } from '../tools/tool-names.js'; import { CONCURRENCY_SAFE_KINDS } from '../tools/tools.js'; +import { isShellCommandReadOnly } from '../utils/shellReadOnlyChecker.js'; +import { stripShellWrapper } from '../utils/shell-utils.js'; import { buildPermissionCheckContext, evaluatePermissionRules, @@ -344,6 +346,17 @@ interface ToolBatch { function isConcurrencySafe(call: ScheduledToolCall): boolean { // Agent tools spawn independent sub-agents with no shared state. if (call.request.name === ToolNames.AGENT) return true; + // Shell commands: check if the command is read-only (e.g., git log, cat). + // Uses the synchronous regex+shell-quote checker (fail-closed). + if (call.tool.kind === Kind.Execute) { + const command = (call.request.args as { command?: string }).command; + if (typeof command !== 'string') return false; + try { + return isShellCommandReadOnly(stripShellWrapper(command)); + } catch { + return false; // fail-closed + } + } return CONCURRENCY_SAFE_KINDS.has(call.tool.kind); } From eb87c7b35176746538de0049d48abe30e56e1c47 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 4 Apr 2026 02:56:20 +0800 Subject: [PATCH 3/8] fix: address Copilot review on tool parallelism - Remove Kind.Think from CONCURRENCY_SAFE_KINDS (save_memory and todo_write write to disk) - Use .finally() instead of .then() in runConcurrently for cleanup - Validate maxConcurrency (clamp to >= 1, default 10) - Add comment explaining why sync checker is used over async AST - Add test for mixed safe/unsafe tool batch partitioning Co-Authored-By: Claude Opus 4.6 (1M context) --- .../core/src/core/coreToolScheduler.test.ts | 110 ++++++++++++++++++ packages/core/src/core/coreToolScheduler.ts | 14 ++- packages/core/src/tools/tools.ts | 7 +- 3 files changed, 125 insertions(+), 6 deletions(-) diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index 86e2a530408..60f10fe91fc 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -3223,5 +3223,115 @@ describe('Fire hook functions integration', () => { expect(start).toBeLessThan(firstEnd); } }); + + it('should partition mixed safe/unsafe tools into correct batches', async () => { + const executionLog: string[] = []; + + const readTool = new MockTool({ + name: 'read_file', + kind: Kind.Read, + execute: async (params) => { + const id = (params as { id: string }).id; + executionLog.push(`read:start:${id}`); + await new Promise((r) => setTimeout(r, 50)); + executionLog.push(`read:end:${id}`); + return { + llmContent: `Read ${id} done`, + returnDisplay: `Read ${id} done`, + }; + }, + }); + + const editTool = new MockTool({ + name: 'edit', + kind: Kind.Edit, + execute: async (params) => { + const id = (params as { id: string }).id; + executionLog.push(`edit:start:${id}`); + await new Promise((r) => setTimeout(r, 20)); + executionLog.push(`edit:end:${id}`); + return { + llmContent: `Edit ${id} done`, + returnDisplay: `Edit ${id} done`, + }; + }, + }); + + const tools = new Map([ + ['read_file', readTool], + ['edit', editTool], + ]); + const onAllToolCallsComplete = vi.fn(); + const onToolCallsUpdate = vi.fn(); + const scheduler = createScheduler( + tools, + onAllToolCallsComplete, + onToolCallsUpdate, + ); + + // [Read₁, Read₂, Edit, Read₃] + // Expected batches: [Read₁,Read₂](parallel) → [Edit](seq) → [Read₃](seq) + const requests = [ + { + callId: '1', + name: 'read_file', + args: { id: '1' }, + isClientInitiated: false, + prompt_id: 'p1', + }, + { + callId: '2', + name: 'read_file', + args: { id: '2' }, + isClientInitiated: false, + prompt_id: 'p1', + }, + { + callId: '3', + name: 'edit', + args: { id: 'E' }, + isClientInitiated: false, + prompt_id: 'p1', + }, + { + callId: '4', + name: 'read_file', + args: { id: '3' }, + isClientInitiated: false, + prompt_id: 'p1', + }, + ]; + + await scheduler.schedule(requests, new AbortController().signal); + + expect(onAllToolCallsComplete).toHaveBeenCalled(); + const completedCalls = onAllToolCallsComplete.mock + .calls[0][0] as ToolCall[]; + expect(completedCalls).toHaveLength(4); + expect(completedCalls.every((c) => c.status === 'success')).toBe(true); + + // Batch 1: Read₁ and Read₂ run in parallel (both start before either ends) + const read1Start = executionLog.indexOf('read:start:1'); + const read2Start = executionLog.indexOf('read:start:2'); + const firstReadEnd = Math.min( + executionLog.indexOf('read:end:1'), + executionLog.indexOf('read:end:2'), + ); + expect(read1Start).toBeLessThan(firstReadEnd); + expect(read2Start).toBeLessThan(firstReadEnd); + + // Batch 2: Edit starts after both reads complete + const lastReadEnd = Math.max( + executionLog.indexOf('read:end:1'), + executionLog.indexOf('read:end:2'), + ); + const editStart = executionLog.indexOf('edit:start:E'); + expect(editStart).toBeGreaterThan(lastReadEnd); + + // Batch 3: Read₃ starts after Edit completes + const editEnd = executionLog.indexOf('edit:end:E'); + const read3Start = executionLog.indexOf('read:start:3'); + expect(read3Start).toBeGreaterThan(editEnd); + }); }); }); diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index a94bd1fb298..5c3edf7b754 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -347,7 +347,10 @@ function isConcurrencySafe(call: ScheduledToolCall): boolean { // Agent tools spawn independent sub-agents with no shared state. if (call.request.name === ToolNames.AGENT) return true; // Shell commands: check if the command is read-only (e.g., git log, cat). - // Uses the synchronous regex+shell-quote checker (fail-closed). + // Uses the synchronous regex+shell-quote checker (not the async AST-based + // one) because partitioning runs synchronously. The sync checker covers + // the same command whitelist and is fail-closed — unknown commands remain + // sequential. The AST version is used separately for permission decisions. if (call.tool.kind === Kind.Execute) { const command = (call.request.args as { command?: string }).command; if (typeof command !== 'string') return false; @@ -1367,13 +1370,16 @@ export class CoreToolScheduler { calls: ScheduledToolCall[], signal: AbortSignal, ): Promise { - const maxConcurrency = - parseInt(process.env['QWEN_CODE_MAX_TOOL_CONCURRENCY'] || '', 10) || 10; + const parsed = parseInt( + process.env['QWEN_CODE_MAX_TOOL_CONCURRENCY'] || '', + 10, + ); + const maxConcurrency = Number.isFinite(parsed) && parsed >= 1 ? parsed : 10; const executing = new Set>(); for (const call of calls) { if (signal.aborted) break; - const p = this.executeSingleToolCall(call, signal).then(() => { + const p = this.executeSingleToolCall(call, signal).finally(() => { executing.delete(p); }); executing.add(p); diff --git a/packages/core/src/tools/tools.ts b/packages/core/src/tools/tools.ts index 8259123adb2..76454b973b6 100644 --- a/packages/core/src/tools/tools.ts +++ b/packages/core/src/tools/tools.ts @@ -736,12 +736,15 @@ export const MUTATOR_KINDS: Kind[] = [ Kind.Execute, ] as const; -/** Tool kinds that are safe to execute concurrently (no side effects). */ +/** + * Tool kinds that are safe to execute concurrently (pure reads, no writes). + * Kind.Think is excluded because some Think tools write to disk + * (e.g., save_memory, todo_write). + */ export const CONCURRENCY_SAFE_KINDS: ReadonlySet = new Set([ Kind.Read, Kind.Search, Kind.Fetch, - Kind.Think, ]); export interface ToolLocation { From 4ca7c233fe4616fc063a07f5a1c7f27b26a423b9 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 4 Apr 2026 03:03:18 +0800 Subject: [PATCH 4/8] fix: update comment to match CONCURRENCY_SAFE_KINDS (remove Think) Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/core/src/core/coreToolScheduler.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 5c3edf7b754..464092dfc26 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -1345,7 +1345,7 @@ export class CoreToolScheduler { ); // Partition tool calls into consecutive batches by concurrency safety. - // Consecutive safe tools (Read, Search, Fetch, Think, Agent) are grouped + // Consecutive safe tools (Read, Search, Fetch, Agent) are grouped // into parallel batches; unsafe tools (Edit, Execute, etc.) each form // their own sequential batch. This preserves ordering semantics while // allowing read-only tools to execute in parallel. From 1c3796f1d9cdbc681a275f7f5e89a862f81f1230 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 4 Apr 2026 03:10:09 +0800 Subject: [PATCH 5/8] fix: remove abort break in runConcurrently to prevent stuck scheduled calls Let all calls go through executeSingleToolCall which handles abort internally, ensuring every tool reaches a terminal state. Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/core/src/core/coreToolScheduler.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 464092dfc26..a39311006e2 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -1378,7 +1378,6 @@ export class CoreToolScheduler { const executing = new Set>(); for (const call of calls) { - if (signal.aborted) break; const p = this.executeSingleToolCall(call, signal).finally(() => { executing.delete(p); }); From 83c2523038bfd84eb2e2bb037bc010e038549cdd Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 4 Apr 2026 03:19:17 +0800 Subject: [PATCH 6/8] test: isolate concurrency tests from QWEN_CODE_MAX_TOOL_CONCURRENCY env Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/core/src/core/coreToolScheduler.test.ts | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index 60f10fe91fc..d9d010b8bd1 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import type { Mock } from 'vitest'; import type { Config, @@ -2992,6 +2992,19 @@ describe('Fire hook functions integration', () => { }); describe('Concurrent agent tool execution', () => { + // Ensure tests are deterministic regardless of environment. + const origEnv = process.env['QWEN_CODE_MAX_TOOL_CONCURRENCY']; + beforeEach(() => { + delete process.env['QWEN_CODE_MAX_TOOL_CONCURRENCY']; + }); + afterEach(() => { + if (origEnv !== undefined) { + process.env['QWEN_CODE_MAX_TOOL_CONCURRENCY'] = origEnv; + } else { + delete process.env['QWEN_CODE_MAX_TOOL_CONCURRENCY']; + } + }); + function createScheduler( tools: Map, onAllToolCallsComplete: Mock, From 3db08eaadec2b4de303be68757c342689fb5c0a7 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 4 Apr 2026 03:27:55 +0800 Subject: [PATCH 7/8] fix: address Copilot review - comment, test label, shell test - Update batching comment to clarify Execute conditional safety - Rename describe block to "Concurrent tool execution" - Add test for shell read-only concurrency (git log + ls parallel, npm install sequential) Co-Authored-By: Claude Opus 4.6 (1M context) --- .../core/src/core/coreToolScheduler.test.ts | 80 ++++++++++++++++++- packages/core/src/core/coreToolScheduler.ts | 7 +- 2 files changed, 82 insertions(+), 5 deletions(-) diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index d9d010b8bd1..87de6ac56bc 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -2991,7 +2991,7 @@ describe('Fire hook functions integration', () => { }); }); - describe('Concurrent agent tool execution', () => { + describe('Concurrent tool execution', () => { // Ensure tests are deterministic regardless of environment. const origEnv = process.env['QWEN_CODE_MAX_TOOL_CONCURRENCY']; beforeEach(() => { @@ -3346,5 +3346,83 @@ describe('Fire hook functions integration', () => { const read3Start = executionLog.indexOf('read:start:3'); expect(read3Start).toBeGreaterThan(editEnd); }); + + it('should run read-only shell commands concurrently and non-read-only sequentially', async () => { + const executionLog: string[] = []; + + const shellTool = new MockTool({ + name: 'run_shell_command', + kind: Kind.Execute, + execute: async (params) => { + const cmd = (params as { command: string }).command; + executionLog.push(`shell:start:${cmd}`); + await new Promise((r) => setTimeout(r, 50)); + executionLog.push(`shell:end:${cmd}`); + return { + llmContent: `Shell ${cmd} done`, + returnDisplay: `Shell ${cmd} done`, + }; + }, + }); + + const tools = new Map([ + ['run_shell_command', shellTool], + ]); + const onAllToolCallsComplete = vi.fn(); + const onToolCallsUpdate = vi.fn(); + const scheduler = createScheduler( + tools, + onAllToolCallsComplete, + onToolCallsUpdate, + ); + + // "git log" and "ls" are read-only → concurrent + // "npm install" is not read-only → sequential, breaks the batch + const requests = [ + { + callId: '1', + name: 'run_shell_command', + args: { command: 'git log' }, + isClientInitiated: false, + prompt_id: 'p1', + }, + { + callId: '2', + name: 'run_shell_command', + args: { command: 'ls' }, + isClientInitiated: false, + prompt_id: 'p1', + }, + { + callId: '3', + name: 'run_shell_command', + args: { command: 'npm install' }, + isClientInitiated: false, + prompt_id: 'p1', + }, + ]; + + await scheduler.schedule(requests, new AbortController().signal); + + expect(onAllToolCallsComplete).toHaveBeenCalled(); + + // "git log" and "ls" should start concurrently (both before either ends) + const gitStart = executionLog.indexOf('shell:start:git log'); + const lsStart = executionLog.indexOf('shell:start:ls'); + const firstReadOnlyEnd = Math.min( + executionLog.indexOf('shell:end:git log'), + executionLog.indexOf('shell:end:ls'), + ); + expect(gitStart).toBeLessThan(firstReadOnlyEnd); + expect(lsStart).toBeLessThan(firstReadOnlyEnd); + + // "npm install" should start after both read-only commands complete + const lastReadOnlyEnd = Math.max( + executionLog.indexOf('shell:end:git log'), + executionLog.indexOf('shell:end:ls'), + ); + const npmStart = executionLog.indexOf('shell:start:npm install'); + expect(npmStart).toBeGreaterThan(lastReadOnlyEnd); + }); }); }); diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index a39311006e2..32a38823b41 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -1345,10 +1345,9 @@ export class CoreToolScheduler { ); // Partition tool calls into consecutive batches by concurrency safety. - // Consecutive safe tools (Read, Search, Fetch, Agent) are grouped - // into parallel batches; unsafe tools (Edit, Execute, etc.) each form - // their own sequential batch. This preserves ordering semantics while - // allowing read-only tools to execute in parallel. + // Consecutive safe tools are grouped into parallel batches; unsafe + // tools each form their own sequential batch. Execute (shell) is safe + // only when isShellCommandReadOnly() returns true; otherwise sequential. const batches = partitionToolCalls(callsToExecute); for (const batch of batches) { From 2efa7fd8a64dfe4dec92d586d8e475b0e1ea59b8 Mon Sep 17 00:00:00 2001 From: wenshao Date: Sat, 4 Apr 2026 03:38:02 +0800 Subject: [PATCH 8/8] fix: add indexOf !== -1 guards to concurrency test assertions Prevents false-positive test passes when expected log entries are missing (indexOf returns -1 which is always < any positive index). Co-Authored-By: Claude Opus 4.6 (1M context) --- packages/core/src/core/coreToolScheduler.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index 87de6ac56bc..32824799868 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -3232,6 +3232,11 @@ describe('Fire hook functions integration', () => { executionLog.indexOf('read:end:2'), executionLog.indexOf('agent:end:B'), ); + // Ensure all entries exist before comparing ordering + for (const start of allStarts) { + expect(start).not.toBe(-1); + } + expect(firstEnd).not.toBe(-1); for (const start of allStarts) { expect(start).toBeLessThan(firstEnd); } @@ -3330,6 +3335,9 @@ describe('Fire hook functions integration', () => { executionLog.indexOf('read:end:1'), executionLog.indexOf('read:end:2'), ); + expect(read1Start).not.toBe(-1); + expect(read2Start).not.toBe(-1); + expect(firstReadEnd).not.toBe(-1); expect(read1Start).toBeLessThan(firstReadEnd); expect(read2Start).toBeLessThan(firstReadEnd); @@ -3339,11 +3347,14 @@ describe('Fire hook functions integration', () => { executionLog.indexOf('read:end:2'), ); const editStart = executionLog.indexOf('edit:start:E'); + expect(editStart).not.toBe(-1); expect(editStart).toBeGreaterThan(lastReadEnd); // Batch 3: Read₃ starts after Edit completes const editEnd = executionLog.indexOf('edit:end:E'); const read3Start = executionLog.indexOf('read:start:3'); + expect(editEnd).not.toBe(-1); + expect(read3Start).not.toBe(-1); expect(read3Start).toBeGreaterThan(editEnd); }); @@ -3413,6 +3424,9 @@ describe('Fire hook functions integration', () => { executionLog.indexOf('shell:end:git log'), executionLog.indexOf('shell:end:ls'), ); + expect(gitStart).not.toBe(-1); + expect(lsStart).not.toBe(-1); + expect(firstReadOnlyEnd).not.toBe(-1); expect(gitStart).toBeLessThan(firstReadOnlyEnd); expect(lsStart).toBeLessThan(firstReadOnlyEnd); @@ -3422,6 +3436,7 @@ describe('Fire hook functions integration', () => { executionLog.indexOf('shell:end:ls'), ); const npmStart = executionLog.indexOf('shell:start:npm install'); + expect(npmStart).not.toBe(-1); expect(npmStart).toBeGreaterThan(lastReadOnlyEnd); }); });