Skip to content

Commit 169fb35

Browse files
committed
fix: correct line count for trailing newlines and fix line-counter tests
Two fixes: 1. Line counting off-by-one: Files ending with \n now count correctly - "line1\nline2\n" now correctly shows lines="1-2" not lines="1-3" - Consistent with countFileLines() behavior - Prevents LLM confusion about line numbers 2. Fixed line-counter.spec.ts mocking: - Use proper Readable stream instead of mock object - Properly mock fs.createReadStream with stream interface - All 63 tests passing (42 readFileTool + 17 fileTokenBudget + 4 line-counter) Files changed: - src/core/tools/readFileTool.ts: Handle trailing newline in line count - src/integrations/misc/__tests__/line-counter.spec.ts: Fix stream mocking
1 parent 100afdf commit 169fb35

File tree

2 files changed

+41
-37
lines changed

2 files changed

+41
-37
lines changed

src/core/tools/readFileTool.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -617,7 +617,11 @@ export async function readFileTool(
617617
content = truncateResult.content
618618

619619
// Reflect actual displayed line count after truncation (count ALL lines, including empty)
620-
const displayedLines = content.length === 0 ? 0 : content.split(/\r?\n/).length
620+
// Handle trailing newline: "line1\nline2\n" should be 2 lines, not 3
621+
let displayedLines = content.length === 0 ? 0 : content.split(/\r?\n/).length
622+
if (displayedLines > 0 && (content.endsWith("\n") || content.endsWith("\r\n"))) {
623+
displayedLines--
624+
}
621625
const lineRangeAttr = displayedLines > 0 ? ` lines="1-${displayedLines}"` : ""
622626
xmlInfo = content.length > 0 ? `<content${lineRangeAttr}>\n${content}</content>\n` : `<content/>`
623627
xmlInfo += `<notice>${truncateResult.notice}</notice>\n`

src/integrations/misc/__tests__/line-counter.spec.ts

Lines changed: 36 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { describe, it, expect, vi, beforeEach } from "vitest"
22
import { countFileLines, countFileLinesAndTokens } from "../line-counter"
3-
import fs from "fs"
43
import { countTokens } from "../../../utils/countTokens"
4+
import { Readable } from "stream"
55

66
// Mock dependencies
77
vi.mock("fs", () => ({
@@ -23,74 +23,74 @@ vi.mock("../../../utils/countTokens", () => ({
2323

2424
const mockCountTokens = vi.mocked(countTokens)
2525

26+
// Get the mocked fs module
27+
const fs = await import("fs")
28+
const mockCreateReadStream = vi.mocked(fs.createReadStream)
29+
const mockFsAccess = vi.mocked(fs.default.promises.access)
30+
2631
describe("line-counter", () => {
2732
beforeEach(() => {
2833
vi.clearAllMocks()
2934
})
3035

3136
describe("countFileLinesAndTokens", () => {
3237
it("should count lines and tokens without budget limit", async () => {
33-
const mockStream = {
34-
on: vi.fn((event, handler) => {
35-
if (event === "data") {
36-
// Simulate reading lines
37-
handler("line1\n")
38-
handler("line2\n")
39-
handler("line3\n")
40-
}
41-
return mockStream
42-
}),
43-
destroy: vi.fn(),
44-
}
45-
46-
vi.mocked(fs.createReadStream).mockReturnValue(mockStream as any)
47-
vi.mocked(fs.promises.access).mockResolvedValue(undefined)
48-
49-
// Mock token counting - simulate ~10 tokens per line
38+
// Create a proper readable stream
39+
const mockStream = new Readable({
40+
read() {
41+
this.push("line1\n")
42+
this.push("line2\n")
43+
this.push("line3\n")
44+
this.push(null) // End of stream
45+
},
46+
})
47+
48+
mockCreateReadStream.mockReturnValue(mockStream as any)
49+
mockFsAccess.mockResolvedValue(undefined)
50+
51+
// Mock token counting - simulate ~10 tokens per chunk
5052
mockCountTokens.mockResolvedValue(30)
5153

5254
const result = await countFileLinesAndTokens("/test/file.txt")
5355

54-
expect(result.lineCount).toBeGreaterThan(0)
55-
expect(result.tokenEstimate).toBeGreaterThan(0)
56+
expect(result.lineCount).toBe(3)
57+
expect(result.tokenEstimate).toBe(30)
5658
expect(result.complete).toBe(true)
5759
})
5860

5961
it("should handle tokenizer errors with conservative estimate", async () => {
60-
const mockStream = {
61-
on: vi.fn((event, handler) => {
62-
if (event === "data") {
63-
handler("line1\n")
64-
}
65-
return mockStream
66-
}),
67-
destroy: vi.fn(),
68-
}
69-
70-
vi.mocked(fs.createReadStream).mockReturnValue(mockStream as any)
71-
vi.mocked(fs.promises.access).mockResolvedValue(undefined)
62+
// Create a proper readable stream
63+
const mockStream = new Readable({
64+
read() {
65+
this.push("line1\n")
66+
this.push(null)
67+
},
68+
})
69+
70+
mockCreateReadStream.mockReturnValue(mockStream as any)
71+
mockFsAccess.mockResolvedValue(undefined)
7272

7373
// Simulate tokenizer error
7474
mockCountTokens.mockRejectedValue(new Error("unreachable"))
7575

7676
const result = await countFileLinesAndTokens("/test/file.txt")
7777

78-
// Should still complete with conservative token estimate
79-
expect(result.lineCount).toBeGreaterThan(0)
78+
// Should still complete with conservative token estimate (content.length)
79+
expect(result.lineCount).toBe(1)
8080
expect(result.tokenEstimate).toBeGreaterThan(0)
8181
expect(result.complete).toBe(true)
8282
})
8383

8484
it("should throw error for non-existent files", async () => {
85-
vi.mocked(fs.promises.access).mockRejectedValue(new Error("ENOENT"))
85+
mockFsAccess.mockRejectedValue(new Error("ENOENT"))
8686

8787
await expect(countFileLinesAndTokens("/nonexistent/file.txt")).rejects.toThrow("File not found")
8888
})
8989
})
9090

9191
describe("countFileLines", () => {
9292
it("should throw error for non-existent files", async () => {
93-
vi.mocked(fs.promises.access).mockRejectedValue(new Error("ENOENT"))
93+
mockFsAccess.mockRejectedValue(new Error("ENOENT"))
9494

9595
await expect(countFileLines("/nonexistent/file.txt")).rejects.toThrow("File not found")
9696
})

0 commit comments

Comments
 (0)