Skip to content

fix(skills): sort disk hashes by relative path - #67300

Closed
creatornader wants to merge 1 commit into
NousResearch:mainfrom
creatornader:fix/skill-hash-path-order-20260719
Closed

fix(skills): sort disk hashes by relative path#67300
creatornader wants to merge 1 commit into
NousResearch:mainfrom
creatornader:fix/skill-hash-path-order-20260719

Conversation

@creatornader

Copy link
Copy Markdown

What

  • Sort installed skill files by their relative POSIX paths before hashing.
  • Add a regression for a sibling file and directory that share a prefix.

Why

Path object ordering is component-aware, while bundle hashing sorts path strings. A bundle containing references/styles.md and references/styles/minimal.md therefore received different disk and bundle hashes even when every byte matched. Hermes then reported the skill as update_available after each successful update.

Verification

  • scripts/run_tests.sh tests/tools/test_skills_hub.py
  • scripts/run_tests.sh tests/tools/test_skills_guard.py tests/tools/test_pr_6656_regressions.py
  • Ruff on the two changed files
  • Reproduced against the installed official baoyu-article-illustrator bundle: disk and bundle hashes both resolve to sha256:15694c74d4ec835e

Tested on macOS 26.5.1 with Python 3.11.13.

@alt-glitch alt-glitch added type/bug Something isn't working tool/skills Skills system (list, view, manage) P3 Low — cosmetic, nice to have duplicate This issue or pull request already exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 19, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #53448 — both change tools/skills_guard.py to sort the digest inputs by relative POSIX path, restoring disk/bundle hash symmetry. The test layouts differ, but the production mechanism is the same.

@creatornader
creatornader force-pushed the fix/skill-hash-path-order-20260719 branch from a5756f1 to f11b310 Compare July 19, 2026 04:07
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression. The premise is confirmed on the canonical checkout: tools/skills_guard.py:695 sorts Path objects, while tools/skills_hub.py:3694 sorts the relative-path strings used in the in-memory hash. tools/skills_hub.py:3645 stores the disk hash and tools/skills_hub.py:3755-3757 compares it with the bundle hash during update checks, so this can produce a false update_available result.

The PR updates the live shared helper, _content_digest() (tools/skills_guard.py:691), which also keeps content_hash() (tools/skills_guard.py:857) and full_content_hash() (tools/skills_guard.py:705) internally canonical. Its sibling-prefix regression extends the existing symmetry contract at tests/tools/test_skills_hub.py:1150 without introducing a snapshot-style test. No additional sibling disk/bundle comparison path was found; tools/skills_sync.py:269-275 delegates to content_hash() when available.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 19, 2026
@creatornader

Copy link
Copy Markdown
Author

Confirmed. #53448 now applies the same relative-POSIX ordering change in _content_digest() and includes an equivalent regression that proves disk and bundle hash symmetry. The production fix is the same, so I am closing this PR to avoid duplicate review. This cleanly attributed branch remains available as a fallback if #53448 cannot clear its current contributor-attribution gate.

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

Labels

duplicate This issue or pull request already exists 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.

3 participants