[PerryLink (PerryLink)]

skillopt_sleep/state.py reads and writes state.json with no explicit encoding, while save() deliberately passes ensure_ascii=False. The file therefore lands in the platform's locale charset, and two things follow -- the second is the serious one.

1. On a locale that cannot represent the text, save() raises. On a GBK machine:

UnicodeEncodeError: 'gbk' codec can't encode character '\u2705' in position ...

2. Even when it does not raise, the bytes are not UTF-8, and load() swallows the failure. load() wraps its decode in a bare except Exception: pass, so any reader whose locale codec is UTF-8 -- a container, CI, WSL, PYTHONUTF8=1, or simply another machine -- gets a decode error, discards it, and returns a fresh state:

locale.getencoding() = cp936
bytes on disk decode as UTF-8 : NO -> 'utf-8' codec can't decode byte 0xd6 in position 67
bytes on disk decode as GBK   : yes
as UTF-8  -> UnicodeDecodeError; load() swallows it via `except Exception: pass`
             -> a fresh state, i.e. every field silently reset

Night counter, harvest cursors, history and the cross-night memory all disappear without a warning. The file is readable only by the machine that wrote it.

This is the defect #124 fixed in skillopt/config.py -- "read YAML config files as UTF-8", merged 2026-07-12, one line:

-    with open(abs_path) as f:
+    with open(abs_path, encoding="utf-8") as f:

state.py was simply missed by that sweep. The change here is the same two lines:

-                with open(path) as f:
+                with open(path, encoding="utf-8") as f:
...
-        with open(tmp, "w") as f:
+        with open(tmp, "w", encoding="utf-8") as f:

Tests

Two tests in tests/test_sleep_state.py, and the second exists because the first is not sufficient:

• test_state_file_round_trips_non_ascii_as_utf8 -- saves a CJK lesson and a CJK project path, asserts the bytes on disk decode as UTF-8, and asserts the round-trip preserves both.
• test_state_io_does_not_fall_back_to_the_locale_codec -- repeats the round-trip in a subprocess under -X warn_default_encoding -W error::EncodingWarning.

The behavioural test cannot fail on a UTF-8 CI runner, because there the locale codec is UTF-8. On its own it would ship a regression test that never goes red. The warning-to-error guard makes CPython's own EncodingWarning a failure on every platform, which is what actually pins the fix.

Before (source fix reverted, tests kept):

FAILED tests/test_sleep_state.py::test_state_file_round_trips_non_ascii_as_utf8
FAILED tests/test_sleep_state.py::test_state_io_does_not_fall_back_to_the_locale_codec
EncodingWarning: 'encoding' argument not specified
2 failed, 1 passed

After:

3 passed

[Yifan Yang (Yif-Yang)]

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.

[PerryLink (PerryLink)]

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.

[PerryLink (PerryLink)]

@microsoft-github-policy-service agree

[Yifan Yang (Yif-Yang)]

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.

[PerryLink (PerryLink)]

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.

[Yifan Yang (Yif-Yang)]

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.