diff --git a/packages/cli/src/ui/hooks/useHistoryManager.ts b/packages/cli/src/ui/hooks/useHistoryManager.ts index ff82eeafa..0e0b56a1b 100644 --- a/packages/cli/src/ui/hooks/useHistoryManager.ts +++ b/packages/cli/src/ui/hooks/useHistoryManager.ts @@ -13,6 +13,10 @@ import { DEFAULT_HISTORY_MAX_ITEMS, } from '../../constants/historyLimits.js'; +// Global counter for generating unique message IDs across all hook instances. +// This prevents ID collisions when multiple useHistory hooks use the same baseTimestamp. +let globalMessageIdCounter = 0; + // Type for the updater function passed to updateHistoryItem type HistoryItemUpdater = ( prevItem: HistoryItem, @@ -49,7 +53,6 @@ export function useHistory( options?: UseHistoryOptions, ): UseHistoryManagerReturn { const [history, setHistory] = useState([]); - const messageIdCounterRef = useRef(0); const maxItems = options?.maxItems; const maxBytes = options?.maxBytes; const limits = useMemo( @@ -63,10 +66,11 @@ export function useHistory( setHistory((prev) => trimHistory(prev, limits)); }, [limits]); - // Generates a unique message ID based on a timestamp and a counter. + // Generates a unique message ID based on a timestamp and a global counter. + // Using a global counter ensures uniqueness across all hook instances. const getNextMessageId = useCallback((baseTimestamp: number): number => { - messageIdCounterRef.current += 1; - return baseTimestamp + messageIdCounterRef.current; + globalMessageIdCounter += 1; + return baseTimestamp * 1000 + globalMessageIdCounter; }, []); const loadHistory = useCallback((newHistory: HistoryItem[]) => { @@ -128,10 +132,10 @@ export function useHistory( [], ); - // Clears the entire history state and resets the ID counter. + // Clears the entire history state. Note: we do NOT reset the global counter + // to ensure IDs remain unique across conversation clears within the same session. const clearItems = useCallback(() => { setHistory([]); - messageIdCounterRef.current = 0; ConversationContext.startNewConversation(); }, []); diff --git a/packages/cli/src/ui/hooks/useReactToolScheduler.ts b/packages/cli/src/ui/hooks/useReactToolScheduler.ts index f091daaed..a4c86bb3b 100644 --- a/packages/cli/src/ui/hooks/useReactToolScheduler.ts +++ b/packages/cli/src/ui/hooks/useReactToolScheduler.ts @@ -310,7 +310,16 @@ export function useReactToolScheduler( ? request.map(ensureAgentId) : ensureAgentId(request); - scheduler.schedule(normalizedRequest, signal); + // The scheduler.schedule() returns a Promise that rejects when the abort + // signal fires while tool calls are queued. We intentionally catch and + // ignore these rejections because: + // 1. Cancellation is an expected user action, not an error + // 2. The UI state is updated via cancelAllToolCalls() synchronously + // 3. Tool results are not needed after cancellation + scheduler.schedule(normalizedRequest, signal).catch(() => { + // Silently ignore cancellation rejections - this is expected behavior + // when the user presses ESC to cancel queued tool calls + }); }, [scheduler], ); diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index 48c093d79..95c90cbe7 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -2254,6 +2254,246 @@ describe('CoreToolScheduler Buffered Parallel Execution', () => { // Verify ordered publishing despite error in tool 2 expect(publishOrder).toEqual([1, 2, 3]); // Request order maintained }); + + it('should handle race condition when later tools complete while publishBufferedResults is exiting', async () => { + // This test exercises the race condition where: + // 1. Tool #3 finishes first, calls publishBufferedResults + // 2. publishBufferedResults waits for tool #1, breaks out of inner while loop + // 3. Just as it checks pendingPublishRequest (false) and is about to exit do-while + // 4. Tool #1 finishes, sets pendingPublishRequest=true, returns immediately + // 5. Without the fix: first publishBufferedResults exits without processing buffered results + // 6. With the fix: the finally block detects pendingResults.size > 0 and reschedules + // + // The fix adds a check in the finally block to reschedule if pendingResults.size > 0 + + const completionOrder: number[] = []; + const publishOrder: number[] = []; + + // Use a deferred promise pattern to precisely control timing + const resolvers: Map void> = new Map(); + + const executeFn = vi + .fn() + .mockImplementation(async (args: { call: number }) => { + const callNum = args.call; + // Create a promise that we can resolve externally + await new Promise((resolve) => { + resolvers.set(callNum, resolve); + }); + completionOrder.push(callNum); + return { llmContent: `Call ${callNum} done` }; + }); + + const mockTool = new MockTool({ name: 'mockTool', execute: executeFn }); + const mockToolRegistry = { + getTool: () => mockTool, + getFunctionDeclarations: () => [], + tools: new Map(), + discovery: {}, + registerTool: () => {}, + getToolByName: () => mockTool, + getToolByDisplayName: () => mockTool, + getTools: () => [], + discoverTools: async () => {}, + getAllTools: () => [], + getToolsByServer: () => [], + } as unknown as ToolRegistry; + + const onToolCallsUpdate = vi.fn(); + const mockPolicyEngine = createMockPolicyEngine(); + + const mockConfig = { + getSessionId: () => 'test-session-id', + getUsageStatisticsEnabled: () => true, + getDebugMode: () => false, + getApprovalMode: () => ApprovalMode.YOLO, + getEphemeralSettings: () => ({}), + getAllowedTools: () => [], + getContentGeneratorConfig: () => ({ + model: 'test-model', + authType: 'oauth-personal', + }), + getToolRegistry: () => mockToolRegistry, + getMessageBus: vi.fn().mockReturnValue(createMockMessageBus()), + getPolicyEngine: vi.fn().mockReturnValue(mockPolicyEngine), + } as unknown as Config; + + const scheduler = new CoreToolScheduler({ + config: mockConfig, + onAllToolCallsComplete: vi.fn(), + onToolCallsUpdate: (calls) => { + onToolCallsUpdate(calls); + calls.forEach((call) => { + if (call.status === 'success') { + const callNum = (call.request.args as { call: number }).call; + if (!publishOrder.includes(callNum)) { + publishOrder.push(callNum); + } + } + }); + }, + getPreferredEditor: () => 'vscode', + onEditorClose: vi.fn(), + }); + + const signal = new AbortController().signal; + + // Schedule 5 tool calls (simulating the scenario from the bug report) + await scheduler.schedule( + [1, 2, 3, 4, 5].map((n) => ({ + callId: `call${n}`, + name: 'mockTool', + args: { call: n }, + isClientInitiated: false, + prompt_id: 'test', + })), + signal, + ); + + // Wait for all tools to start executing and set up their resolvers + await vi.waitFor( + () => { + expect(resolvers.size).toBe(5); + }, + { timeout: 1000 }, + ); + + // Now complete tools in a specific order that triggers the race condition: + // Complete tool 3 first (middle of the batch) + resolvers.get(3)?.(); + + // Small delay to let publishBufferedResults start and break out waiting for tool 1 + await new Promise((resolve) => setTimeout(resolve, 5)); + + // Complete tools 4 and 5 + resolvers.get(4)?.(); + resolvers.get(5)?.(); + + // Small delay + await new Promise((resolve) => setTimeout(resolve, 5)); + + // Complete tool 2 + resolvers.get(2)?.(); + + // Small delay + await new Promise((resolve) => setTimeout(resolve, 5)); + + // Finally complete tool 1 (the blocker) + resolvers.get(1)?.(); + + // Wait for all calls to complete + await vi.waitFor( + () => { + expect(completionOrder.length).toBe(5); + expect(publishOrder.length).toBe(5); + }, + { timeout: 2000 }, + ); + + // Verify that despite the out-of-order completion, all results were published + // and in the correct request order + expect(publishOrder).toEqual([1, 2, 3, 4, 5]); + }); + + it('should recover when all later tools complete before first tool', async () => { + // Edge case: All tools except the first one complete, then the first one completes. + // Without the fix, the buffered results might get stuck. + const completionOrder: number[] = []; + const publishOrder: number[] = []; + + const executeFn = vi + .fn() + .mockImplementation(async (args: { call: number }) => { + // First tool takes longest, all others complete quickly + if (args.call === 1) { + await new Promise((resolve) => setTimeout(resolve, 80)); + } else { + // All other tools complete almost immediately but staggered + await new Promise((resolve) => setTimeout(resolve, args.call * 5)); + } + completionOrder.push(args.call); + return { llmContent: `Call ${args.call} done` }; + }); + + const mockTool = new MockTool({ name: 'mockTool', execute: executeFn }); + const mockToolRegistry = { + getTool: () => mockTool, + getFunctionDeclarations: () => [], + tools: new Map(), + discovery: {}, + registerTool: () => {}, + getToolByName: () => mockTool, + getToolByDisplayName: () => mockTool, + getTools: () => [], + discoverTools: async () => {}, + getAllTools: () => [], + getToolsByServer: () => [], + } as unknown as ToolRegistry; + + const mockPolicyEngine = createMockPolicyEngine(); + + const mockConfig = { + getSessionId: () => 'test-session-id', + getUsageStatisticsEnabled: () => true, + getDebugMode: () => false, + getApprovalMode: () => ApprovalMode.YOLO, + getEphemeralSettings: () => ({}), + getAllowedTools: () => [], + getContentGeneratorConfig: () => ({ + model: 'test-model', + authType: 'oauth-personal', + }), + getToolRegistry: () => mockToolRegistry, + getMessageBus: vi.fn().mockReturnValue(createMockMessageBus()), + getPolicyEngine: vi.fn().mockReturnValue(mockPolicyEngine), + } as unknown as Config; + + const scheduler = new CoreToolScheduler({ + config: mockConfig, + onAllToolCallsComplete: vi.fn(), + onToolCallsUpdate: (calls) => { + calls.forEach((call) => { + if (call.status === 'success') { + const callNum = (call.request.args as { call: number }).call; + if (!publishOrder.includes(callNum)) { + publishOrder.push(callNum); + } + } + }); + }, + getPreferredEditor: () => 'vscode', + onEditorClose: vi.fn(), + }); + + const signal = new AbortController().signal; + + // Schedule 5 tool calls + await scheduler.schedule( + [1, 2, 3, 4, 5].map((n) => ({ + callId: `call${n}`, + name: 'mockTool', + args: { call: n }, + isClientInitiated: false, + prompt_id: 'test', + })), + signal, + ); + + // Wait for all calls to complete + await vi.waitFor( + () => { + expect(completionOrder.length).toBe(5); + expect(publishOrder.length).toBe(5); + }, + { timeout: 2000 }, + ); + + // Completion order: 2, 3, 4, 5, 1 (first is slowest) + expect(completionOrder).toEqual([2, 3, 4, 5, 1]); + + // But publish order should still be in request order + expect(publishOrder).toEqual([1, 2, 3, 4, 5]); + }); }); it('injects agentId into ContextAwareTool context', async () => { diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 5887c2c44..0aafd707b 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -412,6 +412,9 @@ export class CoreToolScheduler { > = new Map(); private nextPublishIndex = 0; private readonly toolContextInteractiveMode: boolean; + // Track the abort signal for each tool call so we can use it when handling + // confirmation responses from the message bus + private callIdToSignal: Map = new Map(); constructor(options: CoreToolSchedulerOptions) { this.config = options.config; @@ -491,12 +494,30 @@ export class CoreToolScheduler { : ToolConfirmationOutcome.Cancel : ToolConfirmationOutcome.Cancel); - const abortController = new AbortController(); + // Use the original signal stored for this call, or a pre-aborted signal as fallback. + // If the original signal is missing, it means the tool call was already completed or + // cancelled (cleaned up in publishBufferedResults), so we use an aborted signal to + // ensure the confirmation handler doesn't proceed with execution. + const originalSignal = this.callIdToSignal.get(callId); + let signal: AbortSignal; + if (originalSignal) { + signal = originalSignal; + } else { + if (toolSchedulerLogger.enabled) { + toolSchedulerLogger.debug( + () => + `Using pre-aborted fallback AbortSignal for callId=${callId} (original signal not found in map)`, + ); + } + const abortedController = new AbortController(); + abortedController.abort(); + signal = abortedController.signal; + } void this.handleConfirmationResponse( callId, waitingToolCall.confirmationDetails.onConfirm, derivedOutcome, - abortController.signal, + signal, response.payload, true, ); @@ -940,6 +961,8 @@ export class CoreToolScheduler { } const { request: reqInfo, invocation } = toolCall; + // Store the signal for this call so we can use it later in message bus responses + this.callIdToSignal.set(reqInfo.callId, signal); try { if (signal.aborted) { @@ -1352,37 +1375,95 @@ export class CoreToolScheduler { }); } + // Reentrancy guard for publishBufferedResults to prevent race conditions + // when multiple async tool completions trigger publishing simultaneously + private isPublishingBufferedResults = false; + // Flag to track if another publish was requested while we were publishing + private pendingPublishRequest = false; + // Total number of tools in the current batch (set when execution starts) + private currentBatchSize = 0; + private async publishBufferedResults(signal: AbortSignal): Promise { - const callsInOrder = this.toolCalls.filter( - (call) => call.status === 'scheduled' || call.status === 'executing', - ); + // If already publishing, mark that we need another pass after current one completes + if (this.isPublishingBufferedResults) { + this.pendingPublishRequest = true; + return; + } + this.isPublishingBufferedResults = true; + this.pendingPublishRequest = false; - // Publish results in original request order - while (this.nextPublishIndex < callsInOrder.length) { - const expectedCall = callsInOrder[this.nextPublishIndex]; - const buffered = this.pendingResults.get(expectedCall.request.callId); + try { + // Loop to handle cases where new results arrive while we're publishing + do { + this.pendingPublishRequest = false; + + // Publish results in execution order using the stored executionIndex. + // We iterate while there are buffered results that match the next expected index. + // This approach doesn't rely on filtering toolCalls by status, which changes + // as we publish results (status goes from 'executing' to 'success'). + while (this.nextPublishIndex < this.currentBatchSize) { + // Find the buffered result with the next expected executionIndex + let nextBuffered: + | { + result: ToolResult; + callId: string; + toolName: string; + scheduledCall: ScheduledToolCall; + executionIndex: number; + } + | undefined; - if (!buffered) { - // Next result not ready yet, stop publishing - break; - } + for (const buffered of this.pendingResults.values()) { + if (buffered.executionIndex === this.nextPublishIndex) { + nextBuffered = buffered; + break; + } + } - // Publish this result - await this.publishResult(buffered, signal); + if (!nextBuffered) { + // The result for the next index isn't ready yet, stop publishing + break; + } - // Remove from buffer - this.pendingResults.delete(buffered.callId); - this.nextPublishIndex++; - } + // Publish this result + await this.publishResult(nextBuffered, signal); - // Check if all tools completed - if ( - this.nextPublishIndex === callsInOrder.length && - callsInOrder.length > 0 - ) { - // Reset for next batch - this.nextPublishIndex = 0; - this.pendingResults.clear(); + // Remove from buffer + this.pendingResults.delete(nextBuffered.callId); + this.nextPublishIndex++; + } + + // Check if all tools in this batch completed + if ( + this.nextPublishIndex === this.currentBatchSize && + this.currentBatchSize > 0 + ) { + // Reset for next batch + this.nextPublishIndex = 0; + this.currentBatchSize = 0; + this.pendingResults.clear(); + } + } while (this.pendingPublishRequest); + } finally { + this.isPublishingBufferedResults = false; + + // After releasing the lock, check if there are still pending results + // that need publishing. This handles the race condition where: + // 1. We break out of the while loop waiting for result N + // 2. Result N arrives and calls publishBufferedResults + // 3. That call sees isPublishingBufferedResults=true, sets pendingPublishRequest=true, and returns + // 4. We then check pendingPublishRequest in the do-while, but it was set AFTER we checked + // 5. We exit without publishing the remaining buffered results + // + // By checking pendingResults.size here after releasing the lock, we ensure + // any buffered results get published. + if (this.pendingResults.size > 0) { + // Use setImmediate to avoid deep recursion and allow the event loop to process + // other pending tool completions first + setImmediate(() => { + void this.publishBufferedResults(signal); + }); + } } } @@ -1459,6 +1540,10 @@ export class CoreToolScheduler { (call) => call.status === 'scheduled', ); + // Store the batch size for ordered publishing - this is set once at the start + // and doesn't change as tools complete, ensuring we know when all are done + this.currentBatchSize = callsToExecute.length; + // Assign execution indices for ordered publishing const executionIndices = new Map(); callsToExecute.forEach((call, index) => { @@ -1548,7 +1633,9 @@ export class CoreToolScheduler { const completedCalls = [...this.toolCalls] as CompletedToolCall[]; this.toolCalls = []; + // Clean up signal mappings for completed calls for (const call of completedCalls) { + this.callIdToSignal.delete(call.request.callId); logToolCall(this.config, new ToolCallEvent(call)); } diff --git a/packages/core/src/tools/grep.ts b/packages/core/src/tools/grep.ts index ba9c0c59d..2019a0921 100644 --- a/packages/core/src/tools/grep.ts +++ b/packages/core/src/tools/grep.ts @@ -29,6 +29,20 @@ import { } from '../utils/toolOutputLimiter.js'; import { MessageBus } from '../confirmation-bus/message-bus.js'; +/** + * Checks if a glob pattern contains brace expansion syntax that git grep doesn't support. + * Git grep pathspecs don't support shell-style brace expansion like {ts,tsx,js}. + * Uses indexOf for O(n) complexity instead of regex to avoid ReDoS vulnerability. + */ +function hasBraceExpansion(pattern: string): boolean { + const braceStart = pattern.indexOf('{'); + if (braceStart === -1) return false; + const braceEnd = pattern.indexOf('}', braceStart); + if (braceEnd === -1) return false; + const commaPos = pattern.indexOf(',', braceStart); + return commaPos !== -1 && commaPos < braceEnd; +} + // --- Interfaces --- /** @@ -500,7 +514,10 @@ class GrepToolInvocation extends BaseToolInvocation< try { // --- Strategy 1: git grep --- - const isGit = isGitRepository(absolutePath); + // Skip git grep if include pattern has brace expansion (e.g., *.{ts,tsx}) + // because git grep pathspecs don't support shell-style brace expansion. + const hasBracePattern = include && hasBraceExpansion(include); + const isGit = !hasBracePattern && isGitRepository(absolutePath); const gitAvailable = isGit && (await this.isCommandAvailable('git')); if (gitAvailable) { @@ -526,12 +543,23 @@ class GrepToolInvocation extends BaseToolInvocation< const stdoutChunks: Buffer[] = []; const stderrChunks: Buffer[] = []; + // Handle abort signal to kill child process + const abortHandler = () => { + if (!child.killed) { + child.kill('SIGTERM'); + } + reject(new Error('git grep aborted')); + }; + options.signal.addEventListener('abort', abortHandler); + child.stdout.on('data', (chunk) => stdoutChunks.push(chunk)); child.stderr.on('data', (chunk) => stderrChunks.push(chunk)); - child.on('error', (err) => - reject(new Error(`Failed to start git grep: ${err.message}`)), - ); + child.on('error', (err) => { + options.signal.removeEventListener('abort', abortHandler); + reject(new Error(`Failed to start git grep: ${err.message}`)); + }); child.on('close', (code) => { + options.signal.removeEventListener('abort', abortHandler); const stdoutData = Buffer.concat(stdoutChunks).toString('utf8'); const stderrData = Buffer.concat(stderrChunks).toString('utf8'); if (code === 0) resolve(stdoutData); @@ -596,6 +624,16 @@ class GrepToolInvocation extends BaseToolInvocation< const stdoutChunks: Buffer[] = []; const stderrChunks: Buffer[] = []; + // Handle abort signal to kill child process + const abortHandler = () => { + if (!child.killed) { + child.kill('SIGTERM'); + } + cleanup(); + reject(new Error('system grep aborted')); + }; + options.signal.addEventListener('abort', abortHandler); + const onData = (chunk: Buffer) => stdoutChunks.push(chunk); const onStderr = (chunk: Buffer) => { const stderrStr = chunk.toString(); @@ -632,6 +670,7 @@ class GrepToolInvocation extends BaseToolInvocation< }; const cleanup = () => { + options.signal.removeEventListener('abort', abortHandler); child.stdout.removeListener('data', onData); child.stderr.removeListener('data', onStderr); child.removeListener('error', onError); diff --git a/packages/core/src/tools/tool-registry.ts b/packages/core/src/tools/tool-registry.ts index af9f0817c..713467e8e 100644 --- a/packages/core/src/tools/tool-registry.ts +++ b/packages/core/src/tools/tool-registry.ts @@ -70,7 +70,7 @@ Signal: Signal number or \`(none)\` if no signal was received. async execute( params: ToolParams, - _signal: AbortSignal, + signal: AbortSignal, _updateOutput?: (output: string) => void, ): Promise { const callCommand = this.config.getToolCallCommand()!; @@ -82,55 +82,67 @@ Signal: Signal number or \`(none)\` if no signal was received. let stderr = ''; let error: Error | null = null; let code: number | null = null; - let signal: NodeJS.Signals | null = null; - - await new Promise((resolve) => { - const onStdout = (data: Buffer) => { - stdout += data?.toString(); - }; - - const onStderr = (data: Buffer) => { - stderr += data?.toString(); - }; + let exitSignal: NodeJS.Signals | null = null; - const onError = (err: Error) => { - error = err; - }; - - const onClose = ( - _code: number | null, - _signal: NodeJS.Signals | null, - ) => { - code = _code; - signal = _signal; - cleanup(); - resolve(); - }; + // Handle abort signal to kill the child process + const abortHandler = () => { + if (!child.killed) { + child.kill('SIGTERM'); + } + }; + signal.addEventListener('abort', abortHandler); - const cleanup = () => { - child.stdout.removeListener('data', onStdout); - child.stderr.removeListener('data', onStderr); - child.removeListener('error', onError); - child.removeListener('close', onClose); - if (child.connected) { - child.disconnect(); - } - }; + try { + await new Promise((resolve) => { + const onStdout = (data: Buffer) => { + stdout += data?.toString(); + }; + + const onStderr = (data: Buffer) => { + stderr += data?.toString(); + }; + + const onError = (err: Error) => { + error = err; + }; + + const onClose = ( + _code: number | null, + _signal: NodeJS.Signals | null, + ) => { + code = _code; + exitSignal = _signal; + cleanup(); + resolve(); + }; + + const cleanup = () => { + child.stdout.removeListener('data', onStdout); + child.stderr.removeListener('data', onStderr); + child.removeListener('error', onError); + child.removeListener('close', onClose); + if (child.connected) { + child.disconnect(); + } + }; - child.stdout.on('data', onStdout); - child.stderr.on('data', onStderr); - child.on('error', onError); - child.on('close', onClose); - }); + child.stdout.on('data', onStdout); + child.stderr.on('data', onStderr); + child.on('error', onError); + child.on('close', onClose); + }); + } finally { + signal.removeEventListener('abort', abortHandler); + } // if there is any error, non-zero exit code, signal, or stderr, return error details instead of stdout - if (error || code !== 0 || signal || stderr) { + if (error || code !== 0 || exitSignal || stderr) { const llmContent = [ `Stdout: ${stdout || '(empty)'}`, `Stderr: ${stderr || '(empty)'}`, `Error: ${error ?? '(none)'}`, `Exit Code: ${code ?? '(none)'}`, - `Signal: ${signal ?? '(none)'}`, + `Signal: ${exitSignal ?? '(none)'}`, ].join('\n'); return { llmContent, diff --git a/project-plans/20251230fixes/plan.md b/project-plans/20251230fixes/plan.md new file mode 100644 index 000000000..a427ffade --- /dev/null +++ b/project-plans/20251230fixes/plan.md @@ -0,0 +1,200 @@ +# Fix Plan for Issues #945, #948, #951, #952, #957 + +**Date**: 2024-12-30 +**Branch**: Current working branch + +## Overview + +This plan addresses five related issues involving tool execution, cancellation, and UI stability. + +--- + +## Issue #945: search_file_content glob include issues + +**Problem**: Brace expansion patterns like `**/*.{ts,tsx,js}` don't work with git grep, causing fallback to slow JS glob scan. + +**Root Cause**: `grep.ts` lines 516-517 pass the `include` pattern directly to git grep via `--` pathspec, but git grep doesn't support shell-style brace expansion `{...}`. + +**Fix Location**: `packages/core/src/tools/grep.ts` + +**Solution**: +1. Add a helper function to detect unsupported glob patterns (brace expansion `{...}`) +2. When detected, skip git grep entirely and use the JS glob fallback directly +3. This is simpler and more reliable than trying to convert patterns + +**Code Changes**: +```typescript +// Add near top of file, after imports +function hasUnsupportedGitGrepPattern(pattern: string): boolean { + // Git grep doesn't support brace expansion {a,b,c} + return /\{[^}]*,[^}]*\}/.test(pattern); +} + +// In searchWithGitGrep method, add early bail-out check: +// Before attempting git grep, check if include pattern is unsupported +if (include && hasUnsupportedGitGrepPattern(include)) { + return null; // Signal to use JS fallback +} +``` + +--- + +## Issue #948: Shell cancellation doesn't kill the process + +**Problem**: ESC shows "cancelled" but the shell process continues running. + +**Root Cause**: `tool-registry.ts` `DiscoveredTool.execute()` (lines 71-149) accepts `_signal: AbortSignal` but ignores it - the underscore prefix indicates it's unused. The spawned child process is never killed on abort. + +**Fix Location**: `packages/core/src/tools/tool-registry.ts` + +**Solution**: +1. Remove the underscore prefix from `_signal` parameter +2. Add abort signal listener that kills the child process +3. Use the same pattern as `shellExecutionService.ts` (lines 312-330, 535-572) + +**Code Changes**: +```typescript +// In DiscoveredTool.execute method: +async execute(signal: AbortSignal): Promise { + // ... existing code to spawn child ... + + // Add abort handling + const abortHandler = () => { + if (child && !child.killed) { + child.kill('SIGTERM'); + } + }; + signal.addEventListener('abort', abortHandler); + + try { + // ... existing execution logic ... + } finally { + signal.removeEventListener('abort', abortHandler); + } +} +``` + +--- + +## Issue #951: History ID uniqueness/key collisions + +**Problem**: React keys can collide when multiple hook instances generate IDs. + +**Root Cause**: `useHistoryManager.ts` lines 67-69 generates IDs as `baseTimestamp + messageIdCounterRef.current`. The counter is per-hook instance (useRef), so different hook instances using the same baseTimestamp can generate identical IDs. + +**Fix Location**: `packages/cli/src/ui/hooks/useHistoryManager.ts` + +**Solution**: +1. Use a module-level counter instead of per-instance ref for the incrementing part +2. Combine with timestamp for monotonically increasing, globally unique IDs + +**Code Changes**: +```typescript +// Add at module level (outside component) +let globalMessageIdCounter = 0; + +// Modify getNextMessageId function: +const getNextMessageId = useCallback((baseTimestamp: number): number => { + globalMessageIdCounter += 1; + return baseTimestamp * 1000 + globalMessageIdCounter; +}, []); + +// Remove messageIdCounterRef since we use global counter now +// Update clearItems to NOT reset the global counter (IDs should remain unique) +``` + +--- + +## Issues #952/#957: Tool read hangs / "Tool call cancelled while in queue" error + +**Problem**: +- #952: ReadFile shows running but nothing happens +- #957: "Tool call cancelled while in queue" error + +**Root Cause**: `coreToolScheduler.ts` has two issues: +1. Line 494 in `handleMessageBusResponse` creates a NEW `AbortController` instead of using the original signal from the queued request. This means confirmation responses lose the original abort context. +2. `publishBufferedResults` (lines 1355-1386) can be called from multiple async completions simultaneously, causing race conditions. + +**Fix Location**: `packages/core/src/core/coreToolScheduler.ts` + +**Solution**: +1. Store the original signal with the queued request +2. Pass original signal through `handleMessageBusResponse` instead of creating new one +3. Add a reentrancy guard to `publishBufferedResults` + +**Code Changes**: + +```typescript +// 1. In the queue item type, ensure signal is stored +interface QueuedToolRequest { + request: ToolCallRequestInfo; + signal: AbortSignal; // Store original signal +} + +// 2. In handleMessageBusResponse, use stored signal instead of creating new: +// REMOVE: const abortController = new AbortController(); +// USE: the signal from the queued request + +// 3. Add reentrancy guard to publishBufferedResults: +private isPublishingBufferedResults = false; + +private async publishBufferedResults(): Promise { + if (this.isPublishingBufferedResults) { + return; // Prevent reentrant calls + } + this.isPublishingBufferedResults = true; + try { + // ... existing logic ... + } finally { + this.isPublishingBufferedResults = false; + } +} +``` + +--- + +## Implementation Order + +1. **#951** (useHistoryManager) - Simplest, isolated change +2. **#945** (grep.ts) - Self-contained, low risk +3. **#948** (tool-registry.ts) - Moderate complexity +4. **#952/#957** (coreToolScheduler.ts) - Most complex, affects core scheduling + +--- + +## Verification Steps (per AGENTS.md) + +After all fixes: +```bash +npm run format +npm run lint +npm run typecheck +npm run test +npm run build +node scripts/start.js --profile-load synthetic --prompt "write me a haiku" +``` + +--- + +## Test Considerations + +- **#945**: Existing `grep.test.ts` has good coverage; add test for brace expansion pattern detection +- **#948**: Add test verifying abort signal kills child process +- **#951**: `useHistoryManager.test.ts` exists; add test for ID uniqueness across instances +- **#952/#957**: `coreToolScheduler.test.ts` has cancellation tests; verify existing tests still pass + +--- + +## Commit Message Template + +``` +fix: resolve tool scheduling and cancellation issues + +- #945: Skip git grep for unsupported brace expansion patterns +- #948: Kill child process on abort in DiscoveredTool.execute +- #951: Use global counter for history message IDs to prevent collisions +- #952/#957: Pass original abort signal through confirmation flow and + add reentrancy guard to publishBufferedResults + +Fixes #945, #948, #951, #952, #957 +```