fix(skills_guard): sort paths as strings to match bundle_content_hash ordering - #64659
Closed
waiwaic wants to merge 1 commit into
Closed
fix(skills_guard): sort paths as strings to match bundle_content_hash ordering#64659waiwaic wants to merge 1 commit into
waiwaic wants to merge 1 commit into
Conversation
… ordering
_content_digest() sorted Path objects via sorted(skill_path.rglob('*')),
which uses POSIX component-wise comparison. bundle_content_hash() sorts
string keys (sorted(bundle.files)). When a skill directory contains a file
and a subdirectory sharing a name prefix (e.g. styles.md and styles/x.md),
the two sort orders differ, causing the same skill content to produce
different digests on each side.
Fix by adding a key=lambda that converts each Path to its relative path
string before sorting, so _content_digest stays symmetric with
bundle_content_hash. Tag the docstring with a comment explaining why.
Fixes: hash mismatch between content_hash() and bundle_content_hash()
for skills with files whose names are prefixes of subdirectory names.
Author
|
Closing: this was submitted in error / no longer needed. |
Collaborator
Duplicate of #53448 — both PRs fix the same |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
_content_digest()intools/skills_guard.pysortsPathobjects viasorted(skill_path.rglob("*")), whilebundle_content_hash()intools/skills_hub.pysorts string keys viasorted(bundle.files). When a skill directory contains a file and a subdirectory sharing a name prefix (e.g.styles.mdandstyles/), these two sort orders produce different sequences, which causes the two functions to compute different digests for identical skill content.Root cause
Path.__lt__compares paths component-wise (by POSIX path components), whereas string comparison is character-by-character. Given a filestyles.mdand a subdirectorystyles/x.md:[styles/x.md, styles.md](component
("styles", "x.md")vs("styles.md",)—"styles"is a prefix of"styles.md", so the shorter tuple first element makesstyles/x.mdsort first)["styles.md", "styles/x.md"](
"."(0x2E) <"/"(0x2F), sostyles.mdsorts first)Impact
check_for_skill_updates().test_bundle_content_hash_matches_installed_content_hashdid not catch this because it usesreferences/(noreferences.mdat top level).Fix
Add
key=lambda p: p.relative_to(skill_path).as_posix()to thesorted()call in_content_digest(), so both hash functions sort by relative path strings using the same comparison.Verification
All three scenarios pass after the fix:
styles.md+styles/x.md(bug case)sha256:1f2c9d8...sha256:1f2c9d8...SKILL.md+references/checklist.md(existing test)sha256:d14a9ea...sha256:d14a9ea...a.py+a.cpp+a/b.py(edge case)sha256:1bc0cef...sha256:1bc0cef...