Repository navigation
fix(agent-core-v2): preserve BOM and encoding when Edit modifies files - #4371
NoX-programer wants to merge 20 commits into
Conversation
Signed-off-by: NoXъ <NoX-programer@users.noreply.github.com>
Signed-off-by: NoXъ <NoX-programer@users.noreply.github.com>
Signed-off-by: NoXъ <NoX-programer@users.noreply.github.com>
Signed-off-by: NoXъ <NoX-programer@users.noreply.github.com>
Signed-off-by: NoXъ <NoX-programer@users.noreply.github.com>
Signed-off-by: NoXъ <NoX-programer@users.noreply.github.com>
Signed-off-by: NoXъ <NoX-programer@users.noreply.github.com>
Signed-off-by: NoXъ <NoX-programer@users.noreply.github.com>
Signed-off-by: NoXъ <NoX-programer@users.noreply.github.com>
🦋 Changeset detectedLatest commit: d319e30 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 8e209735fb
ℹ️ 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".
| async edit(input: FileEditInput, fs: IHostFileSystem = this.fs): Promise<FileEditResult> { | ||
| try { | ||
| const raw = await fs.readText(input.path, { errors: 'strict' }); | ||
| const bom = splitByteOrderMark(await fs.readBytes(input.path, BOM_SNIFF_BYTES)).bom; |
There was a problem hiding this comment.
When Edit runs through AcpHostFileSystem for a client-backed buffer whose path is absent or inaccessible on the agent's local disk, this unconditional readBytes fails before readText can use the ACP reverse-RPC, because AcpHostFileSystem.readBytes always delegates to its local HostFileSystem. The previous implementation began with readText, so the same client buffer was editable; treat an unavailable BOM sniff as the BOM-less text path (or add a client byte capability) instead of making local bytes a prerequisite.
AGENTS.md reference: AGENTS.md:L122-L124
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the BOM sniff is now wrapped in try/catch — when readBytes is unavailable (e.g. an ACP client-backed buffer, where AcpHostFileSystem.readBytes delegates to the local disk backend and throws), Edit falls back to the BOM-less text path via readText, which is the pre-PR behavior. Added a regression test simulating a rejecting readBytes.
| try { | ||
| const text = await fs.readText(path, options); | ||
| if (!text.includes('\0')) { | ||
| return stripLeadingBomChar(text); |
There was a problem hiding this comment.
When an ACP client decodes a BOM-prefixed file whose first content character is itself U+FEFF, readTextFile returns that content character after consuming the encoding marker, but this branch strips it unconditionally; the previous Edit path forwarded it to writeTextFile. The fallback decodeUnicodeText repeats the loss for UTF-8/UTF-16 bodies because its decoder also treats the already-split character as a BOM, so a successful edit silently deletes file content; only strip the actual byte marker and decode the body with ignoreBOM: true.
AGENTS.md reference: AGENTS.md:L130-L132
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: the unconditional U+FEFF strip is gone — the byte marker is already consumed by the readText decoders (and by ACP clients), so a leading U+FEFF in the returned text is content, not a marker. decodeUnicodeText now decodes the BOM-split body with ignoreBOM: true, so the byte path keeps it as well. Added regression tests for both paths (UTF-8 and UTF-16LE files whose content starts with U+FEFF, plus direct decodeUnicodeText cases for all encodings).
Signed-off-by: NoXъ <NoX-programer@users.noreply.github.com>
Signed-off-by: NoXъ <NoX-programer@users.noreply.github.com>
Signed-off-by: NoXъ <NoX-programer@users.noreply.github.com>
Signed-off-by: NoXъ <NoX-programer@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3b8dc7bb1
ℹ️ 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 (!text.includes('\0')) { | ||
| return text; | ||
| } | ||
| } catch {} |
There was a problem hiding this comment.
When a BOM-prefixed file is edited through AcpHostFileSystem and the node-local copy is readable, either a legitimate U+0000 in the client result or any readTextFile failure enters this fallback. AcpHostFileSystem.readBytes in packages/acp-server/src/acp-fs/acpFsService.ts:139-140 reads that local copy, but the later writeBytes is bridged back to writeTextFile; if the editor buffer has unsaved changes, the edit is computed from stale disk data and silently overwrites the buffer, whereas the previous path used the client text or surfaced its error. Restrict this fallback to known local decoder failures instead of rejecting NUL/all ACP errors.
AGENTS.md reference: AGENTS.md:L130-L132
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: IHostFileSystem now carries an optional clientBackedText marker, set by AcpHostFileSystem when the fs.readTextFile capability is negotiated. readBomText uses only the client text for such filesystems — no byte reads, so stale local data can never reach the write path, and readTextFile errors surface instead of being swallowed. Local filesystems read the authoritative bytes directly, which also removes the NUL mis-decode heuristic. Added regression tests for the marker and for the client-backed edit path.
Signed-off-by: NoXъ <NoX-programer@users.noreply.github.com>
Signed-off-by: NoXъ <NoX-programer@users.noreply.github.com>
Signed-off-by: NoXъ <NoX-programer@users.noreply.github.com>
Signed-off-by: NoXъ <NoX-programer@users.noreply.github.com>
Signed-off-by: NoXъ <NoX-programer@users.noreply.github.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b66c79e93
ℹ️ 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".
| const view = new DataView(body.buffer, body.byteOffset, body.byteLength); | ||
| const little = encoding === 'utf-32le'; | ||
| for (let i = 0; i < units.length; i++) { | ||
| view.setUint32(i * 4, units[i]!.codePointAt(0)!, little); |
There was a problem hiding this comment.
When an Edit result contains an unpaired UTF-16 surrogate, such as a lone high surrogate supplied in new_string, this writes 0xD800 as a UTF-32 code point (and the UTF-16 branch likewise emits the raw unpaired code unit), even though decodeUnicodeText rejects those values. A local Edit therefore reports success but creates a file that the next Edit cannot decode; with AcpHostFileSystem, decodeTextPayload rejects the payload and writeBytes silently updates only the node-local copy instead of the client buffer. Reject or normalize non-well-formed strings before writing them.
AGENTS.md reference: AGENTS.md:L130-L132
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed: encodeUnicodeText now rejects strings containing unpaired surrogates (lone high or low) for all five encodings, mirroring decodeUnicodeText, which already rejects them. The Edit fails with a clear error instead of writing a file the next Edit cannot decode, and the ACP bridge no longer falls back to the node-local copy for such payloads. Added regression tests: direct encodeUnicodeText cases (unpaired high/low rejected, paired astral characters accepted, all five encodings) and an end-to-end Edit test asserting the write is never attempted.
Signed-off-by: NoXъ <NoX-programer@users.noreply.github.com>
Signed-off-by: NoXъ <NoX-programer@users.noreply.github.com>
|
@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". |
Requirement or Bug
Resolve #3825
Bug Reproduction Steps
See linked issue.
Root Cause
The Edit tool's read path decodes file content with a UTF-8-only
TextDecoder(defaultignoreBOM: false), which silently strips a leading byte order mark (BOM), and the write path always emits UTF-8. So every time Edit modified a file that started with a BOM, the BOM and the original encoding were lost: the file came back as plain UTF-8 without a BOM.This is a fundamental fix: the read and write paths now detect the BOM and round-trip the original encoding.
Code Changes
packages/agent-core-v2/src/_base/text/encoding.ts— new BOM helpers:splitByteOrderMark/byteOrderMarkBytes(all five BOMs: UTF-8, UTF-16LE/BE, UTF-32LE/BE) and strictdecodeUnicodeText/encodeUnicodeTextfor those encodings.packages/agent-core-v2/src/app/edit/fileEditService.ts— dual-path edit: files without a BOM go through the existing code path unchanged; files with a BOM are read via the text API (so unsaved client buffers in ACP mode are respected), with a strict byte-level fallback if the text read cannot represent the content, and written back asBOM + original encoding.packages/acp-server/src/acp-fs/acpFsService.ts— thewriteBytesbridge now decodes BOM-prefixed UTF-8/UTF-16/UTF-32 payloads into text so the ACP client's buffer is updated and the client saves the file in its own encoding; binary payloads still go to the local backend.packages/agent-core-v2/src/agent/tools/os/read/readTool.ts— updated the model-visible hint for transcoded files: Edit preserves BOM-prefixed encodings; Write still expects UTF-8.encoding.test.ts,edit.test.ts, andacp-fs.test.ts.Related: #3946 (skywolf) fixes the same issue for the UTF-8 BOM plus line endings and
.editorconfig; this PR keeps the change surface small (Edit only, no line-ending or.editorconfigchanges) and generalizes the BOM handling to all five BOM-prefixed Unicode encodings (UTF-8/UTF-16LE/BE/UTF-32LE/BE).Behavior Changes and Affected Users
writeByteswith a BOM-prefixed text payloadwriteTextFileso the client buffer is updatediconvvia Bash)."iconv) otherwise."Affected modules and test coverage:
agent-core-v2_base/text(encoding helpers —encoding.test.ts, round-trips for all five BOMs),agent-core-v2app/edit(dual-path edit —edit.test.ts, BOM + encoding preserved for all five BOMs, no BOM added to BOM-less files),acp-serveracp-fs(writeBytes bridge —acp-fs.test.ts, bridge for all five BOMs).Checklist
/approve).gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.Note on the second checkbox: #3825 is linked above; per repo policy, a maintainer's
/approveon the issue is still pending.