Skip to content

fix: encoding round-trip write side in skills_hub.py (ensure_ascii=False without encoding) - #65440

Closed
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/encoding-write-side-skills-hub
Closed

fix: encoding round-trip write side in skills_hub.py (ensure_ascii=False without encoding)#65440
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/encoding-write-side-skills-hub

Conversation

@AlexFucuson9

Copy link
Copy Markdown
Contributor

Problem

3 write_text() calls in tools/skills_hub.py produce UTF-8 content via ensure_ascii=False but omit the encoding="utf-8" parameter. On Windows, write_text() defaults to the system codepage (cp1252), which corrupts non-ASCII characters in skill names, descriptions, and metadata.

PR #64822 already fixes the read side (read_text()read_text(encoding="utf-8")). This PR fixes the write side to complete the encoding round-trip.

Why This Matters

Without this fix, the write path produces cp1252-encoded JSON on Windows, while PR #64822 changes the read path to expect UTF-8. This mismatch causes UnicodeDecodeError or silent data corruption when reading back cached skill data with non-ASCII content.

Files Changed

Line Function Call
1147 _write_index_cache() cache_file.write_text(json.dumps(..., ensure_ascii=False))
3366 _write_stale_index_cache() cache_file.write_text(json.dumps(..., ensure_ascii=False, default=str))
3406 SkillLockStore.save() self.path.write_text(json.dumps(..., ensure_ascii=False) + "\\n")

… in skills_hub.py

3 write_text calls produce UTF-8 content (ensure_ascii=False) but omit
the encoding parameter. On Windows, write_text defaults to the system
codepage (cp1252), corrupting non-ASCII skill names and descriptions.

PR NousResearch#64822 already fixes the paired read_text calls. This commit fixes
the write side to complete the encoding round-trip.

- _write_index_cache(): cache_file.write_text (line 1147)
- _write_stale_index_cache(): cache_file.write_text (line 3366)
- SkillLockStore.save(): self.path.write_text (line 3406)
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/tools Tool registry, model_tools, toolsets tool/skills Skills system (list, view, manage) platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Jul 16, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for covering the write-side calls: all three target writes are still unencoded on current main, so the diagnosis is valid.

Problems

  • This cannot complete the claimed round trip by itself. The paired current-main readers at tools/skills_hub.py:1137, tools/skills_hub.py:3348, and tools/skills_hub.py:3402 still use read_text() without encoding="utf-8". PR fix: encoding round-trip for skills_hub/skills_sync cache and lock reads (supersedes #62667) #64822 contains corresponding reader changes but remains open, so merging this PR alone would write UTF-8 and then decode via the Windows locale.
  • Please add a non-ASCII persistence regression. The existing TestHubLockFile.test_save_creates_parent_dir at tests/tools/test_skills_hub.py:1334 verifies only file creation.

Suggested changes

  • Fold the three paired reader encodings into this change.
  • Add a cache/lock write-read round-trip test using non-ASCII metadata.

Automated hermes-sweeper review.

@teknium1

Copy link
Copy Markdown
Contributor

Merged via PR #71078 — your commit was cherry-picked onto current main with your authorship preserved in git log (rebase merge). Your 5-PR series (#50655 was authored under your earlier account, #54241, #56385, #66856, #65440) formed the backbone of the class-wide close-out: 68 of the 139 bare sites came from your commits, and the campaign's structure followed your directory-by-directory split. The remaining 71 sites were swept on top and a CI linter rule now prevents regressions. Thanks for the sustained, methodical work on this class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have platform/windows Native Windows-specific behavior or breakage sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants