Conversation
_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>
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
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.
Fixes #590
What was wrong
_discover_analyzers()(src/skillspector/nodes/analyzers/__init__.py) imports every module undernodes/analyzers/and registers the ones that exposeANALYZER_ID/node(). If a module raises during import (missing optional dependency, or any other exception), the exception is logged atERRORand the loop continues — the module is simply never added toANALYZER_NODE_IDS.graph.py:59only wires a graph node for IDs present inANALYZER_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 andanalyzer_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 backstatus: "complete",is_complete: True, recommendation SAFE, and--fail-on-incompleteexits 0, even though one whole analyzer category never ran.Who reaches this / entry point: every
skillspector scaninvocation (CLI) and every graph run goes throughcreate_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. ayara/network-client import failure forstatic_patterns_data_exfiltration) — reproduced by injecting a singleImportErrorfor 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_yarastill reportscompleted/SAFE; #557 fixes it with aPARTIALledger 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 tostatic_yara.py.Fix
ANALYZER_LOAD_ERRORS: dict[str, str]recordsmodule_name -> errorfor both exception paths in_discover_analyzers(), exported via__all__.LedgerReason.ANALYZER_LOAD_ERRORwith a dedicated message (reusingANALYZER_RUNTIME_ERROR's "failed after beginning applicable work" would be wrong here — the module never began any work).finalize_inspection_ledger()emits onePARTIAL/SYSTEM-record ledger event per entry inANALYZER_LOAD_ERRORS, via the samerecord_type=LedgerRecordType.SYSTEMpattern already used there for the finding-output-limit case. This folds intoledger_exceptionsthe same way the existing SYSTEM events do, which degradesanalysis_completeness["status"]to"partial".PARTIAL, notFAILED:cli.pyexits unconditionally with code 2 wheneverexecution_successfulisFalse, regardless of--fail-on-incomplete. The scan itself executes successfully with the other analyzers; a dropped analyzer is a coverage gap, which is whatPARTIAL+is_complete: Falsecommunicates.#557made 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) thatpkgutil.iter_modulespicks up alongside real analyzers and that correctly have noANALYZER_ID/node()by design — logging those as errors would misclassify five legitimate files as failures on every single scan.Testing
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:restoring the fix:
ruff check,ruff format --check, andmypyall 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.