Fix issues with rip grep (#18756)

integration-tests/ripgrep-real.test.ts

@@ -11,6 +11,7 @@ import * as os from 'node:os';
 import { RipGrepTool } from '../packages/core/src/tools/ripGrep.js';
 import { Config } from '../packages/core/src/config/config.js';
 import { WorkspaceContext } from '../packages/core/src/utils/workspaceContext.js';
+import { createMockMessageBus } from '../packages/core/src/test-utils/mock-message-bus.js';
 
 // Mock Config to provide necessary context
 class MockConfig {
@@ -66,7 +67,7 @@ describe('ripgrep-real-direct', () => {
     await fs.writeFile(path.join(tempDir, 'file3.txt'), 'goodbye moon\n');
 
     const config = new MockConfig(tempDir) as unknown as Config;
-    tool = new RipGrepTool(config);
+    tool = new RipGrepTool(config, createMockMessageBus());
   });
 
   afterAll(async () => {
@@ -108,4 +109,24 @@ describe('ripgrep-real-direct', () => {
     expect(result.llmContent).toContain('script.js');
     expect(result.llmContent).not.toContain('file1.txt');
   });
+
+  it('should support context parameters', async () => {
+    // Create a file with multiple lines
+    await fs.writeFile(
+      path.join(tempDir, 'context.txt'),
+      'line1\nline2\nline3 match\nline4\nline5\n',
+    );
+
+    const invocation = tool.build({
+      pattern: 'match',
+      context: 1,
+    });
+    const result = await invocation.execute(new AbortController().signal);
+
+    expect(result.llmContent).toContain('Found 1 match');
+    expect(result.llmContent).toContain('context.txt');
+    expect(result.llmContent).toContain('L2- line2');
+    expect(result.llmContent).toContain('L3: line3 match');
+    expect(result.llmContent).toContain('L4- line4');
+  });
 });

packages/core/src/tools/ripGrep.test.ts

@@ -1408,42 +1408,45 @@ describe('RipGrepTool', () => {
       expect(result.llmContent).toContain('L1: HELLO world');
     });
 
-    it.each([
-      {
-        name: 'fixed_strings parameter',
-        params: { pattern: 'hello.world', fixed_strings: true },
-        mockOutput: {
-          path: { text: 'fileA.txt' },
-          line_number: 1,
-          lines: { text: 'hello.world\n' },
-        },
-        expectedArgs: ['--fixed-strings'],
-        expectedPattern: 'hello.world',
-      },
-    ])(
-      'should handle $name',
-      async ({ params, mockOutput, expectedArgs, expectedPattern }) => {
-        mockSpawn.mockImplementationOnce(
-          createMockSpawn({
-            outputData:
-              JSON.stringify({ type: 'match', data: mockOutput }) + '\n',
-            exitCode: 0,
-          }),
-        );
+    it('should handle fixed_strings parameter', async () => {
+      mockSpawn.mockImplementationOnce(
+        createMockSpawn({
+          outputData:
+            JSON.stringify({
+              type: 'match',
+              data: {
+                path: { text: 'fileA.txt' },
+                line_number: 1,
+                lines: { text: 'hello.world\n' },
+              },
+            }) + '\n',
+          exitCode: 0,
+        }),
+      );
 
-        const invocation = grepTool.build(params);
-        const result = await invocation.execute(abortSignal);
+      const invocation = grepTool.build({
+        pattern: 'hello.world',
+        fixed_strings: true,
+      });
+      const result = await invocation.execute(abortSignal);
 
-        expect(mockSpawn).toHaveBeenLastCalledWith(
-          expect.anything(),
-          expect.arrayContaining(expectedArgs),
-          expect.anything(),
-        );
-        expect(result.llmContent).toContain(
-          `Found 1 match for pattern "${expectedPattern}"`,
-        );
-      },
-    );
+      expect(mockSpawn).toHaveBeenLastCalledWith(
+        expect.anything(),
+        expect.arrayContaining(['--fixed-strings']),
+        expect.anything(),
+      );
+      expect(result.llmContent).toContain(
+        'Found 1 match for pattern "hello.world"',
+      );
+    });
+
+    it('should allow invalid regex patterns when fixed_strings is true', () => {
+      const params: RipGrepToolParams = {
+        pattern: '[[',
+        fixed_strings: true,
+      };
+      expect(grepTool.validateToolParams(params)).toBeNull();
+    });
 
     it('should handle no_ignore parameter', async () => {
       mockSpawn.mockImplementationOnce(
@@ -1681,19 +1684,42 @@ describe('RipGrepTool', () => {
       mockSpawn.mockImplementationOnce(
         createMockSpawn({
           outputData:
+            JSON.stringify({
+              type: 'context',
+              data: {
+                path: { text: 'fileA.txt' },
+                line_number: 1,
+                lines: { text: 'hello world\n' },
+              },
+            }) +
+            '\n' +
             JSON.stringify({
               type: 'match',
               data: {
                 path: { text: 'fileA.txt' },
                 line_number: 2,
                 lines: { text: 'second line with world\n' },
-                lines_before: [{ text: 'hello world\n' }],
-                lines_after: [
-                  { text: 'third line\n' },
-                  { text: 'fourth line\n' },
-                ],
               },
-            }) + '\n',
+            }) +
+            '\n' +
+            JSON.stringify({
+              type: 'context',
+              data: {
+                path: { text: 'fileA.txt' },
+                line_number: 3,
+                lines: { text: 'third line\n' },
+              },
+            }) +
+            '\n' +
+            JSON.stringify({
+              type: 'context',
+              data: {
+                path: { text: 'fileA.txt' },
+                line_number: 4,
+                lines: { text: 'fourth line\n' },
+              },
+            }) +
+            '\n',
           exitCode: 0,
         }),
       );
@@ -1721,9 +1747,10 @@ describe('RipGrepTool', () => {
       );
       expect(result.llmContent).toContain('Found 1 match for pattern "world"');
       expect(result.llmContent).toContain('File: fileA.txt');
+      expect(result.llmContent).toContain('L1- hello world');
       expect(result.llmContent).toContain('L2: second line with world');
-      // Note: Ripgrep JSON output for context lines doesn't include line numbers for context lines directly
-      // The current parsing only extracts the matched line, so we only assert on that.
+      expect(result.llmContent).toContain('L3- third line');
+      expect(result.llmContent).toContain('L4- fourth line');
     });
   });
 

packages/core/src/tools/ripGrep.ts

@@ -140,6 +140,7 @@ interface GrepMatch {
   filePath: string;
   lineNumber: number;
   line: string;
+  isContext?: boolean;
 }
 
 class GrepToolInvocation extends BaseToolInvocation<
@@ -267,8 +268,6 @@ class GrepToolInvocation extends BaseToolInvocation<
         return { llmContent: noMatchMsg, returnDisplay: `No matches found` };
       }
 
-      const wasTruncated = allMatches.length >= totalMaxMatches;
-
       const matchesByFile = allMatches.reduce(
         (acc, match) => {
           const fileKey = match.filePath;
@@ -282,16 +281,19 @@ class GrepToolInvocation extends BaseToolInvocation<
         {} as Record<string, GrepMatch[]>,
       );
 
-      const matchCount = allMatches.length;
+      const matchesOnly = allMatches.filter((m) => !m.isContext);
+      const matchCount = matchesOnly.length;
       const matchTerm = matchCount === 1 ? 'match' : 'matches';
 
+      const wasTruncated = matchCount >= totalMaxMatches;
+
       let llmContent = `Found ${matchCount} ${matchTerm} for pattern "${this.params.pattern}" ${searchLocationDescription}${this.params.include ? ` (filter: "${this.params.include}")` : ''}${wasTruncated ? ` (results limited to ${totalMaxMatches} matches for performance)` : ''}:\n---\n`;
 
       for (const filePath in matchesByFile) {
         llmContent += `File: ${filePath}\n`;
         matchesByFile[filePath].forEach((match) => {
-          const trimmedLine = match.line.trim();
-          llmContent += `L${match.lineNumber}: ${trimmedLine}\n`;
+          const separator = match.isContext ? '-' : ':';
+          llmContent += `L${match.lineNumber}${separator} ${match.line}\n`;
         });
         llmContent += '---\n';
       }
@@ -402,11 +404,15 @@ class GrepToolInvocation extends BaseToolInvocation<
         allowedExitCodes: [0, 1],
       });
 
