fix(tools): require approval for delete_line_range and insert_at_line in DEFAULT mode #1000
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… in DEFAULT mode - Add shouldConfirmExecute override to DeleteLineRangeToolInvocation - Add shouldConfirmExecute override to InsertAtLineToolInvocation - Both tools now show diff and request confirmation in DEFAULT mode - Both tools auto-approve in AUTO_EDIT and YOLO modes - Rename respect_gemini_ignore to respect_llxprt_ignore in ls.ts - Sanitize NODE_OPTIONS to remove --localstorage-file flag in start.js closes #958 closes #979 closes #942
Contributor
WalkthroughAdds diff-based pre-execution confirmation to DeleteLineRange and InsertAtLine tools (new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Tool as DeleteLineRangeTool
participant Config
participant FS as FileSystem
participant IDE as IDE Client
Client->>Tool: shouldConfirmExecute()
Tool->>Config: fetch ApprovalMode
alt ApprovalMode is AUTO_EDIT or YOLO
Tool-->>Client: false (skip confirmation)
else DEFAULT
Tool->>FS: readFile(path)
alt read success
FS-->>Tool: originalContent
else ENOENT
FS-->>Tool: ENOENT (treat as empty/new)
end
Note right of Tool: compute proposedContent (delete lines)
Tool->>Tool: generate diff (originalContent → proposedContent)
alt IDE connected
Tool->>IDE: request diff preview
IDE-->>Tool: previewPayload
end
Tool-->>Client: ToolCallConfirmationDetails (ToolEditConfirmationDetails)
end
sequenceDiagram
autonumber
participant Client
participant Tool as InsertAtLineTool
participant Config
participant FS as FileSystem
participant IDE as IDE Client
Client->>Tool: shouldConfirmExecute()
Tool->>Config: fetch ApprovalMode
alt ApprovalMode is AUTO_EDIT or YOLO
Tool-->>Client: false (skip confirmation)
else DEFAULT
Tool->>FS: try readFile(path)
alt read success
FS-->>Tool: originalContent
else ENOENT
FS-->>Tool: ENOENT (prepare new content)
end
Note right of Tool: validate line_number, compute proposedContent (insert)
Tool->>Tool: generate diff (originalContent → proposedContent)
alt IDE connected
Tool->>IDE: request diff preview
IDE-->>Tool: previewPayload
end
Tool-->>Client: ToolCallConfirmationDetails (ToolEditConfirmationDetails)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
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 |
github-actions
bot
added
the
maintainer:e2e:ok
Contributor
LLxprt PR Review – PR #1000Issue Alignment
Side Effects
Code Quality
Tests & Coverage
Verdict |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/start.js (1)
107-119: LGTM - Consistent with UI path.The sanitization logic correctly mirrors the UI path implementation, maintaining consistency across both execution modes.
Optional: Consider extracting the conditional env update logic into a helper function to reduce duplication between the UI and CLI paths:
🔎 Optional refactor to extract common logic
function applyNodeOptionsToEnv(env, originalNodeOptions) { const sanitized = sanitizeNodeOptions(originalNodeOptions); if (sanitized !== originalNodeOptions) { if (sanitized) { env.NODE_OPTIONS = sanitized; } else { delete env.NODE_OPTIONS; } } }Then use it in both paths:
// UI path const uiEnv = { ...process.env, CLI_VERSION: pkg.version, DEV: 'true', }; applyNodeOptionsToEnv(uiEnv, process.env.NODE_OPTIONS); // CLI path const env = { ...process.env, CLI_VERSION: pkg.version, DEV: 'true', }; applyNodeOptionsToEnv(env, process.env.NODE_OPTIONS);packages/core/src/tools/ls.ts (1)
226-229: Consider using the logging system for file access errors.Line 228 uses
console.errorto log file access errors. While this is different fromconsole.log/console.debugmentioned in the coding guidelines, consider whether these errors should be routed through the sophisticated logging system for consistency with the rest of the codebase.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/core/src/tools/delete_line_range.test.tspackages/core/src/tools/delete_line_range.tspackages/core/src/tools/insert_at_line.test.tspackages/core/src/tools/insert_at_line.tspackages/core/src/tools/ls.tsscripts/start.js
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Don't useany- Always specify proper types. Useunknownif the type is truly unknown and add proper type guards.
Do not useconsole.logorconsole.debug- Use the sophisticated logging system instead. Log files are written to ~/.llxprt/debug/
Fix all linting errors, including warnings aboutanytypes
Files:
packages/core/src/tools/ls.tspackages/core/src/tools/insert_at_line.test.tspackages/core/src/tools/delete_line_range.test.tspackages/core/src/tools/delete_line_range.tspackages/core/src/tools/insert_at_line.ts
🧬 Code graph analysis (2)
packages/core/src/tools/insert_at_line.test.ts (4)
packages/core/src/services/fileSystemService.ts (1)
StandardFileSystemService(44-62)packages/core/src/config/index.ts (1)
ApprovalMode(11-11)packages/core/src/tools/insert_at_line.ts (1)
InsertAtLineTool(251-325)packages/core/src/tools/tools.ts (1)
ToolEditConfirmationDetails(555-570)
packages/core/src/tools/delete_line_range.test.ts (4)
packages/core/src/services/fileSystemService.ts (1)
StandardFileSystemService(44-62)packages/core/src/config/index.ts (1)
ApprovalMode(11-11)packages/core/src/tools/delete_line_range.ts (1)
DeleteLineRangeTool(221-301)packages/core/src/tools/tools.ts (1)
ToolEditConfirmationDetails(555-570)
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test (ubuntu-latest, 24.x)
- GitHub Check: Test (windows-latest, 24.x)
- GitHub Check: Test (macos-latest, 24.x)
- GitHub Check: Slow E2E - Win
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (macOS)
🔇 Additional comments (23)
scripts/start.js (1)
80-92: LGTM - Proper conditional environment update.The sanitization is applied correctly, and the conditional logic ensures NODE_OPTIONS is only updated when necessary. Deleting the environment variable when the sanitized result is empty is appropriate.
packages/core/src/tools/ls.ts (4)
37-42: Consistent rename of interface property fromrespect_gemini_ignoretorespect_llxprt_ignore.The interface property and comment correctly reference
.llxprtignorepatterns. The change is consistent with the PR objective to fix issue #942.
169-172: Internal wiring correctly updated to userespect_llxprt_ignore.The parameter parsing properly maps the new
respect_llxprt_ignoreoption to the internalrespectLlxprtIgnoreproperty.
180-180: Counter variable and message renamed consistently.The variable
llxprtIgnoredCountand user-facing messagellxprt-ignoredare consistently renamed throughout the file.Also applies to: 208-214, 249-251
306-320: Schema property correctly renamed torespect_llxprt_ignore.The tool schema description and property name are updated to reference
.llxprtignore, completing the rename across all touchpoints.packages/core/src/tools/insert_at_line.test.ts (4)
1-35: Well-structured test setup with proper mocking.The test file follows good practices with clear separation of mock configuration, temporary directory management, and proper typing. The mock config includes all necessary methods for the InsertAtLineTool.
75-128: Comprehensive validation tests.Tests cover all validation scenarios: valid parameters, relative paths, paths outside workspace, invalid line numbers, and empty content. This ensures the
validateToolParamValuesmethod is thoroughly tested.
130-209: Thorough confirmation flow testing across all approval modes.The tests properly verify that:
- DEFAULT mode requires confirmation with diff details
- AUTO_EDIT mode auto-approves
- YOLO mode auto-approves
- New file creation in DEFAULT mode still requests confirmation
This directly validates the fix for issue #979.
211-296: Good execution test coverage including edge cases.Tests cover successful insertion, new file creation at line 1, errors for non-existent files with line > 1, line_number exceeding file length, and appending at end of file. The expected file contents are correctly verified.
packages/core/src/tools/delete_line_range.test.ts (4)
1-35: Consistent test setup matching InsertAtLineTool tests.The mock configuration and test structure mirror the insert_at_line.test.ts file, ensuring consistency across the codebase.
75-124: Good validation test coverage.Tests cover valid parameters, relative paths, paths outside workspace, invalid start_line, and end_line < start_line constraint. The validation logic is well-tested.
126-186: Confirmation flow tests validate the fix for issue #979.The tests correctly verify:
- DEFAULT mode shows confirmation with diff containing the lines to be deleted
- AUTO_EDIT and YOLO modes auto-approve (return false)
- Non-existent file returns false to defer error handling to execute()
188-225: Execution tests verify correct line deletion behavior.Tests confirm successful deletion, proper error handling for non-existent files (FILE_NOT_FOUND), and validation of start_line exceeding file length (INVALID_TOOL_PARAMS).
packages/core/src/tools/delete_line_range.ts (5)
7-31: Appropriate imports for the confirmation flow.The new imports bring in Diff for patch generation, confirmation types from tools.js, ApprovalMode for mode checking, DEFAULT_DIFF_OPTIONS for consistent diff formatting, and IDEConnectionStatus for IDE integration.
75-101: Correct early-exit logic for auto-approval modes and error cases.The method properly returns
false(auto-approve) for AUTO_EDIT and YOLO modes. Returningfalseon file read errors and invalid start_line is intentional, deferring toexecute()for proper error reporting to the user.
103-122: Diff generation correctly computes the proposed content.The splice operation properly removes the specified line range. The diff is generated using
Diff.createPatchwith consistent options, showing the user exactly what lines will be deleted.
124-148: IDE integration and confirmation details properly structured.The IDE diff preview is conditionally opened only when IDE mode is enabled and connected. The
onConfirmcallback correctly upgrades to AUTO_EDIT mode when the user selects "ProceedAlways".
150-165: Execute method handles file read errors with appropriate error type.The error handling returns
FILE_NOT_FOUNDfor read errors, which is appropriate for the delete operation since the file must exist.packages/core/src/tools/insert_at_line.ts (5)
7-31: Consistent imports matching DeleteLineRangeTool.The imports for Diff, confirmation types, ApprovalMode, DEFAULT_DIFF_OPTIONS, and IDEConnectionStatus are consistent with the delete_line_range.ts implementation.
76-102: Proper handling of approval modes and file existence.The method correctly:
- Returns
falsefor AUTO_EDIT and YOLO modes- Uses
isNodeErrorto detect ENOENT for new file creation- Returns
falseon other read errors, deferring toexecute()
104-119: Line number validation handles edge cases correctly.The validation properly handles:
- Existing files: allows line_number up to totalLines + 1 (append)
- Non-existent files: only allows line_number = 1
The splice operation correctly inserts new lines at the computed index.
121-160: Diff generation and IDE integration follow established patterns.The implementation matches the pattern in delete_line_range.ts: generates a patch with
Diff.createPatch, conditionally opens IDE diff preview, and returns properly structuredToolEditConfirmationDetails.
162-193: Execute method error handling is thorough.The method properly distinguishes between:
- ENOENT with line_number != 1: returns INVALID_TOOL_PARAMS
- ENOENT with line_number = 1: allows new file creation
- Other read errors: returns FILE_NOT_FOUND
Address CodeRabbit review feedback: update regex to handle both --localstorage-file=value and --localstorage-file value formats.
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes three related issues:
Issue #979: delete_line_range and insert_at_line bypass approval in DEFAULT mode
Problem: The
delete_line_rangeandinsert_at_linetools were executing without asking for user approval even in DEFAULT approval mode. This was a security concern as destructive file operations were happening without user confirmation.Root Cause: The tool invocations did not override the
shouldConfirmExecutemethod. The base implementation inBaseToolInvocationreturnsfalse, which theCoreToolSchedulerinterprets as "auto-approve".Fix: Added
shouldConfirmExecuteoverride to bothDeleteLineRangeToolInvocationandInsertAtLineToolInvocationclasses. These methods:false(auto-approve) in AUTO_EDIT and YOLO modesToolEditConfirmationDetailsin DEFAULT mode, matching the behavior ofwrite_fileandreplaceIssue #942: respect_gemini_ignore should be respect_llxprt_ignore in ls.ts
Problem: The
list_directorytool's file filtering options usedrespect_gemini_ignoreinstead of the correctrespect_llxprt_ignore, creating API inconsistency.Fix: Renamed all occurrences:
geminiIgnoredCounttollxprtIgnoredCountIssue #958: localstorage-file warning on startup
Problem: Users saw "Warning: --localstorage-file was provided without a valid path" when starting the CLI, caused by a dangling
--localstorage-fileflag inNODE_OPTIONS.Fix: Added a
sanitizeNodeOptionsfunction inscripts/start.jsthat strips--localstorage-file(with or without value) fromNODE_OPTIONSbefore spawning child processes.Testing
Added comprehensive tests for
delete_line_range.test.tscovering:Added comprehensive tests for
insert_at_line.test.tscovering:Existing
ls.test.tstests continue to pass with the renameVerification
node scripts/start.js --profile-load synthetic "write me a haiku"closes #958 closes #979 closes #942
Summary by CodeRabbit
New Features
Tests
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.