Skip to content

fix(analyzers): surface analyzer modules dropped at registry load time - #591

Open
udsy19 wants to merge 1 commit into
NVIDIA:mainfrom
udsy19:fix/analyzer-registry-load-error
Open

udsy19 wants to merge 1 commit into
NVIDIA:mainfrom
udsy19:fix/analyzer-registry-load-error

Conversation

@udsy19

@udsy19 udsy19 commented Sep 19, 2026

Copy link
Copy Markdown

Fixes #590

What was wrong

_discover_analyzers() (src/skillspector/nodes/analyzers/__init__.py) imports every module under nodes/analyzers/ and registers the ones that expose ANALYZER_ID/node(). If a module raises during import (missing optional dependency, or any other exception), the exception is logged at ERROR and the loop continues — the module is simply never added to ANALYZER_NODE_IDS.

graph.py:59 only wires a graph node for IDs present in ANALYZER_NODE_IDS. A module dropped at import time therefore gets no node, never runs, and emits no inspection-ledger event. analysis_completeness (inspection_ledger.py) is derived entirely from ledger events and analyzer_status_events, both of which require an analyzer to have actually executed — so a dropped analyzer leaves no trace anywhere for completeness to detect. The report comes back status: "complete", is_complete: True, recommendation SAFE, and --fail-on-incomplete exits 0, even though one whole analyzer category never ran.

Who reaches this / entry point: every skillspector scan invocation (CLI) and every graph run goes through create_graph() -> ANALYZER_NODE_IDS, so this is on the main scan path, not an edge case. Triggered by: any environment where one analyzer's optional dependency is missing or broken at import time (e.g. a yara/network-client import failure for static_patterns_data_exfiltration) — reproduced by injecting a single ImportError for that module and re-running discovery in-process: registered analyzers drop from 27 to 26 with no surface (ANALYZER_LOAD_ERRORS/FAILED_ANALYZERS/etc.) for any consumer to tell 27 from 26.

This is the module-load-time twin of #554/#557: #554 is a single custom YARA rule file dropped silently while static_yara still reports completed/SAFE; #557 fixes it with a PARTIAL ledger event. _discover_analyzers() has the identical fail-open shape one level up (for a whole analyzer module) and is untouched by #557, which is scoped to static_yara.py.

Fix

  • ANALYZER_LOAD_ERRORS: dict[str, str] records module_name -> error for both exception paths in _discover_analyzers(), exported via __all__.
  • New LedgerReason.ANALYZER_LOAD_ERROR with a dedicated message (reusing ANALYZER_RUNTIME_ERROR's "failed after beginning applicable work" would be wrong here — the module never began any work).
  • finalize_inspection_ledger() emits one PARTIAL/SYSTEM-record ledger event per entry in ANALYZER_LOAD_ERRORS, via the same record_type=LedgerRecordType.SYSTEM pattern already used there for the finding-output-limit case. This folds into ledger_exceptions the same way the existing SYSTEM events do, which degrades analysis_completeness["status"] to "partial".
  • Deliberately PARTIAL, not FAILED: cli.py exits unconditionally with code 2 whenever execution_successful is False, regardless of --fail-on-incomplete. The scan itself executes successfully with the other analyzers; a dropped analyzer is a coverage gap, which is what PARTIAL + is_complete: False communicates. #557 made the identical choice for the YARA-rule case for the same reason.

I intentionally did not touch the separate if analyzer_id and callable(node_func): ... (no else) branch a few lines down. nodes/analyzers/ also contains legitimate non-analyzer helper modules (common.py, osv_client.py, pattern_defaults.py, static_runner.py, whitespace_padding.py) that pkgutil.iter_modules picks up alongside real analyzers and that correctly have no ANALYZER_ID/node() by design — logging those as errors would misclassify five legitimate files as failures on every single scan.

Testing

$ PYTHONPATH=src python -m pytest tests/nodes/test_finalize_inspection_ledger.py tests/nodes/analyzers/test_registry.py -q
50 passed

Negative control, reverting only the three production files (inspection_ledger.py, nodes/analyzers/__init__.py, nodes/finalize_inspection_ledger.py) and keeping the new tests:

$ PYTHONPATH=src python -m pytest tests/nodes/test_finalize_inspection_ledger.py::test_analyzer_registry_load_failure_marks_scan_incomplete tests/nodes/test_finalize_inspection_ledger.py::test_no_analyzer_load_errors_leaves_completeness_untouched -q
1 failed, 1 passed

restoring the fix:

$ PYTHONPATH=src python -m pytest tests/nodes/test_finalize_inspection_ledger.py::test_analyzer_registry_load_failure_marks_scan_incomplete tests/nodes/test_finalize_inspection_ledger.py::test_no_analyzer_load_errors_leaves_completeness_untouched -q
2 passed

ruff check, ruff format --check, and mypy all clean on the three changed source files.

Impact: security-boundary

Exploitability: hardening-only — this is defence in depth against a silently-clean scan, not a live vulnerability. The missing/broken analyzer dependency is not attacker-controlled: it depends on what an operator has installed in their own environment, so an attacker with no local control over the SkillSpector deployment cannot force this path. It is exactly the same reachability shape as #554/#557, which are also public fixes for the same "consumer trusted a SAFE verdict without checking whether the detector actually ran" class.

_discover_analyzers() logs an ImportError/Exception at ERROR level and
continues when an analyzer module fails to import, but the module is then
never registered in ANALYZER_NODE_IDS. graph.py only ever wires nodes for
IDs in that list, so the dropped analyzer gets no graph node, runs no
node(), and emits no inspection-ledger event of its own. Nothing in
analysis_completeness can see the gap: the scan reports status "complete"
and the recommendation stays SAFE having never run that analyzer.

Record each load failure in a new ANALYZER_LOAD_ERRORS dict, and have
finalize_inspection_ledger emit one SYSTEM/PARTIAL ledger event per entry
(reason ANALYZER_LOAD_ERROR), following the same SYSTEM-record convention
finalize_inspection_ledger.py already uses for the finding-output-limit
case. This degrades analysis_completeness to "partial" without flipping
execution_successful to False, so it does not trip cli.py's unconditional
exit(2) for a real crash - a missing analyzer is a coverage gap, not an
execution failure.

Regression test simulates a load failure by monkeypatching
ANALYZER_LOAD_ERRORS and asserts the scan reports partial/incomplete
instead of clean; a companion test asserts the unaffected case is
untouched.

Signed-off-by: Udaya Tejas <udayatejas2004@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Analyzer modules dropped at import time are invisible to analysis_completeness

1 participant