feat(analyzer): add phase-1 structured skill summaries#211
Conversation
Signed-off-by: Rod Boev <rod.boev@gmail.com>
Signed-off-by: Rod Boev <rod.boev@gmail.com>
rng1995
left a comment
There was a problem hiding this comment.
Requesting changes — the phase-1 structured-skill summary is well-built and thoroughly tested, but it introduces an unhandled-crash (DoS) path on adversarial input, which matters because the scanner's whole job is to ingest untrusted skills.
Blocking — unbounded recursion → uncaught RecursionError in the core build_context path.
structured_skill.py's walkers (_extract_function_names, _extract_constraint_anchors._walk, _extract_resource_anchors._walk) recurse per nesting level with no depth bound. A crafted *.aisop.json with deeply nested functions/constraints/resources (~2k levels) raises RecursionError. _parse_bundle_path only wraps json.loads in try/except (OSError, json.JSONDecodeError) and then calls _parse_bundle_payload(...) outside that guard, so the error propagates through extract_structured_skill_context() into build_context() — a core node, not an isolated analyzer — and crashes the whole scan. (json.loads itself tolerates the nesting; the pure-Python walk is what blows the stack. Reproduced at depth≈2000.) _is_structured_skill_bundle() in multi_skill.detect_skills has the same exposure during detection.
Suggested fix: bound the nesting depth in the walkers (return/skip past a sane cap) and/or have _parse_bundle_path treat a bundle that raises during parsing as "not a valid bundle" (e.g. also catch RecursionError/ValueError around _parse_bundle_payload) so a malformed bundle fails closed to None instead of crashing the scan. A regression test with a deeply nested bundle asserting the scan completes would lock this in.
Everything else looks good — the two-message contract validation is appropriately defensive, the SSR-1 finding is correctly LOW/informational, and the build_context/detection/registry wiring and tests are solid. Happy to re-review once the recursion is bounded.
Signed-off-by: Rod Boev <rod.boev@gmail.com>
Signed-off-by: Rod Boev <rod.boev@gmail.com>
6efdb55 to
655d925
Compare
|
Pushed 655d925. |
Summary
SkillSpector currently scans AISOP/AISP bundles as generic directories and never says that a structured workflow format was found or which workflow roles were present. This phase-1 slice teaches recursive discovery and build-context to recognize valid
*.aisop.jsonbundles, then emits one LOW informational summary finding without changing scoring or existing rule behavior.Refs #130
This follows the phased implementation sketch and AISOP/AISP bundle examples in #130.
Root cause
src/skillspector/multi_skill.pyonly recognizes rootSKILL.mdlayouts or immediate subdirectories that containSKILL.mdorskill.md, so--recursivemisses structured subdirectories that are expressed only through valid AISOP/AISP bundle files.src/skillspector/nodes/build_context.pybuilds generic scan context and never classifies those bundles, whilesrc/skillspector/nodes/analyzers/__init__.pyhas no structured-skill analyzer in the registry-driven graph. The bundle parser also needed to follow the issue schema itself, with workflow nodes atuser.content.functionsand constraint strings infunctions.*.constraints, rather than an invented nestedaisop.functionsshape. The result is simple: valid structured bundles can be scanned, but the report has no structured-skill summary surface at all.Diff Notes
src/skillspector/structured_skill.pyas the shared detector for valid*.aisop.jsonAISOP/AISP bundles and their summarized metadata, reading workflow nodes fromuser.content.functionsand AISP resources fromuser.content.aisp_contract.resources.src/skillspector/structured_skill.pyand fail closed on over-nested bundles, so adversarialfunctionsorresourcestrees are ignored instead of crashingbuild_context()or recursive skill detection.src/skillspector/multi_skill.pyso--recursivecan treat immediate structured subdirectories as skills while preserving rootSKILL.mdprecedence.structured_skill_contextinsrc/skillspector/nodes/build_context.pyandsrc/skillspector/state.pywithout changing the public report schema.src/skillspector/nodes/analyzers/structured_skill_roles.pyand register it insrc/skillspector/nodes/analyzers/__init__.pyso the graph emits one LOWSSR-1structured summary finding.tests/test_multi_skill.py,tests/nodes/test_build_context.py,tests/nodes/analyzers/test_registry.py,tests/nodes/analyzers/test_structured_skill_roles.py, andtests/unit/test_cli.py.Scope
meta_analyzerchanges, and no changes to existing static, behavioral, MCP, or semantic findings.Findingfields.extensions/skillspector.ts.Verification
python -m pytest tests/test_multi_skill.py tests/nodes/test_build_context.py tests/nodes/analyzers/test_structured_skill_roles.py -q- pass,37 passed, 1 warninguv run ruff check src/ tests/- passuv run ruff format --check src/ tests/- passLint & Test (Python 3.12),Lint & Test (Python 3.13), andDCO Check- pending rerun after pushNotes