Skip to content
Closed
Show file tree
Hide file tree
Changes from all 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
Copy Markdown
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
66 changes: 38 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,23 @@ 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");
// File has 3 lines with trailing newline. Requesting line 4 clamps to line 3.
// Trailing newline is preserved but does not create a phantom empty numbered line.
expect(result).toBe("1| line 1\n2| line 2\n3| line 3\n");
});

it("does not add trailing newline for partial range", async () => {
Expand All @@ -324,7 +334,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
29 changes: 22 additions & 7 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
Copy Markdown
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,32 @@ export const readFileTool: ToolDefinition<z.infer<typeof readFileSchema>> = {
const end = args.end_line_one_indexed_inclusive;

if (start == null && end == null) {
return content;
// Normalize CRLF to LF and handle trailing newlines consistently.
// A file ending with \n should not produce a phantom empty numbered line.
const normalized = content.replace(/\r\n/g, "\n");
const hasTrailingNewline = normalized.endsWith("\n");
const contentToNumber = hasTrailingNewline
? normalized.slice(0, -1)
: normalized;
const numbered = addLineNumberPrefixes(contentToNumber);
// Restore trailing newline in the output
return hasTrailingNewline ? numbered + "\n" : numbered;
}

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");
// Add line number prefixes first, then restore trailing newline if applicable.
// This prevents a phantom empty numbered line for files ending with \n.
const numbered = addLineNumberPrefixes(result, startIdx + 1);
return endIdx >= lines.length && hasTrailingNewline
? result + "\n"
: result;
? numbered + "\n"
: numbered;
Comment on lines 95 to +122

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 MEDIUM | duplication

Duplicated CRLF normalization and trailing-newline handling across both branches

The two branches (full-file read on lines 95-105 and line-range read on lines 108-122) both perform the identical pattern: normalize CRLF → detect trailing newline → strip trailing newline → process → add line numbers → restore trailing newline. If the trailing-newline handling needs to change (e.g., a bug fix), both branches must be updated in lockstep.

💡 Suggestion: Extract the shared trailing-newline-aware numbering into a helper function:

Suggested change
if (start == null && end == null) {
return content;
// Normalize CRLF to LF and handle trailing newlines consistently.
// A file ending with \n should not produce a phantom empty numbered line.
const normalized = content.replace(/\r\n/g, "\n");
const hasTrailingNewline = normalized.endsWith("\n");
const contentToNumber = hasTrailingNewline
? normalized.slice(0, -1)
: normalized;
const numbered = addLineNumberPrefixes(contentToNumber);
// Restore trailing newline in the output
return hasTrailingNewline ? numbered + "\n" : numbered;
}
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");
// Add line number prefixes first, then restore trailing newline if applicable.
// This prevents a phantom empty numbered line for files ending with \n.
const numbered = addLineNumberPrefixes(result, startIdx + 1);
return endIdx >= lines.length && hasTrailingNewline
? result + "\n"
: result;
? numbered + "\n"
: numbered;
if (start == null && end == null) {
return numberContentWithTrailingNewline(content);
}
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 sliced = lines.slice(startIdx, endIdx).join("\n");
const trailingNl = endIdx >= lines.length && hasTrailingNewline;
return numberContentWithTrailingNewline(
trailingNl ? sliced + "\n" : sliced,
startIdx + 1,
);

Where numberContentWithTrailingNewline is a shared helper that handles the normalize → strip → number → restore pattern.

Comment on lines 120 to +122

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 read_file returns "\n" instead of "" when reading past end of a file with a trailing newline

When start_line_one_indexed exceeds the file's line count, lines.slice(startIdx, endIdx) produces an empty array and result is "". However, the trailing-newline restoration check endIdx >= lines.length && hasTrailingNewline still evaluates to true (because endIdx is clamped to lines.length), causing the function to return "\n" instead of "".

Root cause and impact

For example, calling read_file with start_line_one_indexed: 100 on a 3-line file whose content is "line 1\nline 2\nline 3\n" (trailing newline):

  • startIdx = 99
  • endIdx = Math.min(3, 3) = 3
  • lines.slice(99, 3)[]result = ""
  • numbered = addLineNumberPrefixes("", 100)"" (early return for empty)
  • Condition: endIdx (3) >= lines.length (3) && hasTrailingNewline (true)true
  • Returns "" + "\n" = "\n"

The guard at read_file.ts:120 should additionally check that result (or numbered) is non-empty before appending the trailing newline. This bug is pre-existing (the old code at read_file.ts:106 had the same pattern) but is carried into the new changed lines.

Impact: An AI agent reading past the end of a file with a trailing newline receives "\n" instead of "", which could mislead it into thinking content exists at that position.

Suggested change
return endIdx >= lines.length && hasTrailingNewline
? result + "\n"
: result;
? numbered + "\n"
: numbered;
return endIdx >= lines.length && hasTrailingNewline && numbered !== ""
? numbered + "\n"
: numbered;
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

},
};
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