fix(utils): atomic_json_write escapes lone surrogates instead of raising UnicodeEncodeError
The canonical writer defaults to ensure_ascii=False, but ~10 of the sites repointed onto it (terminal breadcrumbs, shell-hook allowlist, active sessions, debug pending, model-catalog cache, the credential writers) previously used json's ensure_ascii=True default. A surrogate-escaped str (os.fsdecode of a non-UTF-8 cwd/argv) that json used to persist as \udcff now made the utf-8 text handle raise UnicodeEncodeError - a ValueError that the callers' `except OSError` never catches, so breadcrumbs silently stopped writing and the other sites leaked a new error type. Fix at the canonical: serialize to a str first (so nothing lands in the temp file on failure), and on UnicodeEncodeError retry the dump with ensure_ascii=True. That escape round-trips - json.loads returns the same str with the lone surrogate - whereas encoding with surrogateescape emits a raw 0xFF byte the reader's utf-8 decode rejects. The happy path is unchanged: normal content keeps its raw UTF-8 bytes on disk.
This commit is contained in:
@@ -27,7 +27,7 @@ class TestAtomicJsonWrite:
|
||||
original = {"preserved": True}
|
||||
target.write_text(json.dumps(original), encoding="utf-8")
|
||||
|
||||
with patch("utils.json.dump", side_effect=SimulatedAbort):
|
||||
with patch("utils.json.dumps", side_effect=SimulatedAbort):
|
||||
with pytest.raises(SimulatedAbort):
|
||||
atomic_json_write(target, {"new": True})
|
||||
|
||||
|
||||
@@ -67,3 +67,20 @@ def test_shell_hooks_allowlist_survives_failed_replace_without_temp(tmp_path, mo
|
||||
shell_hooks.save_allowlist({"approvals": [{"event": "a", "command": "x"}]}) # logs, never raises
|
||||
assert json.loads(target.read_text(encoding="utf-8")) == {"approvals": []}
|
||||
assert _leftovers(target.parent, target.name) == []
|
||||
|
||||
|
||||
def test_surrogate_escaped_strings_round_trip_through_atomic_json_write(tmp_path):
|
||||
"""A non-UTF-8 cwd/argv (``os.fsdecode`` → lone surrogate) must be persisted, not raise.
|
||||
|
||||
Breadcrumbs, the shell-hook allowlist and the active-sessions ledger all persist paths and
|
||||
guard only ``OSError``; a ``UnicodeEncodeError`` (a ValueError) escaping the canonical writer
|
||||
silently stopped those writes.
|
||||
"""
|
||||
from utils import atomic_json_write
|
||||
|
||||
payload = {"cwd": "a\udcffb", "plain": "caf\u00e9"}
|
||||
target = tmp_path / "crumbs" / "crumb.json"
|
||||
atomic_json_write(target, payload)
|
||||
assert json.loads(target.read_bytes()) == payload
|
||||
assert _leftovers(target.parent, target.name) == []
|
||||
|
||||
|
||||
@@ -255,19 +255,36 @@ def atomic_write_bytes(path: Union[str, Path], content: bytes, *, tmp_prefix: st
|
||||
mode=mode if mode is not None else _preserve_file_mode(path), fsync_dir=fsync_dir)
|
||||
|
||||
|
||||
def _dump_json(data: Any, f, *, indent: "int | None", ensure_ascii: bool, dump_kwargs: dict) -> None:
|
||||
"""``json.dump`` that survives surrogate-escaped strings.
|
||||
|
||||
``os.fsdecode`` of a non-UTF-8 filename/argv yields lone surrogates (``'\\udcff'``); a utf-8
|
||||
text handle rejects them with ``UnicodeEncodeError`` — a ValueError, which callers guarding
|
||||
``except OSError`` never see. ``ensure_ascii=True`` escapes them as ``\\udcff`` and
|
||||
``json.loads`` restores the identical str, so the retry round-trips; ``surrogateescape``
|
||||
would emit a raw 0xFF byte that the reader's utf-8 decode rejects. Serializing to a str first
|
||||
keeps the failure before any byte reaches the file, so no partial payload is left behind.
|
||||
"""
|
||||
text = json.dumps(data, indent=indent, ensure_ascii=ensure_ascii, **dump_kwargs)
|
||||
try:
|
||||
f.write(text)
|
||||
except UnicodeEncodeError:
|
||||
f.write(json.dumps(data, indent=indent, ensure_ascii=True, **dump_kwargs))
|
||||
|
||||
|
||||
def atomic_json_write(
|
||||
path: Union[str, Path], data: Any, *, indent: int = 2, mode: int | None = None,
|
||||
ensure_ascii: bool = False, fsync_dir: bool = False, **dump_kwargs: Any,
|
||||
) -> None:
|
||||
"""Write JSON to *path* atomically (temp file + fsync + replace).
|
||||
|
||||
``ensure_ascii=True`` lets callers persist surrogate-escaped strings (non-UTF-8 argv/paths)
|
||||
that a utf-8 text handle would otherwise reject with ``UnicodeEncodeError``. ``mode=0o600``
|
||||
is the private-credential form: the temp file is 0600 from creation (mkstemp), so the payload
|
||||
is never umask-readable.
|
||||
Surrogate-escaped strings (non-UTF-8 argv/paths) are always persisted: the write falls back
|
||||
to ``ensure_ascii=True`` escapes for that payload only, so normal content keeps its raw UTF-8
|
||||
bytes. ``mode=0o600`` is the private-credential form: the temp file is 0600 from creation
|
||||
(mkstemp), so the payload is never umask-readable.
|
||||
"""
|
||||
path = Path(path)
|
||||
_atomic_write(path, lambda f: json.dump(data, f, indent=indent, ensure_ascii=ensure_ascii, **dump_kwargs),
|
||||
_atomic_write(path, lambda f: _dump_json(data, f, indent=indent, ensure_ascii=ensure_ascii, dump_kwargs=dump_kwargs),
|
||||
prefix=f".{path.stem}_", mode=mode if mode is not None else _preserve_file_mode(path),
|
||||
fsync_dir=fsync_dir)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user