-
Notifications
You must be signed in to change notification settings - Fork 210
feat: enhance edit_block with multi-block, flags, and error handling #81
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?
feat: enhance edit_block with multi-block, flags, and error handling #81
Conversation
Implements multi-block support for edit_block. Adds flags for global ('g'), case-insensitive ('i'), counted ('n:X'), and dry-run ('d') replacements. Includes enhanced error reporting for parsing and execution. Maintains full backward compatibility. Adds documentation and comprehensive tests for new features.
WalkthroughThis update introduces a comprehensive enhancement to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Handler as handleEditBlock
participant Parser as parseEditBlock
participant Worker as performSearchReplace
participant FS as File System
User->>Handler: Submit edit_block command
Handler->>Parser: Parse edit block(s) with flags
Parser-->>Handler: Return filePath, searchReplace blocks, errors
alt Errors present
Handler-->>User: Return error response
else No errors
Handler->>Worker: Apply search/replace (multi-block, flags)
Worker->>FS: Read file content
Worker->>FS: (If not dryRun) Write modified content
Worker-->>Handler: Return replacement results
Handler-->>User: Return success/results
end
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 11
🧹 Nitpick comments (3)
docs/enhanced-edit-block.md (2)
12-12
: Minor redundancy in the wordingThe phrase "specific details" is slightly redundant.
-6. **Enhanced Error Handling**: Better error reporting with specific details +6. **Enhanced Error Handling**: Better error reporting with detailsOr alternatively:
-6. **Enhanced Error Handling**: Better error reporting with specific details +6. **Enhanced Error Handling**: Detailed error reporting🧰 Tools
🪛 LanguageTool
[style] ~12-~12: This phrase is redundant. Consider writing “details”.
Context: ...Handling**: Better error reporting with specific details ## Usage ### Basic Syntax with Flags ...(SPECIFIC_DETAILS)
20-27
: Add language specification to the code blockAdding a language identifier to the fenced code block improves syntax highlighting in many Markdown renderers.
-``` +```text filepath <<<<<<< SEARCH[:flags] content to find ======= new content >>>>>>> REPLACE🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
20-20: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
src/tools/edit.ts (1)
211-219
: File size check counts UTF‑16 code units, not bytes
content.length
returns JavaScript string length (UTF‑16 code units).
A multibyte file (e.g., containing emoji or CJK) may be >10 MB in bytes yet pass this test.
Read the file asBuffer
(or convert viaBuffer.byteLength(content, 'utf8')
) when enforcing the byte cap.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.gitignore
(1 hunks)docs/enhanced-edit-block.md
(1 hunks)src/handlers/edit-search-handlers.ts
(2 hunks)src/tools/edit.ts
(1 hunks)test-advanced-edit-block.js
(1 hunks)test-counted-examples.txt
(1 hunks)test-counted-replacements.js
(1 hunks)test-edit-block-original.txt
(1 hunks)test-edit-block.txt
(1 hunks)test-error-handling.js
(1 hunks)test-multi-block.js
(1 hunks)test-multiple-blocks-enhanced.txt
(1 hunks)test-multiple-blocks.txt
(1 hunks)test-single-block.js
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/handlers/edit-search-handlers.ts (3)
src/tools/schemas.ts (1)
EditBlockArgsSchema
(88-90)src/tools/edit.ts (2)
parseEditBlock
(54-179)performSearchReplace
(192-436)src/error-handlers.ts (1)
createErrorResponse
(9-17)
test-error-handling.js (1)
src/tools/edit.ts (2)
parseEditBlock
(54-179)performSearchReplace
(192-436)
test-counted-replacements.js (1)
src/tools/edit.ts (2)
parseEditBlock
(54-179)performSearchReplace
(192-436)
🪛 LanguageTool
docs/enhanced-edit-block.md
[style] ~12-~12: This phrase is redundant. Consider writing “details”.
Context: ...Handling**: Better error reporting with specific details ## Usage ### Basic Syntax with Flags ...
(SPECIFIC_DETAILS)
🪛 markdownlint-cli2 (0.17.2)
docs/enhanced-edit-block.md
20-20: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
36-36: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
49-49: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
74-74: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
87-87: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
100-100: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
115-115: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
130-130: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (41)
.gitignore (1)
39-42
: Appropriate backup ignore rules added.Ignoring
server.log-e
, thebackup/
directory, and*.bak
files is consistent with the intended purpose of excluding temporary and backup artifacts.test-edit-block.txt (1)
1-11
: Test fixture structure looks correct.The sample text covers various patterns and the file serves as a good foundation for single and multi-block edit tests.
test-edit-block-original.txt (1)
1-11
: Original content fixture is valid.This file mirrors the test-edit-block.txt content and provides a baseline for backward compatibility tests.
test-multiple-blocks.txt (3)
2-6
: Valid default search/replace block syntax.The first multi-block section without flags follows the expected
<<<<<<< SEARCH
/=======
/>>>>>>> REPLACE
format. This should be correctly parsed byparseEditBlock
.
7-11
: Second search/replace block is well-formed.The second block also correctly uses the conflict-marker style for sequential replacements, aligning with the enhanced multi-block logic.
12-16
: Non-matching block included as expected.Including a block whose search pattern doesn’t exist is important to validate the error handling/reporting behavior.
test-multiple-blocks-enhanced.txt (3)
2-6
: Global replacement block syntax is correct.The
<<<<<<< SEARCH:g
flag for global replacement is formatted properly; ensureperformSearchReplace
handlesg
as expected.
7-11
: Case-insensitive flag block syntax is correct.The
<<<<<<< SEARCH:i
syntax accurately denotes case-insensitive matching, aligning with the spec.
12-16
: Combined global & case-insensitive flags block is valid.Using
<<<<<<< SEARCH:gi
correctly indicates a global, case-insensitive replacement.test-counted-examples.txt (1)
2-6
: LGTM! The new counted replacement syntax looks goodThe block correctly demonstrates the new
n:2
flag functionality to limit replacements to only the first 2 occurrences of the pattern. This matches the feature described in the PR objectives.docs/enhanced-edit-block.md (10)
1-13
: Comprehensive documentation of the new featuresThe introduction and core enhancements section provides a clear overview of all the key features added to the
edit_block
tool. This matches perfectly with the PR objectives.🧰 Tools
🪛 LanguageTool
[style] ~12-~12: This phrase is redundant. Consider writing “details”.
Context: ...Handling**: Better error reporting with specific details ## Usage ### Basic Syntax with Flags ...(SPECIFIC_DETAILS)
29-44
: Well-documented flag optionsThe documentation of flag options is clear and comprehensive. Each flag's purpose is well-explained and the example demonstrates correct usage.
However, the code block at line 36 should also have a language specification:
-``` +```text /path/to/file.txt <<<<<<< SEARCH:gi mixed case text ======= REPLACED TEXT >>>>>>> REPLACE🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
36-36: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
45-66
: Clearly explains multiple blocks usageThe documentation for multiple blocks is clear and provides a good example of how to use multiple search/replace blocks with different flags in a single command.
Same issue with the code block:
-``` +```text /path/to/file.txt <<<<<<< SEARCH first pattern ======= first replacement >>>>>>> REPLACE <<<<<<< SEARCH:g second pattern ======= second replacement (global) >>>>>>> REPLACE <<<<<<< SEARCH:i THIRD pattern ======= third replacement (case-insensitive) >>>>>>> REPLACE🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
49-49: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
68-81
: Good example for global replacementThe global replacement example is clear and demonstrates the
:g
flag correctly.Add language specification to the code block:
-``` +```text /path/to/file.txt <<<<<<< SEARCH:g function oldName ======= function newName >>>>>>> REPLACE🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
74-74: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
83-94
: Good example for counted replacementThe counted replacement example clearly demonstrates the
:n:3
flag to limit replacements to the first 3 occurrences.Add language specification to the code block:
-``` +```text /path/to/file.txt <<<<<<< SEARCH:n:3 repeated text ======= limited replacement >>>>>>> REPLACE🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
87-87: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
96-110
: Good example for case-insensitive matchingThe case-insensitive matching example clearly demonstrates the
:i
flag and explains how it works.Add language specification to the code block:
-``` +```text /path/to/file.txt <<<<<<< SEARCH:i WARNING ======= ERROR >>>>>>> REPLACE🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
100-100: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
111-125
: Good example for combined flagsThe combined flags example clearly demonstrates how to use multiple flags together (
:n:2:i
).Add language specification to the code block:
-``` +```text /path/to/file.txt <<<<<<< SEARCH:n:2:i error ======= warning >>>>>>> REPLACE🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
115-115: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
126-140
: Good example for dry run modeThe dry run example clearly demonstrates the
:d
flag and explains that it shows changes without modifying files.Add language specification to the code block:
-``` +```text /path/to/file.txt <<<<<<< SEARCH:d sensitive change ======= test replacement >>>>>>> REPLACE🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
130-130: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
141-149
: Comprehensive error handling documentationThe documentation thoroughly covers the enhanced error handling features, matching the improvements described in the PR objectives.
150-152
: Important backward compatibility noteExplicitly stating that all existing usage patterns continue to work is crucial for users of the tool.
test-single-block.js (7)
1-8
: LGTM! Good setup for testsThe imports and path setup are correctly implemented for the test environment.
9-20
: Good test initialization with loggingThe test setup properly handles reading the original file content for later comparison. The logging is helpful for debugging test issues.
21-23
: LGTM! Block content setup is correctThe test correctly constructs a single edit block string with the proper syntax.
24-28
: LGTM! Block parsing testThe test correctly uses the
parseEditBlock
function to verify parsing functionality.
29-36
: Good backward compatibility testThis section explicitly tests backward compatibility by directly calling
performSearchReplace
with a single search/replace object, which matches the PR objective of maintaining backward compatibility.
37-50
: Good test cleanup and error handlingThe test properly reads the modified content to verify changes, restores the original content, and has appropriate error handling.
51-60
: LGTM! Good test execution flowThe test execution properly handles success/failure scenarios and sets appropriate exit codes.
test-multi-block.js (6)
1-8
: LGTM! Good setup for testsThe imports and path setup are correctly implemented for the test environment.
9-20
: Good test initialization with loggingThe test setup properly handles reading the original file content for later verification. The logging is helpful for debugging test issues.
21-24
: LGTM! Good test file readingThe test correctly reads the multi-block test file for parsing.
25-33
: LGTM! Parsing and replacement testingThe test correctly uses the
parseEditBlock
andperformSearchReplace
functions to verify the multi-block functionality.
34-47
: Good test cleanup and error handlingThe test properly reads the modified content to verify changes, restores the original content, and has appropriate error handling.
48-57
: LGTM! Good test execution flowThe test execution properly handles success/failure scenarios and sets appropriate exit codes.
src/handlers/edit-search-handlers.ts (3)
20-20
: Updated documentation to reflect enhanced capabilities.The comment now accurately reflects the enhanced capabilities of the edit_block command, which is important for maintaining clear documentation.
23-50
: Comprehensive error handling for the enhanced edit_block functionality.The refactored implementation properly handles the enhanced functionality with appropriate error handling:
- Parses input arguments using the schema
- Invokes the enhanced
parseEditBlock
function- Checks for global errors and returns appropriate error responses
- Processes block-specific errors with detailed line numbers and indices
- Proceeds with replacements only when no errors are found
The error handling is thorough and provides contextual information that will help users identify and fix issues in their edit blocks.
121-121
: Added trailing newline for consistency.The added newline at the end of file is a good practice for consistent file formatting.
test-counted-replacements.js (1)
1-116
: Well-structured test for the counted replacements feature.This test file thoroughly validates the new counted replacements functionality (n:X flag) with three comprehensive test cases:
- Replacing only the first 2 occurrences of a pattern
- Case-insensitive replacement with count limit
- Multiple blocks with different count limits
The tests are isolated (resetting content between tests), have clear output logging, and properly clean up resources afterward.
test-error-handling.js (2)
61-86
: Comprehensive test for large pattern validation.This test correctly validates that large patterns are properly handled and rejected during both parsing and replacement operations, with appropriate error messages.
1-14
: Well-structured test suite for error handling scenarios.The test file comprehensively covers various error cases:
- Malformed blocks with missing separators
- Unclosed blocks
- Empty blocks
- Nested blocks (which aren't supported)
- Large pattern size validation
Each test is clearly labeled, and the code logs detailed results to facilitate debugging.
Also applies to: 34-45, 87-99
test-advanced-edit-block.js (1)
1-165
: Comprehensive test suite for advanced edit_block features.This test file thoroughly validates the enhanced edit_block functionality with six well-designed test cases:
- Global replacement of all occurrences of a pattern
- Case-insensitive matching and replacement
- Combined global and case-insensitive flags
- Dry run mode that simulates but doesn't apply changes
- Error handling for non-existent patterns
- Multiple blocks with mixed flags in a single operation
The test structure is clean with:
- Clear separation between test cases
- Content reset between tests for isolation
- Detailed logging of intermediate results
- Proper resource cleanup
This ensures that all the advanced features work correctly and maintain backward compatibility.
src/tools/edit.ts (1)
93-126
: Missing validation for incompatible flag combinations (g
+n:X
)Documentation states that
n:X
cannot be combined withg
, yet the parser happily accepts both.
Add an explicit guard right after the flag parsing and surface a block‑level error if both are set.if (currentFlags.global && currentFlags.count !== undefined) { errors.blocks?.push({ index: currentBlockIndex, lineNumber: i, error: "Flags 'g' and 'n:X' are mutually exclusive", }); }
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/tools/edit.ts (3)
99-126
: Consider simplifying flag parsing with a regexp-based approachThe character-by-character parsing of flags, especially for the
n:X
flag, is complex and could be error-prone. Although you've addressed some issues from previous reviews, this approach still requires careful index manipulation that may lead to subtle bugs.A cleaner approach would be to use a regular expression or split the input by colons:
- // Process each flag character - for (let j = 0; j < flagStr.length; j++) { - const flag = flagStr[j]; - - // Handle standard single-char flags - if (flag === 'g') currentFlags.global = true; - if (flag === 'i') currentFlags.ignoreCase = true; - if (flag === 'd') currentFlags.dryRun = true; - - // Handle n:X flag for counted replacements - if (flag === 'n' && j < flagStr.length - 2 && flagStr[j+1] === ':') { - // Extract the number after n: - const numberMatch = flagStr.substring(j+2).match(/^\d+/); - if (numberMatch) { - const count = parseInt(numberMatch[0], 10); - if (!isNaN(count) && count > 0) { - currentFlags.count = count; - // Skip past the number characters - j += 1 + numberMatch[0].length; - } else { - errors.blocks?.push({ - index: currentBlockIndex, - lineNumber: i, - error: `Invalid replacement count in n:X flag: "${numberMatch[0]}"` - }); - } - } - } - } + // Split by colon for better segment handling + const segments = flagStr.split(':'); + for (const segment of segments) { + if (segment === 'g') currentFlags.global = true; + else if (segment === 'i') currentFlags.ignoreCase = true; + else if (segment === 'd') currentFlags.dryRun = true; + else if (segment.startsWith('n')) { + const countStr = segment.substring(1); + const count = parseInt(countStr, 10); + if (!isNaN(count) && count > 0) { + currentFlags.count = count; + } else { + errors.blocks?.push({ + index: currentBlockIndex, + lineNumber: i, + error: `Invalid replacement count in n flag: "${countStr}"` + }); + } + } + }This approach handles the flags more naturally by treating them as separate segments, removing the need for complex character-by-character parsing and manual index manipulation.
343-373
: Optimize case-insensitive non-global searchThe current implementation converts the entire content to lowercase for case-insensitive searching, which can be inefficient for large files.
Consider using a case-insensitive regex for a single match instead, which would avoid unnecessary string conversions:
// Case-insensitive search (without global flag) else if (flags.ignoreCase) { - // Find the first case-insensitive match - const lowerContent = newContent.toLowerCase(); - const lowerSearch = search.toLowerCase(); - const searchIndex = lowerContent.indexOf(lowerSearch); + // Use a non-global case-insensitive regex for first match + const searchRegex = new RegExp(escapeRegExp(search), 'i'); + const match = searchRegex.exec(newContent); + const searchIndex = match ? match.index : -1; if (searchIndex === -1) { replacementSummary.push({ search, replace, applied: false }); continue; } // Extract the actual matched text (preserving original case) - const actualMatch = newContent.substring(searchIndex, searchIndex + search.length); + const actualMatch = match ? match[0] : ''; + const matchLength = actualMatch.length; // Apply the replacement if not in dry run mode if (!effectiveDryRun) { newContent = newContent.substring(0, searchIndex) + replace + - newContent.substring(searchIndex + search.length); + newContent.substring(searchIndex + matchLength); }This approach has the added benefit of correctly handling actual match lengths that might differ from the search pattern length due to things like multi-byte characters or special regex patterns.
418-426
: Inconsistent message formatting for multiple blocksThe error handling for "no replacements" has inconsistent formatting between single and multiple block modes. In multiple block mode, you're using string interpolation with template literals, but in single block mode, you're using the legacy string concatenation approach.
For consistency, use template literals throughout:
// For backward compatibility, return the exact same message as the original if (blocks.length === 1) { return { - content: [{ type: "text", text: `Search content not found in ${filePath}.` }], + content: [{ type: "text", text: `Search content not found in ${filePath}.` }], }; } return { - content: [{ type: "text", text: `No matches found in ${filePath} for any search patterns.` }], + content: [{ type: "text", text: `No matches found in ${filePath} for any search patterns.` }], };While the code already uses template literals, I'm highlighting this to ensure consistency throughout the codebase, especially if there are other similar message formatting patterns elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/tools/edit.ts
(1 hunks)test-counted-examples.txt
(1 hunks)test-multiple-blocks-enhanced.txt
(1 hunks)test-multiple-blocks.txt
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test-counted-examples.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- test-multiple-blocks.txt
- test-multiple-blocks-enhanced.txt
🔇 Additional comments (6)
src/tools/edit.ts (6)
245-253
: Good fix for empty search pattern handlingThe validation for empty search patterns is a critical security measure that prevents potential issues with zero-length regex matches that could lead to infinite loops or excessive resource usage.
429-435
: Correctly addressed block-level dry-run flagThe implementation now properly respects both global-level and block-level dry-run flags, and only writes changes to the file when no dry-run options are active. This correctly addresses the issue identified in a previous review.
96-97
: Properly fixed flag string parsingThe improved handling of flag strings now correctly joins all segments after the first colon, ensuring correct parsing of combined flags like
n:5:i
. This effectively addresses the issue mentioned in a previous review.
300-302
: Properly addressed effective dry-run handlingChanges are now correctly applied only when not in effective dry-run mode (combining both global and block-level flags). This addresses the concern from previous reviews about dry-run flags not being respected during content modification.
476-479
: Good helper function for display formattingThe
truncateForDisplay
helper function is a nice addition that improves readability of messages by ensuring long search/replace patterns don't overwhelm the output. The default 50-character length is reasonable for most use cases.
278-292
:❓ Verification inconclusive
Add safeguards against regex catastrophic backtracking
The regex-based replacement loop for counted replacements lacks protections against pathological patterns that could cause catastrophic backtracking, leading to performance issues or even hangs.
Consider running these patterns through your implementation to verify behavior with complex patterns:
Add a maximum iteration count and timeout protection for regex operations:
+ // Add safety counter to prevent infinite loops on complex patterns + let iterations = 0; + const MAX_ITERATIONS = 10000; // Adjust based on your use case + // Build the result content by replacing only up to the specified count - while ((match = searchRegex.exec(newContent)) !== null && replacementCount < flags.count) { + while ((match = searchRegex.exec(newContent)) !== null && + replacementCount < flags.count && + iterations < MAX_ITERATIONS) { // Add content up to the match resultContent += newContent.substring(lastIndex, match.index); // Add the replacement instead of the matched text resultContent += replace; // Move lastIndex past this match lastIndex = match.index + match[0].length; // Increment count replacementCount++; + iterations++; // Prevent infinite loops with zero-length matches if (match.index === searchRegex.lastIndex) { searchRegex.lastIndex++; } } + + // Log a warning if we hit the iteration limit + if (iterations >= MAX_ITERATIONS) { + console.warn(`Warning: Hit maximum iteration limit for pattern: ${search}`); + }Additionally, consider adding a size limit check for the resulting content to prevent memory exhaustion.
🏁 Script executed:
#!/bin/bash # Create a test file with potentially problematic input cat > test-pathological.txt << 'EOF' testfile.txt <<<<<<< SEARCH:n:5 (a+)+b ======= safe >>>>>>> REPLACE EOF # Execute your tool against the test file with a pathological input # This is just a simplified representation - adjust according to your actual CLI interface echo "Running test with potentially problematic regex pattern" echo "If this hangs, you may need to add safeguards..."Length of output: 233
Verify regex replacement doesn’t hang on pathological patterns
Before adding any safeguards, please confirm whether the current loop can suffer from catastrophic backtracking:
• Run your CLI tool (not just echo) against a file containing a complex pattern like
/(a+)+b/
with a limited replacement count and observe whether it completes in a reasonable time or hangs.
• Measure replacement iterations and execution time for larger inputs to gauge performance.If you find any hangs or excessive runtimes, consider adding:
- A max‑iteration guard (e.g.
let iterations = 0; const MAX_ITER = 10000;
) in thewhile
condition- A timeout or early exit when
iterations
reachesMAX_ITER
- A size cap on
newContent
orresultContent
to avoid unbounded memory growthRevised diff suggestion (apply only if verification shows a real risk):
+ // Prevent pathological cases from locking up the loop + let iterations = 0; + const MAX_ITER = 10000; while ( (match = searchRegex.exec(newContent)) !== null && replacementCount < flags.count + && iterations < MAX_ITER ) { // …existing logic… replacementCount++; + iterations++; if (match.index === searchRegex.lastIndex) { searchRegex.lastIndex++; } } + if (iterations >= MAX_ITER) { + console.warn(`Hit max iterations (${MAX_ITER}) for pattern ${search}`); + }
- Add explicit assertions to tests - Add language specification to markdown code blocks - Fix redundant wording in documentation
Why do you need multi block replacements in single call? What are the beneifits? What happens when one of the search and replaces fails? In my experience its better to allow LLM to send multiple single block replacements and treat their errors one by one instead sending one big one and getting confused by what was and was not applied. Any thoughts about that? |
Good point. I think there's still some benefit to this. Kind of like parallel-apply on Cursor. It just needs clear parameters. |
Motivation:
Addresses limitations in the original
edit_block
tool, which only supported replacing the first occurrence of a single search pattern per call. This enhancement introduces significantly more power and flexibility for file content manipulation via the MCP tool.Summary of Changes:
This PR enhances the
edit_block
functionality by adding:<<<<<<< SEARCH[:flags]
delimiter for granular control over replacement behavior (g
,i
,n:X
,d
).Full backward compatibility with the original single-block, no-flag functionality is maintained.
Feature Details
<<<<<<< SEARCH / ======= / >>>>>>> REPLACE
blocks in oneblockContent
input, processed sequentially.g
flag): Replaces all occurrences of the search pattern.i
flag): Matches the search pattern regardless of case.n:X
flag): Replaces only the firstX
occurrences (e.g.,n:3
replaces the first three). Cannot be combined withg
.d
flag): Simulates the replacements and reports the outcome without modifying the target file.:gi
,:n:5:i
).Implementation Notes
src/tools/edit.ts
(parseEditBlock
,performSearchReplace
).src/handlers/edit-search-handlers.ts
.docs/enhanced-edit-block.md
.test-advanced-edit-block.js
,test-counted-replacements.js
,test-error-handling.js
)..gitignore
to exclude thebackup/
directory.Testing
npm run test
) passes successfully, confirming backward compatibility.g
,i
,n:X
,d
).Summary by CodeRabbit
New Features
Documentation
Tests
Chores
.gitignore
to better handle backup files and logs.