Skip to content

fix(skills): stabilize content_hash ordering for shared-prefix paths - #53877

Closed
lucifer0114 wants to merge 1 commit into
NousResearch:mainfrom
lucifer0114:fix/skills-content-hash-ordering
Closed

fix(skills): stabilize content_hash ordering for shared-prefix paths#53877
lucifer0114 wants to merge 1 commit into
NousResearch:mainfrom
lucifer0114:fix/skills-content-hash-ordering

Conversation

@lucifer0114

Copy link
Copy Markdown

Summary

  • make content_hash() sort on the same relative POSIX path string that it hashes
  • keep on-disk hashing symmetric with bundle_content_hash()
  • add a regression test for file/dir shared-prefix collisions

Why

content_hash() currently sorts Path objects but hashes the relative POSIX string. Those two orderings are not guaranteed to match. When a skill contains both a file and a nested directory sharing a prefix (for example references/styles.md and references/styles/blueprint.md), unchanged content can hash in a different byte order and trigger false update reports.

Test plan

  • run scripts/run_tests.sh tests/tools/test_skills_hub.py
  • verify the new regression test passes:
    • test_bundle_content_hash_matches_on_disk_when_file_and_dir_share_prefix

Fixes #53404
Related to #41176

@lucifer0114

Copy link
Copy Markdown
Author

Boundary note for reviewers:

This PR is intentionally scoped only to the content_hash() ordering bug from #53404.

What it includes:

  • make tools.skills_guard.content_hash() sort on the same relative POSIX path string that is mixed into the digest
  • add a regression test for the shared-prefix case (references/styles.md vs references/styles/blueprint.md)

What it intentionally does not include:

I split those concerns on purpose so this PR can stay narrowly reviewable as the hash-ordering fix, while #41199 / follow-up work can handle the lock-state side separately.

I also validated this split on a live downstream checkout after updating to current origin/main: targeted suite passed with

scripts/run_tests.sh tests/tools/test_skills_hub.py

Result: 154 passed, 0 failed.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have tool/skills Skills system (list, view, manage) duplicate This issue or pull request already exists labels Jun 28, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #53448 -- both Fix #53404 by rewriting content_hash() in tools/skills_guard.py to collect (relative_posix, path) pairs and sort by the string key (mirroring bundle_content_hash), with the same shared-prefix regression test. The diffs are semantically identical; #53448 is the earlier-triaged entrant for #53404. Flagging as a duplicate for the maintainer; not closing -- a human can pick which to merge.

@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

Content hash ordering fix for shared-prefix paths (2 files). Regression test + implementation:

  • Test test_bundle_content_hash_matches_on_disk_when_file_and_dir_share_prefix catches the exact bug scenario (file vs dir prefix collision)
  • Fix uses deterministic sorted() ordering for rglob results
  • Clean, minimal change to the hash computation

Good regression coverage for a subtle ordering bug.

@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

Content hash ordering fix for shared-prefix paths (2 files). Regression test + implementation:

  • Test test_bundle_content_hash_matches_on_disk_when_file_and_dir_share_prefix catches the exact bug scenario (file vs dir prefix collision)
  • Fix uses deterministic sorted() ordering for rglob results by relative POSIX path strings
  • Clean, minimal change to the hash computation

The key insight: sorting Path objects directly can disagree with pure string ordering when a file and nested directory share a prefix (e.g. references/styles.md vs references/styles/blueprint.md). Sorting by the .as_posix() string ensures consistency with bundle_content_hash().

Good regression coverage for a subtle ordering bug.


Reviewed by Hermes Agent

@lucifer0114

Copy link
Copy Markdown
Author

Closing as a duplicate of #53448. I compared both PRs directly: they implement the same content_hash() ordering fix in tools/skills_guard.py and both add equivalent regression coverage for #53404. #53448 is the earlier entrant, so I’m consolidating discussion there to avoid splitting review attention.

This does not affect #53881, which is a separate stale-lock self-heal fix.

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 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

3 participants