Skip to content

Commit 03a7a30

Browse files
committed
fix: address PR review comments
- P2: Make regex more robust by allowing optional leading whitespace in definition line parsing - P3: Add test coverage for readFileTool truncation path Changes: - Updated regex in truncateDefinitions.ts to handle indented output or CRLF artifacts - Added tests for whitespace handling in truncateDefinitions.spec.ts - Added comprehensive tests for readFileTool truncation behavior in readFileTool.spec.ts
1 parent c7238ab commit 03a7a30

File tree

3 files changed

+84
-1
lines changed

3 files changed

+84
-1
lines changed

src/core/tools/__tests__/readFileTool.spec.ts

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -414,6 +414,61 @@ describe("read_file tool with maxReadFileLine setting", () => {
414414
expect(result).toContain(`<list_code_definition_names>`)
415415
expect(result).toContain("<notice>Showing only 3 of 5 total lines")
416416
})
417+
418+
it("should truncate code definitions when file exceeds maxReadFileLine", async () => {
419+
// Setup - file with 100 lines but we'll only read first 30
420+
const content = "Line 1\nLine 2\nLine 3"
421+
const numberedContent = "1 | Line 1\n2 | Line 2\n3 | Line 3"
422+
const fullDefinitions = `# file.txt
423+
10--20 | function foo() {
424+
50--60 | function bar() {
425+
80--90 | function baz() {`
426+
const truncatedDefinitions = `# file.txt
427+
10--20 | function foo() {`
428+
429+
mockedReadLines.mockResolvedValue(content)
430+
mockedParseSourceCodeDefinitionsForFile.mockResolvedValue(fullDefinitions)
431+
addLineNumbersMock.mockReturnValue(numberedContent)
432+
433+
// Execute with maxReadFileLine = 30
434+
const result = await executeReadFileTool({}, { maxReadFileLine: 30, totalLines: 100 })
435+
436+
// Verify that only definitions within the first 30 lines are included
437+
expect(result).toContain(`<file><path>${testFilePath}</path>`)
438+
expect(result).toContain(`<content lines="1-30">`)
439+
expect(result).toContain(`<list_code_definition_names>`)
440+
441+
// Should include foo (starts at line 10) but not bar (starts at line 50) or baz (starts at line 80)
442+
expect(result).toContain("10--20 | function foo()")
443+
expect(result).not.toContain("50--60 | function bar()")
444+
expect(result).not.toContain("80--90 | function baz()")
445+
446+
expect(result).toContain("<notice>Showing only 30 of 100 total lines")
447+
})
448+
449+
it("should handle truncation when all definitions are beyond the line limit", async () => {
450+
// Setup - all definitions start after maxReadFileLine
451+
const content = "Line 1\nLine 2\nLine 3"
452+
const numberedContent = "1 | Line 1\n2 | Line 2\n3 | Line 3"
453+
const fullDefinitions = `# file.txt
454+
50--60 | function foo() {
455+
80--90 | function bar() {`
456+
457+
mockedReadLines.mockResolvedValue(content)
458+
mockedParseSourceCodeDefinitionsForFile.mockResolvedValue(fullDefinitions)
459+
addLineNumbersMock.mockReturnValue(numberedContent)
460+
461+
// Execute with maxReadFileLine = 30
462+
const result = await executeReadFileTool({}, { maxReadFileLine: 30, totalLines: 100 })
463+
464+
// Verify that only the header is included (all definitions filtered out)
465+
expect(result).toContain(`<file><path>${testFilePath}</path>`)
466+
expect(result).toContain(`<content lines="1-30">`)
467+
expect(result).toContain(`<list_code_definition_names>`)
468+
expect(result).toContain("# file.txt")
469+
expect(result).not.toContain("50--60 | function foo()")
470+
expect(result).not.toContain("80--90 | function bar()")
471+
})
417472
})
418473

419474
describe("when maxReadFileLine equals or exceeds file length", () => {

src/core/tools/helpers/__tests__/truncateDefinitions.spec.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,4 +130,31 @@ describe("truncateDefinitionsToLineLimit", () => {
130130

131131
expect(result).toBe(expected)
132132
})
133+
134+
it("should handle definitions with leading whitespace", () => {
135+
const definitions = `# test.ts
136+
10--20 | function foo() {
137+
30--40 | function bar() {
138+
50--60 | function baz() {`
139+
140+
const result = truncateDefinitionsToLineLimit(definitions, 25)
141+
const expected = `# test.ts
142+
10--20 | function foo() {`
143+
144+
expect(result).toBe(expected)
145+
})
146+
147+
it("should handle definitions with mixed whitespace patterns", () => {
148+
const definitions = `# test.ts
149+
10--20 | function foo() {
150+
30--40 | function bar() {
151+
50--60 | function baz() {`
152+
153+
const result = truncateDefinitionsToLineLimit(definitions, 35)
154+
const expected = `# test.ts
155+
10--20 | function foo() {
156+
30--40 | function bar() {`
157+
158+
expect(result).toBe(expected)
159+
})
133160
})

src/core/tools/helpers/truncateDefinitions.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,8 @@ export function truncateDefinitionsToLineLimit(definitions: string, maxReadFileL
2525
const line = lines[i]
2626

2727
// Match definition format: "startLine--endLine | content" or "lineNumber | content"
28-
const rangeMatch = line.match(/^(\d+)(?:--(\d+))?\s*\|/)
28+
// Allow optional leading whitespace to handle indented output or CRLF artifacts
29+
const rangeMatch = line.match(/^\s*(\d+)(?:--(\d+))?\s*\|/)
2930

3031
if (rangeMatch) {
3132
const startLine = parseInt(rangeMatch[1], 10)

0 commit comments

Comments
 (0)