Skip to content

fix(diff): repair truncated Grok diffs with missing markers (#186)#230

Open
proyectoauraorg wants to merge 4 commits into
Zoo-Code-Org:mainfrom
proyectoauraorg:fix/diff-parser-grok-truncation
Open

fix(diff): repair truncated Grok diffs with missing markers (#186)#230
proyectoauraorg wants to merge 4 commits into
Zoo-Code-Org:mainfrom
proyectoauraorg:fix/diff-parser-grok-truncation

Conversation

@proyectoauraorg
Copy link
Copy Markdown
Contributor

@proyectoauraorg proyectoauraorg commented May 21, 2026

Summary

Fixes #186. Grok (4.3) frequently truncates streamed diffs mid-output, so a <<<<<<< SEARCH block can arrive without its ======= separator and/or >>>>>>> REPLACE closer. The parser then rejects the whole edit with "Expected '=======' was not found" and the user sees "Unable to apply diff".

This adds a small recovery layer, repairTruncatedDiff(), called at the start of applyDiff() that reconstructs the missing markers so a salvageable diff can still be applied.

How it works

For each <<<<<<< SEARCH block the repair step:

  • leaves complete blocks (both ======= and >>>>>>> REPLACE) untouched, verbatim;
  • if only >>>>>>> REPLACE is missing, appends it;
  • if both markers are missing, treats the first line after SEARCH as the search content and the rest as the replacement, then inserts ======= and >>>>>>> REPLACE;
  • preserves escaped markers via (?<!\\) look-behinds (won't touch \<<<<<<< content);
  • when a repaired block is followed by more blocks, re-adds an inter-block separator so the appended >>>>>>> REPLACE never gets glued to the next <<<<<<< SEARCH.

Complete, valid diffs pass through unchanged, so behavior for well-formed input is identical.

Testing

Added coverage in multi-search-replace.spec.ts (complete-diff passthrough, missing closer, missing both markers, empty-search edge case, multi-block truncation, no spurious trailing newline).

vitest run core/diff/strategies/__tests__/multi-search-replace.spec.ts
# Test Files 1 passed (1) · Tests 65 passed (65)

Files

  • src/core/diff/strategies/multi-search-replace.tsrepairTruncatedDiff() + call in applyDiff()
  • src/core/diff/strategies/__tests__/multi-search-replace.spec.ts — tests

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of truncated merge-style diffs: missing separators and end markers are detected and synthesized so truncated outputs are repaired without changing well-formed diffs. Preserves directive/header lines, handles empty search blocks, maintains trailing-newline behavior, and limits repair to the first truncated block when later blocks are complete.
  • Tests

    • Added targeted and regression tests covering multiple truncation scenarios and edge cases to prevent regressions.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 21, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 542c4376-f9aa-4187-aa80-5e5814badd3d

📥 Commits

Reviewing files that changed from the base of the PR and between 5c105ac and 2e9ea04.

📒 Files selected for processing (1)
  • src/core/diff/strategies/multi-search-replace.ts
💤 Files with no reviewable changes (1)
  • src/core/diff/strategies/multi-search-replace.ts

📝 Walkthrough

Walkthrough

This PR adds automatic repair of truncated merge-conflict diffs to handle AI model output that cuts off mid-stream. A new private repairTruncatedDiff() method detects incomplete diff blocks and reconstructs them by appending missing markers. The repair step is integrated into applyDiff() before validation, with comprehensive unit tests and regression tests for issue #186.

Changes

Truncated Diff Repair

Layer / File(s) Summary
Repair mechanism implementation
src/core/diff/strategies/multi-search-replace.ts
repairTruncatedDiff() private method splits diff content by <<<<<<< SEARCH markers, detects missing ======= or >>>>>>> REPLACE closers, and reconstructs incomplete blocks by extracting SEARCH/REPLACE content and appending the missing markers.
Integration into diff application
src/core/diff/strategies/multi-search-replace.ts
applyDiff() preprocesses diffContent by calling repairTruncatedDiff() before validateMarkerSequencing(), ensuring truncated diffs are repaired before validation.
Unit tests for repair scenarios
src/core/diff/strategies/__tests__/multi-search-replace.spec.ts
Test suite validates the repair mechanism handles complete diffs unchanged, repairs missing markers, handles partial truncation with complete blocks following, manages empty search content, and preserves existing trailing newlines.
Regression tests for Grok truncation (#186)
src/core/diff/strategies/__tests__/multi-search-replace.spec.ts
End-to-end tests via applyDiff() confirm that diffs truncated at the closing >>>>>>> REPLACE marker and before the ======= separator are successfully applied, and well-formed multi-block diffs remain unchanged.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

A rabbit saw the diff torn short,
It stitched the markers back in port,
Sewed "=======" and ">>>> REPLACE" with care,
Restored the blocks that vanished in thin air,
Hopped off, ears high, with a crumb of code to share.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding repair functionality for truncated Grok diffs with missing markers, directly matching the PR's primary objective.
Description check ✅ Passed The description includes all required sections: Related GitHub Issue (Fixes #186), detailed implementation explanation, comprehensive test coverage details, and pre-submission checklist items are addressed through the test results shown.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 21, 2026

Codecov Report

❌ Patch coverage is 96.82540% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/core/diff/strategies/multi-search-replace.ts 96.82% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Contributor

@edelauna edelauna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a fixture based e2e test which reproduces the issue / acts as a regression guard.

proyectoauraorg and others added 2 commits May 21, 2026 09:34
…Zoo-Code-Org#186)

Grok frequently truncates streamed diffs, leaving SEARCH blocks without the
======= separator and/or the >>>>>>> REPLACE closer, which makes applyDiff
fail with 'Expected ======= was not found'. repairTruncatedDiff() detects
incomplete blocks and reinserts the missing markers while preserving valid
blocks and escaped markers.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…fs (Zoo-Code-Org#186)

Per review feedback: end-to-end applyDiff() regression guards using realistic
truncated-Grok fixtures (missing >>>>>>> REPLACE, missing ======= separator),
plus a well-formed multi-block diff that must pass through unchanged.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@proyectoauraorg
Copy link
Copy Markdown
Contributor Author

Thanks @edelauna — added fixture-based regression tests that run through the full applyDiff() path with realistic truncated-Grok diffs: one missing the closing >>>>>>> REPLACE, one truncated before ======= (the exact "Expected '=======' was not found" case), and a well-formed multi-block diff that must pass through untouched. All pass; pushed in the latest commit.

@proyectoauraorg proyectoauraorg force-pushed the fix/diff-parser-grok-truncation branch from af855d3 to 8c86955 Compare May 21, 2026 15:37
Copy link
Copy Markdown
Contributor

@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: 1

🧹 Nitpick comments (1)
src/core/diff/strategies/multi-search-replace.ts (1)

324-332: 💤 Low value

Reassigning the diffContent parameter.

Minor: prefer a local const repaired = this.repairTruncatedDiff(diffContent) and use repaired downstream rather than mutating the parameter — keeps the original input observable for logging/diagnostics later.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/core/diff/strategies/multi-search-replace.ts` around lines 324 - 332, The
applyDiff method currently reassigns the diffContent parameter with
this.repairTruncatedDiff(diffContent); instead, call
this.repairTruncatedDiff(diffContent) into a new local const (e.g., const
repairedDiff = this.repairTruncatedDiff(diffContent)) and use repairedDiff for
all downstream logic inside applyDiff (while leaving the original diffContent
parameter untouched) so the original input remains available for
logging/diagnostics; update all references within applyDiff that currently use
diffContent after the repair point to use the new local variable (refer to
applyDiff and repairTruncatedDiff).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/core/diff/strategies/multi-search-replace.ts`:
- Around line 294-318: The repair branch mis-handles blocks that contain a
closer without a separator and blocks that include Grok header directives;
update the logic in multi-search-replace.ts around the variables
hasSeparator/hasCloser/block/repaired so that if hasCloser && !hasSeparator you
do not synthesize a second ">>>>>>> REPLACE" but instead insert a "======="
immediately before the existing closer (i.e., locate the existing ">>>>>>>
REPLACE" and splice in "=======\n" before it); additionally, before applying the
firstNewlineIdx / searchMatch heuristic (the code that computes content,
firstNewlineIdx, searchContent, replaceContent) strip any leading directive
lines like ":start_line:\d+", ":end_line:\d+" and lines of "-------" so the
first-line-is-SEARCH heuristic sees only real content and not header metadata.

---

Nitpick comments:
In `@src/core/diff/strategies/multi-search-replace.ts`:
- Around line 324-332: The applyDiff method currently reassigns the diffContent
parameter with this.repairTruncatedDiff(diffContent); instead, call
this.repairTruncatedDiff(diffContent) into a new local const (e.g., const
repairedDiff = this.repairTruncatedDiff(diffContent)) and use repairedDiff for
all downstream logic inside applyDiff (while leaving the original diffContent
parameter untouched) so the original input remains available for
logging/diagnostics; update all references within applyDiff that currently use
diffContent after the repair point to use the new local variable (refer to
applyDiff and repairTruncatedDiff).
🪄 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 Plus

Run ID: ed8cc68c-09f7-4fd2-993c-cf40d9920974

📥 Commits

Reviewing files that changed from the base of the PR and between 166bc3f and 8c86955.

📒 Files selected for processing (2)
  • src/core/diff/strategies/__tests__/multi-search-replace.spec.ts
  • src/core/diff/strategies/multi-search-replace.ts

Comment thread src/core/diff/strategies/multi-search-replace.ts
…oo-Code-Org#186)

- Use a local repairedDiff in applyDiff instead of reassigning the diffContent
  parameter, keeping the original input observable.
- When a block has a closer but no ======= separator, splice the separator in
  before the existing >>>>>>> REPLACE rather than synthesizing a second closer.
- Strip leading Grok header directives (:start_line:, :end_line:, -------) before
  the first-line-is-SEARCH heuristic so metadata isn't treated as content; the
  directives are preserved on the SEARCH section.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@proyectoauraorg
Copy link
Copy Markdown
Contributor Author

Thanks @coderabbitai — both addressed in the latest commit:

  1. applyDiff now uses a local const repairedDiff = this.repairTruncatedDiff(diffContent) and reads from it downstream, leaving the original diffContent parameter untouched.
  2. Repair edge cases:
    • A block with >>>>>>> REPLACE but no ======= now splices the separator in before the existing closer (no duplicate closer).
    • Leading Grok directives (:start_line:, :end_line:, -------) are peeled off before the first-line-is-SEARCH heuristic and preserved on the SEARCH section, so metadata is never treated as content.

Added unit tests for both. 71 tests pass.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 21, 2026

Tip

For best results, initiate chat on the files or code changes.

@proyectoauraorg: Thanks for the thorough follow-up! Both fixes look solid:

  1. Parameter immutability — scoping the repaired output to const repairedDiff keeps diffContent pristine and avoids any unintended side-effects downstream. ✅
  2. Missing-separator splice — inserting ======= before the existing >>>>>>> REPLACE closer (rather than appending it after) is exactly the right fix to avoid a duplicate closer. ✅
  3. Grok directive preservation — peeling off :start_line: / :end_line: / ------- before the first-line heuristic and re-attaching them to the SEARCH section ensures metadata is never misinterpreted as diff content. ✅

71 tests passing across all the new unit and regression cases is a great signal. The PR looks good to me! 🐇 🎉

Copy link
Copy Markdown
Contributor

@edelauna edelauna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the feedback regarding the e2e.

const blocks = diffContent.split(/(?=(?<!\\)<<<<<<< SEARCH)/)

let repaired = ""
let needsRepair = false
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needsRepair is assigned on line 290 but never read — safe to remove?

Address review feedback from @edelauna (code review #3284681514):
needsRepair was assigned but never read, making it a dead store.
The variable served no functional purpose in the repair loop,
so it has been removed.
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.

[BUG] Grok 4.3 "Unable to apply diff" - Expected '=======' was not found.

2 participants