Skip to content

fix(sleep): preserve config limits across encoding upgrades - #304

Merged
Yifan Yang (Yif-Yang) merged 2 commits into
microsoft:mainfrom
KennyMcSimpson:fix/sleep-config-utf8
Oct 17, 2026
Merged

Yifan Yang (Yif-Yang) merged 2 commits into
microsoft:mainfrom
KennyMcSimpson:fix/sleep-config-utf8

Conversation

@KennyMcSimpson

@KennyMcSimpson KennySimpson (KennyMcSimpson) commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Sleep's default-encoding reader can drop UTF-8 settings under a non-UTF-8 locale. Reading only UTF-8 fixes that case but breaks existing GBK/cp1252 files: a configured 1,000-token limit can silently become 400,000.

Read UTF-8 first. On a decode failure, try the system's legacy encoding and warn that the file should be saved as UTF-8 before moving to another locale. The configuration file is left unchanged.

If an existing file cannot be read, decoded or parsed, has a non-mapping root, or requires unavailable PyYAML, raise a configuration error instead of discarding it. The CLI reports the path and how to fix it, then exits with code 2 before starting a cycle. Missing files still use defaults; explicit CLI flags take precedence over loaded values.

Validation

  • 30 focused configuration tests passed, including six legacy upgrade cases and six CLI precedence cases across GBK/cp1252 and JSON/YAML/YML.
  • 26 new regressions fail on the previous UTF-8-only commit (522f3f72b601) and pass with this update.
  • python -m pytest -q: 1,588 passed, 12 skipped; 359 subtests passed.
  • Ruff checks passed for all three changed Python files.
  • python -m mkdocs build --strict passed.

The legacy-codec cases simulate codecs on Linux; they are not native Windows test results. The existing UTF-8 subprocess controls also remain green.

Related: #302 concerns the separate state.json reader and writer.

@Yif-Yang

Copy link
Copy Markdown
Contributor

The UTF-8 configuration positive path is useful, but the October 6 review of 522f3f72b601 found a compatibility regression for existing non-UTF-8 configurations.

JSON/YAML/YML files that the baseline successfully reads under GBK or cp1252 fail after the UTF-8-only change. The swallowed decode failure silently discards explicit settings: in synthetic tests a 1000-token limit falls back to 400000, a two-task limit to 40, and explicit preferences/target_skill_path disappear. No paid calls were made and the file itself was not overwritten; the problem is running with unintended defaults.

The positive slice passed 16 tests. The independent upgrade slice had 6 failures across the two codecs and three formats, with 3 UTF-8 controls passing. This is a Linux codec simulation, not a native Windows claim.

Before merge, provide an explicit legacy migration/compatibility path or stop with an actionable configuration error. An explicitly requested file must not silently lose its budget and target restrictions. Please cover these upgrade cases as well as CLI precedence. #302 needs the equivalent protection for existing state; neither change should be merged with this regression left for a later follow-up.

@KennyMcSimpson KennySimpson (KennyMcSimpson) changed the title fix(sleep): read config files as UTF-8 fix(sleep): preserve config limits across encoding upgrades Oct 7, 2026
@Yif-Yang
Yifan Yang (Yif-Yang) merged commit ea6f380 into microsoft:main Oct 17, 2026
7 checks passed
@Yif-Yang

Copy link
Copy Markdown
Contributor

Reviewed 2a4099b6c18b69f27a65f533fad25ffc7e0e9cf6 and merged as ea6f380be1e0 after official exact-head CI and CLA passed.

Thank you for completing the configuration upgrade handling. The independent 15-case slice confirms that legacy GBK/cp1252 JSON, YAML and YML preserve configured budgets, targets and unknown options, while explicit overrides still win. Invalid configuration stops the actual CLI with exit code 2 before a cycle starts. These are Linux codec simulations, not native Windows results. The separate state-file work in #302/#320 remains outside this fix.

Each step was re-simulated on the then-current main and matched the audited tree; the original PR author is retained as commit author. The final combined main (ae2f766ea02c) was re-fetched and verified: 1,799 tests passed, 13 skipped; 84 independent checks passed; strict documentation build passed. These are bounded functional/compatibility results, not model-performance uplift.

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.

2 participants