fix: improve error messages for diff marker escaping in apply_diff tool #7751
Conversation
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
- Enhanced error messages to clearly explain when and how to escape special markers - Differentiated between structural errors and content escaping issues - Added helpful examples showing correct escaping syntax - Added comprehensive tests for marker escaping scenarios - Fixes issue where Grok Coder models get confused by unescaped markers in content Resolves #7750
dosubot
bot
added
size:XL
| expect(escapeResult.success).toBe(false) | ||
| expect(escapeResult.error).toContain("appears to be part of the content") | ||
| expect(escapeResult.error).toContain("MUST escape it") | ||
| }) |
There was a problem hiding this comment.
Could we add test coverage for these edge cases?
- Escaped
:start_line:and:end_line:markers (the code handles them inunescapeMarkersbut no tests) - Multiple consecutive escaped markers (e.g.,
\=======\=======) - Mixed escaped and unescaped markers in the same diff
These scenarios might trip up AI models in practice.
|
|
||
| it("should provide helpful error message for unescaped markers", () => { | ||
| // This is actually a valid diff structure, not an error case | ||
| const diff = "<<<<<<< SEARCH\n" + "code with\n" + "=======\n" + "replacement\n" + ">>>>>>> REPLACE" |
There was a problem hiding this comment.
The comment says "This is actually a valid diff structure, not an error case" but then correctly tests for success. Could we rename this test to something like "should accept valid diff structure without escaped markers" to better reflect what it's validating?
| ":start_line: (required) The line number of original content where the search block starts.\n" + | ||
| "-------\n" + | ||
| "[exact content to find including whitespace]\n" + | ||
| ":start_line:NUMBER (optional but recommended)\n" + |
There was a problem hiding this comment.
Is the :start_line: parameter actually required or optional? The tool description at line 109 says "(required)" but the error message here says "(optional but recommended)". This inconsistency might confuse users (and AI models) about whether they must provide this parameter.
| ":start_line: (required) The line number of original content where the search block starts.\n" + | ||
| "-------\n" + | ||
| "[exact content to find including whitespace]\n" + | ||
| ":start_line:NUMBER (optional but recommended)\n" + |
There was a problem hiding this comment.
Same inconsistency here - the parameter is described as "(optional but recommended)" but the tool description marks it as required. Should we standardize this across both files?
hannesrudolph
added
the
Issue/PR - Triage
|
Closing, see #7750 (comment) |
Summary
This PR addresses Issue #7750 where Grok Coder models were experiencing parsing errors with the apply_diff tool when diff markers (like
=======) appeared in the actual file content.Problem
When AI models (particularly Grok Coder) try to edit files containing merge conflict markers or similar patterns, the diff parser would get confused and fail with unclear error messages like:
Solution
This PR improves the error handling and messaging to:
Changes
multi-search-replace.tsandmulti-file-search-replace.tsescaping-markers.spec.tsTesting
Impact
This fix will help AI models (especially Grok Coder) successfully edit files that contain diff-like markers, reducing frustration and failed edit attempts.
Fixes #7750
Important
Improves error messages for diff marker escaping in
apply_difftool, adds examples, and enhances test coverage.multi-search-replace.tsandmulti-file-search-replace.tsfor diff marker escaping.escaping-markers.spec.tsfor marker escaping scenarios.multi-search-replace.spec.tsto match new error message format.validateMarkerSequencing()to handle new error cases and provide clearer guidance.This description was created by
for f2a6e42. You can customize this summary. It will automatically update as commits are pushed.