fix(codex): digest the whole skill directory, not SKILL.md alone - #65
Merged
Conversation
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. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Imran Siddique <imran.siddique@opaque.co>
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, still live here
#63 closed this for Claude Code. The Codex engine had the identical hole:
_skill_fingerprintshashedSKILL.mdand nothing else, so a payload swapped into a skill'sscripts/directory left the digest byte-identical and the report said "nothing added, nothing subtracted".Wider blast radius than the Claude Code case. Codex resolves skills from three roots rather than one:
~/.agents/skills~/.codex/skills.agents/skillsThat last one means a repo you clone can carry a skill, so the undetected surface included content arriving over the network rather than only content already on your machine. There is a test covering the workspace root specifically.
What changed
_skill_tree_digesthashes 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 the existing_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 thing being measured decide what gets measured, so a hostile skill could ship a rule exempting its own payload. A test asserts the denylist is engine-side.
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. An alarm the user knows is false is worse than no alarm.Not needed here: Codex already fingerprints instructions and policy files per file, unlike Claude Code before #63, so that half of #63 has no analogue.
Test plan
plugins/agentrust-codex/tests: 25 passed, 12 new.scripts/detected; manifest change still detected; new file detected; rename detected..agents/skills) covered, since those can arrive by clone.state/churn and.log/.pyc/.tmpartifacts produce no alarm.One existing fixture needed
scopedeclared, since it models two snapshots from the same engine version.Pre-existing failure, not from this PR.
test_signed_outputs_verify_and_pass_trace_level_zerofails onmain; I verified it failing with this change stashed on the same base. It is a TRACE conformance check on generated records andfix/codex-trace-pinsis the branch addressing it, so it is untouched here.Why this shape, for the next surface
Three engines now carry the same skill-digest logic written three times, and this is the second time the same bug had to be fixed independently. Extracting a shared core is the next piece of work, and it is what makes a Copilot integration a path list rather than a fourth thousand-line engine. Copilot Agent Skills use the same
SKILL.mdplus supporting-folder shape, so this exact bypass applies there too.