Skip to content

fix(documents): refuse to save a buffer that was decoded lossily - #379

Merged
PathGao merged 1 commit into
sftwrdotdev:masterfrom
PathGao:fix/refuse-saving-lossy-decoded-files
Aug 2, 2026
Merged

fix(documents): refuse to save a buffer that was decoded lossily#379
PathGao merged 1 commit into
sftwrdotdev:masterfrom
PathGao:fix/refuse-saving-lossy-decoded-files

Conversation

@PathGao

@PathGao PathGao commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

The problem

#371 made every read path decode leniently, which is the right call for opening a file: a document in a legacy encoding (GBK, Big5, Shift-JIS, EUC-KR, CP1251…) now opens as U+FFFD mojibake instead of failing outright, and it no longer opens-or-fails purely by size.

What must not follow is writing that buffer back. U+FFFD is not reversible — the original bytes are gone from the buffer, so saving replaces a readable document with permanent mojibake. With auto-save on, that happens 1.5s after the user's first keystroke, before they have any chance to realise the file was never really loaded.

Two paths made it worse than it looks:

  • the ≤ max_bytes branch of open_markdown_preview became lenient too, so small legacy-encoded files went straight into an editable buffer with nothing marking them;
  • session restore read through the bare command, so relaunching the app laundered the fact away even if it had been known before.

The fix

Both read commands report the fidelity of their decode, the tab carries it, and documentSession refuses to write such a buffer over the file it came from. Every writer — Ctrl+S, the auto-save timer, the close dialogs, the task-checkbox toggle — funnels through saveContent/saveContentAs, so that is the one place that has to hold.

Save As to a different path stays open. The new file is genuinely UTF-8, so writing it is correct, and it is the user's way out; the flag clears once the buffer has a file of its own. Picking the source file again in the dialog is refused, because that is the same destructive overwrite reached another way.

This is damage control, not encoding support. Markpad still cannot read GBK — that needs a real decoder. This only stops it from overwriting what it could not read.

Details worth a reviewer's eye

  • decode_utf8_lossy returns DecodedText { content, lossy }. Still exactly one decoder (a test pins that); it just no longer throws away what String::from_utf8 already told it.
  • read_file_content keeps its shape; read_file_content_checked is added alongside. Its two remaining callers are in MarkdownViewer.svelte and read the result through (await invoke(…)) as string — an assertion, not a check — so returning a tuple would have type-checked cleanly and turned rawContent into an array at runtime. Those two call sites only re-read a file whose tab is already flagged, so they are safe today; migrating them is a small follow-up.
  • The truncation order is load-bearing. The preview trims to a UTF-8 character boundary before decoding, so a valid file cut mid-character is not misreported. Reversed, every large CJK or emoji document would be permanently locked out of saving — a guard worse than the bug. Both directions are pinned by tests.
  • hasReplacementChars is a required field of the cross-window transfer payload, strictly validated, following that module's existing rule that a missing field is rejected rather than defaulted. A falsy default here reads as "safe to overwrite", and the failure mode is a destroyed document. The broker is in-memory and both windows are the same binary, so there is no version skew to tolerate.
  • Known boundary, documented in the code: the same-path comparison is exact string equality, so on a case-insensitive filesystem a Save As typed as /notes/Legacy.md for a tab opened as /notes/legacy.md is allowed through. Closing that needs the backend to canonicalize both sides; the same-path check is a courtesy on top of the guard that matters.

Tests

16 new behaviour tests (scripts/lossyDecodeSaveGuard.test.ts) plus 6 Rust tests. Run against unmodified master, 15 of the 16 fail. The one that passes is a deliberate anchor on existing structure — that auto-save and the close dialogs share saveContent — so it turns red if a future change routes a writer around the choke point.

npm run check   0 errors, 0 warnings
npm test        248 / 248
cargo test      82 / 82

Follow-up (not in this PR)

When the guard refuses, saveContent returns false, and the auto-save timer in MarkdownViewer.svelte toasts its generic "Auto-save failed" on every re-arm. The specific explanation here is shown once per tab, but the generic one still repeats while typing. Quieting it needs MarkdownViewer.svelte, which #374 is currently changing; it is best done together with migrating the two read_file_content call sites noted above.

🤖 Generated with Claude Code

