[Bug]: Five data-integrity bugs — edit mangles non-UTF-8 files, replace_all mismatch, non-atomic config rewrites, bricked v2 session, symlinked config clobber #547

Product or interface

CLI - interactive TUI (core packages — affects the Desktop app and CLI alike; found by source review)

Version

main @ da22f11 (v0.6.5)

Platform

Windows

OS version and architecture

Windows 10 x64 (build 26200)

Issue area

File / workspace / path / encoding

Steps to reproduce

Five related data-integrity bugs found during a review of this repository. Findings 1 and 2 were reproduced first-hand by driving the repository's own tool implementations (byte-level, on temp files); findings 3–5 are code-verified with call paths traced end-to-end. None involve security-sensitive material. Per finding: symptom → root cause (file:line) → minimal fix → existing test that can be extended. Happy to provide reproduction scripts or fix patches for any of these.

1. edit silently destroys non-UTF-8 file content

Repro (first-hand, byte-level): a file containing "你好 world\n" in GBK (c4 e3 ba c3 20 77 6f 72 6c 64 0a), LocalEditTool.execute with old_string: "world" → new_string: "earth". The tool reports success, but the file becomes efbfbd efbfbd efbfbd 20 65 61 72 74 68 0a — "��� earth\n". The untouched GBK bytes are permanently replaced with U+FFFD replacement characters, and the diff returned to the user/model only shows world → earth, completely hiding the destruction. replace_all: true behaves the same way (it performs its own full-file decode + replacement in packages/agent-tools/src/shared/replace-all-edit.ts:54-55).

Root cause: LocalEditTool (packages/agent-tools/src/desktop/local-pi-tools.ts:296-299) injects only createCapturingEditOperations(); the injected readFile (packages/agent-tools/src/shared/edit-capture.ts:23-27) hands the raw Buffer to pi, which lossily decodes with buffer.toString("utf-8") (third_party/pi-mono/packages/coding-agent/src/core/tools/edit.ts:341) and writes the whole file back as UTF-8, materializing the U+FFFDs.

The gap is first-party: the Write path already handles encodings — write-capture.ts:97-130 preserves utf16le/BOM, and read-guards.ts:349-354 deliberately lets UTF-16/GBK through the read binary guard — but Edit has no equivalent guard.

Minimal fix: sniff the Buffer in createCapturingEditOperations().readFile and in buildReplaceAllEdit before decoding (reuse detectEncodingFromBuffer from write-capture.ts:106 plus a fatal TextDecoder("utf-8", { fatal: true }) validity check — the same pattern already exists as isValidUtf8 in packages/local-runtime/src/turns/file-changes.ts:709), and either fail with a clear "file is not UTF-8" error or transcode like write-capture does.

Test: packages/agent-tools/src/shared/replace-all-edit.test.ts already drives LocalEditTool end-to-end on temp files; add a GBK-bytes case asserting the edit is rejected and the bytes are unchanged.

2. replace_all is inconsistent with a single edit on BOM / CRLF files

Repro (first-hand):

• UTF-8-BOM file: a single edit with old_string: "TOKEN" succeeds (BOM preserved). The identical call with replace_all: true throws Could not find the exact text in ….
• CRLF file alpha TOKEN\r\nbeta TOKEN\r\ngamma\r\n, multi-line LF old_string: "beta TOKEN\ngamma": a single edit succeeds (file keeps CRLF). The same call with replace_all: true throws replace_all: old_string was not found.

Root cause: buildReplaceAllEdit (packages/agent-tools/src/shared/replace-all-edit.ts:54-55, 67, 84) matches old_string against the RAW file content (BOM and \r\n intact) and builds oldText including the BOM. pi's matching contract — used by single edits — strips the BOM and normalizes both sides to LF before matching (edit.ts:344-348, edit-diff.ts:322-325), then restores BOM/line-endings on write. So the replace_all-built oldText can never match a BOM file, and a multi-line LF-only old_string can never be counted in a CRLF file.

The failure is safe (no write occurs) but it breaks the read→edit loop on every BOM file and on most multi-line replace_all calls under CRLF, and the error text ("the file may have changed since — use the read tool to reload") actively misleads.

