Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
{
"role": "tool",
"tool_call_id": "[[TOOL_CALL_0]]",
"content": "{\"type\":\"text\",\"value\":\"const App = () => <div>Minimal imported app</div>;\\n\\nexport default App;\\n\"}"
"content": "{\"type\":\"text\",\"value\":\"1| const App = () => <div>Minimal imported app</div>;\\n2| \\n3| export default App;\\n4| \"}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 HIGH | stale-snapshot

E2E snapshot contains phantom line 4 that current code will not produce

The snapshot shows the read_file output as 1| const App...\n2| \n3| export default App;\n4| with a phantom empty numbered line 4.

However, the current read_file code (lines 95-105 of read_file.ts) explicitly strips the trailing newline before calling addLineNumberPrefixes and restores it afterward. For the 3-line file const App...;\n\nexport default App;\n, the actual output should be 1| const App...;\n2| \n3| export default App;\n (3 numbered lines + trailing newline, no 4| ).

The unit tests in read_file.spec.ts (the "preserves trailing newline" test) confirm the correct behavior without a phantom line 4. This e2e snapshot will fail when run.

💡 Suggestion: Regenerate the e2e snapshot, or manually fix the value to remove the 4| suffix.

},
{
"role": "assistant",
Expand Down
65 changes: 37 additions & 28 deletions src/pro/main/ipc/handlers/local_agent/tools/read_file.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,12 +151,14 @@ line 5`;
});

describe("execute - full file read", () => {
it("reads entire file when no line range is specified", async () => {
it("reads entire file with line numbers when no line range is specified", async () => {
const result = await readFileTool.execute(
{ path: "test.txt" },
mockContext,
);
expect(result).toBe(testFileContent);
expect(result).toBe(
"1| line 1\n2| line 2\n3| line 3\n4| line 4\n5| line 5",
);
});

it("returns empty string for empty files", async () => {
Expand All @@ -175,28 +177,30 @@ line 5`;
});

