Add line number support to read_file and search_replace tools#2649
Add line number support to read_file and search_replace tools#2649wwwillchen-bot wants to merge 6 commits intodyad-sh:mainfrom
Conversation
wwwillchen-bot
temporarily deployed
to
ai-bots
GitHub Actions
Inactive
wwwillchen-bot
temporarily deployed
to
ai-bots
GitHub Actions
Inactive
|
@BugBot run |
Summary of ChangesHello @wwwillchen-bot, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the interaction of agents with file content by integrating line number support across the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Greptile OverviewGreptile SummaryThis PR adds line number support to Key Changes:
Implementation Quality:
Confidence Score: 5/5
Important Files Changed
Flowchartflowchart TD
A[Agent calls read_file tool] --> B[read_file.ts: Read file content]
B --> C[line_number_utils.addLineNumberPrefixes]
C --> D["Return numbered content<br/>e.g., '1 pipe line one<br/>2 pipe line two'"]
D --> E[Agent receives numbered output]
E --> F[Agent calls search_replace<br/>with numbered old_string]
F --> G[search_replace.ts: Apply changes]
G --> H[search_replace_line_numbers_processor]
H --> I{Has line numbers?}
I -->|Yes| J[stripLineNumberPrefixes<br/>from search content]
J --> K{Line numbers match<br/>file location?}
K -->|Yes| L[Direct match at line number]
K -->|No| M[Cascading fuzzy match]
I -->|No| M
M --> N[Pass 1: Exact match]
N --> O{Match found?}
O -->|No| P[Pass 2: Trailing whitespace ignored]
P --> Q{Match found?}
Q -->|No| R[Pass 3: All edge whitespace ignored]
R --> S{Match found?}
S -->|No| T[Pass 4: Unicode normalized]
T --> U{Match found?}
U -->|Yes| V[Apply replacement]
O -->|Yes| V
Q -->|Yes| V
S -->|Yes| V
L --> V
V --> W[Write updated file]
U -->|No| X[Generate enhanced error<br/>with best partial match]
X --> Y[Throw error]
Last reviewed commit: 154de17 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces line number support for the read_file and search_replace tools, which is a great usability improvement. The implementation is robust, featuring a new line_number_utils module and a significantly enhanced search_replace_line_numbers_processor. The addition of cascading fuzzy matching and detailed error messages in the new processor will greatly improve the reliability and debuggability of the search_replace tool. The test coverage for all new functionality is comprehensive. I have one minor suggestion to ensure trailing whitespace is preserved during replacement operations. Overall, this is a high-quality contribution.
src/pro/main/ipc/processors/search_replace_line_numbers_processor.ts
Outdated
Show resolved
Hide resolved
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 8 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/pro/main/ipc/handlers/local_agent/tools/read_file.ts">
<violation number="1" location="src/pro/main/ipc/handlers/local_agent/tools/read_file.ts:108">
P2: Line numbers reset to 1 when reading a line range, so the returned prefixes no longer match the original file line numbers. This can mislead users who expect the reported line numbers to correspond to the file’s actual lines.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
🔍 Dyadbot Code Review SummaryFound 5 new issue(s) flagged by 3 independent reviewers. Summary
Issues to Address
🟢 Low Priority Issues (3 items)
See inline comments for details. Generated by Dyadbot multi-agent code review (3 independent reviewers with consensus voting) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/pro/main/ipc/processors/search_replace_line_numbers_processor.ts
Outdated
Show resolved
Hide resolved
src/pro/main/ipc/processors/search_replace_line_numbers_processor.ts
Outdated
Show resolved
Hide resolved
src/pro/main/ipc/processors/search_replace_line_numbers_processor.ts
Outdated
Show resolved
Hide resolved
github-actions
bot
added
the
needs-human:review-issue
wwwillchen-bot
temporarily deployed
to
ai-bots
GitHub Actions
Inactive
wwwillchen-bot
temporarily deployed
to
ai-bots
GitHub Actions
Inactive
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 4/5All review comments have been addressed with code changes and tests pass, but the changes involve core functionality that would benefit from human verification. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions - principles were not needed for these technical code review issues. 🤖 Generated by Claude Code |
🔍 Dyadbot Code Review SummaryFound 2 new issue(s) flagged by 3 independent reviewers. Summary
Issues to Address
🟢 Low Priority Issues (3 items)
See inline comments for details. Generated by Dyadbot multi-agent code review (3 independent reviewers with consensus voting) |
|
@BugBot run |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (3)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/ipc/processors/response_processor.ts
Line: 40:40
Comment:
`response_processor.ts` still uses old `applySearchReplace` processor instead of new line-numbers-aware version
```suggestion
import { applySearchReplaceWithLineNumbers } from "../../pro/main/ipc/processors/search_replace_line_numbers_processor";
```
How can I resolve this? If you propose a fix, please make it concise.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: src/ipc/processors/response_processor.ts
Line: 75:75
Comment:
Update call to use line-numbers-aware processor for consistency
```suggestion
const result = applySearchReplaceWithLineNumbers(original, tag.content);
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: src/ipc/processors/response_processor.ts
Line: 382:382
Comment:
Also update this call to use the new processor
```suggestion
const result = applySearchReplaceWithLineNumbers(original, tag.content);
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
src/pro/main/ipc/processors/search_replace_line_numbers_processor.ts
Outdated
Show resolved
Hide resolved
- Normalize CRLF to LF in addLineNumberPrefixes and read_file line-range branch to avoid embedded \r characters in output - Require at least 2 sequential line numbers for stripping (prevents false positives on single-line content) - Add fallback to original content when line number stripping causes match failure (handles false positive edge cases) - Only call unescapeMarkers for diff-format blocks, not 3-arg direct invocation path - Clamp displayed line range in error messages to actual file length - Update test expectations for new single-line behavior Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Simplified retry logic and split handling in main processor - Improved line number range extraction utility - Updated and consolidated test cases Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
wwwillchen-bot
force-pushed
the
agent--1770856289398-1770856325
branch
from
da82765 to
d3ef1eb
Compare
wwwillchen-bot
temporarily deployed
to
ai-bots
GitHub Actions
Inactive
wwwillchen-bot
temporarily deployed
to
ai-bots
GitHub Actions
Inactive
|
@BugBot run |
wwwillchen-bot
removed
the
needs-human:review-issue
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (4 items)
🚫 Dropped False Positives (5 items)
Generated by Dyadbot multi-agent code review |
github-actions
bot
added
the
needs-human:review-issue
wwwillchen-bot
temporarily deployed
to
ai-bots
GitHub Actions
Inactive
wwwillchen-bot
had a problem deploying
to
ai-bots
GitHub Actions
Error
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 4/5All review issues have been addressed with code changes or explanations. The PR implements useful line number handling for read_file and search_replace tools. Unresolved Threads
Resolved Threads
Product Principle SuggestionsNo suggestions - principles were clear enough for all decisions 🤖 Generated by Claude Code |
- Fix phantom empty line for files with trailing newlines in read_file - Only strip line numbers from replace content when search also has line numbers - Truncate large error messages to keep them concise and agent-friendly - Update test expectations to reflect corrected trailing newline behavior Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
wwwillchen-bot
force-pushed
the
agent--1770856289398-1770856325
branch
from
dd41a93 to
154de17
Compare
wwwillchen-bot
temporarily deployed
to
ai-bots
GitHub Actions
Inactive
|
@BugBot run |
wwwillchen-bot
removed
the
needs-human:review-issue
wwwillchen-bot
temporarily deployed
to
ai-bots
GitHub Actions
Inactive
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (4 items)
🚫 Dropped False Positives (2 items)
Generated by Dyadbot multi-agent code review |
| "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| \"}" |
There was a problem hiding this comment.
🔴 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.
| 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"; |
There was a problem hiding this comment.
🟡 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."
| } from "./types"; | ||
| import { safeJoin } from "@/ipc/utils/path_utils"; | ||
| import { applySearchReplace } from "@/pro/main/ipc/processors/search_replace_processor"; | ||
| import { applySearchReplaceWithLineNumbers } from "@/pro/main/ipc/processors/search_replace_line_numbers_processor"; |
There was a problem hiding this comment.
🟡 MEDIUM | misleading-description
old_string description says "must match exactly" but line-number prefixes are silently stripped
The old_string field description (line 30) tells LLM agents the text "must match the file contents exactly, including all whitespace and indentation." However, the tool now silently strips line number prefixes before matching via applySearchReplaceWithLineNumbers. This creates a contradiction: the description says exact match is required, but the behavior accepts line-numbered content that does not literally appear in the file.
💡 Suggestion: Update the description to mention that line number prefixes from read_file output are automatically stripped, e.g.: "You can paste content directly from read_file output — line number prefixes (e.g., 1| ) will be automatically stripped before matching."
| 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; |
There was a problem hiding this comment.
🟡 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:
| 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.
| return endIdx >= lines.length && hasTrailingNewline | ||
| ? result + "\n" | ||
| : result; | ||
| ? numbered + "\n" | ||
| : numbered; |
There was a problem hiding this comment.
🟡 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 = 99endIdx = Math.min(3, 3) = 3lines.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.
| return endIdx >= lines.length && hasTrailingNewline | |
| ? result + "\n" | |
| : result; | |
| ? numbered + "\n" | |
| : numbered; | |
| return endIdx >= lines.length && hasTrailingNewline && numbered !== "" | |
| ? numbered + "\n" | |
| : numbered; |
Was this helpful? React with 👍 or 👎 to provide feedback.
github-actions
bot
added
the
needs-human:review-issue
🎭 Playwright Test Results❌ Some tests failed
Summary: 230 passed, 3 failed, 11 flaky, 6 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/context_manage.spec.ts \
e2e-tests/local_agent_ask.spec.ts \
e2e-tests/themes_management.spec.ts
|
2 participants
Summary
line_number_utilsmodule withaddLineNumberPrefixesandstripLineNumberPrefixesfunctions for consistent line number handlingsearch_replace_line_numbers_processorthat strips line numbers from search blocks before matching and handles line-numbered content in search/replace operationsread_filetool to always return content with line number prefixes (e.g.,1 content)search_replacetool to use the line-numbers-aware processor, allowing users to copy/paste line-numbered content from read_file output directly into search blocksTest plan
line_number_utils(27 tests)search_replace_line_numbers_processor(27 tests)read_fileandsearch_replacetools to ensure backwards compatibility🤖 Generated with Claude Code
Summary by cubic
Added line-number support to read_file and search_replace so you can copy/paste numbered snippets directly from read_file into search_replace. This improves matching reliability and provides clearer errors.
New Features
Migration
Written for commit 7de0083. Summary will update on new commits.