-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Add line number support to read_file and search_replace tools #2649
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 5 commits
17359b1
6c0f09c
4007f53
8e56ec6
d3ef1eb
154de17
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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., 💡 Suggestion: Add a note to the description, e.g.: "Output includes line number prefixes in the format |
||
|
|
||
| const readFile = fs.promises.readFile; | ||
|
|
||
|
|
@@ -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); | ||
wwwillchen-bot marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
wwwillchen-bot marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| 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); | ||
wwwillchen-bot marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| }, | ||
| }; | ||
There was a problem hiding this comment.
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_filecode (lines 95-105 ofread_file.ts) explicitly strips the trailing newline before callingaddLineNumberPrefixesand restores it afterward. For the 3-line fileconst App...;\n\nexport default App;\n, the actual output should be1| const App...;\n2| \n3| export default App;\n(3 numbered lines + trailing newline, no4|).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.