feat(skills): add opt-in AST deep diagnostics (salvage of #30918) - #31198
Merged
Merged
Conversation
Add opt-in AST diagnostics for skill review without making Skills Guard stricter by default. - Add hermes skills inspect --ast-deep to scan fetched skill bundles before installation - Add hermes skills audit --deep to scan already-installed hub skills - Keep AST analysis in tools/skills_ast_audit.py, separate from tools/skills_guard.py - Label output as diagnostic hints, not security verdicts - Cover dynamic import/access patterns: importlib, __import__(computed), getattr(computed), and __dict__[computed] This follows the maintainer guidance from closed PR #7436: useful AST-level analysis belongs in an opt-in diagnostic path, not in Skills Guard's default heuristic scan.
Trim ~600 LOC off the original contribution while keeping the same operator-facing surface and detection coverage. - Collapse three entry points (file / dir / bundle) into one ast_scan_path(path) that handles both files and directories. - Drop AstFinding dataclass + severity field — replaced with plain (file, line, pattern_id, description) tuples. Severity ordering was display-only for a diagnostic that explicitly disclaims security verdicts, so the field added bookkeeping without earning its place. - Replace Rich-markup formatter with plain text grouped by file. - Drop the 'inspect --ast-deep' surface — same scanner, same output as 'audit --deep', single CLI entry is enough. Operators audit after install; pre-install inspection signal isn't worth the second surface. - Trim test file to the cases that earn their place: bypass payload, syntax error survival, RecursionError survival, false-positive guard (importer lookalike), literal-arg false-positive guard, non-.py ignored, directory recursion + cache-dir skipping, missing-path, getattr/__dict__ detection, formatter empty + populated. Net: tools/skills_ast_audit.py 353 -> 133 LOC, tests/tools/test_skills_ast_audit.py 299 -> 103 LOC, full diff +704/-12 -> +264/-6. No change to tools/skills_guard.py — Skills Guard verdicts remain untouched per SECURITY.md §2.4.
Contributor
🔎 Lint report:
|
alt-glitch
added
type/feature
1 task
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
3 participants
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.
Salvages @Tranquil-Flow's PR #30918 onto current main, then trims it from +704/-12 to +264/-6.
Summary
hermes skills audit [name] --deepnow runs an opt-in AST pass over installed hub skills' Python files and prints anyimportlib/ dynamic-import / dynamic-getattr /__dict__[<computed>]patterns it finds. Diagnostic only — Skills Guard verdicts are unchanged (tools/skills_guard.pyhas zero diff). Matches the direction laid out when closing #7436.Changes
tools/skills_ast_audit.py(new, 133 LOC): singleast_scan_path(path)handles file or directory; plain tuple findings;format_ast_report()returns plain text.hermes_cli/skills_hub.py:do_audit(..., deep=False)runs the scan after the existing Skills Guard scan when--deepis passed.hermes_cli/main.py:skills audit --deepargparse flag.hermes_cli/commands.py: registerauditas a/skillssubcommand.tests/tools/test_skills_ast_audit.py(103 LOC): bypass payload, syntax-error / RecursionError survival, false-positive guards, dir recursion + ignored dirs, formatter.What changed vs the original PR
inspect --ast-deepsurface — same scanner, same output, redundant. Operators audit after install.ast_scan_path.AstFindingdataclass + severity field — display theater for a diagnostic that explicitly disclaims security verdicts.Validation
--deep--deeppytest tests/tools/test_skills_ast_audit.py tests/tools/test_skills_guard.py tests/hermes_cli/test_skills_hub.py -q→ 91 passed.HERMES_HOME=tmp hermes skills audit evil-skill --deepagainst an installed skill containing the exact [Bug] Hermes Agent Skills Guard Complete Bypass via Dynamic Import and String Construction — Silent Environment Variable Exfiltration #7072 bypass payload — Skills Guard still reports SAFE; AST section flags all three patterns. Without--deep, output is byte-identical to pre-PR.Authorship
feat(skills): add opt-in AST deep diagnostics): @Tranquil-Flow, cherry-picked from feat(skills): add opt-in AST diagnostics #30918.refactor(skills): slim AST diagnostic to single entry point): @teknium1.Closes #30918.
Infographic