feat: add replace_lines tool - token-efficient line-based editing#385
feat: add replace_lines tool - token-efficient line-based editing#385hl9020 wants to merge 2 commits intowonderwhy-er:mainfrom
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as MCP Server
participant Handler as handleReplaceLines
participant FS as File System
participant Telemetry
Client->>Server: CallToolRequest (replace_lines)
Server->>Handler: invoke with args
Handler->>Handler: parse & validate args
Handler->>FS: read file as text
FS-->>Handler: file content
Handler->>Handler: detect line endings
Handler->>Handler: validate line range
Handler->>Handler: split into lines
Handler->>Handler: replace line range
Handler->>Handler: rejoin with original separator
Handler->>FS: write updated content
FS-->>Handler: success
Handler->>Telemetry: capture (removed/inserted lines, extension)
Handler-->>Server: ServerResult with context
Server-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:L
Sequence DiagramThis PR adds a new replace_lines tool that lets clients update files by line number range instead of sending old and new blocks. The core flow is a read to obtain line numbers, then a targeted line replacement that rewrites the file and returns an edit summary. sequenceDiagram
participant Client
participant Server
participant FileSystem
Client->>Server: Read file to get numbered lines
Server-->>Client: Return file content with line numbers
Client->>Server: Call replace_lines with path and line range
Server->>FileSystem: Read file and replace requested lines
FileSystem-->>Server: Save updated file content
Server-->>Client: Return replaced line range summary
Generated by CodeAnt AI |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/tools/edit.ts (2)
475-490: Standalone\rline endings not fully handled.The split regex
/\r?\n/handles\nand\r\nbut not standalone\r(classic Mac format). IfdetectLineEndingreturns'\r', the file won't split into lines correctly, and the separator fallback on line 490 would use'\n'instead, potentially corrupting the file.This is a rare edge case (pre-OS X Mac format), but for consistency with
detectLineEnding's contract, consider:♻️ Suggested handling for standalone \r
- const lines = content.split(/\r?\n/); + // Split on any line ending style the file uses + const lines = content.split(/\r\n|\r|\n/); const totalLines = lines.length; ... - const sep = fileLineEnding === '\r\n' ? '\r\n' : '\n'; + const sep = fileLineEnding;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/edit.ts` around lines 475 - 490, The code assumes only '\n' or '\r\n' in detectLineEnding and uses content.split(/\r?\n/) and sep fallback to '\n', which breaks when detectLineEnding returns '\r'; update splitting to handle standalone '\r' (use content.split(/\r\n|\r|\n/) or equivalent) and ensure newLines = parsed.newContent.split(/\r\n|\r|\n/), and set sep to fileLineEnding (handle '\r' explicitly) so fileLineEnding, detectLineEnding, content.split, parsed.newContent.split, and sep consistently handle '\r', '\n', and '\r\n'.
486-491: EmptynewContentinserts a blank line rather than deleting.When
newContentis an empty string,"".split(/\r?\n/)returns[""](array with one empty string), so the result inserts a blank line rather than truly deleting lines 5-8 as the tool description suggests.Consider whether this is the intended behavior. If true deletion is expected:
♻️ Handle empty content as true deletion
- const newLines = parsed.newContent.split(/\r?\n/); + const newLines = parsed.newContent === '' ? [] : parsed.newContent.split(/\r?\n/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/edit.ts` around lines 486 - 491, The code uses parsed.newContent.split(...) which turns an empty string into [""] and inserts a blank line instead of deleting; update the logic around parsed.newContent (used to build newLines/result/newContent) to treat an empty string as an empty array (i.e., true deletion) — e.g., when parsed.newContent === '' set newLines = [] (otherwise use the existing split), then rebuild result and newContent as before using fileLineEnding to join.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/tools/edit.ts`:
- Around line 475-490: The code assumes only '\n' or '\r\n' in detectLineEnding
and uses content.split(/\r?\n/) and sep fallback to '\n', which breaks when
detectLineEnding returns '\r'; update splitting to handle standalone '\r' (use
content.split(/\r\n|\r|\n/) or equivalent) and ensure newLines =
parsed.newContent.split(/\r\n|\r|\n/), and set sep to fileLineEnding (handle
'\r' explicitly) so fileLineEnding, detectLineEnding, content.split,
parsed.newContent.split, and sep consistently handle '\r', '\n', and '\r\n'.
- Around line 486-491: The code uses parsed.newContent.split(...) which turns an
empty string into [""] and inserts a blank line instead of deleting; update the
logic around parsed.newContent (used to build newLines/result/newContent) to
treat an empty string as an empty array (i.e., true deletion) — e.g., when
parsed.newContent === '' set newLines = [] (otherwise use the existing split),
then rebuild result and newContent as before using fileLineEnding to join.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a04241a-67da-4fd7-b210-e6ba86f3b8b4
📒 Files selected for processing (4)
src/handlers/edit-search-handlers.tssrc/server.tssrc/tools/edit.tssrc/tools/schemas.ts
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
|
|
||
| const before = lines.slice(0, parsed.startLine - 1); | ||
| const after = lines.slice(parsed.endLine); | ||
| const newLines = parsed.newContent.split(/\r?\n/); |
There was a problem hiding this comment.
Suggestion: Splitting newContent directly turns an empty string into [''], so a delete operation (newContent: "") inserts one blank line instead of removing the target range. Treat empty replacement content as zero lines to preserve the documented delete behavior. [logic error]
Severity Level: Major ⚠️
- ❌ replace_lines delete mode leaves unintended blank line.
- ⚠️ Line numbers shift, subsequent read_file-based edits misalign.
- ⚠️ Deletion telemetry reports inserted line count as one.| const newLines = parsed.newContent.split(/\r?\n/); | |
| const newLines = parsed.newContent === '' ? [] : parsed.newContent.split(/\r?\n/); |
Steps of Reproduction ✅
1. Trigger MCP tool execution through `server.setRequestHandler(CallToolRequestSchema,
...)` at `src/server.ts:1201` with tool name `replace_lines`.
2. The request is routed by the switch case `case "replace_lines"` at
`src/server.ts:1452-1453` into `handlers.handleReplaceLines(args)`.
3. Use the documented delete flow shown in the tool description at `src/server.ts:809`:
`startLine=5, endLine=8, newContent=""` (empty string). This input is valid because schema
allows any string via `newContent: z.string()` at `src/tools/schemas.ts:212`.
4. In `handleReplaceLines` (`src/tools/edit.ts:466`), line splitting at
`src/tools/edit.ts:488` executes `''.split(/\r?\n/)`, producing `['']` (one empty line),
then `result = [...before, ...newLines, ...after]` at `src/tools/edit.ts:489` keeps one
blank line instead of removing the range.
5. File is written with the unintended blank line at `src/tools/edit.ts:493`, and
response/telemetry report `insertedCount = newLines.length` (`src/tools/edit.ts:496`) as
`1`, contradicting expected delete semantics.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/tools/edit.ts
**Line:** 488:488
**Comment:**
*Logic Error: Splitting `newContent` directly turns an empty string into `['']`, so a delete operation (`newContent: ""`) inserts one blank line instead of removing the target range. Treat empty replacement content as zero lines to preserve the documented delete behavior.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
|
Hi @hl9020 Thanks for this contribution! The core idea is solid and I want to merge something here, but I have a few thoughts before we do. ✅ On merging: I like that this is designed to complement rather than replace There's an irony worth calling out — we added a tool to save tokens, but every registered tool expands the system prompt/context sent to the model on every request. So the net win is smaller than it appears, and for light usage it might be a net loss. Proposed solution — configurable tool sets: I'd like to introduce a config option (e.g. 📖 Some historical context Line-based replacement is actually where DC started, and I moved away from it deliberately. Two failure modes I kept hitting:
String-based matching fails loudly in both cases — it either finds the string or it doesn't. That "false positive prevention" is a meaningful reliability improvement, not just a style preference. 🔭 Looking ahead — diff-based editing I've been thinking about revisiting unified diff support. I tested it a few years ago and models made a lot of mistakes, but modern models handle diffs much better. It sits in an interesting middle ground:
Longer term I'd like DC to support all three via a single config value, and potentially auto-select based on the connected client/model. Would you be open to building this PR on top of that config scaffolding so it slots naturally into that future design? |
|
Hi @wonderwhy-er, thanks for the detailed feedback! Really appreciate the historical context - knowing DC started with line-based editing and you moved away deliberately is exactly the kind of insight we needed. Your points are well taken: On the token irony - completely valid. Every registered tool adds to the system prompt, so a tool meant to save tokens could be a net loss for light usage. The configurable On reliability - we actually ran into the "stale line numbers" problem ourselves while developing this. That's why we added context lines in the response (3 lines before/after with On the config scaffolding - happy to build this. We'll add We'll look at the implementation details and come back with a concrete update to this PR. |
hl9020
force-pushed
the
feature/replace-lines
branch
from
5e65cf2 to
e2b086f
Compare
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:L
|
Hi @wonderwhy-er, we've reworked the PR based on your feedback. Here's what changed: editMode config scaffolding - added as you suggested:
Config field definition added to CodeRabbit fixes incorporated:
Tested across 24 scenarios including all three editMode values with real restarts, boundary cases, deletion, expansion, shrink, error handling, and validation. Changes: 6 files ( |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/config-field-definitions.ts (1)
41-45: Expose allowededitModevalues in field metadata (not just runtime).
valueType: 'string'works, but it doesn’t communicate valid options (string-replace | line-replace | both) to config consumers/UI. Consider adding constrained options inConfigFieldDefinitionso invalid values are blocked before submission.♻️ Proposed metadata extension
export type ConfigFieldDefinition = { label: string; description: string; valueType: ConfigFieldValueType; + allowedValues?: readonly string[]; };editMode: { label: 'Edit Mode', description: 'Controls which file editing tools are registered. "string-replace" (default) uses edit_block with fuzzy matching. "line-replace" uses replace_lines with line numbers. "both" registers both tools. Fewer tools means a leaner system prompt.', valueType: 'string', + allowedValues: ['string-replace', 'line-replace', 'both'], },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config-field-definitions.ts` around lines 41 - 45, The editMode field currently only sets valueType: 'string' and should include explicit allowed options so UIs and validators can surface/block invalid values; update the editMode entry in src/config-field-definitions.ts to add a constrained-values metadata property (e.g., allowedValues, enum, options — whichever the existing ConfigFieldDefinition type uses or extend ConfigFieldDefinition to include it) listing 'string-replace', 'line-replace', and 'both', and update the ConfigFieldDefinition type (or its consumer) to accept this property so runtime validation and UI pickers can use it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config-manager.ts`:
- Around line 215-221: The editMode check in setValue() is insufficient because
updateConfig() and init() can accept invalid values; add a single shared
validation routine (e.g., validateEditMode or validateConfigEntry) that enforces
the allowed values ['string-replace','line-replace','both'] and use it from
setValue(), updateConfig(), and wherever init() loads on-disk config; when
invalid, either throw a clear Error or normalize to a safe default and log the
rejection so invalid editMode cannot slip into runtime config or tool
registration in src/server.ts.
In `@src/terminal-manager.ts`:
- Around line 48-67: The buildWindowsPath function currently rebuilds PATH only
from Machine/User registry values and nodeDir, dropping any live process PATH
entries; update buildWindowsPath to include process.env.PATH (the live PATH)
when constructing merged (e.g., merge [...new Set([nodeDir, ...sys.split(';'),
...user.split(';'), ...existing.split(';')].filter(Boolean))]) so
wrapper/launch-time injected entries are preserved, and ensure the code that
forces the WINDOWS_PATH into spawned child processes continues to use this
merged value so runtime PATH entries are not lost.
- Around line 34-45: The current resolveShellPath function replaces any string
whose basename matches known shells, which incorrectly rewrites explicit paths
like "C:\tools\pwsh.exe" or "./powershell.exe"; change the logic to only apply
the PS5_PATH/PS7_PATH/CMD_PATH replacements when the caller passed a plain shell
name (no directory separators and not a relative path). In resolveShellPath,
detect explicit paths by checking for path separators or a leading
"."/path.isAbsolute and if the input contains one return shellName unchanged;
only when the input is just a basename (e.g. path.basename(shellName) ===
shellName) perform the existing existence checks against PS5_PATH, PS7_PATH,
CMD_PATH and return the mapped path.
- Around line 136-140: The PowerShell invocation currently passes the
-NonInteractive flag in the branches that handle shellName === 'pwsh' /
'pwsh.exe' and shellName === 'powershell' / 'powershell.exe' which prevents
interactive prompts from working; remove the '-NonInteractive' arg from the args
arrays returned by those branches in the terminal manager (leave '-NoProfile',
'-Command', command and useShellOption: false intact) so sendInputToProcess()
can satisfy Read-Host/confirmation/credential prompts.
In `@src/tools/edit.ts`:
- Line 493: The write uses parsed.path while the file was read using validPath,
causing possible read/write drift; change the write to use the same resolved
validated path (validPath) instead of parsed.path so the writeFile call writes
newContent to validPath (replace the reference to parsed.path in the writeFile
invocation), ensuring both read and write operate on the same validated path.
- Line 523: The warning message uses insertEnd as the anchor and can produce
"after line 0" when a deletion starts at the top of the file; update the string
construction around msg (the line that uses lineDelta and insertEnd) to
special-case insertEnd === 0 and substitute a clearer anchor such as "start of
file" (or "before line 1") instead of "after line 0". Locate the code building
msg (references: msg, lineDelta, insertEnd) and change only the wording logic so
it selects the normal "after line ${insertEnd}" text for insertEnd > 0 and the
clearer anchor text for insertEnd === 0.
---
Nitpick comments:
In `@src/config-field-definitions.ts`:
- Around line 41-45: The editMode field currently only sets valueType: 'string'
and should include explicit allowed options so UIs and validators can
surface/block invalid values; update the editMode entry in
src/config-field-definitions.ts to add a constrained-values metadata property
(e.g., allowedValues, enum, options — whichever the existing
ConfigFieldDefinition type uses or extend ConfigFieldDefinition to include it)
listing 'string-replace', 'line-replace', and 'both', and update the
ConfigFieldDefinition type (or its consumer) to accept this property so runtime
validation and UI pickers can use it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2fd4cfca-dc2d-422e-8f84-09094b73393c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
src/config-field-definitions.tssrc/config-manager.tssrc/handlers/edit-search-handlers.tssrc/server.tssrc/terminal-manager.tssrc/tools/edit.tssrc/tools/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/handlers/edit-search-handlers.ts
- src/tools/schemas.ts
- src/server.ts
| function resolveShellPath(shellName: string): string { | ||
| const n = path.basename(shellName).toLowerCase(); | ||
| if (n === 'powershell.exe' || n === 'powershell') { | ||
| if (fs.existsSync(PS5_PATH)) return PS5_PATH; | ||
| } | ||
| if (n === 'pwsh.exe' || n === 'pwsh') { | ||
| if (fs.existsSync(PS7_PATH)) return PS7_PATH; | ||
| } | ||
| if (n === 'cmd.exe' || n === 'cmd') { | ||
| if (fs.existsSync(CMD_PATH)) return CMD_PATH; | ||
| } | ||
| return shellName; |
There was a problem hiding this comment.
Don't rewrite explicit shell paths.
Lines 34-45 key off path.basename(), so a configured value like C:\tools\pwsh.exe or .\powershell.exe gets replaced with the hardcoded default install path whenever that default exists. That ignores the caller's explicit shell selection and can silently bypass wrappers or shims.
Suggested fix
function resolveShellPath(shellName: string): string {
- const n = path.basename(shellName).toLowerCase();
+ if (path.isAbsolute(shellName) || /[\\/]/.test(shellName)) {
+ return shellName;
+ }
+ const n = shellName.toLowerCase();
if (n === 'powershell.exe' || n === 'powershell') {
if (fs.existsSync(PS5_PATH)) return PS5_PATH;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function resolveShellPath(shellName: string): string { | |
| const n = path.basename(shellName).toLowerCase(); | |
| if (n === 'powershell.exe' || n === 'powershell') { | |
| if (fs.existsSync(PS5_PATH)) return PS5_PATH; | |
| } | |
| if (n === 'pwsh.exe' || n === 'pwsh') { | |
| if (fs.existsSync(PS7_PATH)) return PS7_PATH; | |
| } | |
| if (n === 'cmd.exe' || n === 'cmd') { | |
| if (fs.existsSync(CMD_PATH)) return CMD_PATH; | |
| } | |
| return shellName; | |
| function resolveShellPath(shellName: string): string { | |
| if (path.isAbsolute(shellName) || /[\\/]/.test(shellName)) { | |
| return shellName; | |
| } | |
| const n = shellName.toLowerCase(); | |
| if (n === 'powershell.exe' || n === 'powershell') { | |
| if (fs.existsSync(PS5_PATH)) return PS5_PATH; | |
| } | |
| if (n === 'pwsh.exe' || n === 'pwsh') { | |
| if (fs.existsSync(PS7_PATH)) return PS7_PATH; | |
| } | |
| if (n === 'cmd.exe' || n === 'cmd') { | |
| if (fs.existsSync(CMD_PATH)) return CMD_PATH; | |
| } | |
| return shellName; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/terminal-manager.ts` around lines 34 - 45, The current resolveShellPath
function replaces any string whose basename matches known shells, which
incorrectly rewrites explicit paths like "C:\tools\pwsh.exe" or
"./powershell.exe"; change the logic to only apply the
PS5_PATH/PS7_PATH/CMD_PATH replacements when the caller passed a plain shell
name (no directory separators and not a relative path). In resolveShellPath,
detect explicit paths by checking for path separators or a leading
"."/path.isAbsolute and if the input contains one return shellName unchanged;
only when the input is just a basename (e.g. path.basename(shellName) ===
shellName) perform the existing existence checks against PS5_PATH, PS7_PATH,
CMD_PATH and return the mapped path.
| function buildWindowsPath(): string { | ||
| if (process.platform !== 'win32') return process.env.PATH ?? ''; | ||
| try { | ||
| const sys = execSync( | ||
| `${PS5_PATH} -NoProfile -NonInteractive -Command "[System.Environment]::GetEnvironmentVariable('Path','Machine')"`, | ||
| { encoding: 'utf8', timeout: 3000 } | ||
| ).replace(/\r?\n|\r|"/g, '').trim(); | ||
| const user = execSync( | ||
| `${PS5_PATH} -NoProfile -NonInteractive -Command "[System.Environment]::GetEnvironmentVariable('Path','User')"`, | ||
| { encoding: 'utf8', timeout: 3000 } | ||
| ).replace(/\r?\n|\r|"/g, '').trim(); | ||
| // Also include the directory of the current node executable | ||
| const nodeDir = path.dirname(process.execPath); | ||
| const merged = [...new Set([nodeDir, ...sys.split(';'), ...user.split(';')].filter(Boolean))]; | ||
| return merged.join(';'); | ||
| } catch { | ||
| const nodeDir = path.dirname(process.execPath); | ||
| const existing = process.env.PATH ?? ''; | ||
| return `${nodeDir};${existing}`; | ||
| } |
There was a problem hiding this comment.
Preserve the live process PATH when building WINDOWS_PATH.
Lines 59-62 rebuild Path from Machine/User registry values plus nodeDir, and Lines 112-113 then force that value into every Windows child process. That drops any launch-time or wrapper-injected entries that exist only in the current process, so commands can work in the server and then stop resolving once spawned.
Suggested fix
function buildWindowsPath(): string {
if (process.platform !== 'win32') return process.env.PATH ?? '';
try {
@@
// Also include the directory of the current node executable
const nodeDir = path.dirname(process.execPath);
- const merged = [...new Set([nodeDir, ...sys.split(';'), ...user.split(';')].filter(Boolean))];
+ const current = process.env.Path ?? process.env.PATH ?? '';
+ const merged = [...new Set([
+ ...current.split(';'),
+ nodeDir,
+ ...sys.split(';'),
+ ...user.split(';'),
+ ].filter(Boolean))];
return merged.join(';');
} catch {
const nodeDir = path.dirname(process.execPath);
- const existing = process.env.PATH ?? '';
+ const existing = process.env.Path ?? process.env.PATH ?? '';
return `${nodeDir};${existing}`;
}
}Also applies to: 112-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/terminal-manager.ts` around lines 48 - 67, The buildWindowsPath function
currently rebuilds PATH only from Machine/User registry values and nodeDir,
dropping any live process PATH entries; update buildWindowsPath to include
process.env.PATH (the live PATH) when constructing merged (e.g., merge [...new
Set([nodeDir, ...sys.split(';'), ...user.split(';'),
...existing.split(';')].filter(Boolean))]) so wrapper/launch-time injected
entries are preserved, and ensure the code that forces the WINDOWS_PATH into
spawned child processes continues to use this merged value so runtime PATH
entries are not lost.
| if (shellName === 'pwsh' || shellName === 'pwsh.exe') { | ||
| return { | ||
| executable: shellPath, | ||
| args: ['-Login', '-Command', command], | ||
| useShellOption: false | ||
| }; | ||
| return { executable: resolved, args: ['-NoProfile', '-NonInteractive', '-Command', command], useShellOption: false }; | ||
| } | ||
|
|
||
| // Windows PowerShell 5.1 (no login flag support) | ||
| if (shellName === 'powershell' || shellName === 'powershell.exe') { | ||
| return { | ||
| executable: shellPath, | ||
| args: ['-Command', command], | ||
| useShellOption: false | ||
| }; | ||
| return { executable: resolved, args: ['-NoProfile', '-NonInteractive', '-Command', command], useShellOption: false }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PowerShell CLI: does -NonInteractivedisableRead-Host, confirmation prompts, or other interactive prompts when invoking powershell.exe -Command ...orpwsh -Command ...?
💡 Result:
Yes, the -NonInteractive switch for both powershell.exe and pwsh.exe disables interactive prompts including Read-Host and confirmation prompts when used with -Command (or -File). Instead of hanging waiting for input, these attempts result in statement-terminating errors such as "Windows PowerShell is in NonInteractive mode. Read and Prompt functionality is not available." or equivalent. This behavior is explicitly documented in the official Microsoft documentation for both Windows PowerShell (about_PowerShell_exe) and PowerShell 7+ (about_Pwsh): - "-NonInteractive | -noni: This switch is used to create sessions that shouldn't require user input. This is useful for scripts that run in scheduled tasks or CI/CD pipelines. Any attempts to use interactive features, like Read-Host or confirmation prompts, result in statement terminating errors rather than hanging." Practical examples confirm this: - powershell -NonInteractive -Command "Read-Host" throws the error. - Confirmation prompts from cmdlets (e.g., those using ShouldProcess/ShouldContinue) also fail similarly. Note: If -NonInteractive is used without -Command/-File (e.g., pwsh -NonInteractive alone), it still starts an interactive shell, but interactive features within it will error out.
Citations:
- 1: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_pwsh?view=powershell-7.4
- 2: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_pwsh?view=powershell-7.2
- 3: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_powershell_exe?view=powershell-5.1
- 4: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_pwsh?view=powershell-7.5
- 5: CLI: Prevent nonsensical interactive invocation with -NonInteractive PowerShell/PowerShell#13518
- 6: https://stackoverflow.com/questions/71113043/windows-powershell-is-in-noninteractive-mode-read-and-prompt
Remove -NonInteractive from PowerShell invocations.
Hardcoding -NonInteractive will cause Read-Host, confirmation prompts, and credential prompts to fail immediately instead of allowing sendInputToProcess() to provide input. The -NonInteractive flag explicitly disables interactive features in PowerShell, converting them to statement-terminating errors. Since the class is designed to support blocked sessions and follow-up input, and lines 270-271 already handle prompt detection with multiline anchoring, removing this flag allows interactive commands to work as intended.
Suggested fix
if (shellName === 'pwsh' || shellName === 'pwsh.exe') {
- return { executable: resolved, args: ['-NoProfile', '-NonInteractive', '-Command', command], useShellOption: false };
+ return { executable: resolved, args: ['-NoProfile', '-Command', command], useShellOption: false };
}
if (shellName === 'powershell' || shellName === 'powershell.exe') {
- return { executable: resolved, args: ['-NoProfile', '-NonInteractive', '-Command', command], useShellOption: false };
+ return { executable: resolved, args: ['-NoProfile', '-Command', command], useShellOption: false };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (shellName === 'pwsh' || shellName === 'pwsh.exe') { | |
| return { | |
| executable: shellPath, | |
| args: ['-Login', '-Command', command], | |
| useShellOption: false | |
| }; | |
| return { executable: resolved, args: ['-NoProfile', '-NonInteractive', '-Command', command], useShellOption: false }; | |
| } | |
| // Windows PowerShell 5.1 (no login flag support) | |
| if (shellName === 'powershell' || shellName === 'powershell.exe') { | |
| return { | |
| executable: shellPath, | |
| args: ['-Command', command], | |
| useShellOption: false | |
| }; | |
| return { executable: resolved, args: ['-NoProfile', '-NonInteractive', '-Command', command], useShellOption: false }; | |
| if (shellName === 'pwsh' || shellName === 'pwsh.exe') { | |
| return { executable: resolved, args: ['-NoProfile', '-Command', command], useShellOption: false }; | |
| } | |
| if (shellName === 'powershell' || shellName === 'powershell.exe') { | |
| return { executable: resolved, args: ['-NoProfile', '-Command', command], useShellOption: false }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/terminal-manager.ts` around lines 136 - 140, The PowerShell invocation
currently passes the -NonInteractive flag in the branches that handle shellName
=== 'pwsh' / 'pwsh.exe' and shellName === 'powershell' / 'powershell.exe' which
prevents interactive prompts from working; remove the '-NonInteractive' arg from
the args arrays returned by those branches in the terminal manager (leave
'-NoProfile', '-Command', command and useShellOption: false intact) so
sendInputToProcess() can satisfy Read-Host/confirmation/credential prompts.
| let msg = `Replaced lines ${parsed.startLine}-${parsed.endLine} (${removedCount} lines) with ${insertedCount} lines in ${parsed.path}`; | ||
|
|
||
| if (lineDelta !== 0) { | ||
| msg += `\n\nWARNING: Line count changed by ${lineDelta > 0 ? '+' : ''}${lineDelta}. All line numbers after line ${insertEnd} have shifted. Re-read before further edits.`; |
There was a problem hiding this comment.
Avoid “after line 0” in shift warnings for top-of-file deletions.
At Line 523, deleting from Line 1 with zero inserted lines produces a confusing warning (“after line 0”). Use a clearer anchor for this edge case.
Proposed fix
- if (lineDelta !== 0) {
- msg += `\n\nWARNING: Line count changed by ${lineDelta > 0 ? '+' : ''}${lineDelta}. All line numbers after line ${insertEnd} have shifted. Re-read before further edits.`;
- }
+ if (lineDelta !== 0) {
+ const shiftAnchorText =
+ insertEnd <= 0
+ ? 'from line 1 onward'
+ : `after line ${insertEnd}`;
+ msg += `\n\nWARNING: Line count changed by ${lineDelta > 0 ? '+' : ''}${lineDelta}. All line numbers ${shiftAnchorText} have shifted. Re-read before further edits.`;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| msg += `\n\nWARNING: Line count changed by ${lineDelta > 0 ? '+' : ''}${lineDelta}. All line numbers after line ${insertEnd} have shifted. Re-read before further edits.`; | |
| if (lineDelta !== 0) { | |
| const shiftAnchorText = | |
| insertEnd <= 0 | |
| ? 'from line 1 onward' | |
| : `after line ${insertEnd}`; | |
| msg += `\n\nWARNING: Line count changed by ${lineDelta > 0 ? '+' : ''}${lineDelta}. All line numbers ${shiftAnchorText} have shifted. Re-read before further edits.`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/edit.ts` at line 523, The warning message uses insertEnd as the
anchor and can produce "after line 0" when a deletion starts at the top of the
file; update the string construction around msg (the line that uses lineDelta
and insertEnd) to special-case insertEnd === 0 and substitute a clearer anchor
such as "start of file" (or "before line 1") instead of "after line 0". Locate
the code building msg (references: msg, lineDelta, insertEnd) and change only
the wording logic so it selects the normal "after line ${insertEnd}" text for
insertEnd > 0 and the clearer anchor text for insertEnd === 0.
|
CodeAnt AI Incremental review completed. |
…case, editMode validation hardening
hl9020
force-pushed
the
feature/replace-lines
branch
from
e2b086f to
8de629b
Compare
2 participants
Summary
Adds a new
replace_linestool with configurableeditModescaffolding, directly addressing #68 and incorporating @wonderwhy-er's feedback on configurable tool sets.Problem
edit_blockrequires sending bothold_string+new_string, which doubles token usage for edits where the position is already known from a previousread_filecall.Solution
1.
replace_linestoolreplace_lines(path, startLine, endLine, newContent)- when line numbers are known fromread_fileoutput, only line numbers + new content are needed. Roughly 50% fewer tokens per edit.pathstartLineread_fileoutput)endLinenewContentResponse includes context and shift warnings:
2.
editModeconfig optionPer @wonderwhy-er's suggestion, a new config value controls which editing tools are registered:
"string-replace"(default)edit_blockonly"line-replace"replace_linesonly"both"Set via
set_config_value("editMode", "both"), takes effect after restart. Invalid values are rejected with a clear error message. Naturally extensible for a future"diff"mode.Design decisions
replace_linesnot registered unless explicitly enabled, keeping the default context leanedit_blockread_fileoutput+markers and line-shift warningsChanges
6 files:
src/config-field-definitions.ts-editModefield definition for settings panelsrc/config-manager.ts-editModeinServerConfiginterface + input validationsrc/tools/schemas.ts-ReplaceLinesArgsSchemawith Zod validationsrc/tools/edit.ts-handleReplaceLinesimplementation with context outputsrc/handlers/edit-search-handlers.ts- Export new handlersrc/server.ts-shouldIncludeToolwith async editMode check, tool definition, switch caseTesting
24 test scenarios across 4 categories, all passing:
Config (A1-A8): All three editMode values tested with real restarts - correct tools registered/hidden in each mode.
Functionality (B1-B8): Equal line count, expansion (+4 shift), shrink (-4 shift), deletion (true removal), boundary cases (first/last line), error handling (out of bounds, invalid range).
Compatibility (C1):
edit_blockworks correctly in "both" mode.Validation (E1-E4): Invalid editMode values rejected, config persists on disk.
Closes #68
Summary by CodeRabbit
editModeconfiguration option to control which editing tools are available:string-replace,line-replace, orboth.