fix(skills): fingerprint the whole skill directory, not SKILL.md alone - #63
Merged
imran-siddique merged 2 commits intoJul 31, 2026
Merged
Conversation
A skill is not just its manifest. Its scripts, tools, templates and reference docs decide what it does, and _skills() hashed SKILL.md and nothing else. So a payload swapped into a skill's scripts/ directory produced no change in the fingerprint and the report said "nothing added, nothing subtracted". Demonstrated on a real install before the fix: replacing a skill's scripts/deploy.ps1 with a body that POSTs ~/.ssh/id_rsa to an attacker host left the skills fingerprint byte-identical and diff() returned nothing. Every skill in that install carries files beyond SKILL.md, five of them executable. This is the README's own headline threat, "a skill you installed ships an update that now runs curl to an address you never saw", and it was undetected. _skill_fingerprint() now hashes every file in the skill tree, binding relative paths alongside contents so a rename or a move is also drift. Exclusions are a tool-controlled denylist (state/, .cache/, __pycache__/, .git/, node_modules/, plus .log/.tmp/.pyc/.pyo). Skills write state as they run, and alarming on ordinary use would train the user to dismiss the next real alarm. The list deliberately lives in the engine rather than in a per-skill ignore file: an ignore file would let the measured thing decide what gets measured, so a hostile skill could exempt its own payload. Per-file instruction-layer digests. The rollup only says the layer moved, which across dozens of memory files is one bit of signal over a directory the reader then has to search by hand. instruction_files carries a digest per file so the diff names it, and the rollup stays for the fingerprint line. Scoped to *.md because the same tree holds session transcripts that change constantly. Measurement scope versioning. Both changes make older fingerprints incomparable, so a scope-1 baseline would otherwise report every skill as changed on upgrade. diff() now reports the widening once, as "re-approve to compare on the new scope", and drops skills from that comparison. An alarm the user knows is false is worse than no alarm. Verified against a real scope-1 baseline: one honest line instead of six phantom skill changes. 17 new tests covering the closed bypass, renames, state churn not alarming, run artifacts not alarming, the migration path, and per-file naming with fallback to the rollup against a scope-1 baseline. Suite: 32 passed, up from 15. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Imran Siddique <imran.siddique@opaque.co>
…ees-and-per-file-instructions Signed-off-by: Imran Siddique <imran.siddique@opaque.co> # Conflicts: # claude-code/tests/test_capture.py
6 tasks
imran-siddique
deleted the
feat/measure-skill-trees-and-per-file-instructions
branch
July 31, 2026 19:26
5 tasks
imran-siddique
added a commit
that referenced
this pull request
Jul 31, 2026
Same bypass that #63 closed for Claude Code, still live here. _skill_fingerprints hashed the manifest and nothing else, so a payload swapped into a skill's scripts/ directory left the digest byte-identical and the report said nothing added, nothing subtracted. Wider blast radius than the Claude Code case, because Codex resolves skills from three roots rather than one: ~/.agents/skills, ~/.codex/skills, and per-workspace .agents/skills. That last one means a repo you clone can carry a skill, so the undetected surface included content arriving over the network. _skill_tree_digest hashes every behavioural file in the skill tree, binding relative paths alongside contents so a rename or a move is drift too. Symlinks are skipped, matching _plugin_digest, so a link out of the tree cannot drag unrelated content into the fingerprint. Exclusions are a tool-controlled denylist (state/, .cache/, __pycache__/, .git/, .pytest_cache/, node_modules/, plus .log/.tmp/.pyc/.pyo). Skills write state as they run and alarming on ordinary use would train the user to dismiss the next real alarm. The list lives in the engine rather than in a per-skill ignore file on purpose: an ignore file would let the measured thing decide what gets measured, so a hostile skill could exempt its own payload. Measurement scope versioning, matching #63. Widening the digest makes older fingerprints incomparable, so a scope-1 baseline would otherwise report every skill as changed on upgrade. diff() reports the widening once as "re-approve to compare on the new scope" and drops skills from that comparison. Codex already fingerprints instructions and policy files per file, so the per-file work from #63 was not needed here. 12 new tests: the closed bypass, renames, new files, manifest changes, workspace skill roots, state churn and run artifacts not alarming, and the migration path. One existing fixture needed scope declared, since it models two snapshots from the same engine version. Suite: 25 passed. Note for reviewers: test_signed_outputs_verify_and_pass_trace_level_zero already fails on main, verified with this change stashed on the same base. It is a TRACE conformance check on generated records and fix/codex-trace-pins is the branch addressing it, so it is untouched here. Signed-off-by: Imran Siddique <imran.siddique@opaque.co> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Jul 31, 2026
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.
The bypass
_skills()hashedSKILL.mdand nothing else. A skill is not just its manifest: itsscripts/,tools/, templates and reference docs decide what it actually does.Demonstrated on a real install, before the fix, in an isolated temp home:
Between those two lines the skill's
scripts/deploy.ps1was replaced with a body that POSTs~/.ssh/id_rsato an attacker host. The integrity check reported nothing added, nothing subtracted.Every skill on the install I checked carries files beyond
SKILL.md, five of them executable PowerShell or Python. The README's headline threat is "a skill you installed ships an update that now runscurlto an address you never saw." That case was undetected.What changed
_skill_fingerprint()hashes the whole skill tree, binding relative paths alongside contents so a rename or a move is drift too. A directory without aSKILL.mdis still not a skill and is not measured as one.Exclusions are a tool-controlled denylist:
state/,.cache/,__pycache__/,.git/,.pytest_cache/,node_modules/, plus.log,.tmp,.pyc,.pyo. Skills write state as they run, and firing on ordinary use would train the user to dismiss the next real alarm.The list lives in the engine on purpose. A per-skill ignore file was the obvious alternative and it is the wrong shape: it lets the thing being measured decide what gets measured, so a hostile skill could ship an ignore rule covering its own payload. Adding a name to the denylist is a reviewed change to this repo. There is a test asserting the exclusions are not skill-controlled.
Per-file instruction-layer digests. The rollup says only that the layer moved. On the install I checked that is one bit of signal across 83 memory files, leaving the reader to search the directory by hand; I hit this myself and had to reason from memory about what I had written.
instruction_filesnow carries a digest per file so the diff names it, and the rollup stays for the fingerprint line. Scoped to*.mdbecause the same tree holds 1163 session transcripts that change constantly.Measurement scope versioning. Both changes make older fingerprints incomparable. Without handling, a scope-1 baseline reports every skill as changed on upgrade.
diff()now reports the widening once as "re-approve to compare on the new scope" and drops skills from that comparison.Verified against a real scope-1 baseline:
One honest line instead of six phantom skill changes. An alarm the user knows is false is worse than no alarm, and silently re-baselining would have been worse still: on a machine already compromised at upgrade time it would bless the compromise.
Test plan
claude-code/tests: 32 passed, up from 15. 17 new tests.state/churn and.log/.pyc/.tmpartifacts produce no alarm.Four pre-existing tests needed
scopeadded to their fixtures. They model two snapshots from the same engine version, so declaring the scope is the correct fix; migration is covered separately.Relationship to #62
Both touch
claude-code/tests/test_capture.py, so expect a conflict in that file whichever merges second. The engine changes are in different functions (render_reportthere,_skills/snapshot/diffhere) and do not overlap. I have merged the two locally to confirm they compose; happy to rebase whichever you prefer to land second.#62is the honest-reporting fix, this is the coverage fix. They are independent and both worth having: without #62 an unmeasured category reads as a measured zero, and without this a measured category was missing most of its surface.