Skip to content

fix(skills): sort content_hash by relative posix string, not Path objects (#53404) - #53448

Open
Tranquil-Flow wants to merge 1 commit into
NousResearch:mainfrom
Tranquil-Flow:fix/53404-content-hash-order-rev2
Open

fix(skills): sort content_hash by relative posix string, not Path objects (#53404)#53448
Tranquil-Flow wants to merge 1 commit into
NousResearch:mainfrom
Tranquil-Flow:fix/53404-content-hash-order-rev2

Conversation

@Tranquil-Flow

Copy link
Copy Markdown
Contributor

What

content_hash() (on-disk skill integrity hash) sorted Path objects while bundle_content_hash() (in-memory bundle hash) sorts the relative-posix string keys. For layouts where these orderings disagree, the two functions produced different digests for identical content, breaking the symmetry both docstrings require and corrupting skill integrity tracking / update detection.

Root cause

sorted(skill_path.rglob("*")) sorts Path objects, which compare across path parts (tuple comparison). The value actually hashed is the relative-posix string (parts joined with /). The directory separator / sorts differently inside a flat string than across tuple parts, so for a layout like:

  • lib/helper.py (in a subdirectory)
  • lib-helper.py (top level)

Path sort puts lib/helper.py first (('lib',) < ('lib-helper.py',)), but posix-string sort puts lib-helper.py first ('-'(45) < '/'(47)). The bytes are fed into the SHA-256 in a different order → different digest.

Fix

Collect (relative_posix, path) pairs first, then sort by the string key that is fed into the hash — matching bundle_content_hash, which already sorts its string keys correctly.

How verified

  • New test test_content_hash_symmetric_when_path_and_posix_order_diverge uses a layout that provably triggers the Path-vs-posix divergence (asserts the layouts disagree before comparing hashes), then asserts content_hash(skill_dir) == bundle_content_hash(bundle).
  • RED phase: the new test fails on unpatched main (sha256:ede3ce2c…sha256:a6e11ca3…).
  • GREEN phase: passes after the fix.
  • All 4 existing bundle_content_hash symmetry tests still pass; full test_skills_guard.py + test_skills_hub.py suite: 234 passed.

Closes #53404.


Auto-published by Moonsong via Path B automated pipeline.

@alt-glitch alt-glitch added type/bug Something isn't working tool/skills Skills system (list, view, manage) P3 Low — cosmetic, nice to have labels Jun 27, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Approved

Elegant fix for a platform-dependent hashing bug. Sorting Path objects differs from sorting relative-posix strings (directory separators sort differently), which broke symmetry between content_hash (on disk) and bundle_content_hash (in memory). The fix collects (relative_posix, path) pairs and sorts by the string key. The test layout (lib/helper.py vs lib-helper.py) provably triggers the divergence.


Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression and diagnosis. The ordering bug remains on current main: tools/skills_guard.py:695 sorts Path objects, while tools/skills_hub.py:3692 sorts the relative-path strings used in the in-memory hash.

Problems

  • The implementation needs relocation before it can merge. Commit 51382ac244 moved the hash loop into _content_digest(); current content_hash() is only a wrapper at tools/skills_guard.py:857. Applying the PR's edit to the old function body would conflict and would not update the live helper.

Suggested changes

  • Apply the relative-POSIX ordering change to _content_digest() at tools/skills_guard.py:691. This preserves the PR's intended content_hash() parity and also makes full_content_hash()/scanner cache identity use the same canonical ordering.
  • Retain the divergence regression test in tests/tools/test_skills_hub.py.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@Tranquil-Flow
Tranquil-Flow force-pushed the fix/53404-content-hash-order-rev2 branch from 1a1cf96 to 6bd71c5 Compare July 18, 2026 00:34
@RichardHojunJang

Copy link
Copy Markdown
Contributor

Independent verification against current main (d604141d097eec4a49493ad1eaceb9b2ca1e496d):

  • Confirmed the bug is still present in tools/skills_guard.py::_content_digest on current main (Path ordering vs. the relative POSIX string ordering used by bundle_content_hash).
  • Applied this PR's current diff cleanly in an isolated worktree based on that commit.
  • Ran:
python -m pytest -q tests/tools/test_skills_hub.py tests/tools/test_skills_guard.py

Result: 239 passed in 2.78s on macOS arm64 / Python 3.11.15. git diff --check also passed, and the diff remained limited to tools/skills_guard.py plus its regression test.

This independently reproduces the issue we observed in a live Hermes install and validates the fix on the latest upstream base. Supporting this existing PR rather than opening a duplicate.

@Hyperion5088

Copy link
Copy Markdown

I reproduced this on current main and applied the maintainer-requested relocation to _content_digest() rather than the old content_hash() body.

Verified implementation commit: Hyperion5088@84f39d8

Changes:

Verification:

  • tests/tools/test_skills_guard.py
  • tests/tools/test_pr_6656_regressions.py
  • tests/tools/test_skills_hub.py
  • result: 250 passed
  • Ruff: all checks passed
  • independent review: no security concerns or logic errors

Supporting this canonical PR rather than opening another duplicate. The commit is available to cherry-pick if useful.

@Tranquil-Flow
Tranquil-Flow force-pushed the fix/53404-content-hash-order-rev2 branch from 6bd71c5 to a074126 Compare August 5, 2026 06:34
@redcube2

redcube2 commented Aug 6, 2026

Copy link
Copy Markdown

Confirming this on macOS. Worth flagging because the sibling PRs (#71252, #62313, #62519, #78082) all frame the divergence as a Windows path-separator problem — it reproduces on a plain macOS install with no Windows and no case collision involved.

Environment: Hermes v0.20.0 (2026.8.3), macOS 26.1 (APFS), Python 3.11.15.

Scanning the skill trees on a live install, 2 of 25 directories in ~/.hermes/skills hash differently under sorted(Path) vs sorted(relative-posix-string):

creative
  Path-sort:   baoyu-article-illustrator/references/styles/blueprint.md
  String-sort: baoyu-article-illustrator/references/styles.md

openclaw-imports
  Path-sort:   supabase/SKILL.md
  String-sort: supabase-postgres-best-practices/SKILL.md

Both are pure shared-prefix shapes. The separator / (0x2F) sorts above . (0x2E) and - (0x2D), so a file/directory prefix pair flips order between the two bases. Neither pair differs in case — the original issue text hypothesised case-insensitive filesystems as the driver, but that isn't required to trigger it, which is probably why this keeps getting re-filed as a Windows-only bug.

Agreement between content_hash() and bundle_content_hash() over those 25 directories:

  • current main ordering: 23/25
  • this PR's ordering: 25/25

tests/tools/test_skills_guard.py and tests/tools/test_skills_hub.py pass with the change applied (91 tests).

The first shape is the same baoyu-article-illustrator / references/styles.md pair @Kayhusk reported in #53404 on Linux, so that reproduction isn't Linux-specific either — it's the shared-prefix ordering, independent of platform.

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

Labels

P3 Low — cosmetic, nice to have 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 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.

Bug: content_hash is order-unstable — sorts on Path objects, not the posix string mixed into the digest

7 participants