fix(agent-core): refuse multi-line empty Edit deletions - #2511
mangeshraut712 wants to merge 5 commits into
Conversation
🦋 Changeset detectedLatest commit: 2ba98cb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 097b1774d0
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
commit: |
097b177 to
eb224a2
Compare
|
Rebased onto latest CI is currently stuck in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 380b0df96d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a17e3b9f8c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
2f08cd7 to
51cc962
Compare
|
Rebased onto latest |
c6652fd to
1a45ffa
Compare
|
Rebased onto latest |
|
rebase it |
744fcd0 to
1ee01a3
Compare
1ee01a3 to
4aeab95
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 403a94d376
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| export function isOversizedEmptyDeletion(oldString: string, newString: string): boolean { | ||
| return newString.trim().length === 0 && countEditLines(oldString) >= LARGE_DELETE_MIN_OLD_LINES; |
There was a problem hiding this comment.
Require opt-in for cumulative replace_all deletions
When replace_all=true, this predicate considers only one occurrence of old_string. For example, a two-line string appearing twice passes the guard and both matches are erased with an empty new_string, deleting four lines without allow_large_delete=true. Both engine implementations have this behavior, so the guard should account for the matched occurrence count or total resulting deletion before permitting the edit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed: replace_all empty deletes now pass the matched occurrence count into the guard. Multi-line old_strings still accumulate (linesPerMatch * count); a one-line token is not scaled into a multi-line span.
8cc1aee to
bf501f5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf501f5b79
ℹ️ 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".
| if (input.allow_large_delete !== true && isOversizedEmptyDeletion(input.old_string, input.new_string)) { | ||
| return { ok: false, error: oversizedDeletionMessage(input.path) }; | ||
| } |
There was a problem hiding this comment.
Check for a match before refusing the deletion
When replace_all is false and a stale or mistyped old_string spans three or more lines with an empty new_string, this branch returns the large-deletion refusal before countOccurrences runs, even though nothing can be deleted. This suppresses the newly strengthened old_string not found recovery guidance and may prompt the model to retry with allow_large_delete=true instead of rereading; perform the existence and uniqueness checks before applying the deletion guard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed: countOccurrences (and the uniqueness check) now run before the large-delete refusal, so a stale or duplicate multi-line old_string gets not-found / not-unique guidance instead of allow_large_delete.
| newString.trim().length === 0 && | ||
| occurrenceCount > 0 && | ||
| countEditLines(oldString) * occurrenceCount >= LARGE_DELETE_MIN_OLD_LINES |
There was a problem hiding this comment.
Avoid counting repeated single-line matches as multiple lines
For a replace_all deletion of a one-line token occurring at least three times—for example removing ", " from "a, b, c, d"—multiplying its one-line count by the occurrence count classifies the edit as a multi-line deletion. The tool documentation says the opt-in is for deleting a multi-line span, and the resulting error incorrectly says that old_string itself spans three or more lines, so ordinary single-line bulk removals are refused unless the model supplies a misleading override; determine the affected spans from actual line boundaries rather than treating every occurrence as another line.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed: the guard uses countEditLines(old_string) and only multiplies by occurrence count when that span is already multi-line. replace_all of a one-line token (including the , × N case) is allowed without allow_large_delete.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Following up on the earlier rebases. This is mergeable and still needs a maintainer review: multi-line empty Edit deletions are refused. I will leave the branch as it is unless you want a change. |
b1c9995 to
b2db743
Compare
Guard Edit against accidental +0/−N wipeouts when new_string is empty across 3+ lines unless allow_large_delete is set. Strengthen the old_string-not-found recovery guidance so the model rereads a large enough region instead of looping short Reads (fixes MoonshotAI#2427).
Count matched occurrences when guarding empty replace_all edits so a short old_string cannot delete 3+ lines without allow_large_delete.
Match and uniqueness checks run before refusing an empty deletion. replace_all still accumulates multi-line matches, but a one-line token is not treated as a multi-line span.
Re-record loop and tool inline snapshots so they include the post-rebase tool list and allow_large_delete hashes.
b2db743 to
2ba98cb
Compare
Summary
new_stringunlessallow_large_delete=true(fixes Edit 失败后陷入死循环:反复"Edit 失败 → 重读 → 再失败",并逐步删空了被编辑文件的多个章节 #2427).old_string not foundrecovery guidance to reread a large enough region.Test plan
pnpm exec vitest run test/tools/edit.test.tsinpackages/agent-core(23 passed)pnpm exec vitest run test/app/edit/tools/edit.test.tsinpackages/agent-core-v2(26 passed)Fixes #2427