fix: resolve tool scheduling, cancellation, and search issues

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<HistoryItem[]>([]);
-  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();
   }, []);
 

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<string, AbortSignal> = new Map();
 
   constructor(options: CoreToolSchedulerOptions) {
     this.config = options.config;
@@ -491,12 +494,14 @@ export class CoreToolScheduler {
           : ToolConfirmationOutcome.Cancel
         : ToolConfirmationOutcome.Cancel);
 
-    const abortController = new AbortController();
+    // Use the original signal stored for this call, or create a new one as fallback
+    const originalSignal = this.callIdToSignal.get(callId);
+    const signal = originalSignal ?? new AbortController().signal;
     void this.handleConfirmationResponse(
       callId,
       waitingToolCall.confirmationDetails.onConfirm,
       derivedOutcome,
-      abortController.signal,
+      signal,
       response.payload,
       true,
     );
@@ -940,6 +945,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 +1359,51 @@ export class CoreToolScheduler {
     });
   }
 
+  // Reentrancy guard for publishBufferedResults to prevent race conditions
+  // when multiple async tool completions trigger publishing simultaneously
+  private isPublishingBufferedResults = false;
+
   private async publishBufferedResults(signal: AbortSignal): Promise<void> {
-    const callsInOrder = this.toolCalls.filter(
-      (call) => call.status === 'scheduled' || call.status === 'executing',
-    );
+    // Prevent reentrant calls which can cause race conditions
+    if (this.isPublishingBufferedResults) {
+      return;
+    }
+    this.isPublishingBufferedResults = true;
 
-    // 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 {
+      const callsInOrder = this.toolCalls.filter(
+        (call) => call.status === 'scheduled' || call.status === 'executing',
+      );
 
-      if (!buffered) {
-        // Next result not ready yet, stop publishing
-        break;
-      }
+      // Publish results in original request order
+      while (this.nextPublishIndex < callsInOrder.length) {
+        const expectedCall = callsInOrder[this.nextPublishIndex];
+        const buffered = this.pendingResults.get(expectedCall.request.callId);
 
-      // Publish this result
-      await this.publishResult(buffered, signal);
+        if (!buffered) {
+          // Next result not ready yet, stop publishing
+          break;
+        }
 
-      // Remove from buffer
-      this.pendingResults.delete(buffered.callId);
-      this.nextPublishIndex++;
-    }
+        // Publish this result
+        await this.publishResult(buffered, 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(buffered.callId);
+        this.nextPublishIndex++;
+      }
+
+      // Check if all tools completed
+      if (
+        this.nextPublishIndex === callsInOrder.length &&
+        callsInOrder.length > 0
+      ) {
+        // Reset for next batch
+        this.nextPublishIndex = 0;
+        this.pendingResults.clear();
+      }
+    } finally {
+      this.isPublishingBufferedResults = false;
     }
   }
 
@@ -1548,7 +1569,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));
       }
 

packages/core/src/tools/grep.ts

@@ -29,6 +29,14 @@
 } 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}.
