fix(sleep): read and write state.json as UTF-8 - #302

Explicit UTF-8 for newly written state is useful, but the October 6 review of 129d249b4ab8 found an upgrade data-loss regression that must be addressed before merge.

A state file successfully produced and read by the existing writer under GBK or cp1252 becomes unreadable by the new UTF-8-only loader on that same simulated locale. The existing broad exception handler then returns fresh state, and the next save overwrites the original memory/history/cursors. The baseline producer was used to verify the legacy fixture bytes, not merely a hand-written corrupt file.

The positive state slice passed 13 tests. The independent upgrade slice had 4 failures and 1 UTF-8 control pass; the original-main compatibility controls passed. These are Linux simulations of the target module's implicit codec, not native Windows results. No user data was accessed.

Please preserve UTF-8 for new writes while adding an explicit legacy migration/compatibility path, or at minimum fail with an actionable diagnostic and prevent overwriting an undecodable existing state. Add a load-then-save upgrade regression that preserves prior fields, including unknown fields. This is a data migration, not a safe two-line change to merge first. Related #304 has the analogous configuration-upgrade concern.

Fixed in 9dc00d4, and you were right that this is a data migration rather than a two-line change.

What the previous revision did. I reproduced the loss directly, with a state file written by the old locale-codec writer:

loader on a GBK state file   -> night = 0     (the whole state discarded)
after load + save            -> night = 1, slow_memory = '', history = [],
                                unknown field gone

The file on disk was replaced by a fresh one. except Exception: pass is what made it silent — a reset state is indistinguishable from a first run.

The change. load() now reads bytes and decodes them as UTF-8 first; on UnicodeDecodeError it falls back to the locale codec, which is the one the old writer used on the machine that produced the file, and returns that data. The next save() rewrites it as UTF-8, so the migration completes on the first write instead of needing a separate tool.

loader on the same GBK file  -> night = 42, slow_memory and history intact,
                                unknown field preserved
after load + save            -> night = 43, every field carried over, file now UTF-8

A file that decodes under neither is no longer replaced. load() raises StateFileError, whose message names the path and says it was left untouched. I took the louder option rather than "return fresh state but block save()", because continuing would still run a night against a reset counter and empty harvest cursors — the failure belongs before the work, not after it. That is a deliberate behaviour change to load(), so it is worth a look: it now raises where it used to return a default.

Tests. test_legacy_locale_encoded_state_is_migrated_not_discarded (load then save, asserting every field survives including an unknown one, and that the bytes end up UTF-8) and test_unreadable_state_is_reported_and_left_untouched (raises, and the bytes are identical afterwards). Both fail against the previous revision — I ran them there to confirm — and neither can pass by accident on a UTF-8 runner, since the locale codec is injected and the fixture is asserted to be genuinely non-UTF-8.

tests/test_sleep_state.py: 5 passed. The 4 failures in tests/test_model_change_warning.py are a missing optional llm_client import and are present before this change as well.

Thanks for testing this with the real producer rather than a hand-written corrupt file — a hand-written fixture would have missed the case entirely.

@microsoft-github-policy-service agree

The original encoding-upgrade regression is fixed at 9dc00d49f3a7e42623d1c72147c87dc9518acc82: files produced by the real prior writer under simulated GBK/cp1252 retain history, cursors, memory and unknown fields after load/save, and are rewritten as UTF-8. Invalid JSON and undecodable bytes now stop without replacement.

One gap remains in the new unreadable-state contract: a JSON list, null or scalar is returned as an empty mapping, so load followed by save can replace it with fresh state. The three independent non-object cases fail on this head; this behavior is inherited from main, not a newly introduced codec regression. Please reject non-object state with the same actionable StateFileError and preserve its bytes, while keeping the working migration tests. I am holding this migration change for that narrow contract completion, and coordinating it with #320 rather than merging overlapping reader implementations blindly.

Pushed 4739102 for the narrow contract you described.

_read_state() no longer coerces non-object bodies to an empty mapping. It raises the same
StateFileError, with the path and the same "move or delete it, it will not be overwritten"
wording, and the bytes are left untouched:

if not isinstance(data, dict):
    raise StateFileError(
        f"{path} holds a JSON {type(data).__name__}, not a JSON object, so it cannot be "
        f"used as state. It has been left untouched. ..."
    )

Tests: test_non_object_state_is_reported_and_left_untouched is parametrized over
[], [1, 2, 3], null, 42, "a string" and asserts both the exception and that
path.read_bytes() is unchanged. The existing migration tests are untouched.

Locally, a standalone harness loading the real state.py (stdlib only) reproduces the five
cases: all raise StateFileError, all preserve bytes, and a valid {"night": 7} still loads.
CI on this head will confirm the full suite.

Merged as 618d5d208c2d after all six exact-head CI jobs, CLA and the reviewed integration checks passed. The original contributor credit is preserved.

Re-reviewed 4739102ddeb991c82cb9b4501b597009c64ea807. The remaining non-object-state request is resolved: list/null/scalar inputs now stop with a path-bearing StateFileError and leave the file unchanged. The real prior writer's GBK/cp1252 data still survives load/save with every old and unknown field preserved, and the output is UTF-8. Actual CLI failures stop before cycle writes. The isolated merge onto the review baseline passed 1,808 tests with 13 skips; the independent migration and CLI controls pass.

The actual merged main 1a44050fc53a was re-fetched and verified: 1829 tests passed, 13 skipped, 43 independent checks passed, and strict documentation, official main CI and scans passed. These are correctness/compatibility checks, not model-performance experiments.

The earlier non-object behavior was inherited from main and is no longer a blocker on this head. The split-exposure defaults already introduced by #272 are expected schema additions, not lost state. Thank you for the focused follow-up.