fix(agent-core): improve Edit tool backslash handling and error diagn…#663
fix(agent-core): improve Edit tool backslash handling and error diagn…#663LifeJiggy wants to merge 10 commits into
Conversation
…ostics When old_string is not found, the Edit tool now attempts a backslash unescape fallback to recover from common LLM JSON encoding mistakes (e.g. double-escaped backslashes). Fixes MoonshotAI#601. Changes: - Added findBackslashAdjustedMatch() that tries common backslash encoding fixes when old_string doesn't match - Added buildNotFoundHint() that shows the closest matching line in the file when old_string is not found - Updated edit.md with guidance on backslash encoding Tests: - Matches files with multiple literal backslashes (Rust string escapes) - Recovers when old_string has double-escaped backslashes from LLM - Provides diagnostic hint when old_string with backslashes not found
🦋 Changeset detectedLatest commit: 0d0452f The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
- Use replaceAll with string args instead of regex for simple patterns - Use includes() instead of indexOf() !== -1 - Prefix unused filePath parameter with _
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16ba388a21
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…back Collapse doubled backslashes (\\\\ → \\) BEFORE translating escape sequences (\\n → newline). The previous order converted the second backslash of \\\\n into a real newline before the doubled backslash could be collapsed, producing an incorrect candidate that never matched. Also fixed the overEscaped variant to use replaceAll with string args instead of regex, and correctly re-escape escape sequences.
When the backslash encoding fallback adjusts old_string, new_string must receive the same transformation. Otherwise the replacement text has mismatched encoding (e.g. old_string gets backslash-n but new_string keeps real newlines), producing incorrect output. Also refactored the fix logic into a shared applyBackslashFix() helper used by both variants, and fixed the overEscaped variant which was a no-op (replaced backslash-n with backslash-n instead of replacing real newlines with backslash-n).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68a203ce46
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…llback The escape variant (real newlines → backslash-n) must be checked before the unescape variant (doubled backslashes → single). Otherwise the unescape variant incorrectly matches real newlines, producing backslash + newline instead of the intended backslash + n.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fb69c44de
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
No single replacement order handles all LLM encoding mistakes. Split into three variants: - collapse-first: \\\\n → \\n (double backslash collapsed before escape seq) - seq-first: \\n → \\n (real newline converted before collapse) - escape: \\n → \\\\n (real newline re-escaped to backslash-n) Each handles a distinct encoding mistake. The first matching variant is used for both old_string and new_string.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31705a2512
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The collapse-first variant still processed escape sequences after collapsing, breaking the double-backslash case. Added a pure collapse-only variant (\\\\ → \\) that handles LLM double-escaping without touching escape sequences. Also cleaned up orphaned duplicate function definitions.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27a38928cd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ect approval display The backslash encoding fallback was applied inside execution() after the approval panel had already rendered with the original args. This meant users approved one diff but the tool applied a different one. Moved the file read and fallback into resolveExecution() so the approval display shows the exact text that will be written. The no-op check was also moved before file I/O to avoid unnecessary reads.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f524bf0075
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…h bypass resolveExecution() runs BEFORE the authorization hook in tool-call.ts. Reading the file there bypasses permission checks for sensitive paths. - Reverted resolveExecution to synchronous (no file I/O) - Moved backslash fallback to execution() which runs AFTER auth - Fallback adjusts both old_string and new_string consistently
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a7ca8750e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…adjustment The backslash encoding fallback was silently adjusting old_string/new_string after the approval display showed the original text, creating a mismatch between what the user approved and what was applied (P2). Also, resolveExecution runs BEFORE the authorization hook, so reading the file there would bypass permission checks (P1). Fix: return a diagnostic error when a backslash encoding mismatch is detected, telling the LLM to resend with correct encoding. No silent adjustment, no auth bypass, no display mismatch.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
1 participant
…ostics
When old_string is not found, the Edit tool now attempts a backslash unescape fallback to recover from common LLM JSON encoding mistakes (e.g. double-escaped backslashes). Fixes #601.
Changes:
Tests:
Related Issue
Resolve #601
Problem
The Edit tool fails with "old_string not found" when the file contains multiple backslashes (e.g., Rust string literals with \n, \t). The root cause is that the LLM misencodes backslashes when sending old_string in JSON — double-escaping or under-escaping them.
What changed
Checklist