+ */
+function hasBraceExpansion(pattern: string): boolean {
+  return /\{[^}]*,[^}]*\}/.test(pattern);
+}
+
 // --- Interfaces ---
 
 /**
@@ -500,7 +508,10 @@
 
     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) {

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<ToolResult> {
     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<void>((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<void>((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,

project-plans/20251230fixes/issues/issue-945.json

@@ -0,0 +1 @@
+{"body":"## Summary\nThe search_file_content tool can return false negatives when include is a glob (especially brace expansion like **/*.{ts,tsx,js} or when include is prefixed with the path). This often triggers the JS fallback scan, which can appear to hang on large trees.\n\n## Repro\n1. Run search with: pattern=sandbox, path=packages/cli/src, include=**/*.{ts,tsx,js}\n2. Expected to match packages/cli/src/config/sandboxConfig.ts\n3. Actual: No matches found OR very slow fallback traversal\n\nIf include is path-prefixed (e.g. packages/cli/src/**/*.{ts,tsx,js}) it never matches when cwd is already packages/cli/src.\n\n## Root Cause (likely)\n- include is passed directly to git grep as a pathspec, but git pathspec does not support brace expansion and relative-path assumptions differ.\n- When git grep returns empty, tool falls back to JS glob and scans huge trees, which feels like a hang.\n\n## Suggested Fix\n- Normalize include to be relative to path (strip leading search dir).\n- Detect {} / ** / other glob patterns and either convert to git pathspec (:(glob)) or skip git grep and go straight to JS glob.\n- Add a timeout / progress / early exit for JS fallback to avoid long hangs.\n","coderabbit_comments":[{"author":"coderabbitai","body":"\u003c!-- This is an auto-generated issue plan by CodeRabbit --\u003e\n\n\u003cdetails\u003e\n\u003csummary\u003e\u003cb\u003e🔗 Similar Issues\u003c/b\u003e\u003c/summary\u003e\n\n**Related Issues**\n- https://github.com/vybestack/llxprt-code/issues/274\n- https://github.com/vybestack/llxprt-code/issues/391\n- https://github.com/vybestack/llxprt-code/issues/63\n- https://github.com/vybestack/llxprt-code/issues/473\n- https://github.com/vybestack/llxprt-code/issues/751\n\u003c/details\u003e\n\u003cdetails\u003e\n\u003csummary\u003e\u003cb\u003e👤 Suggested Assignees\u003c/b\u003e\u003c/summary\u003e\n\n- [acoliver](https://github.com/acoliver)\n- [pidgeon777](https://github.com/pidgeon777)\n\u003c/details\u003e\n\n\n---\n\u003cdetails\u003e\n\u003csummary\u003eEnable issue planning\u003c/summary\u003e\n\n\u003csub\u003eTo enable issue planning, add the following to your `.coderabbit.yaml`:\u003c/sub\u003e\n```yaml\nissue_enrichment:\n  planning:\n    enabled: true\n```\n\n\u003csub\u003eYou can then request a plan by commenting `@coderabbitai plan` on any issue.\u003c/sub\u003e\n\u003c/details\u003e\n\n\u003cdetails\u003e\n\u003csummary\u003e 🧪 Issue enrichment is currently in open beta.\u003c/summary\u003e\n\n\nYou can configure auto-planning by selecting labels in the issue_enrichment configuration.\n\nTo disable automatic issue enrichment, add the following to your `.coderabbit.yaml`:\n```yaml\nissue_enrichment:\n  auto_enrich:\n    enabled: false\n```\n\u003c/details\u003e\n\n💬 Have feedback or questions? Drop into our [discord](https://discord.gg/coderabbit) or [schedule a call](https://calendly.com/parth-coderabbit/issues-feedback)!","createdAt":"2025-12-22T22:31:19Z","url":"https://github.com/vybestack/llxprt-code/issues/945#issuecomment-3684417170"},{"author":"coderabbitai","body":"\u003c!-- This is an auto-generated reply by CodeRabbit --\u003e\n\u003e [!TIP]\n\u003e For best results, initiate chat on the files or code changes.\n\n\u003cdetails\u003e\n\u003csummary\u003e🧩 Analysis chain\u003c/summary\u003e\n\n🏁 Script executed:\n```\n#!/bin/bash\n# Find the search_file_content tool implementation\nfd -e ts -e js -e tsx \"search.*file.*content\" --type f\n```\n\nLength of output: 0\n\n---\n\n🏁 Script executed:\n```\n#!/bin/bash\n# Search for files that might contain the implementation using ripgrep\nrg -n \"search_file_content\" -A 3 -B 3 --type ts --type js\n```\n\nLength of output: 7474\n\n---\n\n🏁 Script executed:\n```\n#!/bin/bash\n# Look for git grep usage in the codebase\nrg -n \"git grep\" -A 5 -B 5 --type ts --type js\n```\n\nLength of output: 5344\n\n---\n\n🏁 Script executed:\n```\n#!/bin/bash\n# Search for fallback glob logic\nrg -n \"fallback\" -A 3 -B 3 --type ts --type js | head -100\n```\n\nLength of output: 6350\n\n---\n\n🏁 Script executed:\n```\n#!/bin/bash\n# Read the grep.ts file to understand the implementation\ncat packages/core/src/tools/grep.ts\n```\n\nLength of output: 29158\n\n---\n\n\n\n\u003c/details\u003e\n\n@acoliver I've investigated issue #945 and can confirm the problem. Here's my analysis:\n\n## Root Cause\n\nThe `search_file_content` tool (implemented in `packages/core/src/tools/grep.ts`) has a **critical flaw in how it handles the `include` parameter with git grep**.\n\n**At lines 514-516:**\n```typescript\nif (include) {\n  gitArgs.push('--', include);\n}\n```\n\nThe `include` parameter is passed directly to git grep as a pathspec. However:\n\n1. **Git pathspec does NOT support brace expansion** - Patterns like `**/*.{ts,tsx,js}` are not valid git pathspec syntax\n2. **Path-prefixed globs fail** - If `include` is `packages/cli/src/**/*.ts` and the cwd is already `packages/cli/src`, it won't match (double-prefixing issue)\n3. **Git pathspec uses different syntax** - Git supports glob patterns via the `:(glob)` magic signature, but the code doesn't use it\n\nWhen git grep returns no matches (exit code 1), the code correctly treats it as \"no matches found\" and falls back to the JavaScript implementation (line 665+), which uses `globStream` that DOES support proper glob syntax. However, this fallback scans the entire directory tree, which can be extremely slow on large codebases.\n\n## Suggested Fixes\n\n### Option 1: Detect Glob Patterns and Skip Git Grep\n```typescript\n// Check if include contains glob patterns that git pathspec can't handle\nconst hasUnsupportedGlob = include \u0026\u0026 (\n  include.includes('{') || \n  include.includes('}') ||\n  include.startsWith(absolutePath) // Path-prefixed\n);\n\nif (gitAvailable \u0026\u0026 !hasUnsupportedGlob) {\n  // Use git grep\n}\n```\n\n### Option 2: Convert to Git Pathspec Format\n```typescript\nif (include) {\n  // Normalize path-prefixed includes\n  let normalizedInclude = include;\n  if (include.startsWith(absolutePath)) {\n    normalizedInclude = path.relative(absolutePath, include);\n  }\n  \n  // Use git's glob magic signature for patterns with braces\n  if (normalizedInclude.includes('{') || normalizedInclude.includes('}')) {\n    gitArgs.push('--', `:(glob)${normalizedInclude}`);\n  } else {\n    gitArgs.push('--', normalizedInclude);\n  }\n}\n```\n\n### Option 3: Add Progress/Timeout for JS Fallback\nThe JavaScript fallback should have better user experience:\n```typescript\n// Add progress logging\nconsole.debug('Falling back to JavaScript grep - this may take a moment for large directories...');\n\n// Consider adding a file count limit or early exit\nlet filesScanned = 0;\nconst MAX_FILES_TO_SCAN = 10000;\n```\n\nWould you like me to create a PR with one of these fixes, or would you prefer to handle this internally?","createdAt":"2025-12-23T13:12:18Z","url":"https://github.com/vybestack/llxprt-code/issues/945#issuecomment-3686595519"},{"author":"coderabbitai","body":"\u003c!-- This is an auto-generated reply by CodeRabbit --\u003e\n\u003e [!TIP]\n\u003e For best results, initiate chat on the files or code changes.\n\n\u003cdetails\u003e\n\u003csummary\u003e🧩 Analysis chain\u003c/summary\u003e\n\n🏁 Script executed:\n```\n#!/bin/bash\n# Fetch details of issues #952 and #957\ngh issue view 952 --json number,title,body,labels,state,author\necho \"---SEPARATOR---\"\ngh issue view 957 --json number,title,body,labels,state,author\n```\n\nLength of output: 4063\n\n---\n\n\n\n\u003c/details\u003e\n\n@acoliver Yes, you're absolutely right! These issues are definitely related. Here's the connection:\n\n## Root Cause → Symptoms Chain\n\n**#945** (this issue) is the **root cause**:\n- `search_file_content` tool's glob pattern handling is broken\n- Brace expansion and `**/*.ts` patterns fail with git grep\n- Falls back to slow JavaScript glob traversal that can take minutes on large trees\n\n**#952** is experiencing the **hanging symptom**:\n- The SearchText example you posted uses `integration-tests/**/*.ts` \n- This is exactly the type of glob pattern that triggers #945's bug\n- Git grep fails → JS fallback kicks in → appears to hang\n- Returns \"No matches found\" which could be either a false negative OR the JS scan eventually completing with no results\n\n**#957** is the **downstream effect**:\n- When tools take unexpectedly long (due to #945's slow fallback), they:\n  - Get stuck in the tool scheduler queue\n  - Eventually get cancelled with \"Tool call cancelled while in queue\"\n- Your note \"this was on something that was taking awhile for no reason\" aligns perfectly with #945's slow fallback behavior\n\n## Smoking Gun Evidence\n\nIn #952, you have:\n```\n✓  SearchText 'output-format|outputFormat|...' in integration-tests/**/*.ts within ./\n   No matches found\n```\n\nThat `integration-tests/**/*.ts` glob is being passed directly to git grep (which doesn't support `**` properly), failing, then triggering the slow JS fallback scan.\n\n## Fix Priority\n\nFixing #945 should resolve or significantly improve both #952 and #957. The fixes I suggested earlier (detecting unsupported globs, converting to git pathspec format, or adding timeouts) would prevent the slow fallback from causing these queue issues.","createdAt":"2025-12-29T19:20:18Z","url":"https://github.com/vybestack/llxprt-code/issues/945#issuecomment-3697310851"}],"number":945,"title":"search_file_content: include glob can false-negative and trigger slow fallback"}