diff --git a/src/skillspector/cli.py b/src/skillspector/cli.py index dd71792f7..5165a1f7b 100644 --- a/src/skillspector/cli.py +++ b/src/skillspector/cli.py @@ -1001,6 +1001,40 @@ def _ledger_work_identity(entry: dict[str, object]) -> str: return f"{record_value}:{entry.get('phase', '')}" +def _ledger_work_identities(value: object) -> dict[str, str]: + """Map each child ledger row's own work ID to the identity it was built from. + + A status ``planned_work`` target carries only the child's work ID, path and + range, not the identity behind it. Rows whose identity is not the + analyzer's own -- the static_yara rule-set row is ``rule_set:static``, not + ``static_yara`` -- must be re-scoped with that same identity in the status + path, or the status target and its ledger row get different scoped IDs + and the target is dropped as unretained. + """ + identities: dict[str, str] = {} + for event in _coerce_dict_list(value): + work_id = event.get("work_id") + if isinstance(work_id, str) and work_id: + identities[work_id] = _ledger_work_identity(event) + return identities + + +def _source_scoped_work_id(identity: str, item: dict[str, object]) -> str: + """Build the scoped work ID for an already re-pathed ledger row or status target. + + Shared by :func:`_source_aware_ledger` and :func:`_source_aware_status_events` + so the two scoping paths cannot derive different IDs for the same work. + """ + start_line = item.get("start_line") + end_line = item.get("end_line") + return inspection_work_id( + identity, + str(item.get("path", "SKILL.md")), + start_line if isinstance(start_line, int) else None, + end_line if isinstance(end_line, int) else None, + ) + + def _source_aware_ledger( value: object, *, @@ -1026,15 +1060,7 @@ def _source_aware_ledger( for item in ids if isinstance(item, str) ] - scoped_path = str(entry.get("path", "SKILL.md")) - start_line = entry.get("start_line") - end_line = entry.get("end_line") - entry["work_id"] = inspection_work_id( - _ledger_work_identity(entry), - scoped_path, - start_line if isinstance(start_line, int) else None, - end_line if isinstance(end_line, int) else None, - ) + entry["work_id"] = _source_scoped_work_id(_ledger_work_identity(entry), entry) events.append(entry) return events @@ -1047,8 +1073,10 @@ def _source_aware_status_events( source_digest: str, retained_work_ids: set[str], max_planned_work: int, + work_identities: dict[str, str] | None = None, ) -> list[dict[str, object]]: statuses: list[dict[str, object]] = [] + identities = work_identities or {} planned_retained = 0 for status in _coerce_dict_list(value): if len(statuses) >= _TRANSITIVE_MAX_STATUS_EVENTS: @@ -1070,14 +1098,11 @@ def _source_aware_status_events( path = scoped_target.get("path") if isinstance(path, str) and path: scoped_target["path"] = _transitive_component_key(source_identity, path) - start_line = scoped_target.get("start_line") - end_line = scoped_target.get("end_line") - scoped_target["work_id"] = inspection_work_id( - analyzer_id, - str(scoped_target.get("path", "SKILL.md")), - start_line if isinstance(start_line, int) else None, - end_line if isinstance(end_line, int) else None, - ) + # Re-scope with the identity the matching ledger row used, so + # both paths agree on the scoped ID; the analyzer ID is only the + # fallback for targets with no child ledger row. + identity = identities.get(str(target.get("work_id", "")), analyzer_id) + scoped_target["work_id"] = _source_scoped_work_id(identity, scoped_target) if scoped_target["work_id"] not in retained_work_ids: continue scoped_work.append(scoped_target) @@ -1375,6 +1400,7 @@ def _scope_finding(finding: Finding) -> Finding: source_digest=source_digest, retained_work_ids=retained_work_ids, max_planned_work=len(retained_work_ids), + work_identities=_ledger_work_identities(child_result.get("inspection_ledger")), ) child_metadata = _decorate_component_metadata( _coerce_component_metadata(child_result.get("component_metadata")), diff --git a/src/skillspector/inspection_ledger.py b/src/skillspector/inspection_ledger.py index 953ace9f2..a2a17c269 100644 --- a/src/skillspector/inspection_ledger.py +++ b/src/skillspector/inspection_ledger.py @@ -36,6 +36,15 @@ class LedgerRecordType(StrEnum): WORK_ITEM = "work_item" SYSTEM = "system" SCOPE_BOUNDARY = "scope_boundary" + # An analyzer's own configuration (e.g. its YARA rule set), not a skill + # artifact. Its ``path`` is only a report-safe label, and every relative + # path is also a legal file name, so path-keyed accounting must exclude + # these records by type -- no choice of label can be collision-free. + RULE_SET = "rule_set" + + +RULE_SET_SCOPE: Final = "rule_set" +"""Public ``scope`` of exception rows describing a rule set rather than a file.""" class LedgerReason(StrEnum): @@ -295,6 +304,7 @@ class InspectionLedgerException(TypedDict): error_class: NotRequired[str] analyzers: NotRequired[list[str]] fatal: NotRequired[bool] + scope: NotRequired[str] class AnalysisCompleteness(TypedDict): @@ -589,6 +599,7 @@ def _exception( error_class: str | None = None, analyzers: Iterable[str] = (), fatal: bool, + scope: str | None = None, ) -> InspectionLedgerException: """Build the public, safe projection of one exceptional ledger fact.""" exception: InspectionLedgerException = { @@ -606,9 +617,16 @@ def _exception( exception["analyzers"] = analyzer_ids if error_class: exception["error_class"] = error_class + if scope: + exception["scope"] = scope return exception +def _is_rule_set_record(event: Mapping[str, object]) -> bool: + """Return whether a ledger row describes a rule set rather than an artifact.""" + return event.get("record_type") == LedgerRecordType.RULE_SET + + def _exception_from_event( event: InspectionLedgerEvent, *, fatal: bool ) -> InspectionLedgerException: @@ -629,6 +647,7 @@ def _exception_from_event( error_class=event.get("error_class"), analyzers=[str(event.get("analyzer_id", ""))], fatal=fatal, + scope=RULE_SET_SCOPE if _is_rule_set_record(event) else None, ) @@ -647,6 +666,9 @@ def _merge_exception_projection( exception["start_line"], exception["end_line"], exception.get("error_class"), + # A rule-set row and a real file's row can share a path label; + # they must never merge into one public row. + exception.get("scope"), ) existing = grouped.get(key) if existing is None: @@ -926,9 +948,17 @@ def accounting_error(path: object = None) -> None: ledger_exceptions = _merge_exception_projection(exceptional_rows) scope_exclusions = _merge_exception_projection(scope_rows) + # Rule-set rows describe an analyzer's configuration, not an artifact. Their + # path is only a label, so folding them into per-component coverage would + # charge the rule set's incompleteness to any real file with that name. + rule_set_work_ids = { + str(event.get("work_id", "")) for event in events if _is_rule_set_record(event) + } per_component: dict[str, list[LedgerOutcome]] = {component: [] for component in components} if primary_targets: for _analyzer_id, target, matches in primary_targets: + if str(target.get("work_id", "")) in rule_set_work_ids: + continue path = _safe_path(target.get("path"), components) outcomes = per_component.setdefault(path, []) if len(matches) == 1: diff --git a/src/skillspector/nodes/analyzers/static_yara.py b/src/skillspector/nodes/analyzers/static_yara.py index 4a949170b..c0a03f191 100644 --- a/src/skillspector/nodes/analyzers/static_yara.py +++ b/src/skillspector/nodes/analyzers/static_yara.py @@ -29,6 +29,7 @@ import os import re import stat +import threading import time from collections.abc import Callable from contextvars import ContextVar @@ -47,6 +48,7 @@ InspectionLedgerEvent, LedgerOutcome, LedgerReason, + LedgerRecordType, analyzer_status_event, ledger_event, ) @@ -204,9 +206,90 @@ def _enforce_rule_load_deadline() -> None: _check_rule_load_budget(budget) -# Module-level cache keyed by a content hash of all rule directories. -_compiled_rules: yara.Rules | None = None -_rules_hash: str | None = None +@dataclass(frozen=True, slots=True) +class _RuleCacheEntry: + """One compiled rule set, the hash it came from, and its own dropped-file count. + + Frozen, and only ever published by replacing :data:`_rule_cache` wholesale, + so the three halves cannot drift apart. They used to be three independent + globals, and the non-populating paths of :func:`_load_rules` wrote the skip + count while leaving the compiled rules and their hash in place. A later + request for that stale hash then hit the cache and returned those rules + paired with the intervening load's count -- zero, when the intervening load + found no rule files at all -- so a rule set that had silently dropped a + detector reported a complete scan, which is the false-clean result #554 is + about. + """ + + rules: yara.Rules + rules_hash: str + skipped_count: int + + +# Module-level cache keyed by a content hash of all rule directories. ``None`` +# means nothing usable is cached; there is deliberately no way to represent a +# half-populated cache, so every non-populating load path simply clears it. +_rule_cache: _RuleCacheEntry | None = None + +# Not cache state: the skip count of whichever load most recently ran, published +# under ``_RULES_LOCK`` so :func:`load_rules_with_skips` can read it inside the +# same transaction that produced it. On a cache hit it is assigned *from the +# cache entry*, so it always describes the rules actually returned. +_rules_skipped_count: int = 0 + +# Reentrant so the load-and-read transaction in :func:`load_rules_with_skips` +# can hold it across its own call to :func:`_load_rules`. +_RULES_LOCK = threading.RLock() + + +class _RulesLockWithinDeadline: + """Hold :data:`_RULES_LOCK`, waiting no longer than the caller's rule-load deadline. + + An unconditional wait cannot honour :data:`_RULE_LOAD_DEADLINE`: a scan whose + budget has expired would sit behind an unrelated, slow rule load in another + MCP/graph request for as long as that load takes. The wait is bounded by + the workflow wall-clock deadline (waiting consumes no thread CPU, so the + active-processing allowance cannot bound it), and expiry raises the same + ``runtime_limit`` signal the loader already raises at its other deadline + checks, which :func:`node` reports as partial work. + + Without a deadline (direct callers outside :func:`node`) this blocks as it + always did. The lock is reentrant, so a thread that already holds it (the + nested :func:`_load_rules` call inside :func:`load_rules_with_skips`) + re-acquires it at once whatever time remains. + + A plain class rather than :func:`contextlib.contextmanager`: the generator + form re-raises a body exception by assigning its ``__traceback__``, which a + frozen, slotted :class:`_YaraRuleResourceLimitError` rejects with a + ``TypeError``, so every rule-load limit raised under the lock would surface + as a crash instead of its partial result. + """ + + __slots__ = () + + def __enter__(self) -> None: + budget = _RULE_LOAD_DEADLINE.get() + if budget is None: + _RULES_LOCK.acquire() + return + elapsed = max(0.0, time.monotonic() - budget.workflow_started_at) + remaining = budget.workflow_limit_seconds - elapsed + acquired = ( + _RULES_LOCK.acquire(timeout=remaining) + if remaining > 0 + else _RULES_LOCK.acquire(blocking=False) + ) + if not acquired: + raise _YaraRuleResourceLimitError( + LedgerReason.RUNTIME_LIMIT, + { + "observed_seconds": max(0.0, time.monotonic() - budget.workflow_started_at), + "limit_seconds": budget.workflow_limit_seconds, + }, + ) + + def __exit__(self, *exc_info: object) -> None: + _RULES_LOCK.release() def _collect_rule_files(*dirs: Path) -> list[Path]: @@ -366,13 +449,36 @@ def _read_rule_source(rule_file: Path, data: bytes | None = None) -> str: return base64.b64decode("".join(encoded_source.split())).decode("utf-8") +#: Cap on how much of a decode/compile error is echoed into logs. Rule sources +#: are attacker-influenced when ``--yara-rules-dir`` points at untrusted content, +#: and YARA syntax errors can quote the offending source line, so the reason is +#: truncated rather than passed through whole. +MAX_RULE_REJECTION_REASON_CHARS = 200 + + +def _bounded_rejection_reason(exc: Exception) -> str: + """Return a single-line, length-capped description of a rule rejection.""" + reason = " ".join(str(exc).split()) + if len(reason) > MAX_RULE_REJECTION_REASON_CHARS: + reason = f"{reason[:MAX_RULE_REJECTION_REASON_CHARS]}..." + return reason or exc.__class__.__name__ + + def _build_namespace_map( rule_files: list[Path], temp_dir: Path | None = None, *, raw_cache: dict[Path, bytes] | None = None, + namespace_files: dict[str, str] | None = None, ) -> tuple[dict[str, str], int]: - """Build a {namespace: source} dict and count malformed rule files.""" + """Build a {namespace: source} dict and count malformed rule files. + + If ``namespace_files`` is given it is populated with ``{namespace: filename}`` + so a later compile failure can name the file the operator has to fix -- a + namespace has its extension stripped, so it is not a usable filename on its + own. Passed in rather than returned to keep this function's two-value + signature, which existing callers and tests unpack directly. + """ del temp_dir sources: dict[str, str] = {} skipped = 0 @@ -383,17 +489,35 @@ def _build_namespace_map( ns = _rule_namespace(rf) if ns in sources: ns = f"{rf.parent.name}/{ns}" + if namespace_files is not None: + namespace_files[ns] = rf.name try: sources[ns] = _read_rule_source(rf, raw_cache[rf]) except (binascii.Error, UnicodeDecodeError, ValueError) as exc: skipped += 1 - logger.debug("%s: skipping malformed encoded rule %s: %s", ANALYZER_ID, rf, exc) + # WARNING, not DEBUG: a dropped rule silently removes a detector, so + # the operator has to be able to identify and repair the file from a + # default-level run (#554). The filename is named explicitly because + # the ledger event is scoped to the rule set, not to one file. + logger.warning( + "%s: rejected rule file %s (could not decode): %s", + ANALYZER_ID, + rf.name, + _bounded_rejection_reason(exc), + ) return sources, skipped -def _compile_rules(sources: dict[str, str]) -> tuple[yara.Rules | None, int]: +def _compile_rules( + sources: dict[str, str], + *, + namespace_files: dict[str, str] | None = None, +) -> tuple[yara.Rules | None, int]: """Compile YARA rules from a namespace map. Falls back to per-source compilation on error. + ``namespace_files`` maps namespace to filename so a rejection can name the + file the operator has to fix rather than its extension-stripped namespace. + Returns (compiled_rules, skipped_count). """ _enforce_rule_load_deadline() @@ -414,7 +538,14 @@ def _compile_rules(sources: dict[str, str]) -> tuple[yara.Rules | None, int]: good[ns] = source except (yara.SyntaxError, yara.Error) as exc: skipped += 1 - logger.debug("%s: skipping %s: %s", ANALYZER_ID, ns, exc) + # WARNING for the same reason as the decode path above: without it a + # broken detector disappears with no default-level trace (#554). + logger.warning( + "%s: rejected rule file %s (could not compile): %s", + ANALYZER_ID, + (namespace_files or {}).get(ns, ns), + _bounded_rejection_reason(exc), + ) _enforce_rule_load_deadline() compiled = yara.compile(sources=good) if good else None @@ -426,38 +557,124 @@ def _load_rules(extra_dir: Path | None = None) -> yara.Rules | None: """Compile YARA rules from built-in and optional user-supplied directories. Results are cached at module level and reused if directory contents haven't changed. - """ - global _compiled_rules, _rules_hash # noqa: PLW0603 - dirs = [_BUILTIN_RULES_DIR] - if extra_dir and extra_dir.is_dir(): - dirs.append(extra_dir) - elif extra_dir: - logger.warning("%s: user rules directory %s does not exist", ANALYZER_ID, extra_dir) + Rule files that fail to decode (malformed base64) or fail to compile (YARA + syntax errors) are dropped from the active rule set. The count is recorded + in the module-level ``_rules_skipped_count`` (read via + :func:`rules_skipped_count`) rather than returned here, so this keeps its + original single-value signature and every existing + ``monkeypatch.setattr(static_yara, "_load_rules", ...)`` test double stays + valid; callers that care about the skip count must surface it themselves + or a scan can report ``completed``/SAFE while some of its own detections + never ran (#554). + + A successful load publishes rules, hash and count together as one + :class:`_RuleCacheEntry`, and every path that does not produce usable rules + clears that entry outright. Both halves matter: without the first a cache + hit could answer with another load's count, and without the second the + stale rules would stay reachable under their old hash. + + Callers should prefer :func:`load_rules_with_skips`, which returns both + halves as one value; reading the count separately after this returns is + racy across concurrent scans. + """ + global _rule_cache, _rules_skipped_count # noqa: PLW0603 + + with _RulesLockWithinDeadline(): + # Cleared up front so that a load which raises part way through cannot + # leave a previous load's total readable through + # :func:`rules_skipped_count`. Every return path below assigns its own. + # ``_rule_cache`` is deliberately *not* cleared here: an entry is + # self-consistent, so on an exception it stays a valid answer for its + # own hash rather than forcing a needless recompile. + _rules_skipped_count = 0 + + dirs = [_BUILTIN_RULES_DIR] + if extra_dir and extra_dir.is_dir(): + dirs.append(extra_dir) + elif extra_dir: + logger.warning("%s: user rules directory %s does not exist", ANALYZER_ID, extra_dir) + + rule_files = _collect_rule_files(*dirs) + if not rule_files: + logger.info("%s: no YARA rule files found", ANALYZER_ID) + # Non-populating: discard the entry instead of leaving the previous + # rules cached under their old hash. Keeping them would let the next + # request for that hash return them alongside this load's zero. + _rule_cache = None + return None - rule_files = _collect_rule_files(*dirs) - if not rule_files: - logger.info("%s: no YARA rule files found", ANALYZER_ID) - return None + raw_cache = _read_rule_bytes_cache(rule_files) + current_hash = _content_hash(rule_files, raw_cache) + cached = _rule_cache + if cached is not None and cached.rules_hash == current_hash: + # The count is taken from the entry, so it describes these rules and + # not whichever load happened to run in between. + _rules_skipped_count = cached.skipped_count + return cached.rules + + namespace_files: dict[str, str] = {} + sources, materialize_skipped = _build_namespace_map( + rule_files, raw_cache=raw_cache, namespace_files=namespace_files + ) + compiled, compile_skipped = _compile_rules(sources, namespace_files=namespace_files) + skipped = materialize_skipped + compile_skipped + _rules_skipped_count = skipped + + if compiled is None: + logger.warning("%s: failed to compile any YARA rules", ANALYZER_ID) + # Non-populating for the same reason as the no-rule-files path above. + _rule_cache = None + return None + + _rule_cache = _RuleCacheEntry( + rules=compiled, + rules_hash=current_hash, + skipped_count=skipped, + ) + loaded = len(sources) - compile_skipped + logger.info("%s: compiled %d YARA rule file(s) (%d skipped)", ANALYZER_ID, loaded, skipped) + return compiled + + +def load_rules_with_skips(extra_dir: Path | None = None) -> tuple[yara.Rules | None, int]: + """Load rules and return them with their own skip count, as one value. + + The two halves must be obtained in a single locked transaction. Reading the + count separately after :func:`_load_rules` returns lets two concurrent + MCP/graph scans interleave: scan A loads rule set A, scan B loads rule set B + and overwrites the module-level count, then scan A reads B's count. Scan A + would then run rules A while reporting B's skip total -- and if B skipped + nothing, A reports ``completed`` even though one of A's own rules was + dropped, which is exactly the false-clean result #554 is about. + + :func:`_load_rules` is called through the module global so existing + ``monkeypatch.setattr(static_yara, "_load_rules", ...)`` doubles still apply. + + The lock wait is bounded by the caller's rule-load deadline (see + :class:`_RulesLockWithinDeadline`), so a scan queued behind another + scan's slow load returns its ``runtime_limit`` result on time instead of + waiting the other load out. The snapshot stays atomic either way: rules + and count are still read inside one hold of the lock, or not at all. + """ + with _RulesLockWithinDeadline(): + rules = _load_rules(extra_dir) + return rules, _rules_skipped_count - raw_cache = _read_rule_bytes_cache(rule_files) - current_hash = _content_hash(rule_files, raw_cache) - if _compiled_rules is not None and _rules_hash == current_hash: - return _compiled_rules - sources, materialize_skipped = _build_namespace_map(rule_files, raw_cache=raw_cache) - compiled, compile_skipped = _compile_rules(sources) - skipped = materialize_skipped + compile_skipped +def rules_skipped_count() -> int: + """Return how many rule files the rules from the most recent load dropped. - if compiled is None: - logger.warning("%s: failed to compile any YARA rules", ANALYZER_ID) - return None + On a cache hit this is the cached entry's own count, not zero: the whole + point is that the number travels with the rules it describes, so a rule set + that dropped a detector keeps reporting it on every later cache hit. - _compiled_rules = compiled - _rules_hash = current_hash - loaded = len(sources) - compile_skipped - logger.info("%s: compiled %d YARA rule file(s) (%d skipped)", ANALYZER_ID, loaded, skipped) - return compiled + Retained for callers that already hold :data:`_RULES_LOCK` or run + single-threaded. Anything reading this straight after :func:`_load_rules` + should use :func:`load_rules_with_skips` instead. + """ + with _RULES_LOCK: + return _rules_skipped_count def _bounded_match_instances( @@ -1025,7 +1242,9 @@ def _rule_limit_response( ) deadline_token = _RULE_LOAD_DEADLINE.set(load_budget) try: - rules = _load_rules(extra_dir) + # One transaction: the skip count must describe *these* rules, not + # whatever a concurrent scan loaded in between. + rules, rules_skipped = load_rules_with_skips(extra_dir) except _YaraRuleResourceLimitError as exc: return _rule_limit_response(exc.reason, dict(exc.metrics)) finally: @@ -1181,6 +1400,47 @@ def _rule_limit_response( ) logger.info("%s: %d findings", ANALYZER_ID, len(findings)) + if rules_skipped: + # A rule that fails to compile or decode is dropped from the active + # set with no per-file signal: every scanned component can still + # report COMPLETED, because the rule that would have flagged it + # simply never ran. Surface that as its own ledger event, scoped to + # the rule directory rather than a skill file, so it isn't silently + # absorbed into a clean-looking events list (#554). + events.append( + ledger_event( + # analyzer_id is deliberately omitted. ledger_event derives the + # work identity as ``analyzer_id or f"{record_type}:{phase}"``, + # so passing it would identify this event as + # ``static_yara`` + path -- identical to the planned work item + # for a *scanned component of the same name*. A skill file + # literally named ``yara_rules`` then collides with this event, + # both planned targets resolve to two matching events, and + # reconciliation raises a fatal ``unaccounted_work`` instead of + # the nonfatal partial scan this is meant to record. Falling + # back to ``rule_set:static`` makes the identity disjoint from + # every analyzer work item by construction, so no choice of + # filename can collide -- renaming the synthetic path alone + # would only move the collision to the next unlucky name. + outcome=LedgerOutcome.PARTIAL, + # RULE_SET, not SYSTEM: SYSTEM rows describe real artifacts + # and feed path-keyed accounting (per-component coverage and + # AE1 reference findings). This row describes the rule set, + # so finalization excludes it from that accounting by type, + # and a benign file named ``yara_rules`` cannot be charged + # with this partial outcome. + record_type=LedgerRecordType.RULE_SET, + phase="static", + # Not a scanned skill file: a report-safe label for the rule + # set. Ledger paths must be relative POSIX paths, and the real + # rules directory (builtin or --yara-rules-dir) is absolute, so + # it cannot be used here. + path="yara_rules/", + reason=LedgerReason.READ_ERROR, + observed_artifacts=rules_skipped, + limit_artifacts=0, + ) + ) if not events: status = analyzer_status_event( analyzer_id=ANALYZER_ID, diff --git a/src/skillspector/nodes/finalize_inspection_ledger.py b/src/skillspector/nodes/finalize_inspection_ledger.py index 4c2b9dad7..e9097ad1d 100644 --- a/src/skillspector/nodes/finalize_inspection_ledger.py +++ b/src/skillspector/nodes/finalize_inspection_ledger.py @@ -532,6 +532,10 @@ def _reference_coverage_findings( else [] ) for event in ledger_events: + if event.get("record_type") == LedgerRecordType.RULE_SET: + # A rule set's path is a label, not an artifact: a real file that + # happens to share it was not inspected any less completely. + continue # Reference resolution records a mention of a path that the bundle does # not carry, or that matches more than one bundled file, as a partial # event on the file that contains the mention. Neither leaves bytes of diff --git a/src/skillspector/nodes/report.py b/src/skillspector/nodes/report.py index cb9a2d937..446ff7012 100644 --- a/src/skillspector/nodes/report.py +++ b/src/skillspector/nodes/report.py @@ -41,6 +41,7 @@ from skillspector.inference_usage import sanitize_inference_usage from skillspector.inspection_ledger import ( MAX_FINDING_OUTPUT_RECORDS, + RULE_SET_SCOPE, AnalysisCompleteness, finalize_ledger, ) @@ -842,8 +843,11 @@ def notification_from_exception( path = str(exception.get("path", "")) start_line = exception.get("start_line") end_line = exception.get("end_line") + scope = exception.get("scope") locations = None - if path: + # A rule-set row's path is a label, not an artifact; giving it a + # physical location would attribute it to any real file of that name. + if path and not scope: region = ( SarifRegion( startLine=int(start_line), @@ -867,6 +871,8 @@ def notification_from_exception( } if exception.get("fatal") is not None: properties["fatal"] = bool(exception["fatal"]) + if scope: + properties["scope"] = str(scope) analyzers = exception.get("analyzers") if isinstance(analyzers, list): properties["analyzers"] = list(analyzers) @@ -1003,6 +1009,9 @@ def render_rows(title: str, rows: object) -> None: end_line = row.get("end_line") if isinstance(start_line, int): location += f":{start_line}" + (f"-{end_line}" if end_line else "") + if row.get("scope") == RULE_SET_SCOPE: + # The path of a rule-set row is a label, not an artifact. + location = f"rule set {location}" reason = str(row.get("reason_code", row.get("status", "status"))) message = str(row.get("message", "")) console.print(f" - {escape(reason)} {escape(location)}: {escape(message)}") @@ -1477,8 +1486,12 @@ def render_rows(title: str, rows: object) -> None: if isinstance(start_line, int): location += f":{start_line}" + (f"-{end_line}" if end_line else "") reason = row.get("reason_code", row.get("status", "status")) + location_cell = _markdown_code(location, table_cell=True) + if row.get("scope") == RULE_SET_SCOPE: + # The path of a rule-set row is a label, not an artifact. + location_cell = f"rule set {location_cell}" lines.append( - f"| {_markdown_cell(reason)} | {_markdown_code(location, table_cell=True)} | " + f"| {_markdown_cell(reason)} | {location_cell} | " f"{_markdown_cell(row.get('message', ''))} |" ) lines.append("") diff --git a/tests/nodes/analyzers/test_static_yara.py b/tests/nodes/analyzers/test_static_yara.py index 57b7d79dc..cebfa1daf 100644 --- a/tests/nodes/analyzers/test_static_yara.py +++ b/tests/nodes/analyzers/test_static_yara.py @@ -22,12 +22,19 @@ from __future__ import annotations import base64 +import dataclasses import json +import logging +import threading +import time from pathlib import Path from unittest.mock import MagicMock import pytest +from typer.testing import CliRunner +from skillspector import cli +from skillspector.cli import app from skillspector.inspection_ledger import LedgerReason from skillspector.nodes.analyzers import static_yara from skillspector.nodes.analyzers.static_runner import MAX_FILE_CHARS @@ -37,12 +44,17 @@ @pytest.fixture(autouse=True) def _clear_rule_cache(): - """Reset the module-level compiled rules cache between tests.""" - static_yara._compiled_rules = None - static_yara._rules_hash = None + """Reset the module-level compiled rules cache between tests. + + The skip count is part of that cache entry: it is only meaningful alongside + the hash it was produced from, so leaving it set would leak a previous + test's dropped-rule total into the next one. + """ + static_yara._rule_cache = None + static_yara._rules_skipped_count = 0 yield - static_yara._compiled_rules = None - static_yara._rules_hash = None + static_yara._rule_cache = None + static_yara._rules_skipped_count = 0 def _write_rule( @@ -2232,22 +2244,22 @@ def test_rules_are_cached(self, tmp_path): tmp_path, "rule_cache", category="malware", severity="HIGH", strings={"a": "CACHETEST"} ) _run("CACHETEST", "f.txt", str(tmp_path)) - first_rules = static_yara._compiled_rules + first_rules = static_yara._rule_cache.rules _run("CACHETEST", "f.txt", str(tmp_path)) - assert static_yara._compiled_rules is first_rules + assert static_yara._rule_cache.rules is first_rules def test_cache_invalidated_on_new_rule(self, tmp_path): _write_rule( tmp_path, "rule_v1", category="malware", severity="HIGH", strings={"a": "V1MARKER"} ) _run("V1MARKER", "f.txt", str(tmp_path)) - first_hash = static_yara._rules_hash + first_hash = static_yara._rule_cache.rules_hash _write_rule( tmp_path, "rule_v2", category="malware", severity="HIGH", strings={"a": "V2MARKER"} ) _run("V2MARKER", "f.txt", str(tmp_path)) - assert static_yara._rules_hash != first_hash + assert static_yara._rule_cache.rules_hash != first_hash # ── Internal helpers ────────────────────────────────────────────────── @@ -2397,6 +2409,48 @@ def test_build_namespace_map_skips_malformed_encoded_rules(self, tmp_path): assert "invalid" not in ns_map assert skipped == 1 + def test_malformed_rule_is_reported_not_silently_dropped(self, tmp_path, monkeypatch): + """A custom rule that can't compile must not report a clean, SAFE scan (#554). + + Reproduces the issue's own scenario: a valid rule plus a rule with a + YARA syntax error in the same --yara-rules-dir. The good rule must + still fire, but the analyzer status must not be "completed" -- that + claim would be false, since the broken rule never ran against + anything. + """ + static_yara._rule_cache = None + static_yara._rules_skipped_count = 0 + monkeypatch.setattr(static_yara, "_BUILTIN_RULES_DIR", tmp_path / "empty_builtin") + (tmp_path / "empty_builtin").mkdir() + + rules_dir = tmp_path / "rules" + rules_dir.mkdir() + (rules_dir / "good.yar").write_text( + 'rule good_rule { meta: category = "malware" ' + 'strings: $a = "ACME_CANARY" condition: $a }' + ) + # Missing closing brace: a real YARA syntax error, not a decode failure. + (rules_dir / "bad.yar").write_text('rule bad_rule { strings: $a = "x" condition: $a') + + result = static_yara.node( + { + "components": ["skill.md"], + "file_cache": {"skill.md": "contains ACME_CANARY"}, + "yara_rules_dir": str(rules_dir), + } + ) + + assert any("good_rule" in f.message for f in result["findings"]), ( + "the valid rule must still fire" + ) + status = result["analyzer_status_events"][0] + assert status["status"] != "completed", "a dropped custom rule must not report a clean scan" + assert any( + event.get("reason_code") == LedgerReason.READ_ERROR + and event.get("observed_artifacts") == 1 + for event in result["inspection_ledger"] + ) + @pytest.mark.parametrize("payload", ["not base64", "not base64 é"]) def test_malformed_extra_encoded_rule_does_not_block_builtin_rules(self, tmp_path, payload): (tmp_path / "bad.yar.b64").write_text(payload, encoding="utf-8") @@ -2766,3 +2820,664 @@ def test_yara_does_not_start_without_one_enforceable_engine_second(self) -> None match.assert_not_called() assert matched.reason == "runtime_limit" assert matched.metrics == {"observed_seconds": 0.0, "limit_seconds": 0.5} + + +class TestRuleSkipAccounting: + """Regressions for the three review findings on the #554 skip-count surface. + + All three share one root shape: the dropped-rule total was reported through + channels not tied to the scan that produced it -- a module global read after + the fact, a ledger work ID shared with component work, and a DEBUG log the + operator never sees at default verbosity. + """ + + @staticmethod + def _isolated_builtin(tmp_path: Path, monkeypatch) -> None: + """Point the built-in rule dir at an empty dir so counts are only ours.""" + builtin = tmp_path / "empty_builtin" + builtin.mkdir(exist_ok=True) + monkeypatch.setattr(static_yara, "_BUILTIN_RULES_DIR", builtin) + + @staticmethod + def _rule_dir(tmp_path: Path, name: str, *, broken: int, good: bool = True) -> Path: + """Build a rule dir with ``broken`` uncompilable rules, optionally one valid one. + + ``good=False`` with ``broken=0`` yields an existing but empty directory, + which is how the "no rule files at all" load path is reached. + """ + rules_dir = tmp_path / name + rules_dir.mkdir(parents=True, exist_ok=True) + marker = f"MARKER_{name.upper()}" + if good: + (rules_dir / "good.yar").write_text( + f'rule good_{name} {{ strings: $a = "{marker}" condition: $a }}' + ) + for index in range(broken): + # Missing closing brace: a real YARA syntax error, not a decode failure. + (rules_dir / f"bad{index}.yar").write_text( + f'rule bad_{name}_{index} {{ strings: $a = "x" condition: $a' + ) + return rules_dir + + def test_skip_count_travels_with_the_rules_it_describes(self, tmp_path, monkeypatch): + """Two loads in sequence must each report their own skip total. + + Deterministic form of the concurrency finding: reading the count as a + separate step after the load is what lets a later load answer for an + earlier one. ``load_rules_with_skips`` returns both halves together, so + the pairing cannot be broken by anything that happens afterwards. + """ + self._isolated_builtin(tmp_path, monkeypatch) + dir_a = self._rule_dir(tmp_path, "a", broken=1) + dir_b = self._rule_dir(tmp_path, "b", broken=0) + + rules_a, skipped_a = static_yara.load_rules_with_skips(dir_a) + rules_b, skipped_b = static_yara.load_rules_with_skips(dir_b) + + assert rules_a is not None + assert rules_b is not None + assert skipped_a == 1, "rule set A dropped one rule and must say so" + assert skipped_b == 0, "rule set B dropped nothing and must not inherit A's count" + + # The separate-read path is what made this unsafe: after B's load the + # module global describes B, so anyone still holding A's rules and + # reading the global now would report a clean scan for A. + assert static_yara.rules_skipped_count() == 0 + + @pytest.mark.parametrize( + ("label", "b_broken", "a_broken"), + [ + # B finds no rule files at all. This path forced the count to zero, + # so A's cache hit reported zero dropped rules: a false-complete scan. + ("no_rule_files", 0, 1), + # B compiles nothing because every one of its rules is rejected. + # Counts are deliberately asymmetric (A drops 2, B drops 1) so an + # inherited count is visible rather than coincidentally equal. + ("all_rejected", 1, 2), + ], + ) + def test_cached_rules_never_report_a_later_loads_skip_count( + self, tmp_path, monkeypatch, label, b_broken, a_broken + ): + """load A -> load a non-populating B -> load A again must still report A's count. + + ``_load_rules`` used to set the skip count and return on both + non-populating paths without replacing *or* clearing the cached rules + and hash. The entry left behind still matched A's hash, so the third + load hit the cache and paired A's rules with B's count -- zero for the + empty/no-files case -- and a rule set that had dropped a detector went + back to reporting a complete scan. + + Deterministic and single-threaded: this is a cache-integrity defect, not + a race, so it reproduces purely from the order of the three loads. + """ + self._isolated_builtin(tmp_path, monkeypatch) + dir_a = self._rule_dir(tmp_path, f"a_{label}", broken=a_broken) + dir_b = self._rule_dir(tmp_path, f"b_{label}", broken=b_broken, good=False) + + rules_a, skipped_a = static_yara.load_rules_with_skips(dir_a) + assert rules_a is not None + assert skipped_a == a_broken + + rules_b, skipped_b = static_yara.load_rules_with_skips(dir_b) + assert rules_b is None, "B must not produce usable rules in this scenario" + + rules_a_again, skipped_a_again = static_yara.load_rules_with_skips(dir_a) + + assert rules_a_again is not None, "A's rules must still be available" + assert skipped_a_again == a_broken, ( + f"rule set A dropped {a_broken} rule(s) but the reload reported " + f"{skipped_a_again}, which is B's count ({skipped_b})" + ) + + @pytest.mark.parametrize( + ("label", "broken", "good"), + [ + ("no_rule_files", 0, False), + ("all_rejected", 1, False), + ], + ) + def test_non_populating_load_leaves_no_cache_entry( + self, tmp_path, monkeypatch, label, broken, good + ): + """A load that yields no usable rules must not leave a populated cache entry. + + Covers the invalidation half directly, so a future change that starts + writing one part of the entry on these paths fails here rather than + only showing up as a wrong skip count three loads later. + """ + self._isolated_builtin(tmp_path, monkeypatch) + dir_a = self._rule_dir(tmp_path, f"seed_{label}", broken=1) + assert static_yara._load_rules(dir_a) is not None + assert static_yara._rule_cache is not None, "the seed load must populate the cache" + + dir_b = self._rule_dir(tmp_path, f"empty_{label}", broken=broken, good=good) + + assert static_yara._load_rules(dir_b) is None + assert static_yara._rule_cache is None, ( + "the previous rules and hash are still cached after a load that " + "produced nothing, so a later request for that hash can be served " + "rules paired with this load's count" + ) + + def test_cache_entry_cannot_be_mutated_in_place(self, tmp_path, monkeypatch): + """The three halves are one immutable value, not three fields to edit. + + Reassigning ``_rule_cache`` wholesale is the only supported way to + publish a change, which is what makes a cache hit unable to mix one + load's rules with another's count. + """ + self._isolated_builtin(tmp_path, monkeypatch) + static_yara._load_rules(self._rule_dir(tmp_path, "frozen", broken=1)) + entry = static_yara._rule_cache + assert entry is not None + assert entry.skipped_count == 1 + + with pytest.raises(dataclasses.FrozenInstanceError): + entry.skipped_count = 0 + + def test_rescan_after_a_non_populating_load_still_reports_the_dropped_rule( + self, tmp_path, monkeypatch + ): + """End to end: the A -> B -> A sequence must not resurrect a clean scan. + + The load-level assertions above pin the count; this pins the user-visible + consequence the issue is actually about. Before the fix the second scan + of A reported ``completed`` with no rule-skip event at all, even though + one of A's own rules had never run. + """ + self._isolated_builtin(tmp_path, monkeypatch) + rules_dir = self._rule_dir(tmp_path, "scan", broken=1) + empty_dir = self._rule_dir(tmp_path, "empty_scan", broken=0, good=False) + state = { + "components": ["skill.md"], + "file_cache": {"skill.md": "contains MARKER_SCAN"}, + "yara_rules_dir": str(rules_dir), + } + + first = static_yara.node(state) + assert first["analyzer_status_events"][0]["status"] != "completed" + + static_yara.node( + { + "components": ["skill.md"], + "file_cache": {"skill.md": "nothing to match"}, + "yara_rules_dir": str(empty_dir), + } + ) + + second = static_yara.node(state) + + assert any("good_scan" in finding.message for finding in second["findings"]), ( + "the valid rule must still fire on the rescan" + ) + assert second["analyzer_status_events"][0]["status"] != "completed", ( + "a rescan served from cache must not claim a complete scan while one " + "of its own rules is still dropped" + ) + assert any( + event.get("reason_code") is LedgerReason.READ_ERROR + and event.get("observed_artifacts") == 1 + for event in second["inspection_ledger"] + ), "the dropped rule must still be surfaced in the ledger on the rescan" + + def test_load_and_read_is_serialized_against_other_scans(self, tmp_path, monkeypatch): + """The load-and-read pair must be atomic, not merely adjacent. + + Proves the lock is genuinely held across the whole transaction rather + than racing threads and hoping, so the test cannot pass by luck of + timing: mid-transaction, another thread must not be able to acquire the + rules lock at all. + """ + self._isolated_builtin(tmp_path, monkeypatch) + dir_a = self._rule_dir(tmp_path, "a", broken=2) + + lock_was_held: list[bool] = [] + real_load = static_yara._load_rules + + def probing_load(extra_dir=None): + rules = real_load(extra_dir) + acquired_elsewhere: list[bool] = [] + + def try_acquire() -> None: + got = static_yara._RULES_LOCK.acquire(blocking=False) + acquired_elsewhere.append(got) + if got: + static_yara._RULES_LOCK.release() + + probe = threading.Thread(target=try_acquire) + probe.start() + probe.join() + lock_was_held.append(not acquired_elsewhere[0]) + return rules + + monkeypatch.setattr(static_yara, "_load_rules", probing_load) + _, skipped = static_yara.load_rules_with_skips(dir_a) + + assert skipped == 2 + assert lock_was_held == [True], ( + "another scan could enter the load-and-read transaction, so the rules " + "and their skip count are not obtained atomically" + ) + + def test_concurrent_scans_never_report_another_rule_sets_count(self, tmp_path, monkeypatch): + """Under real contention every scan must still see its own total.""" + self._isolated_builtin(tmp_path, monkeypatch) + dir_a = self._rule_dir(tmp_path, "a", broken=1) + dir_b = self._rule_dir(tmp_path, "b", broken=0) + + mismatches: list[tuple[str, int, int]] = [] + failures: list[BaseException] = [] + observations = 0 + + def scan(label: str, rules_dir: Path, expected: int) -> None: + nonlocal observations + try: + for _ in range(25): + _, skipped = static_yara.load_rules_with_skips(rules_dir) + observations += 1 + if skipped != expected: + mismatches.append((label, expected, skipped)) + except BaseException as exc: # noqa: BLE001 - re-raised in the main thread + failures.append(exc) + + threads = [ + threading.Thread(target=scan, args=("A", dir_a, 1)), + threading.Thread(target=scan, args=("B", dir_b, 0)), + ] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + # An exception inside a worker thread does not fail the test on its own, + # so it is surfaced explicitly -- otherwise this test passes vacuously + # when the scans never actually ran. + assert failures == [], f"a scan thread raised: {failures!r}" + assert observations == 50, f"expected 50 observations, made {observations}" + assert mismatches == [], f"scans observed another rule set's skip count: {mismatches}" + + def test_rule_load_event_does_not_collide_with_a_component_of_the_same_name( + self, tmp_path, monkeypatch + ): + """A skill file named ``yara_rules`` must not collide with the rule-load event. + + The ledger derives a work ID from ``analyzer_id`` plus the normalized + path. The synthetic rule-set scope normalizes to ``yara_rules``, so + attributing the event to ``static_yara`` gave it the same work ID as a + scanned component of that name: both planned targets then resolved to two + matching events and reconciliation raised a fatal ``unaccounted_work`` + instead of recording a nonfatal partial scan. Renaming the synthetic path + alone would only move the collision to the next unlucky filename. + """ + self._isolated_builtin(tmp_path, monkeypatch) + rules_dir = self._rule_dir(tmp_path, "r", broken=1) + + result = static_yara.node( + { + "components": ["yara_rules"], + "file_cache": {"yara_rules": "contains MARKER_R"}, + "yara_rules_dir": str(rules_dir), + } + ) + + events = result["inspection_ledger"] + work_ids = [event["work_id"] for event in events] + assert len(work_ids) == len(set(work_ids)), ( + "the rule-load event shares a work ID with the scanned component" + ) + + # The planned work the status advertises must be equally distinct, since + # reconciliation requires exactly one event per planned target. + planned = result["analyzer_status_events"][0]["planned_work"] + planned_ids = [target["work_id"] for target in planned] + assert len(planned_ids) == len(set(planned_ids)) + + # The dropped rule is still surfaced, and the scan is partial not clean. + assert result["analyzer_status_events"][0]["status"] != "completed" + assert any( + event.get("reason_code") == LedgerReason.READ_ERROR + and event.get("observed_artifacts") == 1 + for event in events + ) + + @pytest.mark.parametrize("referenced_name", ["yara_rules", "normal.txt"]) + def test_referenced_file_named_like_the_rule_set_is_not_charged_with_its_dropped_rule( + self, tmp_path, referenced_name + ): + """A real file sharing the rule-set label must be scored like any other file. + + The rule-load event is labelled ``yara_rules``. Finalization groups + reference outcomes and per-component coverage by path, so a benign, + fully read file of that name, linked from ``SKILL.md``, was charged with + the rule set's partial outcome: a false HIGH AE1, risk score 25 and 50% + coverage, all of which vanished when only the filename changed. The scan + must stay partial (a rule really was dropped), but nothing file-specific + may be inferred from the label. Driven through the real CLI so the + finalizer and report generation are both exercised. + """ + skill = tmp_path / "skill" + skill.mkdir() + (skill / "SKILL.md").write_text( + "---\nname: demo\ndescription: A harmless demo skill.\n---\n\n" + f"# Demo\n\nSee [the notes]({referenced_name}) for details.\n", + encoding="utf-8", + ) + (skill / referenced_name).write_text("Plain harmless notes.\n", encoding="utf-8") + rules_dir = tmp_path / "rules" + rules_dir.mkdir() + (rules_dir / "valid.yar").write_text("rule never_fires { condition: false }\n") + (rules_dir / "broken.yar").write_text('rule broken { strings: $a = "x" condition: $a\n') + + def scan(output_format: str, *extra: str) -> tuple[int, dict]: + out = tmp_path / f"report-{output_format}-{len(extra)}.json" + result = CliRunner().invoke( + app, + [ + "scan", + str(skill), + "--no-llm", + "--yara-rules-dir", + str(rules_dir), + "--format", + output_format, + "--output", + str(out), + *extra, + ], + ) + return result.exit_code, json.loads(out.read_text(encoding="utf-8")) + + exit_code, report = scan("json") + assert exit_code == 0 + assert [issue["id"] for issue in report["issues"]] == [], ( + "a rule-set failure must not invent a finding against a file of the same name" + ) + assert report["risk_assessment"]["score"] == 0 + completeness = report["analysis_completeness"] + assert completeness["coverage_percent"] == 100.0 + assert completeness["partially_inspected_files"] == 0 + assert completeness["entirely_uninspected_files"] == 0 + + # The dropped rule itself is still reported, as a nonfatal partial scan + # explicitly scoped to the rule set rather than to an artifact. + assert completeness["is_complete"] is False + assert completeness["execution_successful"] is True + rule_set_rows = [ + row for row in completeness["ledger_exceptions"] if row.get("scope") == "rule_set" + ] + assert len(rule_set_rows) == 1 + assert rule_set_rows[0]["reason_code"] == LedgerReason.READ_ERROR + assert rule_set_rows[0]["fatal"] is False + assert all( + row.get("scope") == "rule_set" + for row in completeness["ledger_exceptions"] + if row["reason_code"] == LedgerReason.READ_ERROR + ) + + strict_exit, _ = scan("json", "--fail-on-incomplete") + assert strict_exit == 1 + + # SARIF must not point the rule-set notification at an artifact either. + _, sarif = scan("sarif") + notifications = sarif["runs"][0]["invocations"][0]["toolExecutionNotifications"] + rule_set_notifications = [ + item for item in notifications if item.get("properties", {}).get("scope") == "rule_set" + ] + assert len(rule_set_notifications) == 1 + assert not rule_set_notifications[0].get("locations") + + @pytest.mark.parametrize( + ("filename", "content", "expected_fragment"), + [ + ("acme.yar", b'rule broken { strings: $a = "x" condition: $a', "could not compile"), + ( + "bom.yar", + b'\xef\xbb\xbfrule bomrule { strings: $a = "y" condition: $a }', + "could not compile", + ), + ( + "bad_utf8.yar", + b'rule u { strings: $a = "\xff\xfe" condition: $a }', + "could not decode", + ), + ], + ) + def test_rejected_rule_is_named_at_default_log_level( + self, tmp_path, monkeypatch, caplog, filename, content, expected_fragment + ): + """Each rejected rule must be reported at WARNING, naming the file (#554). + + A dropped rule removes a detector. At DEBUG the operator gets no signal + at default verbosity, and the ledger event is scoped to the rule set + rather than to one file, so without this the specific file that needs + repairing cannot be identified. + """ + self._isolated_builtin(tmp_path, monkeypatch) + rules_dir = tmp_path / "rejected" + rules_dir.mkdir() + (rules_dir / filename).write_bytes(content) + + with caplog.at_level(logging.WARNING, logger=static_yara.logger.name): + static_yara._load_rules(rules_dir) + + rejections = [ + record.getMessage() + for record in caplog.records + if record.levelno == logging.WARNING and "rejected rule file" in record.getMessage() + ] + assert len(rejections) == 1, f"expected one rejection warning, got {rejections}" + assert filename in rejections[0], f"the warning must name {filename}: {rejections[0]}" + assert expected_fragment in rejections[0] + + def test_rejection_reason_is_length_bounded(self): + """Rule sources can be untrusted, so the echoed reason must be capped.""" + reason = static_yara._bounded_rejection_reason(ValueError("x" * 5_000)) + + assert len(reason) <= static_yara.MAX_RULE_REJECTION_REASON_CHARS + 3 + assert reason.endswith("...") + + def test_rejection_reason_collapses_newlines(self): + """A multi-line YARA error must stay one log line.""" + reason = static_yara._bounded_rejection_reason(ValueError("line one\nline two\r\nthree")) + + assert "\n" not in reason + assert reason == "line one line two three" + + def test_rule_lock_wait_honours_the_callers_deadline(self, tmp_path, monkeypatch): + """A scan queued behind another scan's slow rule load must still stop on time. + + Scan A is held inside the real rule-read path, so it owns the rules lock. + Scan B has its own short workflow budget; an unconditional lock wait kept + it blocked until A finished, long after B's deadline. B must instead + return the existing ``runtime_limit`` result within its own budget, and + A must still get its own rules and skip count once it resumes. + """ + self._isolated_builtin(tmp_path, monkeypatch) + rules_dir = self._rule_dir(tmp_path, "a", broken=1) + + a_reading = threading.Event() + release_a = threading.Event() + real_read = static_yara._read_rule_bytes_cache + + def paused_read(rule_files): + if threading.current_thread().name == "scan-a": + a_reading.set() + # Bounded so a regression fails the timing assertion below + # instead of hanging the suite. + release_a.wait(timeout=5.0) + return real_read(rule_files) + + monkeypatch.setattr(static_yara, "_read_rule_bytes_cache", paused_read) + + a_result: list[tuple[object, int]] = [] + a_failures: list[BaseException] = [] + + def scan_a() -> None: + try: + a_result.append(static_yara.load_rules_with_skips(rules_dir)) + except BaseException as exc: # noqa: BLE001 - re-raised in the main thread + a_failures.append(exc) + + budget_seconds = 1.2 + + class Budget: + def __init__(self) -> None: + self.deadline = time.monotonic() + budget_seconds + + def remaining_seconds(self) -> float: + return self.deadline - time.monotonic() + + thread_a = threading.Thread(target=scan_a, name="scan-a") + thread_a.start() + try: + assert a_reading.wait(timeout=5.0), "scan A never reached the rule-read path" + started = time.monotonic() + b = static_yara.node( + { + "components": ["a.py", "b.py"], + "file_cache": {"a.py": "a", "b.py": "b"}, + "yara_rules_dir": str(rules_dir), + "transitive_traversal_state": Budget(), + } + ) + elapsed = time.monotonic() - started + finally: + release_a.set() + thread_a.join(timeout=10.0) + + assert elapsed < budget_seconds + 0.5, ( + f"scan B waited {elapsed:.3f}s for another scan's rule load, " + f"past its own {budget_seconds}s budget" + ) + assert [event["path"] for event in b["inspection_ledger"]] == ["a.py", "b.py"] + assert all( + event["reason_code"] == LedgerReason.RUNTIME_LIMIT for event in b["inspection_ledger"] + ) + assert b["analyzer_status_events"][0]["status"] == "degraded" + + # A's transaction is untouched by B giving up: same rules, own count. + assert a_failures == [], f"scan A raised: {a_failures!r}" + assert len(a_result) == 1 + rules, skipped = a_result[0] + assert rules is not None + assert skipped == 1 + + def test_rule_lock_reentry_does_not_wait_on_an_expired_deadline(self): + """The nested acquire inside ``load_rules_with_skips`` must not time out. + + The thread already owns the reentrant lock, so re-acquiring it is + immediate even with no time left, and releasing it leaves the outer + hold intact. + """ + expired = static_yara._new_rule_load_budget( + 1.0, + workflow_limit_seconds=0.0, + workflow_started_at=time.monotonic() - 1.0, + ) + token = static_yara._RULE_LOAD_DEADLINE.set(expired) + try: + with static_yara._RULES_LOCK: + with static_yara._RulesLockWithinDeadline(): + pass + held_elsewhere: list[bool] = [] + + def probe() -> None: + got = static_yara._RULES_LOCK.acquire(blocking=False) + held_elsewhere.append(not got) + if got: + static_yara._RULES_LOCK.release() + + prober = threading.Thread(target=probe) + prober.start() + prober.join() + assert held_elsewhere == [True], "the inner release dropped the outer hold" + finally: + static_yara._RULE_LOAD_DEADLINE.reset(token) + + def test_rule_set_work_survives_transitive_status_scoping(self, tmp_path, monkeypatch): + """Every scope's rule-set work must be counted, not just the root's. + + ``_source_aware_ledger`` re-scopes the rule-set row with its own + ``rule_set:static`` identity, but the status path rebuilt the matching + planned target with ``static_yara``, so in each child the target no + longer matched any retained row and was dropped. The exceptions survived + while the per-analyzer counts silently lost each child's rejected rule: + 4 planned / 1 partial instead of 6 / 3 for a root plus two children. + Driven through the real CLI and the real graph for all three scopes. + """ + children = { + "https://github.com/org/child-one": tmp_path / "child-one", + "https://github.com/org/child-two": tmp_path / "child-two", + } + root = tmp_path / "root" + for directory in (root, *children.values()): + directory.mkdir() + (root / "SKILL.md").write_text( + "---\nname: root\ndescription: A harmless root skill.\n---\n\n" + "# Root\n\nUses https://github.com/org/child-one.git and " + "https://github.com/org/child-two.git.\n", + encoding="utf-8", + ) + for url, directory in children.items(): + name = url.rsplit("/", 1)[-1] + (directory / "SKILL.md").write_text( + f"---\nname: {name}\ndescription: A harmless child skill.\n---\n\n# Child\n", + encoding="utf-8", + ) + rules_dir = tmp_path / "rules" + rules_dir.mkdir() + (rules_dir / "valid.yar").write_text("rule never_fires { condition: false }\n") + (rules_dir / "broken.yar").write_text('rule broken { strings: $a = "x" condition: $a\n') + + real_run_graph_scan = cli._run_graph_scan + scanned: list[str] = [] + + def run_graph_scan(input_path: str, *args, **kwargs): + scanned.append(input_path) + for url, directory in children.items(): + if input_path.rstrip("/").removesuffix(".git") == url: + input_path = str(directory) + break + return real_run_graph_scan(input_path, *args, **kwargs) + + monkeypatch.setattr(cli, "_run_graph_scan", run_graph_scan) + out = tmp_path / "report.json" + result = CliRunner().invoke( + app, + [ + "scan", + str(root), + "--no-llm", + "--yara-rules-dir", + str(rules_dir), + "--transitive", + "--transitive-depth", + "1", + "--format", + "json", + "--output", + str(out), + ], + ) + assert result.exit_code == 0, result.output + assert len(scanned) == 3, f"expected root plus two children, scanned {scanned}" + completeness = json.loads(out.read_text(encoding="utf-8"))["analysis_completeness"] + + rule_set_rows = [ + row for row in completeness["ledger_exceptions"] if row.get("scope") == "rule_set" + ] + assert len(rule_set_rows) == 3, "one rule-set exception per scope" + + yara_statuses = [ + row for row in completeness["analyzer_statuses"] if row["analyzer_id"] == "static_yara" + ] + assert yara_statuses + planned = sum(row["planned_work"] for row in yara_statuses) + partial = sum(row["partial"] for row in yara_statuses) + assert (planned, partial) == (6, 3), ( + "each scope plans its SKILL.md plus its rule set, and each rule set " + f"is partial; got {planned} planned / {partial} partial" + ) + assert all(row["unaccounted"] == 0 for row in yara_statuses) + assert all(row["status"] == "degraded" for row in yara_statuses) diff --git a/tests/nodes/test_finalize_inspection_ledger.py b/tests/nodes/test_finalize_inspection_ledger.py index 67f2ca594..ecadda591 100644 --- a/tests/nodes/test_finalize_inspection_ledger.py +++ b/tests/nodes/test_finalize_inspection_ledger.py @@ -1842,6 +1842,103 @@ def test_unrelated_fatal_does_not_reclassify_a_format_only_reference() -> None: ) +def test_rule_set_row_is_excluded_from_path_keyed_accounting_by_type() -> None: + """A rule-set label equal to a real file's path must not touch that file. + + ``components`` holds a fully read file whose path equals the rule-set label, + and SKILL.md links to it. The rule-set row must keep the scan partial without + lowering that file's coverage, synthesizing AE1, or merging with the file's + own public exception row. + """ + file_event = ledger_event( + analyzer_id="static_runner", + outcome=LedgerOutcome.COMPLETED, + phase="static", + path="yara_rules", + ) + rule_set_event = ledger_event( + outcome=LedgerOutcome.PARTIAL, + record_type=LedgerRecordType.RULE_SET, + phase="static", + path="yara_rules/", + reason=LedgerReason.READ_ERROR, + observed_artifacts=1, + limit_artifacts=0, + ) + # A real, same-path, same-reason row from the file itself must stay separate. + real_file_row = ledger_event( + outcome=LedgerOutcome.PARTIAL, + record_type=LedgerRecordType.SYSTEM, + phase="static", + path="yara_rules", + reason=LedgerReason.READ_ERROR, + ) + assert rule_set_event["path"] == file_event["path"] == "yara_rules" + state: SkillspectorState = { + "components": ["SKILL.md", "yara_rules"], + "findings": [], + "effective_finding_ids": [], + "artifact_references": [ + { + "source_path": "SKILL.md", + "line": 3, + "column": 5, + "evidence": "See [the notes](yara_rules).", + "target_path": "yara_rules", + "status": "resolved", + "disposition": "complete", + } + ], + "inspection_ledger": [ + ledger_event( + analyzer_id="static_runner", + outcome=LedgerOutcome.COMPLETED, + phase="static", + path="SKILL.md", + ), + file_event, + rule_set_event, + ], + "analyzer_status_events": [ + analyzer_status_event( + analyzer_id="static_runner", + status="completed", + planned_work=[ + _target( + inspection_work_id("static_runner", "SKILL.md", None, None), "SKILL.md" + ), + _target(file_event["work_id"], "yara_rules"), + ], + ), + analyzer_status_event( + analyzer_id="static_yara", + status="degraded", + planned_work=[_target(rule_set_event["work_id"], "yara_rules")], + ), + ], + } + + result = finalize_inspection_ledger(state) + + assert [finding.rule_id for finding in result["findings"]] == [] + completeness = result["analysis_completeness"] + assert completeness["coverage_percent"] == 100.0 + assert completeness["fully_inspected_files"] == 2 + assert completeness["partially_inspected_files"] == 0 + assert completeness["is_complete"] is False + assert completeness["execution_successful"] is True + assert [row.get("scope") for row in completeness["ledger_exceptions"]] == ["rule_set"] + + # Control: the same label on a SYSTEM row is real artifact evidence and must + # still drive both AE1 and coverage, and must not merge with the rule-set row. + control_state = dict(state) + control_state["inspection_ledger"] = [*state["inspection_ledger"], real_file_row] + control = finalize_inspection_ledger(control_state) + assert [finding.rule_id for finding in control["findings"]] == ["AE1"] + rows = control["analysis_completeness"]["ledger_exceptions"] + assert sorted(str(row.get("scope")) for row in rows) == ["None", "rule_set"] + + @pytest.mark.parametrize( ("disposition", "reason", "expected_ae7"), [ diff --git a/tests/nodes/test_report.py b/tests/nodes/test_report.py index 1aa620825..283a96f18 100644 --- a/tests/nodes/test_report.py +++ b/tests/nodes/test_report.py @@ -719,6 +719,56 @@ def test_report_names_the_analyzer_of_each_status_row(self, output_format: str) assert "static_patterns_tool_misuse" in body assert "semantic_quality_policy" in body + @pytest.mark.parametrize("output_format", ["markdown", "terminal"]) + def test_report_labels_rule_set_exception_as_rule_set_not_file( + self, output_format: str + ) -> None: + """A rule-set row must not read like a file row that shares its path label.""" + state: SkillspectorState = { + "filtered_findings": [], + "component_metadata": [], + "has_executable_scripts": False, + "manifest": {}, + "skill_path": None, + "output_format": output_format, + "execution_successful": True, + "analysis_completeness": { + "coverage_percent": 100.0, + "fully_inspected_files": 1, + "partially_inspected_files": 0, + "entirely_uninspected_files": 0, + "is_complete": False, + "execution_successful": True, + "ledger_exceptions": [ + { + "reason_code": "read_error", + "path": "yara_rules", + "message": "Rule dropped.", + "fatal": False, + "scope": "rule_set", + }, + { + "reason_code": "read_error", + "path": "yara_rules", + "message": "File unreadable.", + "fatal": False, + }, + ], + "scope_exclusions": [], + "analyzer_statuses": [], + "limitations": [], + }, + } + + body = report(state)["report_body"] + + if output_format == "markdown": + assert r"| read\_error | rule set `yara_rules` | Rule dropped\. |" in body + assert r"| read\_error | `yara_rules` | File unreadable\. |" in body + else: + assert "read_error rule set yara_rules: Rule dropped." in body + assert "read_error yara_rules: File unreadable." in body + def test_report_output_format_terminal(self) -> None: """output_format terminal produces Rich-formatted output.""" state: SkillspectorState = {