Skip to content

fix(core): reject an edit whose strings differ only in line ending style - #47088

Closed
devYRPauli wants to merge 2 commits into
anomalyco:v2from
devYRPauli:edit-crlf-guard
Closed

fix(core): reject an edit whose strings differ only in line ending style#47088
devYRPauli wants to merge 2 commits into
anomalyco:v2from
devYRPauli:edit-crlf-guard

Conversation

@devYRPauli

@devYRPauli devYRPauli commented Sep 3, 2026 •
edited
Loading

Copy link
Copy Markdown

Issue for this PR

Closes #45927

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

packages/core/src/tool/plugin/edit.ts rejects an edit where oldString equals newString. That check runs on the raw input at line 135. The tool converts both strings to the file's line ending at lines 169-170.

Two strings that differ only in line ending style pass the raw check. After the conversion they hold the same text, so the replacement does nothing. findOccurrences still returns one match.

The tool then asks for edit approval, writes the file back unchanged, and reports one replacement.

Measured on v2 at f94eefa, through the registered tool. The file is CRLF and holds before\r\nrest\r\n. The call passes oldString: "before\r\nrest" and newString: "before\nrest".

status: completed
content: [{"type":"text","text":"Edited crlf.txt (1 replacement)"}]
output: {"files":[{"file":"crlf.txt","patch":"Index: crlf.txt\n===...\n--- crlf.txt\n+++ crlf.txt\n","additions":0,"deletions":0,"status":"modified"}],"replacements":1}
file unchanged: true
writes: 1
permission asserts: 1

The model reads one replacement and believes the edit landed. An LF file gives the same result, because the conversion rewrites a CRLF oldString to \n as well.

This PR runs the same check again after the conversion. The raw check stays where it is. It fails before the file read and before any permission prompt, which is the right place for the case it catches.

After the change the tool returns an error and writes nothing.

status: error
error: {"type":"tool.execution","message":"No changes to apply: oldString and newString are identical."}
file unchanged: true
writes: 0
permission asserts: 0

#46849 fixes the same defect on dev, where the tool lives at packages/core/src/tool/edit.ts. The two diffs touch different files, so neither one covers the other branch.

How did you verify your code works?

The new test is rejects an edit whose strings differ only in line ending style in packages/core/test/tool-edit.test.ts. It drives the registered tool through executeTool against a real CRLF file. It checks the error, the file content, the permission calls and the writes.

The test fails on v2 before the change. The failure reports status: "completed" with replacements: 1 and an empty patch.

bun test test/tool-edit.test.ts from packages/core: 17 pass, 0 fail.

bun run test from packages/core: 4040 pass, 39 skip, 3 fail. The failures are PluginSupervisor reload twice and Watcher lifecycle once. All three fail the same way on origin/v2 with this branch reverted.

bun typecheck from packages/core is clean. bun run lint:effect-simplifications, oxlint and prettier --check are clean on both files.

I also moved the new guard below permission.assert as a mutation check. The test failed, which shows the guard runs before the approval prompt.

Screenshots / recordings

Not a UI change.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

@devYRPauli
fix(core): reject an edit whose strings differ only in line ending style
da9ac28 
The identity guard runs on the raw input at line 135. The tool converts
both strings to the file's line ending at lines 169-170, 34 lines later.

Two strings that differ only in line ending style pass the raw guard and
become the same text. The replacement is then the identity function.
findOccurrences still returns one match, so the tool asks for edit
approval, writes the file back unchanged, and reports one replacement
with an empty patch.

Run the same check again after the conversion. The raw check stays where
it is. It fails before the file read and before any permission prompt,
which is still the right place for the case it catches.
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Thanks for your contribution!

This PR doesn't have a linked issue. All PRs must reference an existing issue.

Please:

  1. Open an issue describing the bug/feature (if one doesn't exist)
  2. Add Fixes #<number> or Closes #<number> to this PR description

See CONTRIBUTING.md for details.

This was referenced Sep 3, 2026
Closed
Closed
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

The following comment was made by an LLM, it may be inaccurate:

Related PRs Found

#46849 - fix(edit): reject an edit whose strings differ only in line ending style
#46849

This is NOT a duplicate but rather a companion fix on a different branch. As noted in the PR description:

Both PRs address the same root cause (rejecting edits where oldString and newString differ only in line ending style) but on different branches/locations.

@devYRPauli

devYRPauli commented Sep 3, 2026

Copy link
Copy Markdown
Author

The body has Closes #45927 under ### Issue for this PR, and #45927 is open.

The needs:issue label comes from a workflow mismatch. pr-standards.yml reads closingIssuesReferences. The v2 copy of that file states GitHub only fills that field for a PR that targets the default branch, and it falls back to a closes #N match in the body.

The copy that ran is the one on dev, which has no fallback. Run 33776835808 printed if (linkedIssues === 0) {, a line that exists only in the dev copy. The v2 fallback never runs, so every outside PR that targets v2 gets this label whatever the body says.

The check-compliance job in the same workflow passed, because it reads the body.

@devYRPauli
test(core): cover an LF file in the line ending identity test
f05bb44 
The test drove a CRLF file only. The defect does not depend on the
file's own line ending. An LF file with oldString spelled CRLF converts
to the same text as an LF newString, so it takes the same no-op path.

Measured on this head with the guard removed and the LF case asserted
first: status completed, replacements 1, an empty patch, "Edited
unix.txt (1 replacement)", and the file unchanged on disk.
@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Automated PR Cleanup

Thank you for contributing to opencode.

Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions.

This PR was closed because it matched the following cleanup criteria:

  • The PR was created more than 1 month ago
  • The PR had fewer than 2 positive reactions
  • Positive reactions are counted as thumbs-up, heart, celebration, or rocket reactions on the PR

PRs created within the last month are not affected by this cleanup.

If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate.

Thanks again for taking the time to contribute.

@github-actions github-actions Bot closed this Oct 3, 2026
@devYRPauli

devYRPauli commented Oct 5, 2026

Copy link
Copy Markdown
Author

This still reproduces on v2 at 2c33922. A focused harness executing the current edit function against LF and CRLF files reports one replacement, requests approval and writes unchanged content. The post-normalization guard rejects both cases before approval or writing. Could you reopen this PR? It targets v2; #46849 targets dev.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

automated-pr-cleanup needs:issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@devYRPauli