feat: implement dynamic analyzer discovery and risk score validation

[umran666]

Resolves TODO A.2.1-A.2.4 (Analyzer discovery) and TODO A.3.2 (Risk score assertion).

[rng1995]

The move to dynamic analyzer discovery is reasonable and removes a lot of boilerplate, and wiring requires_api_key / is_available() gating into create_graph matches the intent of the TODO it resolves. A few issues, one of which I'd consider blocking for a security tool.

1. Import failures are silently swallowed (blocking). In _discover_analyzers, a module that fails to import is caught with except Exception and logged at logger.debug(...), then skipped. Since the default log level is WARNING, a broken analyzer (syntax error, bad import, refactor mistake) silently disappears from every scan with no visible signal — the scan still completes and can report a low/zero risk while an entire detection category isn't running. That's a meaningful regression from the previous explicit-import behaviour, which failed loudly. Please log this at WARNING/ERROR (and ideally surface it in output) so a dropped analyzer is never invisible. It would also help to distinguish "module isn't an analyzer" (no ANALYZER_ID) from "an analyzer failed to import".

2. Confirm the gating attributes actually exist. The new skip logic reads getattr(mod, "is_available", None) and getattr(mod, "requires_api_key", False). The static analyzer modules don't define these (correctly — they need no key), but if the goal is to skip the LLM/semantic analyzers when no key is present, those modules must declare requires_api_key = True; otherwise they default to False and are always wired, making the gating a no-op. Please confirm the LLM-dependent analyzers actually set this.

3. Edge case: if discovery/gating ends up skipping every analyzer, wired_analyzers is empty and nothing connects to meta_analyzer. Worth a guard or at least a warning.

Non-blocking nits:

• Discovery order now depends on pkgutil.iter_modules (filesystem order) rather than the curated list, and the registry test was loosened to a set comparison. Fine if analyzer order is genuinely irrelevant (parallel fan-out), but a comment noting that ordering is no longer guaranteed would help future readers.
• Several of the new lines in graph.py carry trailing whitespace, which will likely trip ruff/lint.

Happy to re-review once import failures are visible and the requires_api_key wiring is confirmed.

[umran666]

Thanks for the review. I've pushed a commit addressing all the feedback:

1. Explicitly catching ImportError in __init__.py and logging at ERROR level so broken analyzers don't fail silently.
2. Added requires_api_key = True to the LLM/semantic analyzers.
3. Added a warning guard in graph.py if wired_analyzers ends up empty.
4. Fixed the trailing whitespace and added a comment noting that discovery order is filesystem-dependent.

[rng1995]

[Automated SkillSpector Review]

Re-review: blockers resolved — approving.

1. Import failures are no longer silent: _discover_analyzers catches ImportError/Exception and logs at ERROR (distinguishing import failure from "not an analyzer").
2. requires_api_key = True is now declared on the three semantic analyzers and create_graph gates them on is_llm_available(), so the skip logic is a real gate rather than a no-op.
3. An empty-wired_analyzers warning guard was added.

Non-blocking: the inserted module-level statements leave a single blank line before def create_graph() (in graph.py) and before def _discover_analyzers() (in __init__.py); ruff format wants two, so please run make format to avoid a CI format-check failure.

[rng1995]

Re-review — approving. The only change since my last approval is a style: run ruff format commit (graph.py, analyzers/__init__.py); the dynamic-analyzer-discovery logic I previously approved is unchanged.

Heads-up (non-blocking for the review, but blocks merge): GitHub currently reports this branch as conflicting with main (mergeable_state: dirty) — please rebase/resolve before merging.

[umran666]

Hey @rng1995 just checking in. All review feedback has been addressed and the latest push includes the ruff format fix. Is this good to merge, or is there anything else you'd like me to change?

[rng1995]

@umran666 - Please resolve merge conflicts, so that I can merge

[umran666]

@umran666 - Please resolve merge conflicts, so that I can merge

@rng1995 Rebased on latest main and resolved merge conflicts.Ready to merge.

[umran666]

@rng1995 Rebased again on latest main this is the third rebase since approval. The upstream churn keeps creating conflicts on __init__.py because the hardcoded analyzer list keeps growing (structured_skill_roles was the latest), which is exactly the problem this PR eliminates.

[rng1995]

[SkillSpector Review]

Re-review approved. The rebased current head preserves the previously approved dynamic-discovery behavior: analyzer import failures are visible, semantic analyzers declare their LLM requirement, and the empty-registry case is warned. Current-main analyzers are included and all required checks pass.

[rng1995]

@umran666 - Thanks for your patience and contribution. CI looks good but merge conflicts need to be fixed. Please fix it in order to merge the PR.

[umran666]

@rng1995 Thanks for the approval! I've rebased on top of the latest main (resolving the conflict with the new artifact_integrity analyzer), verified test_registry.py passes 100%, and force-pushed.

The branch is completely conflict-free and ready to merge.

[rng1995]

[SkillSpector Review]\n\nApproved after re-reviewing head . The rebase resolves the prior conflict, and dynamic analyzer discovery still includes artifact-integrity analyzers without weakening existing coverage. Current required checks pass.