Every read path decodes leniently, so a file in a legacy encoding (GBK,
Big5, Shift-JIS, EUC-KR, CP1251...) opens as U+FFFD mojibake instead of
failing. U+FFFD is not reversible: the original bytes are gone from the
buffer. Writing that buffer back over the source file - auto-save does
it 1.5s after the first keystroke - destroys the document permanently.

Both read commands now report the fidelity of their decode, the tab
carries it, and `documentSession` refuses to write such a buffer over
the file it came from. "Save As" to a different path stays open: the new
file is genuinely UTF-8, so writing it is correct, and it is the user's
way out.

- `decode_utf8_lossy` returns `DecodedText { content, lossy }`. Still one
  decoder; it just no longer throws away what it knew.
- `read_file_content_checked` is added rather than changing
  `read_file_content`, whose two remaining callers live in a file this
  branch must not touch and read the result through `as string` - an
  assertion, not a check, so a tuple would type-check and break at
  runtime.
- The truncated-preview branch trims to a character boundary *before*
  decoding, so a valid UTF-8 file cut mid-character is not misreported
  as lossy. That order is load-bearing: reversed, every large CJK
  document would be locked out of saving.
- Window restore reads through the checked command too. Without it,
  relaunching the app laundered the flag away and the next auto-save
  destroyed the document.
- `hasReplacementChars` is a required field of the cross-window transfer
  payload and is strictly validated, per that module's existing rule
  that a missing field is rejected rather than defaulted. A falsy
  default here reads as "safe to overwrite".

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@PathGao
PathGao force-pushed the fix/refuse-saving-lossy-decoded-files branch from bb942bc to 34bcdd6 Compare August 2, 2026 20:02
@PathGao

PathGao commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto c983d7a now that #374 has landed. Three files overlapped and one of the resolutions is a behaviour decision worth calling out:

#374 added a second entry into loadMarkdown — a pane that can write (edit or split mode, the F5 / "reload from disk" path) skips the preview and reads the whole file up front, so the editor is never bound to a partial buffer. That branch fills an editable buffer, which makes it exactly the kind of caller that must not use the unchecked command: it would hand the editor unflagged mojibake. It now reads through read_file_content_checked and reports fidelity like the preview branch does.

ensureFullContent is the one place the bare read_file_content survives, and only because it re-reads a file whose tab loadMarkdown already flagged — setTabRawContent does not clear the flag. A test pins that it is the only remaining unchecked call in documentSession, so a future caller cannot quietly join it.

The other two overlaps were mechanical: isTruncated and hasReplacementChars are independent fields, and the two toast keys are independent strings.

One new test (the editable-pane shortcut reports fidelity too), verified to fail when that branch is reverted to the bare command.

npm run check   0 errors, 0 warnings
npm test        304 / 304
cargo test      82 / 82

