feat(analyzer): add phase-1 structured skill summaries

[rodboev]

Summary

SkillSpector now recognizes valid *.aisop.json AISOP/AISP bundles during recursive discovery and build-context creation, including skills scanned from installed .claude/... paths or from roots nested under skip-named ancestors such as venv. Recursive discovery still ignores actual skip-dir children such as venv, node_modules, and __pycache__, so vendored bundles in those trees do not become phantom skills. The scan still surfaces one report-only structured summary through structured_summaries, and the report node no longer re-appends its sanitized copy into final graph state.

Refs #130

This follows the phased implementation sketch and AISOP/AISP bundle examples in #130. The review updates keep SSR-1 visible as context instead of modeling it as a LOW finding, and keep discovery keyed to the actual scan root instead of absolute ancestors above it.

Root cause

src/skillspector/multi_skill.py only recognized root SKILL.md layouts or immediate subdirectories that contain SKILL.md or skill.md, so --recursive missed structured subdirectories expressed only through valid AISOP/AISP bundle files. The original detector and sibling build-context walker were both checking absolute path components, which meant ancestors above the scan root could silently suppress valid bundles or even empty components when a skill lived under paths such as .claude/... or venv/.... Once the detector was corrected to be root-relative, detect_skills() also needed to skip actual _SKIP_DIRS children explicitly so vendored bundles inside venv, node_modules, or __pycache__ would not be promoted into skills. The branch also returned structured_summaries from report() even though that channel is additive, so final graph state kept both the analyzer copy and the sanitized report copy.

Diff Notes

• Add src/skillspector/structured_skill.py as the shared detector for valid *.aisop.json AISOP/AISP bundles and their summarized metadata, reading workflow nodes from user.content.functions and AISP resources from user.content.aisp_contract.resources.
• Bound structured bundle metadata walking and fail closed on over-nested bundles, so adversarial functions or resources trees are ignored instead of crashing build_context() or recursive skill detection.
• Reuse that detector in src/skillspector/multi_skill.py so --recursive can treat immediate structured subdirectories as skills while preserving root SKILL.md precedence.
• Skip child directories whose names are in _SKIP_DIRS before structured-bundle detection in src/skillspector/multi_skill.py, so vendored bundle trees do not become phantom skills once discovery is correctly root-relative.
• Evaluate structured-bundle and build-context skip filters relative to skill_dir, which restores detection for installed skills under .claude/... and scan roots nested under skip-named ancestors such as venv.
• Populate structured_skill_context and add structured_summaries as a separate state channel.
• Add src/skillspector/nodes/analyzers/structured_skill_roles.py and register it so the graph emits one SSR-1 structured summary without adding it to ordinary findings.
• Render structured summaries in terminal, markdown, JSON, and SARIF notifications while keeping them out of risk score inputs, meta analyzer inputs, JSON issues, SARIF results, and MCP findings.
• Stop report() from returning structured_summaries into final graph state after sanitization; rendered output still includes the summary, but the additive channel keeps only the analyzer copy.
• Add focused coverage in 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, tests/nodes/test_report.py, and tests/unit/test_cli.py.

Scope

• Phase 1 only. This PR detects valid AISOP/AISP bundles and summarizes their workflow shape through a report-only channel.
• No severity adjustments, no meta analyzer policy changes, and no changes to existing static, behavioral, MCP, or semantic findings.
• No Pi extension changes in extensions/skillspector.ts.
• No full role-aware score downgrades, no malformed-bundle warning surface, and no claim of complete AISOP/AISP spec coverage.

Verification

• python -m pytest tests/nodes/test_build_context.py tests/test_multi_skill.py tests/nodes/test_report.py -q
• python -m pytest 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 tests/nodes/test_report.py tests/unit/test_cli.py -q
• uv run ruff check src/ tests/
• uv run ruff format --check src/ tests/

Notes

This slice stops at detection and report-visible summary context. Role-aware severity adjustments remain follow-up work on top of the new structured context.

[rng1995]

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.

[rodboev]

Pushed 655d925. structured_skill.py now bounds nested workflow and resource walking, and _parse_bundle_path() treats an over-nested bundle as invalid instead of letting it crash the scan. The branch also adds focused regressions for both the build_context() path and recursive child-skill detection.

[rng1995]

The previously reported recursion/DoS issue is resolved: all structured-metadata walkers now enforce a depth cap, parser failures are contained, and the new tests cover fail-closed behavior for functions and resources. However, this head no longer merges cleanly with current main; there are content conflicts in tests/nodes/test_build_context.py and tests/unit/test_cli.py. Please rebase or merge current main and preserve both sets of tests.

[rodboev]

Rebased this branch onto current main and kept both sides of the newer test additions. The structured-skill coverage that was already on the branch is still there in tests/nodes/test_build_context.py and tests/unit/test_cli.py, and the accepted parser and analyzer changes are unchanged.

