Skip to content

fix: encoding round-trip for skills_hub/skills_sync cache and lock reads (supersedes #62667) - #64822

Closed
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/62667-v2
Closed

fix: encoding round-trip for skills_hub/skills_sync cache and lock reads (supersedes #62667)#64822
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/62667-v2

Conversation

@AlexFucuson9

Copy link
Copy Markdown
Contributor

Supersedes #62667

Addresses all issues raised by @teknium1 in the #62667 review:

Problem

The write-side callers in skills_hub.py use ensure_ascii=False (UTF-8 output), but the paired read-side callers use read_text() without specifying encoding. On non-UTF-8 Windows locales (e.g. cp1252), this decodes UTF-8 bytes with the system default and corrupts non-ASCII skill names, descriptions, and metadata.

Fixed read sites (7 total)

File Line Paired write
tools/skills_hub.py 1045 :1055 ensure_ascii=False
tools/skills_hub.py 3234 :3254 ensure_ascii=False
tools/skills_hub.py 3288 :3294 HubLockFile ensure_ascii=False
tools/skills_hub.py 3360 HubLockFile taps read
tools/skills_hub.py 3678 :3732 hermes_index_cache write
tools/skills_hub.py 3744 :3732 hermes_index_cache write
tools/skills_sync.py 405 reads HubLockFile from skills_hub

What this does NOT change

  • Read calls that already specify encoding (skills_sync.py:108, 141, 186) are untouched.
  • The ignore_file.write_text(...) at :3249 writes ASCII-only content, no encoding needed.
  • Binary mode reads/writes ("rb"/"wb") are unaffected.

…d skills_sync

Addresses teknium1 review on NousResearch#62667:

The write-side callers already use ensure_ascii=False (UTF-8 output),
but the paired read-side callers used read_text() without encoding,
which on non-UTF-8 Windows locales would decode with the system
default (e.g. cp1252) and corrupt non-ASCII skill names/descriptions.

Fixed read sites:
- tools/skills_hub.py:1045 (paired with :1055 write)
- tools/skills_hub.py:3234 (paired with :3254 write)
- tools/skills_hub.py:3288 (paired with :3294 HubLockFile write)
- tools/skills_hub.py:3360 (HubLockFile taps read)
- tools/skills_hub.py:3678,3744 (hermes_index_cache read, paired with :3732 write)
- tools/skills_sync.py:405 (reads HubLockFile written by skills_hub)
@alt-glitch alt-glitch added type/bug Something isn't working comp/tools Tool registry, model_tools, toolsets tool/skills Skills system (list, view, manage) platform/windows Native Windows-specific behavior or breakage P3 Low — cosmetic, nice to have labels Jul 15, 2026

@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 addressing the missing read-side calls identified on #62667. The underlying Windows issue is still present on current main, but this patch needs the matching producer changes to make the encoding contract valid.

Problems

  • The changed readers become UTF-8-only, while the paired writers remain locale-dependent: tools/skills_hub.py:1147, :3368, and :3408 use ensure_ascii=False without encoding="utf-8". Read-only changes do not guarantee the advertised round trip on non-UTF-8 Windows locales.
  • The changed reader paths catch OSError and JSONDecodeError, not UnicodeDecodeError; existing locale-encoded state can therefore raise after this patch rather than take the established fallback.
  • No non-ASCII round-trip regression test is included. Existing HubLockFile tests at tests/tools/test_skills_hub.py:1311-1402 use ASCII fixtures.

Suggested changes

  • Apply UTF-8 explicitly to the paired writers, retain the matching readers, and add behavioral non-ASCII cache/lock round-trip coverage. Please avoid a source-text guard; AGENTS.md prohibits source-inspection tests.

Automated hermes-sweeper review.

Comment thread tools/skills_hub.py
if time.time() - stat.st_mtime > INDEX_CACHE_TTL:
return None
return json.loads(cache_file.read_text())
return json.loads(cache_file.read_text(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.

This reader-only change does not create a UTF-8 round trip: its paired writer still calls write_text(json.dumps(..., ensure_ascii=False)) without encoding="utf-8". On a non-UTF-8 Windows locale it can write locale bytes (or fail for unrepresentable characters), and this UTF-8 reader can then raise UnicodeDecodeError, which the surrounding handler does not catch.

Comment thread tools/skills_hub.py
return {"version": 1, "installed": {}}
try:
return json.loads(self.path.read_text())
return json.loads(self.path.read_text(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.

HubLockFile.save() still writes ensure_ascii=False JSON without an explicit encoding, so making only this consumer UTF-8-only breaks the producer/consumer encoding contract. Update the paired writer and cover a non-ASCII lock entry round trip.

@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 16, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing as resolved by PR #71078 (merged, commit d372fda): the class-wide close-out salvaged your #50655/#54241/#56385/#66856/#65440 series as the backbone (authorship preserved in git log) and swept the remaining sites, so every read_text/write_text call this PR touches is now guarded on current main — verified per-site. A CI linter rule in check-windows-footguns.py plus the AST guard test now prevent regressions. Your overlapping/split variants of the same series are being closed together; the credit for the class rests on your commits.

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