@PathGao
PathGao merged commit 4f065b4 into sftwrdotdev:master Aug 2, 2026
4 checks passed
@PathGao
PathGao deleted the fix/refuse-saving-lossy-decoded-files branch August 2, 2026 20:47
PathGao added a commit that referenced this pull request Aug 3, 2026
… the auto-save toast from repeating (#406)

#379 added `read_file_content_checked` so a lossily decoded buffer can
refuse to overwrite its source. Three call sites kept the bare command -
`toggleEdit`, `toggleSplitView`, `ensureFullContent`. They were safe, but
by an argument rather than by construction: each re-reads a file whose
tab is already flagged. That is two call sites agreeing with each other,
and it holds only until someone adds a fourth reader. All three now read
through `_checked` and set the flag from what they read, which also
clears a stale flag on a file since converted to UTF-8.

`ensureFullContent` is the one with a live hazard: it replaces a >50KB
preview with the whole file, and a file can be valid UTF-8 for its first
50KB and not after.

Separately, a refused save returns false, and the auto-save timer treated
that like any other failure - so the generic "Auto-save failed" toast
fired on every re-arm while the user typed, even though the specific
explanation appears once per tab. The generic toast is now suppressed
when the refusal has explained itself, and a refused tab stops re-arming.
The first attempt still runs, since that is what produces the
explanation.

Co-authored-by: PathGao <gaoyanbo@gaoyanbodeMacBook-Air.local>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
PathGao pushed a commit that referenced this pull request Aug 3, 2026
`in_code_region` is a `binary_search_by`, so `code_region_ranges` must
return its regions in document order. It did — by calling
`sort_unstable()` on the last line, after a second pass had appended
every inline code span behind the fenced regions. Deleting that one line
left `cargo test` at 144 passed, while markers inside a fenced block
(`![[embed]]`, `[[wikilink]]`, `==highlight==`, `^[footnote]`, `$x$`)
were reported as prose and rewritten.

The order is now produced by construction: the scan records each plain
segment's inline spans at the moment it closes that segment, immediately
before the fence that ended it, so every push is at a higher offset than
the last. The sort is gone, the `plain_segments` vector is gone, and a
`debug_assert!` names the invariant at its one construction site. Four
tests cover the consequence — one per consumer of `code_region_ranges`.

Also in this change:

- `convert_markdown` captures its parameter as `raw_buffer` before any
  preprocessing runs, and hands that to `annotate_task_checkboxes`. The
  fail-safe only works while its second argument is the unpreprocessed
  buffer, and the natural way to add a step — `let content = ...` near
  the top — silently retargeted it. A source-level test pins the three
  properties the capture depends on; provenance is not a type, so a
  source check is what is available.

- `annotate_task_checkboxes`'s doc comment claimed the frontend "writes
  a `- [x]` marker into whatever happens to sit on that line". That
  describes the pre-#352 frontend. Rewritten to the current behaviour
  and to the two cases that still corrupt.

- `read_file_content` is deleted: no call site since #379, and its
  defining property is that it hides the lossy-decode verdict. Its
  frontend guard was a hard-coded three-file allowlist; it is now a
  whole-tree scan plus an assertion that the command stays deleted.

- `update_pinned_tags`'s comment said `localStorage` makes an RMW cycle
  atomic by construction. It does not — that claim came from #424, this
  project's own recent work — and the passage now states the real
  asymmetry.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PathGao added a commit that referenced this pull request Aug 3, 2026
…ort (#434)

`in_code_region` is a `binary_search_by`, so `code_region_ranges` must
return its regions in document order. It did — by calling
`sort_unstable()` on the last line, after a second pass had appended
every inline code span behind the fenced regions. Deleting that one line
left `cargo test` at 144 passed, while markers inside a fenced block
(`![[embed]]`, `[[wikilink]]`, `==highlight==`, `^[footnote]`, `$x$`)
were reported as prose and rewritten.

The order is now produced by construction: the scan records each plain
segment's inline spans at the moment it closes that segment, immediately
before the fence that ended it, so every push is at a higher offset than
the last. The sort is gone, the `plain_segments` vector is gone, and a
`debug_assert!` names the invariant at its one construction site. Four
tests cover the consequence — one per consumer of `code_region_ranges`.

Also in this change:

- `convert_markdown` captures its parameter as `raw_buffer` before any
  preprocessing runs, and hands that to `annotate_task_checkboxes`. The
  fail-safe only works while its second argument is the unpreprocessed
  buffer, and the natural way to add a step — `let content = ...` near
  the top — silently retargeted it. A source-level test pins the three
  properties the capture depends on; provenance is not a type, so a
  source check is what is available.

- `annotate_task_checkboxes`'s doc comment claimed the frontend "writes
  a `- [x]` marker into whatever happens to sit on that line". That
  describes the pre-#352 frontend. Rewritten to the current behaviour
  and to the two cases that still corrupt.

- `read_file_content` is deleted: no call site since #379, and its
  defining property is that it hides the lossy-decode verdict. Its
  frontend guard was a hard-coded three-file allowlist; it is now a
  whole-tree scan plus an assertion that the command stays deleted.

- `update_pinned_tags`'s comment said `localStorage` makes an RMW cycle
  atomic by construction. It does not — that claim came from #424, this
  project's own recent work — and the passage now states the real
  asymmetry.

Co-authored-by: PathGao <gaoyanbo@gaoyanbodeMacBook-Air.local>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
PathGao added a commit that referenced this pull request Aug 6, 2026
…itch (#471)

One editor, one implicit model, and a tab switch that overwrote it with
`editor.setValue(content)`. `TextModel.setValue` is *defined* to throw
the undo stack away — `_setValueFromTextBuffer` runs
`this._commandManager.clear()` under the comment "Destroy my edit history
and settings" — so leaving a document and coming back cost the user
everything they could have undone (#391).

Each tab now has its own `ITextModel`, and a switch is
`editor.setModel(...)`. Undo belongs to the document, which is Monaco's
intended usage and how VS Code works. `tabs.svelte.ts` already said so in
a comment: "Text undo belongs to Monaco's model, not to the tab."

## Lifetime

Monaco models are registered with the model service and are NOT collected
while registered, so the invariant is: the set of live models is exactly
the set of live tabs. It is held by reconciliation, not by a dispose
paired with each removal — `retainTabModels(liveTabIds)` asks "which
models have no tab?" after every removal, so a route nobody has thought
of yet is still covered. Three call sites, because there are three places
a tab stops existing: `closeTab` (the close button, Ctrl/Cmd+W,
close-others, close-to-the-right, a clean tab losing its path in
`claimPath`, a tab moved to another window, a rolled-back transfer),
`closeAll`, and `restoreState`, which replaces the whole array and is the
one removal that never calls `closeTab`.

Disposing a model the editor has attached needs no special case:
`CodeEditorWidget._attachModel` registers
`model.onWillDispose(() => this.setModel(null))`, so the editor detaches
itself and the activation effect attaches the new tab's model in the same
flush. A window close destroys the webview, which frees everything.

The models live in `utils/tabModels.ts`, not on `Tab`: a model field on
the store would either drag Monaco back into the startup chunk (~86% of
the startup JavaScript, kept out by a dynamic import and locked by
`monacoStartupGraph.test.ts`) or be typed `any`. That module imports
Monaco for its TYPES only, which is erased.

## Close and reopen: the undo history is gone, and says so

Monaco cannot preserve an undo stack across `dispose()`. Closing a tab
disposes its model, so reopening the document gives a new tab, a new
model and an empty history — same as VS Code, and the same reason. A
cross-window move is the same case: a model is a JavaScript object in one
webview and cannot travel, so the arriving tab starts fresh. Both are
pinned by tests rather than left to be discovered.

## Assumptions that had to change

- Keyed by TAB ID, not by path. A path is not stable for the life of a
  tab (Save As, rename, following a link, back/forward), a model's URI
  cannot change after creation, untitled tabs have no path, and
  `createModel` throws on a duplicate URI. A UUID collides with nothing,
  including the two-tabs-one-path window that #413/#416 are about.
- The language was applied once, at `create`. Now that the model outlives
  every route that repoints a tab, it is re-applied on acquire — which
  also fixes a tab that follows a link from `.md` into `.ts` keeping the
  old language.
- The word count and the language reading arrived for free, because
  `setValue` fires a content change as a side effect. `setModel` does
  not — no content changed, a different document arrived — so the switch
  asks for them explicitly (`syncStatusFromModel`, which also replaces
  two hand-rolled copies of the word count).
- `editor.dispose()` no longer takes the model with it. That is the
  point: `StandaloneEditor` only disposes a model it built itself, and
  this component is unmounted every time a tab goes to reading mode.
- The view state is still saved and restored by hand. It is the editor's,
  not the model's, and it has to be applied after the model it describes
  is attached.

## What `setValue` is still for

External writes — a reload from disk, an accepted external change, a
truncated buffer completed, a task checkbox toggled from the preview, a
link followed inside the tab — hand the tab a DIFFERENT document, and an
undo stack from the old one would splice two texts together. Those still
go through `setValue` and still clear undo, unchanged from today; the
`getValue()` comparison is what tells them from an ordinary switch.
Making them undoable with `pushEditOperation` is what #391 suggests and
is deliberately NOT here: it would let Ctrl+Z resurrect a buffer that the
truncation and lossy-decode guards (#374, #379) exist to keep away from
the file, which is a second behaviour change and deserves its own.

## Verification

`npm run check` 0 errors, `npm test` 700/700, `cargo test` 157/157,
`npm audit` 0 vulnerabilities, `npm run build` clean and Monaco still in
its own chunk.

Falsification: with `scripts/undoHistoryPerTab.test.ts` kept and the two
source files reverted to master, 11 of its 17 assertions go red —
including "undo did nothing after a round trip through another tab" and
"models built during this test: 0 !== 2" on every closing path. The six
that stay green are deliberate locks on behaviour this change preserves
(an external write still clears undo, a navigate clears it, a rename does
not, the word count, the view state, and a negative control that undo
does not reach past the opened state).

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant