[diyaavirmani]

Description

Closes #1854.

string_replace and diff_edit read the file through the file cache, which decodes it with buffer.toString('utf-8'), then write the whole file back as UTF-8. In a file that isn't valid UTF-8, such as Latin-1, Windows-1252 or Shift-JIS, every byte that doesn't decode becomes U+FFFD. So an edit rewrote bytes on lines it never touched (a Latin-1 E9 became EF BF BD) and still reported success.

As agreed in the issue, both tools now refuse these files with a clear error, the same way they already refuse PDF and DOCX files.

Change:

• source/utils/file-cache.ts: the cache records whether the bytes it read are valid UTF-8 (notUtf8, from buffer.isUtf8). It already holds the raw buffer, so this costs no extra read.

• source/utils/path-validators.ts: a new validateEditableEncoding(), next to validateEditableFormat() (the PDF/DOCX refusal), with a message in the same style:

  ⚒ Cannot edit "legacy.txt": it is not valid UTF-8, so it probably uses a legacy encoding such as Latin-1, Windows-1252 or Shift-JIS. The edit would be written back as UTF-8, which would replace every byte that isn't valid UTF-8 with U+FFFD (�), including on lines the edit doesn't touch. Do not retry — tell the user, who can convert the file to UTF-8 first (for example with iconv) or make the change in an editor that keeps the file's encoding.

• string_replace and diff_edit: both call it in their execute path and in their validator, right after reading the cached file and before any content matching. So a non-UTF-8 file gets this error, not a confusing "content not found".

UTF-8 files edit exactly as before, including ones with non-ASCII text or a BOM.

write_file isn't changed. It only reads the cache to show the previous content, and its write is a full overwrite the user approves.

No docs change: the docs don't describe the edit tools' existing PDF/DOCX refusal either.

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

Changeset

• Added a changeset (pnpm changeset) describing this change for the changelog

Testing

Automated Tests

• New features include passing tests in .spec.ts/tsx files
• All existing tests pass (pnpm test:all completes successfully): see below; the only failures also happen without this change
• Tests cover both success and error scenarios

New tests, covering the cases asked for on the issue, for both tools:

• string_replace.spec.tsx:
  • a Latin-1 file is refused and every byte stays the same
  • Windows-1252 and Shift-JIS files are refused and stay the same
  • the validator refuses a Latin-1 file
  • a UTF-8 file with non-ASCII text (café ✓ 日本) still edits, and the untouched line keeps its bytes
  • a UTF-8 file with a BOM still edits and keeps the BOM
• diff-edit.spec.tsx:
  • a Latin-1 file is refused and stays the same
  • the validator refuses a Latin-1 file
  • a UTF-8 file with non-ASCII text still edits, and untouched lines keep their bytes
• file-cache.spec.ts: notUtf8 is set for a Latin-1 file and not for UTF-8 with non-ASCII text and a BOM.
• path-validators.spec.ts: validateEditableEncoding accepts and refuses as expected.

With the four source files reverted, every refusal test fails, as do the two cache-flag tests. The UTF-8 and BOM tests pass either way: they guard against refusing good files.

Format, lint, types and knip pass. On macOS the full AVA suite passed 9,729 tests (20 skipped), with three failures that this PR doesn't cause:

• two gh-pr-worktree tests fail the same way on upstream main (/var vs /private/var tmpdir paths on macOS)
• one bash-executor cancel timing test ("SIGKILL fallback terminates processes that ignore SIGTERM") failed while the machine was under heavy load (load average 13). bash-executor.spec.ts passes 51/51 when run on its own, twice, and this PR doesn't touch source/services.

Manual Testing

• Tested with Ollama
• Tested with OpenRouter
• Tested with OpenAI-compatible API
• Tested MCP integration (if applicable)

End to end through the real CLI on macOS with Node 22.23.3:

• --plain run, auto-accept,
• a scripted local OpenAI-compatible model that calls read_file on legacy.txt, then the edit from the issue (old → new).

The source of truth is the file's bytes before and after (xxd -p) and the tool result the model was sent.

Case: file bytes before	v1.31.0 (npm, latest release)	upstream main c72945d3	v1.32.0 release PR (e4c0df94)	this branch	v1.32.0 release PR + this fix
string_replace, Latin-1: 63 61 66 e9 0a 6f 6c 64 0a	63 61 66 ef bf bd 0a 6e 65 77 0a, "Successfully replaced"	same	same	refused, bytes unchanged	refused, bytes unchanged
string_replace, Shift-JIS 日本 + old: 93 fa 96 7b 0a 6f 6c 64 0a	ef bf bd ef bf bd ef bf bd 7b 0a 6e 65 77 0a, "Successfully replaced"	same	same	refused, bytes unchanged	refused, bytes unchanged
diff_edit, Latin-1	… ef bf bd …, "Successfully applied"	same	same	refused, bytes unchanged	refused, bytes unchanged
string_replace, UTF-8 control: 63 61 66 c3 a9 0a 6f 6c 64 0a	63 61 66 c3 a9 0a 6e 65 77 0a	same	same	same	same
diff_edit, UTF-8 control	63 61 66 c3 a9 0a 6e 65 77 0a	same	same	same	same

When it refuses, the model is sent ⚒ Validation failed: ⚒ Cannot edit "legacy.txt": it is not valid UTF-8, …. The doubled ⚒ is the existing prefix every validator error gets, the same as the PDF refusal. The fix cherry-picks cleanly onto the v1.32.0 release PR.

Checklist

• If this was for an open issue, I was assigned to it
• Code follows project style guidelines
• Self-review completed
• Documentation updated (if needed)
• No breaking changes (or clearly documented)
• Appropriate logging added using structured logging (no new runtime paths that need logging; the refusal is returned to the model like the PDF/DOCX one)

[github-actions]

nc-review: comments — 1 important, 3 nits

@diyaavirmani — a few things worth a look, none blocking.

The change records a notUtf8 flag on cached file reads (from buffer.isUtf8, computed from the buffer already in hand) and has string_replace and diff_edit refuse non-UTF-8 files with a clear message, mirroring the existing PDF/DOCX refusal. It does exactly what issue #1854 asked for, the placement in both the execute and validator paths is correct, the changeset package name resolves, and the tests assert the behaviour that matters (bytes unchanged, refusal surfaced, UTF-8/BOM files still edit) rather than just importing the new function. Not blocking; the two remaining same-class write-back paths are worth raising before or shortly after merge.

🟠 important · completeness · source/tools/lsp-format-document.tsx:292

formatFileWithLsp has the identical lossy read/rewrite pattern and is not covered by this change:

const original = await readFile(absPath, 'utf-8');
const formatted = applyTextEdits(original, edits);
await writeFile(absPath, formatted, 'utf-8');

Formatting a Latin-1 or Shift-JIS source file rewrites every undecodable byte as U+FFFD and reports "Formatted path (N edits applied)". This is the same data loss as #1854, and arguably worse in intent terms: the user asked for formatting, not to rewrite the file's encoding, so "the write is an approved full overwrite" does not cover it. Now that CachedFile.notUtf8 exists, the guard is a few lines — either add it here too, or note in the changeset/issue that the format path is deliberately left open and file a follow-up.

⚪ nit · completeness · source/tools/file-ops/write-file.tsx:57

The PR argues write_file is unaffected because it writes the content the model supplied. That covers a deliberate rewrite, but not the common case where the model echoes back what read_file gave it: read_file returns U+FFFD-mangled text for a legacy-encoded file, so a write_file that intends to preserve the untouched lines still corrupts them and still reports success. The justification is defensible for a first fix — the linked issue only asked about the two edit tools, so this does not weaken the Closes #1854 claim — but it is the same remaining hole, and notUtf8 is now available on the cached entry the write path already reads.

⚪ nit · correctness · source/tools/read-file.tsx:85

The metadata path reports Encoding: UTF-8 for any file readFile(absPath, 'utf-8') does not throw on — and Node never throws, it substitutes U+FFFD — so a Latin-1 file is currently labelled UTF-8. With cached.notUtf8 now available in the same scope, that branch could report a legacy encoding instead. Not required to fix the issue; noting it because the label is actively misleading to the model and to the user, and the fix is one line in a block already reading the flag.

⚪ nit · design · source/utils/path-validators.ts:52

validateEditableEncoding takes a Pick<CachedFile, 'notUtf8'> from file-cache, so the path-validator module now imports a type from the cache layer. It sits next to validateEditableFormat and the narrow Pick keeps the coupling to a single boolean, so this is defensible — but worth being deliberate about: if a second caller later needs the raw bytes rather than the flag, this is the point where the module boundary should be revisited rather than widening the parameter.

---

_{🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional}

_{Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.}

[diyaavirmani]

The lsp_format_document point from nc-review is real; I reproduced it (along with read_file metadata always reporting Encoding: UTF-8) and filed it as #1867, so this PR stays scoped to the two tools #1854 covers.