Minimal fix: in buildReplaceAllEdit, strip the BOM and LF-normalize both the content and old_string before counting/replacing (pi's pipeline re-adds the BOM and restores line endings on write). Test: extend replace-all-edit.test.ts with BOM and CRLF cases.

Related: files with mixed line endings are normalized wholesale — edit-diff.ts detects one dominant ending and restoreLineEndings rewrites every line, so editing one line flips the endings of untouched lines (verified: aaa\r\nbbb\nccc\n + edit bbb→B → aaa\r\nB\r\nccc\r\n).

3. Read-path config rewriters: truncate+write, no lock, no backup

Where: writePrivateConfigFileSync (packages/config/src/private-config-file.ts:23-34) does openSync(O_WRONLY|O_CREAT) → fchmodSync → ftruncateSync(fd, 0) → writeFileSync. No temp file, no rename, no lock, no backup. Its callers are the read-path rewriters run by prepareConfigFileForRead (packages/config/src/config.ts:1881-1891) on every config load: syncManagedPresetBaseUrl (write at config.ts:1617-1620) and migrateLegacyByokProvidersOnDisk (packages/config/src/byok-config.ts:277-281; that one at least calls backupConfigForMigration first — the preset sync does not).

The trigger is real, not theoretical: the managed preset sync fires whenever provider.minimax.options.baseURL is a managed preset URL that differs from the current build's preset. LEGACY_MANAGED_PRESET_BASE_URLS (config.ts:1545-1547) exists precisely because past gateway migrations put stale preset URLs into user configs — an app update that changes PRESET_BASE_URLS makes every install rewrite its full config on first boot.

Loss scenario: a crash/power loss between ftruncateSync and write completion leaves config.yaml at 0 bytes (or half-written). The next boot parses "" → null → returns {} with no warning; the app "works" but BYOK providers, API keys, and all preferences are gone with nothing on disk to recover. The repository's own test codifies the destroyed end state: packages/config/test/managed-preset-sync.test.ts:105-114 ("does not hide a failure after the original file has been truncated") asserts the file is "" after a mid-write failure.

Contrast: every settings writer uses the proper discipline — proper-lockfile + temp-file + rename (local-model-provider-write.ts:220, tui-status-line-write.ts:24); the atomic-write helper even already exists (local-model-provider-write.ts:246-264, duplicated in packages/local-runtime/src/config/update.ts:285-303).

Additional risk (same root): the unlocked read-modify-write races the locked settings writers across processes — a second window/CLI holding a stale snapshot can silently revert a just-saved setting.

Minimal fix: route these writes through the shared lock + temp + rename helper (hoist atomicWriteFile into @mavis/config); fsync before rename. At absolute minimum, snapshot to config.yaml.bak.<reason>.<ts> first (the exact pattern exists at byok-config.ts:537-545). Test: replace the truncated-to-empty assertion in managed-preset-sync.test.ts with a crash-consistency case (partial write then throw ⇒ original content still readable, or a .bak exists).

4. One torn JSONL line permanently bricks a v2 session

Where: readJsonl (packages/local-runtime-v2/src/infra/file/jsonl.ts:60) throws Malformed JSONL in <path> at line N when no onMalformedLine observer is supplied; the cached strict path throws identically (:96-98), and it also treats a trailing line without \n as a line (:88), so a torn tail from a crash between write() and sync() is a parse failure. The production provider builds its adapter without an observer (packages/local-runtime-v2/src/service/session-system/messages/history/canonical-history-provider.ts:214), so readSnapshot (:340), append (:253), replace (:272), compact (:293), and syncIndex (:404) all hit the strict throw.

Consequence: a power loss mid-append leaves a partial last line in messages.jsonl; on next startup nothing detects or repairs it, and every read and every write of that session throws forever. The messages are still on disk but permanently inaccessible from the app without hand-editing the file.

This is a gap, not a design choice: the v1 ledger handles exactly this scenario (per-line parse failures are skipped and the scan continues: packages/local-runtime/src/sessions/ledger/ledger-read.ts:176-195, 226-239; the next append starts on a fresh line boundary: ledger-append-recovery.ts:78-93). All the tolerant-decode and repair machinery already exists in v2 (readActive/reportActiveMalformedLine in canonical-history-jsonl.ts:423-427, 1228-1233; repairCanonicalHistory in canonical-history-recovery.ts) — only the wiring is missing.

Minimal fix: in the provider's read path, catch the malformed-line error, fall back to the tolerant decode, run repairCanonicalHistory(...), and persist the repaired prefix via the already-atomic files.replaceActive(...). Test: add a "messages.jsonl with a torn trailing line is repaired on open" case to test/history-processing.test.ts.

5. Settings writers replace a symlinked config.yaml with a regular file

Where: fs.rename(tmpPath, configPath) in packages/config/src/local-model-provider-write.ts:259 and packages/local-runtime/src/config/update.ts:298. rename does not follow symlinks, so a symlink at configPath is silently replaced by the temp file.

Why this matters: symlinked configs are a supported setup — tui-status-line-write.ts:20-22 deliberately resolves realpath first, commenting "Replace the real file, preserving any symlink used to manage profile settings", and config-file-permissions.test.ts:241-250 exercises a symlinked config. The other writers don't, so a dotfiles user (config.yaml → repo copy) who changes one setting in the TUI silently breaks the link; the dotfiles copy keeps stale settings and the next sync reverts the user's change.

Minimal fix: mirror tui-status-line-write.ts — resolve realpath before temp-file creation and rename onto the target. Test: add a symlinked-config case to config-update-preservation.test.ts.

Expected and actual behavior

1. Editing a non-UTF-8 file should either preserve the file's encoding (like the Write path already does for UTF-16/BOM) or refuse with a clear error — not report success while replacing every non-UTF-8 byte in the file with U+FFFD.
2. replace_all: true should accept exactly the old_string values a single edit accepts, including on BOM and CRLF files.
3. A failure or crash during a read-path config rewrite must leave the original config.yaml intact (atomic rename, or at least a .bak), instead of leaving a 0-byte file that silently resets to default settings on next load.
4. A v2 session with one torn JSONL line should be repaired on open (v1 already does this), instead of throwing Malformed JSONL on every subsequent read and write forever.
5. Saving settings should preserve a symlinked config.yaml (like tui-status-line-write already does), instead of replacing the symlink with a regular file.

Redacted error summary

Finding 1 (first-hand, synthetic GBK file): before c4 e3 ba c3 20 77 6f 72 6c 64 0a → after a successful world→earth edit: ef bf bd ef bf bd ef bf bd 20 65 61 72 74 68 0a; tool result reported success, diff showed only the intended change.

Finding 2 (first-hand): Error: Could not find the exact text in <path>. The old text must match exactly… on a BOM file where a single edit with identical old_string succeeded; Error: replace_all: old_string was not found in <path>. on a CRLF file where a single multi-line edit with an LF old_string succeeded.

Finding 4: Malformed JSONL in <path> at line N thrown by every readSnapshot/append/compact after a crash left a partial last line.

Before submitting

• I have searched existing issues.
• I have included my version and removed sensitive information.