diff --git a/src/skillspector/cli.py b/src/skillspector/cli.py index 35718800..c21f918a 100644 --- a/src/skillspector/cli.py +++ b/src/skillspector/cli.py @@ -44,6 +44,7 @@ build_baseline_dict, discover_baseline, dump_baseline, + effective_findings, load_baseline, ) @@ -510,8 +511,7 @@ def _scan_multi_skill( continue score = result.get("risk_score", 0) severity = result.get("risk_severity", "LOW") - filtered = result.get("filtered_findings") or result.get("findings") - finding_count = len(filtered) if isinstance(filtered, list) else 0 + finding_count = len(effective_findings(result)) execution = "failed" if result.get("execution_successful") is False else "successful" console.print( f" {skill.name:<30} {score:<8} {severity:<12} {finding_count:<10} {execution:<10}" @@ -533,8 +533,7 @@ def _scan_multi_skill( combined_skills.append({"name": skill.name, "error": result["error"]}) else: payload = _recursive_json_payload(result) or {} - selected_findings = result.get("filtered_findings") or result.get("findings") or [] - finding_count = len(selected_findings) if isinstance(selected_findings, list) else 0 + finding_count = len(effective_findings(result)) entry = { "name": skill.name, "path": skill.relative_path, @@ -668,7 +667,7 @@ def baseline( state = _scan_state(input_path, FormatChoice.json, no_llm) state["baseline_path"] = os.path.abspath(output.expanduser()) result = graph.invoke(state) - findings = result.get("filtered_findings") or result.get("findings") or [] + findings = effective_findings(result) data = build_baseline_dict( findings, reason=reason, diff --git a/src/skillspector/mcp_server.py b/src/skillspector/mcp_server.py index 2fbababb..90d16c17 100644 --- a/src/skillspector/mcp_server.py +++ b/src/skillspector/mcp_server.py @@ -36,6 +36,7 @@ from skillspector.graph import graph from skillspector.llm_utils import is_llm_available from skillspector.logging_config import get_logger +from skillspector.suppression import effective_findings if TYPE_CHECKING: from mcp.server.fastmcp import FastMCP @@ -137,7 +138,7 @@ async def run_scan( }, }, ) - findings = result.get("filtered_findings") or result.get("findings") or [] + findings = effective_findings(result) risk_score = int(result.get("risk_score") or 0) execution_successful = bool(result.get("execution_successful", True)) analysis_completeness = result.get("analysis_completeness") or {} diff --git a/src/skillspector/suppression.py b/src/skillspector/suppression.py index 719f48ef..c4dc5354 100644 --- a/src/skillspector/suppression.py +++ b/src/skillspector/suppression.py @@ -387,6 +387,49 @@ def partition_findings( return kept, suppressed +def effective_findings(result: Mapping[str, object]) -> list[Finding]: + """Return the findings from a graph *result* that actually drove its risk score. + + The report node returns ``filtered_findings`` as the full pre-partition set + (kept plus baseline-suppressed) alongside ``suppressed_findings``, but scores + and SARIF results from the kept subset alone. Consumers that want the numbers + the report itself published must therefore subtract the suppressed partition. + + Two failure modes this exists to prevent, both of which over-report: + + * ``result.get("filtered_findings") or result.get("findings")`` treats an + empty filtered list as absent and falls back to the raw pre-filter + findings. An empty list is a real answer -- every finding was filtered out + or suppressed -- not a missing one. + * Using ``filtered_findings`` directly counts baseline-suppressed findings + that the report excluded from the score, so a fully suppressed skill + reports risk 0 alongside a non-zero finding count. + + Falls back to the raw ``findings`` list only when ``filtered_findings`` is + absent or malformed, and does not subtract there: raw findings are not the + population that produced ``suppressed_findings``. + """ + filtered = result.get("filtered_findings") + if not isinstance(filtered, list): + raw = result.get("findings") + return list(raw) if isinstance(raw, list) else [] + + suppressed = result.get("suppressed_findings") + if not isinstance(suppressed, list) or not suppressed: + return list(filtered) + + suppressed_ids = { + entry.finding.finding_id + for entry in suppressed + if isinstance(entry, SuppressedFinding) and entry.finding is not None + } + return [ + finding + for finding in filtered + if not isinstance(finding, Finding) or finding.finding_id not in suppressed_ids + ] + + def build_baseline_dict( findings: list[Finding], reason: str = "Accepted finding (auto-generated baseline)", diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index 438d5827..a61900ca 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -16,6 +16,7 @@ """Tests for skillspector CLI (skillspector scan, --version).""" import json +import re from pathlib import Path from types import SimpleNamespace from typing import Any @@ -28,7 +29,9 @@ from skillspector import __version__ from skillspector.cli import FormatChoice, _scan_multi_skill, app +from skillspector.models import Finding from skillspector.multi_skill import MultiSkillDetectionResult, SkillDirectory +from skillspector.suppression import SuppressedFinding runner = CliRunner() @@ -1133,3 +1136,139 @@ def fake_invoke(state: dict[str, Any], config: Any = None) -> dict[str, Any]: assert payload["issues"] == [{"id": "X-1", "severity": "low"}] assert payload["suppressed_count"] == 0 assert payload["suppressed"] == [] + + +def _combined_json_counts(results: list[dict[str, Any]], tmp_path: Path) -> list[int]: + """Run a recursive JSON scan over stubbed results and return per-skill counts.""" + skills = [ + SkillDirectory(path=tmp_path / f"skill{i}", name=f"skill{i}", relative_path=f"skill{i}") + for i in range(1, len(results) + 1) + ] + detection = MultiSkillDetectionResult(is_multi_skill=True, skills=skills, has_root_skill=False) + out = tmp_path / "combined.json" + + with patch("skillspector.cli.graph.invoke", side_effect=results): + _scan_multi_skill( + detection, FormatChoice.json, out, no_llm=True, yara_rules_dir=None, verbose=False + ) + + data = json.loads(out.read_text(encoding="utf-8")) + return [entry["finding_count"] for entry in data["skills"]] + + +def test_cli_recursive_json_count_excludes_suppressed_findings(tmp_path: Path) -> None: + """Combined JSON counts the active findings, not the pre-partition set. + + `report` returns `filtered_findings` as kept+suppressed and scores only the + kept subset, so counting `filtered_findings` made a fully suppressed + sub-skill report risk 0 alongside a non-zero finding count. + """ + findings = [ + Finding(rule_id="SQP-1", message="one"), + Finding(rule_id="SQP-2", message="two"), + Finding(rule_id="SQP-3", message="three"), + ] + fully_suppressed = { + "report_body": "{}", + "risk_score": 0, + "risk_severity": "LOW", + "findings": list(findings), + "filtered_findings": list(findings), + "suppressed_findings": [ + SuppressedFinding(finding=finding, reason="baselined") for finding in findings + ], + } + partly_suppressed = { + "report_body": "{}", + "risk_score": 20, + "risk_severity": "LOW", + "findings": list(findings), + "filtered_findings": list(findings), + "suppressed_findings": [ + SuppressedFinding(finding=finding, reason="baselined") for finding in findings[:2] + ], + } + + assert _combined_json_counts([fully_suppressed, partly_suppressed], tmp_path) == [0, 1] + + +def test_cli_recursive_json_count_respects_an_empty_filtered_list(tmp_path: Path) -> None: + """Every-finding-filtered is reported as 0, not as the raw pre-filter count.""" + result = { + "report_body": "{}", + "risk_score": 0, + "risk_severity": "LOW", + "findings": [Finding(rule_id="SQP-1", message="one")], + "filtered_findings": [], + "suppressed_findings": [], + } + + assert _combined_json_counts([result], tmp_path) == [0] + + +def test_cli_recursive_summary_count_excludes_suppressed( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """The terminal summary's Findings column uses the same active count. + + Pinned separately from the JSON path: the two call sites are independent + lines, so a regression in one is invisible to a test covering the other. + """ + findings = [Finding(rule_id="SQP-1", message="one"), Finding(rule_id="SQP-2", message="two")] + result = { + "report_body": "# report", + "risk_score": 0, + "risk_severity": "LOW", + "findings": list(findings), + "filtered_findings": list(findings), + "suppressed_findings": [ + SuppressedFinding(finding=finding, reason="baselined") for finding in findings + ], + } + detection = MultiSkillDetectionResult( + is_multi_skill=True, + skills=[SkillDirectory(path=tmp_path / "solo", name="solo", relative_path="solo")], + has_root_skill=False, + ) + + with patch("skillspector.cli.graph.invoke", side_effect=[result]): + _scan_multi_skill( + detection, FormatChoice.terminal, None, no_llm=True, yara_rules_dir=None, verbose=False + ) + + summary = re.sub(r"\x1b\[[0-9;]*m", "", capsys.readouterr().out) + row = next(line for line in summary.splitlines() if line.strip().startswith("solo")) + assert row.split() == ["solo", "0", "LOW", "0", "successful"] + + +def test_cli_baseline_command_excludes_filtered_out_findings(tmp_path: Path) -> None: + """`skillspector baseline` fingerprints what the scan reported, not raw findings. + + Closes a mutation survivor: reverting this call site to the old + `filtered_findings or findings` passed the entire suite, because nothing + drove the baseline command through an empty filtered list. An empty filtered + list means every finding was filtered out, so building a baseline from the + raw list would write fingerprints suppressing findings the scan never + reported, and would fail closed on the next run for no reason. + """ + skill = tmp_path / "skill" + skill.mkdir() + source = "---\nname: b\n---\nbody\n" + (skill / "SKILL.md").write_text(source, encoding="utf-8") + out = tmp_path / "baseline.yaml" + + result = { + "findings": [Finding(rule_id="SQP-1", message="one", file="SKILL.md")], + "filtered_findings": [], + "suppressed_findings": [], + "file_cache": {"SKILL.md": source}, + "risk_score": 0, + } + + with patch("skillspector.cli.graph.invoke", return_value=result): + invocation = runner.invoke(app, ["baseline", str(skill), "-o", str(out), "--no-llm"]) + + assert invocation.exit_code == 0, invocation.output + written = yaml.safe_load(out.read_text(encoding="utf-8")) + assert written.get("fingerprints", []) == [] + assert "0 suppressed finding(s)" in re.sub(r"\x1b\[[0-9;]*m", "", invocation.output) diff --git a/tests/unit/test_mcp_server.py b/tests/unit/test_mcp_server.py index bccc9f4f..59b8342a 100644 --- a/tests/unit/test_mcp_server.py +++ b/tests/unit/test_mcp_server.py @@ -26,7 +26,9 @@ from skillspector import mcp_server from skillspector.mcp_server import run_scan +from skillspector.models import Finding from skillspector.providers import reset_provider, use_provider +from skillspector.suppression import SuppressedFinding def _write_skill(tmp_path: Path, body: str = "# Safe skill") -> Path: @@ -478,3 +480,48 @@ async def test_mcp_stdio_initialize_registers_scan_skill() -> None: tools = await asyncio.wait_for(session.list_tools(), timeout=15) assert "scan_skill" in {tool.name for tool in tools.tools} + + +async def test_run_scan_findings_exclude_the_suppressed_partition( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The MCP verdict lists the findings that drove the score, not kept+suppressed. + + `run_scan` serialises this list straight to the calling agent, so a + baseline-suppressed finding leaking in tells the agent a skill is dirtier + than the risk score it is gating on. + """ + kept = Finding(rule_id="SQP-1", message="kept") + dropped = Finding(rule_id="SQP-2", message="suppressed") + result = { + "findings": [kept, dropped], + "filtered_findings": [kept, dropped], + "suppressed_findings": [SuppressedFinding(finding=dropped, reason="baselined")], + "risk_score": 10, + "risk_severity": "LOW", + "report_body": "# report", + } + monkeypatch.setattr(mcp_server.graph, "ainvoke", AsyncMock(return_value=result)) + + verdict = await run_scan(str(_write_skill(tmp_path)), use_llm=False, output_format="json") + + assert [finding["id"] for finding in verdict["findings"]] == ["SQP-1"] + + +async def test_run_scan_respects_an_empty_filtered_list( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Every-finding-filtered reports no findings, not the raw pre-filter list.""" + result = { + "findings": [Finding(rule_id="SQP-1", message="one")], + "filtered_findings": [], + "suppressed_findings": [], + "risk_score": 0, + "risk_severity": "LOW", + "report_body": "# report", + } + monkeypatch.setattr(mcp_server.graph, "ainvoke", AsyncMock(return_value=result)) + + verdict = await run_scan(str(_write_skill(tmp_path)), use_llm=False, output_format="json") + + assert verdict["findings"] == [] diff --git a/tests/unit/test_suppression.py b/tests/unit/test_suppression.py index e2eaa8b7..3a88b897 100644 --- a/tests/unit/test_suppression.py +++ b/tests/unit/test_suppression.py @@ -26,11 +26,13 @@ from skillspector.suppression import ( SHIPPED_BASELINE_FILENAME, Baseline, + SuppressedFinding, SuppressionRule, baseline_from_dict, build_baseline_dict, discover_baseline, dump_baseline, + effective_findings, finding_fingerprint, load_baseline, partition_findings, @@ -578,3 +580,178 @@ def test_discover_baseline_ignores_directory_named_like_baseline(tmp_path: Path) d = tmp_path / SHIPPED_BASELINE_FILENAME d.mkdir() assert discover_baseline(tmp_path) is None + + +def _partitioned_finding(rule_id: str) -> Finding: + """Build a distinct finding with a stable, inspectable rule id.""" + return Finding(rule_id=rule_id, message=f"message for {rule_id}", file="SKILL.md") + + +def test_effective_findings_keeps_an_empty_filtered_list() -> None: + """An empty filtered list is a real answer, not a missing one. + + The previous `filtered_findings or findings` idiom treated `[]` as falsy and + fell back to the raw pre-filter findings, over-reporting a skill whose + findings were all filtered out. + """ + raw = [_partitioned_finding("SQP-1"), _partitioned_finding("SQP-2")] + result = {"findings": raw, "filtered_findings": [], "suppressed_findings": []} + + assert effective_findings(result) == [] + + +def test_effective_findings_subtracts_the_suppressed_partition() -> None: + """`filtered_findings` is kept+suppressed, so suppressed must be removed.""" + kept = _partitioned_finding("SQP-1") + dropped = _partitioned_finding("SQP-2") + result = { + "findings": [kept, dropped], + "filtered_findings": [kept, dropped], + "suppressed_findings": [SuppressedFinding(finding=dropped, reason="baselined")], + } + + assert effective_findings(result) == [kept] + + +def test_effective_findings_fully_suppressed_skill_reports_none() -> None: + """A fully baselined skill scores 0, so it must report 0 findings too.""" + findings = [_partitioned_finding("SQP-1"), _partitioned_finding("SQP-2")] + result = { + "findings": findings, + "filtered_findings": list(findings), + "suppressed_findings": [ + SuppressedFinding(finding=finding, reason="baselined") for finding in findings + ], + } + + assert effective_findings(result) == [] + + +def test_effective_findings_passes_through_without_a_baseline() -> None: + """With nothing suppressed the filtered set is returned unchanged.""" + findings = [_partitioned_finding("SQP-1"), _partitioned_finding("SQP-2")] + result = {"findings": findings, "filtered_findings": list(findings)} + + assert effective_findings(result) == findings + + +def test_effective_findings_falls_back_to_raw_findings_without_subtracting() -> None: + """Raw findings are not the population that produced `suppressed_findings`. + + When `filtered_findings` is absent the report never ran its partition, so + subtracting a suppressed list against the raw findings would be unsound. + """ + raw = [_partitioned_finding("SQP-1"), _partitioned_finding("SQP-2")] + result = { + "findings": raw, + "suppressed_findings": [SuppressedFinding(finding=raw[0], reason="baselined")], + } + + assert effective_findings(result) == raw + + +@pytest.mark.parametrize("malformed", ["not-a-list", 7, None, {}]) +def test_effective_findings_treats_malformed_filtered_as_absent(malformed: object) -> None: + """A non-list `filtered_findings` degrades to the raw list, never to a crash.""" + raw = [_partitioned_finding("SQP-1")] + + assert effective_findings({"findings": raw, "filtered_findings": malformed}) == raw + + +def test_effective_findings_on_an_empty_result_is_empty() -> None: + """A result carrying neither key yields no findings rather than raising.""" + assert effective_findings({}) == [] + assert effective_findings({"findings": "malformed"}) == [] + + +def test_effective_findings_matches_on_finding_id_not_rule_id() -> None: + """Suppression is keyed on finding_id, so a shared rule_id must not over-subtract. + + Closes a mutation survivor: swapping the match key to rule_id passed the + whole suite, because no test had a kept and a suppressed finding sharing + one. Two hits of the same rule at different sites is the common case, and + keying on rule_id would silently drop the finding that was never baselined. + """ + kept = Finding(rule_id="SQP-1", message="first site", file="a.md") + dropped = Finding(rule_id="SQP-1", message="second site", file="b.md") + result = { + "findings": [kept, dropped], + "filtered_findings": [kept, dropped], + "suppressed_findings": [SuppressedFinding(finding=dropped, reason="baselined")], + } + + assert effective_findings(result) == [kept] + + +def test_effective_findings_ignores_malformed_suppressed_entries() -> None: + """A malformed suppressed entry is skipped rather than crashing the report.""" + kept = _partitioned_finding("SQP-1") + result = { + "filtered_findings": [kept], + "suppressed_findings": ["not-a-suppressed-finding", None, 42], + } + + assert effective_findings(result) == [kept] + + +def test_effective_findings_keeps_non_finding_members() -> None: + """A non-Finding member of filtered_findings is passed through, not dropped. + + The helper cannot establish a foreign object's identity, so it fails open on + that member. Silently removing it would under-report a security finding, + which is the worse direction to be wrong in. + """ + kept = _partitioned_finding("SQP-1") + foreign = {"rule_id": "SQP-2"} + result = { + "filtered_findings": [kept, foreign], + "suppressed_findings": [SuppressedFinding(finding=kept, reason="baselined")], + } + + assert effective_findings(result) == [foreign] + + +def test_effective_findings_ignores_suppressed_outside_the_filtered_population() -> None: + """A suppressed entry absent from `filtered_findings` removes nothing. + + Subtraction is by membership, so an id that is not in the filtered + population is simply not found. This pins that the helper never removes an + extra member to balance an unmatched suppressed entry. + """ + kept = _partitioned_finding("SQP-1") + stranger = _partitioned_finding("SQP-9") + result = { + "filtered_findings": [kept], + "suppressed_findings": [SuppressedFinding(finding=stranger, reason="baselined")], + } + + assert effective_findings(result) == [kept] + + +@pytest.mark.parametrize("malformed", ["not-a-list", 42, 3.5, {"a": 1}]) +def test_effective_findings_treats_a_non_list_suppressed_as_nothing_suppressed( + malformed: object, +) -> None: + """A malformed `suppressed_findings` container subtracts nothing. + + The container type check earns its place on the non-iterable cases: without + it, an int or float here raises TypeError out of the comprehension and takes + down the whole report instead of degrading to "nothing suppressed". + """ + findings = [_partitioned_finding("SQP-1"), _partitioned_finding("SQP-2")] + + assert ( + effective_findings({"filtered_findings": list(findings), "suppressed_findings": malformed}) + == findings + ) + + +def test_effective_findings_skips_a_suppressed_entry_with_no_finding() -> None: + """A SuppressedFinding carrying no finding is skipped, not dereferenced.""" + kept = _partitioned_finding("SQP-1") + result = { + "filtered_findings": [kept], + "suppressed_findings": [SuppressedFinding(finding=None, reason="malformed")], # type: ignore[arg-type] + } + + assert effective_findings(result) == [kept]