From 3cecd667d57e19ee27c591460fb5bd97ea41e880 Mon Sep 17 00:00:00 2001 From: tt-a1i <53142663+tt-a1i@users.noreply.github.com> Date: Fri, 19 Jun 2026 13:47:13 +0800 Subject: [PATCH] fix(core): validate grep result limits (#5389) Co-authored-by: Shaojin Wen --- docs/developers/tools/file-system.md | 2 +- packages/core/src/tools/grep.test.ts | 17 +++++++++++++++-- packages/core/src/tools/grep.ts | 12 ++++++++++-- packages/core/src/tools/ripGrep.test.ts | 14 ++++++++++++++ packages/core/src/tools/ripGrep.ts | 12 ++++++++++-- 5 files changed, 50 insertions(+), 7 deletions(-) diff --git a/docs/developers/tools/file-system.md b/docs/developers/tools/file-system.md index d07fd805c6..f384da195e 100644 --- a/docs/developers/tools/file-system.md +++ b/docs/developers/tools/file-system.md @@ -155,7 +155,7 @@ notebook_edit( - `pattern` (string, required): The regular expression pattern to search for in file contents (e.g., `"function\\s+myFunction"`, `"log.*Error"`). - `path` (string, optional): File or directory to search in. Defaults to current working directory. - `glob` (string, optional): Glob pattern to filter files (e.g. `"*.js"`, `"src/**/*.{ts,tsx}"`). - - `limit` (number, optional): Limit output to first N matching lines. Optional - shows all matches if not specified. + - `limit` (integer, optional): Limit output to first N matching lines. Must be a positive integer. Optional - shows all matches if not specified. - **Behavior:** - Uses ripgrep for fast search when available; otherwise falls back to a JavaScript-based search implementation. - Returns matching lines with file paths and line numbers. diff --git a/packages/core/src/tools/grep.test.ts b/packages/core/src/tools/grep.test.ts index e3fcbe8419..156de3bed6 100644 --- a/packages/core/src/tools/grep.test.ts +++ b/packages/core/src/tools/grep.test.ts @@ -154,6 +154,20 @@ describe('GrepTool', () => { expect(grepTool.validateToolParams(params)).toBeNull(); }); + it('should return null for a positive integer limit', () => { + const params: GrepToolParams = { pattern: 'hello', limit: 2 }; + expect(grepTool.validateToolParams(params)).toBeNull(); + }); + + it.each([ + [0, 'params/limit must be >= 1'], + [-1, 'params/limit must be >= 1'], + [1.5, 'params/limit must be integer'], + ])('should return error for invalid limit %s', (limit, expectedError) => { + const params: GrepToolParams = { pattern: 'hello', limit }; + expect(grepTool.validateToolParams(params)).toBe(expectedError); + }); + it('should return error if pattern is missing', () => { const params = { path: '.' } as unknown as GrepToolParams; expect(grepTool.validateToolParams(params)).toBe( @@ -658,8 +672,7 @@ describe('GrepTool', () => { expect(result.returnDisplay).toBe('Found 30 matches'); }); - it('should not validate limit parameter', () => { - // limit parameter has no validation constraints in the new implementation + it('should validate a positive limit parameter', () => { const params = { pattern: 'test', limit: 5 }; const error = grepTool.validateToolParams(params as GrepToolParams); expect(error).toBeNull(); diff --git a/packages/core/src/tools/grep.ts b/packages/core/src/tools/grep.ts index d0d30712f5..10bc72f691 100644 --- a/packages/core/src/tools/grep.ts +++ b/packages/core/src/tools/grep.ts @@ -629,9 +629,10 @@ export class GrepTool extends BaseDeclarativeTool { 'File or directory to search in. Defaults to current working directory.', }, limit: { - type: 'number', + type: 'integer', + minimum: 1, description: - 'Limit output to first N matching lines. Optional - shows all matches if not specified.', + 'Limit output to first N matching lines. Must be a positive integer. Optional - shows all matches if not specified.', }, }, required: ['pattern'], @@ -648,6 +649,13 @@ export class GrepTool extends BaseDeclarativeTool { protected override validateToolParamValues( params: GrepToolParams, ): string | null { + if ( + params.limit !== undefined && + (!Number.isInteger(params.limit) || params.limit <= 0) + ) { + return 'limit must be a positive integer'; + } + // Validate pattern is a valid regex try { new RegExp(params.pattern); diff --git a/packages/core/src/tools/ripGrep.test.ts b/packages/core/src/tools/ripGrep.test.ts index 57b12e067a..b415c0fcbe 100644 --- a/packages/core/src/tools/ripGrep.test.ts +++ b/packages/core/src/tools/ripGrep.test.ts @@ -118,6 +118,20 @@ describe('RipGrepTool', () => { expect(grepTool.validateToolParams(params)).toBeNull(); }); + it('should return null for a positive integer limit', () => { + const params: RipGrepToolParams = { pattern: 'hello', limit: 2 }; + expect(grepTool.validateToolParams(params)).toBeNull(); + }); + + it.each([ + [0, 'params/limit must be >= 1'], + [-1, 'params/limit must be >= 1'], + [1.5, 'params/limit must be integer'], + ])('should return error for invalid limit %s', (limit, expectedError) => { + const params: RipGrepToolParams = { pattern: 'hello', limit }; + expect(grepTool.validateToolParams(params)).toBe(expectedError); + }); + it('should return error if pattern is missing', () => { const params = { path: '.' } as unknown as RipGrepToolParams; expect(grepTool.validateToolParams(params)).toBe( diff --git a/packages/core/src/tools/ripGrep.ts b/packages/core/src/tools/ripGrep.ts index 34299d3b35..1e569b65f4 100644 --- a/packages/core/src/tools/ripGrep.ts +++ b/packages/core/src/tools/ripGrep.ts @@ -528,9 +528,10 @@ export class RipGrepTool extends BaseDeclarativeTool< 'File or directory to search in (rg PATH). Defaults to current working directory.', }, limit: { - type: 'number', + type: 'integer', + minimum: 1, description: - 'Limit output to first N lines/entries. Optional - shows all matches if not specified.', + 'Limit output to first N lines/entries. Must be a positive integer. Optional - shows all matches if not specified.', }, }, required: ['pattern'], @@ -555,6 +556,13 @@ export class RipGrepTool extends BaseDeclarativeTool< return errors; } + if ( + params.limit !== undefined && + (!Number.isInteger(params.limit) || params.limit <= 0) + ) { + return 'limit must be a positive integer'; + } + // Validate pattern is a valid regex try { new RegExp(params.pattern);