[rng1995]

[Automated SkillSpector Review]

Re-review: still requesting changes. The prior recursion/DoS guard is resolved and conflicting tests are reconciled. However, SSR-1 is wired as an ordinary LOW finding: it changes risk scoring despite being described as informational, and the meta-analyzer can remove it entirely. Keep the summary report-visible but scoring-neutral and exempt from vulnerability filtering, with LLM and no-LLM regressions.

[rodboev]

Thanks for the review. I pushed a rework that changes SSR-1 from a LOW finding into report-visible structured context:

• The structured workflow summary now flows through structured_summaries, not ordinary vulnerability findings.
• JSON, terminal, markdown, and SARIF notifications still show the structured AISOP/AISP summary.
• Risk scoring, meta filtering, JSON issues, SARIF results, and MCP findings no longer receive SSR-1.
• The existing bundle detection and recursive structured-skill discovery behavior stays in place.

I added coverage for the report-only channel and reran the focused structured-role, report, CLI, registry, build-context, and multi-skill tests, plus ruff check, ruff format check, and invariant enumeration.

[rng1995]

[Automated SkillSpector Review]

Re-review of the SSR-1 rework (head 808f476). The report-only rework does what the previous review asked: SSR-1 now flows through a dedicated structured_summaries state channel, the analyzer returns findings: [], and the summary renders in terminal/markdown/JSON/SARIF notifications while staying out of risk scoring, meta filtering, JSON issues, SARIF results, and MCP findings (verified against meta_analyzer.py, report.py, and mcp_server.py on current main).

Prior-issue resolution checklist

• Unbounded recursion / RecursionError DoS in structured_skill.py walkers (2026-06-27) — Resolved. All walkers enforce _MAX_BUNDLE_NESTING = 128 via _ensure_supported_nesting, and _parse_bundle_path catches RecursionError/ValueError so over-nested bundles fail closed to None. Regression tests with depth-140 trees exist in tests/nodes/test_build_context.py and tests/test_multi_skill.py.
• Merge conflicts in tests/nodes/test_build_context.py / tests/unit/test_cli.py (2026-06-30) — Resolved. PR is mergeable against main and both the structured-skill tests and the upstream test additions are present.
• SSR-1 scored as an ordinary LOW finding (2026-07-09) — Resolved. SSR-1 is emitted only via structured_summaries; the risk score is computed solely from findings. test_report_json_structured_summary_survives_llm_mode and the CLI --no-llm test assert risk_score == 0 / SAFE with the summary present, and the summary payload carries no severity/confidence.
• SSR-1 subject to meta filtering, could disappear in LLM mode (2026-07-09) — Resolved. meta_analyzer consumes and returns only findings/filtered_findings and never reads or writes structured_summaries; report reads summaries directly from state. Presence is asserted with use_llm=True (report node) and via a full CLI --no-llm run.

New blocker

_iter_aisop_files filters on absolute path.parts, so bundle detection silently fails when the scan root sits under a hidden or skip-listed ancestor directory. Both filter checks (part in _SKIP_DIRS over path.parts, and the hidden-dir check over path.parts[:-1]) evaluate every component of the full path, including directories above skill_dir. Scanning a skill at ~/.claude/skills/foo (the canonical installed-skill location) matches .claude and returns no bundles; an ancestor literally named venv or node_modules does the same. Reproduced against this head: a valid bundle is detected in a plain directory but returns None under .../.claude/skills/... and .../venv/.... The failure is a silent false negative in build_context() (no SSR-1 summary) and in multi_skill.detect_skills() — with --recursive, a structured-only child skill under such a path is not recognized as a skill at all, so it is never scanned. For a security scanner that is a detection gap. Fix: filter on components relative to the scan root, e.g. rel_parts = path.relative_to(skill_dir).parts and apply both checks to rel_parts (hidden-dir check on rel_parts[:-1]). Please add a regression test with the skill directory nested under a dot-directory (e.g. tmp_path / ".claude" / "skills" / "foo").

Non-blocking notes

• report() returns structured_summaries (the sanitized list) on a channel declared with an operator.add reducer, so after a graph run the final state contains each summary twice (the analyzer's raw copy plus the sanitized copy). No current consumer reads final-state structured_summaries (CLI and MCP read report_body/findings), but a future consumer would get duplicates and might pick the unsanitized copy. Consider dropping the key from report()'s return or making the channel last-value.
• _first_non_empty in structured_skill.py is a misnomer: it returns the deduplicated union of all candidate lists, not the first non-empty one. Rename (e.g. _merged_string_list) or adjust semantics.
• The one-line reformat in src/skillspector/nodes/analyzers/mcp_least_privilege.py is unrelated to this feature (harmless, likely a formatter artifact of the rebase).