Skip to content

fix: add encoding="utf-8" to write_text() with ensure_ascii=False in skills_hub - #62667

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

fix: add encoding="utf-8" to write_text() with ensure_ascii=False in skills_hub#62667
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/write-text-encoding-skills-hub

Conversation

@AlexFucuson9

Copy link
Copy Markdown
Contributor

Summary

Path.write_text() without encoding= defaults to the platform encoding — cp1252 on Windows, utf-8 on Linux/macOS. Two call sites in skills_hub.py pass ensure_ascii=False to json.dumps() (allowing non-ASCII characters through) but omit encoding="utf-8" on the subsequent write_text(). On Windows, this silently corrupts non-ASCII skill names, descriptions, and metadata into mojibake, which then fails to parse on read.

All other write_text() calls in the codebase already specify encoding="utf-8" (checked: curator.py, curator_backup.py, skill_bundles.py, agent_init.py, skills_sync.py, checkpoint_manager.py).

Changes

Line Before After
1055 write_text(json.dumps(data, ensure_ascii=False)) ..., encoding="utf-8")
3254 write_text(json.dumps(data, ensure_ascii=False, default=str)) ..., encoding="utf-8")

Test Plan

  • ruff check tools/skills_hub.py passes
  • Cache files with non-ASCII content survive a round-trip on Windows

… in skills_hub

Path.write_text() without encoding= defaults to the platform encoding
(cp1252 on Windows). When combined with ensure_ascii=False, non-ASCII
characters in skill names, descriptions, and metadata are silently
corrupted on Windows — mojibake in cache files that then fail to parse.

Affected sites:
- _write_index_cache(): cache_file.write_text(json.dumps(data, ensure_ascii=False))
- _search_catalog_fallback(): same pattern with default=str

All other write_text() calls in the codebase already pass encoding="utf-8".
@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 labels Jul 11, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: this narrow fix (2 write_text() callsites in tools/skills_hub.py) is a strict subset of two broader open PRs that already contain these exact hunks byte-for-byte:

Not marking duplicate since it's a subset of an open superset rather than a same-scope copy — maintainer may prefer landing one of the broader PRs instead.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating a real Windows encoding issue.

Problems

  • The changed writers at tools/skills_hub.py:1055 and :3254 now emit UTF-8, but their paired readers at :1045 and :3234 still use read_text() without encoding. The proposed write-only change therefore does not guarantee the advertised round trip on a non-UTF-8 Windows locale.
  • The same state path remains in HubLockFile: tools/skills_hub.py:3294 writes ensure_ascii=False JSON, while :3288 reads without an encoding; tools/skills_sync.py:405 also reads that lock file without one.
  • The diff adds no regression test. The existing HubLockFile coverage at tests/tools/test_skills_hub.py:1334-1402 does not exercise non-ASCII persistence.

Suggested changes

  • Update the paired cache reads, lock-file producer/consumers, and add a non-ASCII round-trip regression test. The verified open #47489 diff already covers these related paths.

Automated hermes-sweeper review.

Comment thread tools/skills_hub.py
cache_file = index_cache_dir / f"{key}.json"
try:
cache_file.write_text(json.dumps(data, ensure_ascii=False))
cache_file.write_text(json.dumps(data, ensure_ascii=False), encoding="utf-8")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the paired read_text() calls too (_read_cache at current main line 1045 and _read_index_cache at line 3234). Writing UTF-8 while reading through the Windows default locale does not preserve the non-ASCII metadata this change is intended to protect.

@teknium1 teknium1 added 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 sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 11, 2026
@AlexFucuson9

Copy link
Copy Markdown
Contributor Author

Superseded by #64822 — adds encoding='utf-8' to all paired cache/lock reads in skills_hub and skills_sync.

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-moderate Sweeper blast radius: moderate — a subsystem or single platform 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