From 4157494d2fbd3e52053b8c642811c119298d98e8 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sun, 13 Sep 2026 00:06:31 -0700 Subject: [PATCH] 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. --- tests/hermes_cli/test_atomic_json_write.py | 2 +- tests/test_atomic_json_writers_unified.py | 17 ++++++++++++++ utils.py | 27 ++++++++++++++++++---- 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/tests/hermes_cli/test_atomic_json_write.py b/tests/hermes_cli/test_atomic_json_write.py index 74d90d4ea7..2ed2dddb7a 100644 --- a/tests/hermes_cli/test_atomic_json_write.py +++ b/tests/hermes_cli/test_atomic_json_write.py @@ -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}) diff --git a/tests/test_atomic_json_writers_unified.py b/tests/test_atomic_json_writers_unified.py index aa4fb9b43d..d635e28365 100644 --- a/tests/test_atomic_json_writers_unified.py +++ b/tests/test_atomic_json_writers_unified.py @@ -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) == [] + diff --git a/utils.py b/utils.py index ec44431cd3..9b3fe3e7e7 100644 --- a/utils.py +++ b/utils.py @@ -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)