describe("execute - start_line_one_indexed only", () => {
it("reads from start line to end of file", async () => {
it("reads from start line to end of file with line numbers reflecting actual positions", async () => {
const result = await readFileTool.execute(
{ path: "test.txt", start_line_one_indexed: 3 },
mockContext,
);
expect(result).toBe("line 3\nline 4\nline 5");
expect(result).toBe("3| line 3\n4| line 4\n5| line 5");
});

it("reads from line 1 (same as full file)", async () => {
it("reads from line 1 with line numbers (same as full file)", async () => {
const result = await readFileTool.execute(
{ path: "test.txt", start_line_one_indexed: 1 },
mockContext,
);
expect(result).toBe(testFileContent);
expect(result).toBe(
"1| line 1\n2| line 2\n3| line 3\n4| line 4\n5| line 5",
);
});

it("reads last line when start equals line count", async () => {
it("reads last line with line numbers when start equals line count", async () => {
const result = await readFileTool.execute(
{ path: "test.txt", start_line_one_indexed: 5 },
mockContext,
);
expect(result).toBe("line 5");
expect(result).toBe("5| line 5");
});

it("returns empty string when start exceeds line count", async () => {
Expand All @@ -209,41 +213,45 @@ line 5`;
});

describe("execute - end_line_one_indexed_inclusive only", () => {
it("reads from beginning to end line", async () => {
it("reads from beginning to end line with line numbers", async () => {
const result = await readFileTool.execute(
{ path: "test.txt", end_line_one_indexed_inclusive: 3 },
mockContext,
);
expect(result).toBe("line 1\nline 2\nline 3");
expect(result).toBe("1| line 1\n2| line 2\n3| line 3");
});

it("reads first line only", async () => {
it("reads first line only with line numbers", async () => {
const result = await readFileTool.execute(
{ path: "test.txt", end_line_one_indexed_inclusive: 1 },
mockContext,
);
expect(result).toBe("line 1");
expect(result).toBe("1| line 1");
});

it("reads entire file when end equals line count", async () => {
it("reads entire file with line numbers when end equals line count", async () => {
const result = await readFileTool.execute(
{ path: "test.txt", end_line_one_indexed_inclusive: 5 },
mockContext,
);
expect(result).toBe(testFileContent);
expect(result).toBe(
"1| line 1\n2| line 2\n3| line 3\n4| line 4\n5| line 5",
);
});

it("clamps to file length when end exceeds line count", async () => {
const result = await readFileTool.execute(
{ path: "test.txt", end_line_one_indexed_inclusive: 100 },
mockContext,
);
expect(result).toBe(testFileContent);
expect(result).toBe(
"1| line 1\n2| line 2\n3| line 3\n4| line 4\n5| line 5",
);
});
});

describe("execute - both start and end", () => {
it("reads a middle range", async () => {
it("reads a middle range with line numbers reflecting actual positions", async () => {
const result = await readFileTool.execute(
{
path: "test.txt",
Expand All @@ -252,10 +260,10 @@ line 5`;
},
mockContext,
);
expect(result).toBe("line 2\nline 3\nline 4");
expect(result).toBe("2| line 2\n3| line 3\n4| line 4");
});

it("reads a single line when start equals end", async () => {
it("reads a single line with line numbers when start equals end", async () => {
const result = await readFileTool.execute(
{
path: "test.txt",
Expand All @@ -264,7 +272,7 @@ line 5`;
},
mockContext,
);
expect(result).toBe("line 3");
expect(result).toBe("3| line 3");
});

it("clamps both to valid range", async () => {
Expand All @@ -276,20 +284,20 @@ line 5`;
},
mockContext,
);
expect(result).toBe("line 4\nline 5");
expect(result).toBe("4| line 4\n5| line 5");
});
});

describe("execute - single-line file", () => {
it("reads full single-line file", async () => {
it("reads full single-line file with line numbers", async () => {
const result = await readFileTool.execute(
{ path: "single-line.txt" },
mockContext,
);
expect(result).toBe("only one line");
expect(result).toBe("1| only one line");
});

it("reads single-line file with line range", async () => {
it("reads single-line file with line range and line numbers", async () => {
const result = await readFileTool.execute(
{
path: "single-line.txt",
Expand All @@ -298,21 +306,22 @@ line 5`;
},
mockContext,
);
expect(result).toBe("only one line");
expect(result).toBe("1| only one line");
});
});

describe("execute - trailing newline file", () => {
it("preserves trailing newline on full read via line range", async () => {
it("preserves trailing newline on full read via line range with line numbers", async () => {
const result = await readFileTool.execute(
{
path: "trailing-newline.txt",
start_line_one_indexed: 1,
end_line_one_indexed_inclusive: 3,
end_line_one_indexed_inclusive: 4,
},
mockContext,
);
expect(result).toBe("line 1\nline 2\nline 3\n");
// Note: trailing newline becomes an empty line 4 when split, so we get 4 lines
expect(result).toBe("1| line 1\n2| line 2\n3| line 3\n4| ");
});

it("does not add trailing newline for partial range", async () => {
Expand All @@ -324,7 +333,7 @@ line 5`;
},
mockContext,
);
expect(result).toBe("line 1\nline 2");
expect(result).toBe("1| line 1\n2| line 2");
});

it("full file read matches line-range full read", async () => {
Expand Down
20 changes: 12 additions & 8 deletions src/pro/main/ipc/handlers/local_agent/tools/read_file.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import fs from "node:fs";
import { z } from "zod";
import { ToolDefinition, AgentContext, escapeXmlAttr } from "./types";
import { safeJoin } from "@/ipc/utils/path_utils";
import { addLineNumberPrefixes } from "@/pro/main/ipc/processors/line_number_utils";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM | discoverability

read_file tool description does not mention line-numbered output format

The description (line 47) says only "Read the content of a file from the codebase." It does not mention that the output now includes line number prefixes (e.g., 1| content). LLM agents using this tool will encounter the new format without any indication in the tool contract that the output changed.

💡 Suggestion: Add a note to the description, e.g.: "Output includes line number prefixes in the format N| content for easy reference. These prefixes can be used directly in search_replace old_string — they will be automatically stripped."


const readFile = fs.promises.readFile;

Expand Down Expand Up @@ -92,18 +93,21 @@ export const readFileTool: ToolDefinition<z.infer<typeof readFileSchema>> = {
const end = args.end_line_one_indexed_inclusive;

if (start == null && end == null) {
return content;
return addLineNumberPrefixes(content);
}

const hasTrailingNewline = content.endsWith("\n");
const lines = (hasTrailingNewline ? content.slice(0, -1) : content).split(
"\n",
);
// Normalize CRLF to LF before line operations to avoid embedded \r characters
const normalized = content.replace(/\r\n/g, "\n");
const hasTrailingNewline = normalized.endsWith("\n");
const lines = (
hasTrailingNewline ? normalized.slice(0, -1) : normalized
).split("\n");
const startIdx = Math.max(0, (start ?? 1) - 1);
const endIdx = Math.min(lines.length, end ?? lines.length);
const result = lines.slice(startIdx, endIdx).join("\n");
return endIdx >= lines.length && hasTrailingNewline
? result + "\n"
: result;
const slicedContent =
endIdx >= lines.length && hasTrailingNewline ? result + "\n" : result;
// Pass the actual starting line number so line numbers reflect file positions
return addLineNumberPrefixes(slicedContent, startIdx + 1);
},
};
128 changes: 128 additions & 0 deletions src/pro/main/ipc/handlers/local_agent/tools/search_replace.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -397,4 +397,132 @@ describe("searchReplaceTool", () => {
expect(preview).toBe("Edit src/test.ts");
});
});

describe("line number stripping", () => {
it("strips line number prefixes from old_string and matches", async () => {
const originalContent = [
"function greet() {",
" console.log('Hello');",
" return true;",
"}",
].join("\n");

vi.mocked(fs.existsSync).mockReturnValue(true);
vi.mocked(fs.promises.readFile).mockResolvedValue(originalContent);

// old_string has line number prefixes (as if copied from read_file output)
const result = await searchReplaceTool.execute(
{
file_path: "test.ts",
old_string: "1| function greet() {\n2| console.log('Hello');",
new_string: "function greet() {\n console.log('Hi there');",
},
mockContext,
);

expect(result).toContain("Successfully");
expect(fs.promises.writeFile).toHaveBeenCalledWith(
"/test/app/test.ts",
expect.stringContaining("console.log('Hi there')"),
);
});

it("strips line number prefixes from both old_string and new_string", async () => {
const originalContent = [
"function test() {",
" const x = 1;",
" return x;",
"}",
].join("\n");

vi.mocked(fs.existsSync).mockReturnValue(true);
vi.mocked(fs.promises.readFile).mockResolvedValue(originalContent);

const result = await searchReplaceTool.execute(
{
file_path: "test.ts",
old_string: "1| function test() {\n2| const x = 1;",
new_string: "1| function test() {\n2| const y = 2;",
},
mockContext,
);

expect(result).toContain("Successfully");
expect(fs.promises.writeFile).toHaveBeenCalledWith(
"/test/app/test.ts",
expect.stringContaining("const y = 2"),
);
});

it("handles padded line numbers (right-aligned)", async () => {
const originalContent = Array.from(
{ length: 15 },
(_, i) => `line ${i + 1}`,
).join("\n");

vi.mocked(fs.existsSync).mockReturnValue(true);
vi.mocked(fs.promises.readFile).mockResolvedValue(originalContent);

// Padded line numbers for a file with 15+ lines
const result = await searchReplaceTool.execute(
{
file_path: "test.ts",
old_string: "10| line 10\n11| line 11\n12| line 12",
new_string: "line 10 modified\nline 11 modified\nline 12 modified",
},
mockContext,
);

expect(result).toContain("Successfully");
expect(fs.promises.writeFile).toHaveBeenCalledWith(
"/test/app/test.ts",
expect.stringContaining("line 10 modified"),
);
});
});

describe("enhanced error messages", () => {
it("provides detailed error message for no-match failures", async () => {
const originalContent = [
"function greet() {",
" console.log('Hello');",
" return true;",
"}",
].join("\n");

vi.mocked(fs.existsSync).mockReturnValue(true);
vi.mocked(fs.promises.readFile).mockResolvedValue(originalContent);

await expect(
searchReplaceTool.execute(
{
file_path: "test.ts",
old_string:
"function greet() {\n console.log('Hi there');\n return true;\n}",
new_string:
"function greet() {\n console.log('Hello World');\n return true;\n}",
},
mockContext,
),
).rejects.toThrow(/SEARCH CONTENT|BEST PARTIAL MATCH|SUGGESTION/);
});

it("provides detailed error message for ambiguous matches", async () => {
const originalContent = ["foo", "bar", "baz", "bar", "qux"].join("\n");

vi.mocked(fs.existsSync).mockReturnValue(true);
vi.mocked(fs.promises.readFile).mockResolvedValue(originalContent);

await expect(
searchReplaceTool.execute(
{
file_path: "test.ts",
old_string: "bar",
new_string: "BAR",
},
mockContext,
),
).rejects.toThrow(/MATCHED LOCATIONS|SUGGESTION|context/i);
});
});
});
Loading
Loading