Skip to content

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

davidleathers113
Copy link

@davidleathers113 davidleathers113 commented Apr 19, 2025

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:

  • Support for processing multiple search/replace blocks in a single operation.
  • Optional flags within the <<<<<<< SEARCH[:flags] delimiter for granular control over replacement behavior (g, i, n:X, d).
  • Improved error handling and reporting for both block parsing and replacement execution.
  • Comprehensive testing for the new features.
  • Updated documentation detailing the new capabilities.

Full backward compatibility with the original single-block, no-flag functionality is maintained.

Feature Details

  • Multiple Block Support: Allows multiple <<<<<<< SEARCH / ======= / >>>>>>> REPLACE blocks in one blockContent input, processed sequentially.
  • Global Replacement (g flag): Replaces all occurrences of the search pattern.
  • Case-Insensitive Matching (i flag): Matches the search pattern regardless of case.
  • Counted Replacement (n:X flag): Replaces only the first X occurrences (e.g., n:3 replaces the first three). Cannot be combined with g.
  • Dry Run (d flag): Simulates the replacements and reports the outcome without modifying the target file.
  • Flag Combinations: Supports combining flags (e.g., :gi, :n:5:i).
  • Enhanced Error Handling: Provides more specific error messages for invalid block syntax, flag combinations, and pattern size limits.
  • Detailed Result Reporting: Output messages now detail the outcome of each block, including counts for global/counted replacements and indicating patterns not found.

Implementation Notes

  • Core logic updated in src/tools/edit.ts (parseEditBlock, performSearchReplace).
  • Handler updated in src/handlers/edit-search-handlers.ts.
  • New documentation added: docs/enhanced-edit-block.md.
  • New dedicated test files added (e.g., test-advanced-edit-block.js, test-counted-replacements.js, test-error-handling.js).
  • Updated .gitignore to exclude the backup/ directory.

Testing

  • The existing project test suite (npm run test) passes successfully, confirming backward compatibility.
  • New, dedicated test scripts were created and executed successfully to validate:
    • Multi-block processing.
    • Each individual flag (g, i, n:X, d).
    • Combinations of flags.
    • Enhanced error handling scenarios.
  • Manual testing using custom test files further verified expected behavior.

Summary by CodeRabbit

  • New Features

    • Enhanced edit block functionality now supports multiple search/replace blocks, global and counted replacements, case-insensitive matching, and dry run mode.
    • Improved error handling with detailed messages for malformed blocks, pattern size warnings, and block-specific errors.
    • Backward compatibility with previous single-block usage is maintained.
  • Documentation

    • Added comprehensive documentation outlining the new enhanced edit block features, syntax, and usage examples.
  • Tests

    • Introduced new test scripts and sample files to validate advanced, counted, multi-block, error-handling, and backward compatibility scenarios for the enhanced edit block functionality.
  • Chores

    • Updated .gitignore to better handle backup files and logs.

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.
Copy link

coderabbitai bot commented Apr 19, 2025

Walkthrough

This update introduces a comprehensive enhancement to the edit_block tool, enabling support for multiple search/replace blocks with advanced features such as global replacement, case-insensitive matching, counted replacements, and dry run mode. The implementation involves a complete rewrite of the core logic, improved error handling, and the addition of detailed documentation and extensive test coverage. The .gitignore file is updated to ignore new backup-related files. Several new test scripts and test data files are added to validate the new and backward-compatible functionalities, ensuring robustness and correctness.

Changes

File(s) Change Summary
.gitignore Updated to ignore server.log-e, backup/ directory, and .bak files; added relevant comment.
docs/enhanced-edit-block.md New documentation detailing enhanced edit_block features, syntax, usage examples, and error handling.
src/handlers/edit-search-handlers.ts Refactored handleEditBlock for improved error handling and support for multi-block parsing; minor newline in handleSearchCode.
src/tools/edit.ts Completely rewritten performSearchReplace to support multiple blocks, flags (global, ignoreCase, dryRun, count), detailed error handling, and backward compatibility. Added parseEditBlock and helper interfaces/functions.
test-advanced-edit-block.js New test script validating advanced multi-block and flag features, error handling, and dry run mode.
test-counted-examples.txt, test-edit-block-original.txt, test-edit-block.txt New test data files for counted, single, and multi-block search/replace scenarios.
test-counted-replacements.js New test script for counted replacements, case-insensitive, and multi-block tests with verification.
test-error-handling.js New test script validating error handling for malformed blocks, large patterns, and edge cases.
test-multi-block.js, test-multiple-blocks-enhanced.txt, test-multiple-blocks.txt New test script and data files for multi-block and enhanced flag scenarios.
test-single-block.js New test script verifying backward compatibility with single-block edit operations.
fix_markdown.sh New shell script to modify markdown code block delimiters in docs/enhanced-edit-block.md to specify language as text.

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
Loading