+      let matchesFound = 0;
       for await (const line of generator) {
         const match = this.parseRipgrepJsonLine(line, absolutePath);
         if (match) {
           results.push(match);
-          if (results.length >= maxMatches) {
+          if (!match.isContext) {
+            matchesFound++;
+          }
+          if (matchesFound >= maxMatches) {
             break;
           }
         }
@@ -425,11 +431,11 @@ class GrepToolInvocation extends BaseToolInvocation<
   ): GrepMatch | null {
     try {
       const json = JSON.parse(line);
-      if (json.type === 'match') {
-        const match = json.data;
+      if (json.type === 'match' || json.type === 'context') {
+        const data = json.data;
         // Defensive check: ensure text properties exist (skips binary/invalid encoding)
-        if (match.path?.text && match.lines?.text) {
-          const absoluteFilePath = path.resolve(basePath, match.path.text);
+        if (data.path?.text && data.lines?.text) {
+          const absoluteFilePath = path.resolve(basePath, data.path.text);
           const relativeCheck = path.relative(basePath, absoluteFilePath);
           if (
             relativeCheck === '..' ||
@@ -443,8 +449,9 @@ class GrepToolInvocation extends BaseToolInvocation<
 
           return {
             filePath: relativeFilePath || path.basename(absoluteFilePath),
-            lineNumber: match.line_number,
-            line: match.lines.text.trimEnd(),
+            lineNumber: data.line_number,
+            line: data.lines.text.trimEnd(),
+            isContext: json.type === 'context',
           };
         }
       }
@@ -573,10 +580,12 @@ export class RipGrepTool extends BaseDeclarativeTool<
   protected override validateToolParamValues(
     params: RipGrepToolParams,
   ): string | null {
-    try {
-      new RegExp(params.pattern);
-    } catch (error) {
-      return `Invalid regular expression pattern provided: ${params.pattern}. Error: ${getErrorMessage(error)}`;
+    if (!params.fixed_strings) {
+      try {
+        new RegExp(params.pattern);
+      } catch (error) {
+        return `Invalid regular expression pattern provided: ${params.pattern}. Error: ${getErrorMessage(error)}`;
+      }
     }
 
     // Only validate path if one is provided