Skip to content

Conversation

@roomote
Copy link
Contributor

@roomote roomote bot commented Aug 20, 2025

Fixes #7247

Summary

This PR improves the error messages in the diff editing system to better support the Qwen3 235B Instruct model, which was having trouble with diff editing due to confusing error messages about escaping.

Problem

The Qwen3 model was receiving error messages that told it to escape "=======" and other diff markers, but the messages didn't clearly distinguish between:

  1. Diff structure markers (which should NEVER be escaped)
  2. File content containing these patterns (which SHOULD be escaped when they appear in the actual file content)

This confusion caused the model to try escaping the diff structure markers themselves, leading to failed diff edits.

Solution

  • Clarified error messages to explicitly state that diff structure markers should NEVER be escaped
  • Added "IMPORTANT CLARIFICATION" sections to emphasize the distinction
  • Updated tool descriptions with clear escaping rules
  • Maintained backward compatibility with existing test expectations

Changes Made

  1. Updated reportMergeConflictError in both diff strategy files to clarify when escaping is needed
  2. Updated reportInvalidDiffError to maintain backward compatibility while improving clarity
  3. Added escaping rules section to tool descriptions in both strategies
  4. All existing tests pass without modification

Testing

  • ✅ All existing tests pass (npx vitest run core/diff/strategies/__tests__/multi-search-replace.spec.ts)
  • ✅ Linting passes
  • ✅ Type checking passes

Impact

This change will help AI models (especially Qwen3 and similar models) better understand when to escape special characters in diff operations, reducing confusion and improving success rates for diff editing operations.


Important

Improves diff editing error messages for Qwen3 model by clarifying escaping rules and updating tool descriptions in strategy files.

  • Behavior:
    • Clarified error messages in multi-file-search-replace.ts and multi-search-replace.ts to specify that diff structure markers should never be escaped.
    • Added "IMPORTANT CLARIFICATION" sections to emphasize distinction between diff markers and file content.
    • Updated tool descriptions with clear escaping rules.
  • Compatibility:
    • Maintained backward compatibility with existing test expectations.
  • Testing:
    • All existing tests pass without modification.
    • Linting and type checking pass.
  • Misc:
    • Updated latestAnnouncementId in ClineProvider.ts for stealth model announcement.
    • Minor UI changes in Announcement.tsx to support new features.

This description was created by Ellipsis for 4d36750. You can customize this summary. It will automatically update as commits are pushed.

mrubens and others added 2 commits August 19, 2025 22:45
- Clarified error messages to distinguish between diff structure markers and file content
- Added explicit guidance that diff markers should NEVER be escaped in the structure
- Emphasized that escaping is only needed for markers within file content being modified
- Maintained backward compatibility with existing test expectations

This fixes issue #7247 where Qwen3 235B model was confused by error messages
and tried to escape diff structure markers, causing diff editing to fail.
@roomote roomote bot requested review from cte, jr and mrubens as code owners August 20, 2025 06:40
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working documentation Improvements or additions to documentation labels Aug 20, 2025
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Aug 20, 2025
Copy link
Contributor Author

@roomote roomote bot left a comment

Choose a reason for hiding this comment

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

I reviewed my own code and found it has more issues than a magazine subscription service.

public settingsImportedAt?: number
public readonly latestAnnouncementId = "jul-29-2025-3-25-0" // Update for v3.25.0 announcement
public readonly latestAnnouncementId = "aug-20-2025-stealth-model" // Update for stealth model announcement
public readonly providerSettingsManager: ProviderSettingsManager
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this intentional? This file contains changes for a 'stealth model announcement' feature that seems completely unrelated to the diff editing error message improvements described in the PR. Should this be in a separate PR to keep changes focused?

<Trans
i18nKey="chat:announcement.feature3"
i18nKey="chat:announcement.stealthModel.feature"
components={{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These announcement UI changes appear unrelated to fixing Qwen3 diff editing issues. Consider splitting these changes into a separate PR for better change tracking and review clarity.

"<<<<<<< SEARCH\n" +
"content before\n" +
`\\${found} <-- Note the backslash here in this example\n` +
`\\${found} <-- Escape ONLY when this is part of the file content\n` +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minor inconsistency: This line has 4 spaces after the escape example while the same line in multi-file-search-replace.ts has only 1 space. Consider making the formatting consistent across both files.

When applying the diffs, be extra careful to remember to change any closing brackets or other syntax that may be affected by the diff farther down in the file.
ALWAYS make as many changes in a single 'apply_diff' request as possible using multiple SEARCH/REPLACE blocks
**IMPORTANT ESCAPING RULES:**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new escaping rules section is helpful! Could we add one more example showing what happens when these patterns appear in actual file content vs. diff markers? This would make it even clearer for AI models.

@daniel-lxs
Copy link
Member

Not scoped yet

@daniel-lxs daniel-lxs closed this Aug 20, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Aug 20, 2025
@github-project-automation github-project-automation bot moved this from Triage to Done in Roo Code Roadmap Aug 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working documentation Improvements or additions to documentation Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Diff editing not working with Qwen3 235B Instruct 2507 (self-hosted by vLLM)

5 participants