Poem

🐇
In the garden of code, new features bloom,
Multi-block edits now banish old gloom.
With flags for each case, and a dry run to try,
The rabbits have tested, so bugs say goodbye!
Patterns replaced, with precision and care—
Our editing magic is now quite rare.
🥕✨

Tip

⚡💬 Agentic Chat (Pro Plan, General Availability)
  • We're introducing multi-step agentic chat in review comments and issue comments, within and outside of PR's. This feature enhances review and issue discussions with the CodeRabbit agentic chat by enabling advanced interactions, including the ability to create pull requests directly from comments and add commits to existing pull requests.

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a4d5674 and 92932fd.

📒 Files selected for processing (4)
  • docs/enhanced-edit-block.md (1 hunks)
  • fix_markdown.sh (1 hunks)
  • test-multi-block.js (1 hunks)
  • test-single-block.js (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • fix_markdown.sh
🚧 Files skipped from review as they are similar to previous changes (3)
  • docs/enhanced-edit-block.md
  • test-single-block.js
  • test-multi-block.js

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a 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 wording

The phrase "specific details" is slightly redundant.

-6. **Enhanced Error Handling**: Better error reporting with specific details
+6. **Enhanced Error Handling**: Better error reporting with details

Or 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 block

Adding 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 as Buffer (or convert via Buffer.byteLength(content, 'utf8')) when enforcing the byte cap.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5935c3e and 3bdaf2b.

📒 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, the backup/ 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 by parseEditBlock.


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; ensure performSearchReplace handles g 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 good

The 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 features

The 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 options

The 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 usage

The 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 replacement

The 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 replacement

The 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 matching

The 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 flags

The 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 mode

The 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 documentation

The documentation thoroughly covers the enhanced error handling features, matching the improvements described in the PR objectives.


150-152: Important backward compatibility note

Explicitly 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 tests

The imports and path setup are correctly implemented for the test environment.


9-20: Good test initialization with logging

The 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 correct

The test correctly constructs a single edit block string with the proper syntax.


24-28: LGTM! Block parsing test

The test correctly uses the parseEditBlock function to verify parsing functionality.


29-36: Good backward compatibility test

This 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 handling

The test properly reads the modified content to verify changes, restores the original content, and has appropriate error handling.


51-60: LGTM! Good test execution flow

The test execution properly handles success/failure scenarios and sets appropriate exit codes.

test-multi-block.js (6)

1-8: LGTM! Good setup for tests

The imports and path setup are correctly implemented for the test environment.


9-20: Good test initialization with logging

The 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 reading

The test correctly reads the multi-block test file for parsing.


25-33: LGTM! Parsing and replacement testing

The test correctly uses the parseEditBlock and performSearchReplace functions to verify the multi-block functionality.


34-47: Good test cleanup and error handling

The test properly reads the modified content to verify changes, restores the original content, and has appropriate error handling.


48-57: LGTM! Good test execution flow

The 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:

  1. Parses input arguments using the schema
  2. Invokes the enhanced parseEditBlock function
  3. Checks for global errors and returns appropriate error responses
  4. Processes block-specific errors with detailed line numbers and indices
  5. 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:

  1. Replacing only the first 2 occurrences of a pattern
  2. Case-insensitive replacement with count limit
  3. 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:

  1. Malformed blocks with missing separators
  2. Unclosed blocks
  3. Empty blocks
  4. Nested blocks (which aren't supported)
  5. 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:

  1. Global replacement of all occurrences of a pattern
  2. Case-insensitive matching and replacement
  3. Combined global and case-insensitive flags
  4. Dry run mode that simulates but doesn't apply changes
  5. Error handling for non-existent patterns
  6. 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 with g, 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",
  });
}

Copy link

@coderabbitai coderabbitai bot left a 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 approach

The 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 search

The 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 blocks

The 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

📥 Commits

Reviewing files that changed from the base of the PR and between 3bdaf2b and a4d5674.

📒 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 handling

The 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 flag

The 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 parsing

The 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 handling

Changes 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 formatting

The 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 the while condition
  • A timeout or early exit when iterations reaches MAX_ITER
  • A size cap on newContent or resultContent to avoid unbounded memory growth

Revised 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
@wonderwhy-er
Copy link
Owner

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?

@davidleathers113
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants