diff --git a/CHANGELOG.md b/CHANGELOG.md index 8673520..60be1a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ All notable changes to SkillEvaluator are documented in this file. ### Fixed +- SkillSpector 2.10 incomplete scans now report the authoritative analysis- + completeness failure instead of a false recommendation/severity + contradiction. Complete reports and older reports without completeness + metadata retain their existing validation behavior. - License detection no longer treats a frontmatter `license` identifier as authoritative when a LICENSE file declares a different license. Claiming MIT while shipping GPL-3.0 now fails closed. Every LICENSE/COPYING file is diff --git a/src/skillevaluator/validators/security.py b/src/skillevaluator/validators/security.py index 62f01a3..7a90657 100644 --- a/src/skillevaluator/validators/security.py +++ b/src/skillevaluator/validators/security.py @@ -764,6 +764,95 @@ def _validate_skillspector_report( result.add_error("skillspector JSON field 'success' must be a boolean; security scan did not complete") return False + execution_successful = data.get("execution_successful") + if "execution_successful" in data and not isinstance(execution_successful, bool): + result.add_error( + "skillspector JSON field 'execution_successful' must be a boolean; " + "security scan did not complete" + ) + return False + if execution_successful is False: + result.add_error("skillspector reported execution_successful=false; security scan did not complete") + return False + + analysis_completeness = data.get("analysis_completeness") + if "analysis_completeness" in data: + if not isinstance(analysis_completeness, dict): + result.add_error( + "skillspector JSON field 'analysis_completeness' must be an object; " + "security scan did not complete" + ) + return False + is_complete = analysis_completeness.get("is_complete") + if not isinstance(is_complete, bool): + result.add_error( + "skillspector JSON field 'analysis_completeness.is_complete' must be a boolean; " + "security scan did not complete" + ) + return False + completeness_status = analysis_completeness.get("status") + if not isinstance(completeness_status, str) or completeness_status not in { + "complete", + "partial", + "failed", + }: + result.add_error( + "skillspector JSON field 'analysis_completeness.status' is not recognized; " + "security scan did not complete" + ) + return False + completeness_execution_successful = analysis_completeness.get("execution_successful") + if not isinstance(completeness_execution_successful, bool): + result.add_error( + "skillspector JSON field 'analysis_completeness.execution_successful' must be a boolean; " + "security scan did not complete" + ) + return False + if ( + execution_successful is not None + and execution_successful is not completeness_execution_successful + ): + result.add_error( + "skillspector JSON execution_successful fields contradict each other; " + "security scan did not complete" + ) + return False + if ( + not is_complete + or completeness_status != "complete" + or not completeness_execution_successful + ): + result.add_error( + "skillspector JSON field 'analysis_completeness' reports incomplete analysis " + f"(status '{completeness_status}'); security scan did not complete" + ) + return False + coverage_percent = analysis_completeness.get("coverage_percent") + incomplete_file_counts = ( + analysis_completeness.get("partially_inspected_files"), + analysis_completeness.get("entirely_uninspected_files"), + ) + ledger_exceptions = analysis_completeness.get("ledger_exceptions") + limitations = analysis_completeness.get("limitations") + if ( + isinstance(coverage_percent, bool) + or not isinstance(coverage_percent, (int, float)) + or coverage_percent != 100 + or any( + isinstance(count, bool) or not isinstance(count, int) or count != 0 + for count in incomplete_file_counts + ) + or not isinstance(ledger_exceptions, list) + or bool(ledger_exceptions) + or not isinstance(limitations, list) + or bool(limitations) + ): + result.add_error( + "skillspector JSON field 'analysis_completeness' details contradict complete analysis; " + "security scan did not complete" + ) + return False + status = data.get("status") if status is not None and not isinstance(status, str): result.add_error("skillspector JSON field 'status' must be a string; security scan did not complete") diff --git a/tests/validators/test_security.py b/tests/validators/test_security.py index 8cce7ac..1133084 100644 --- a/tests/validators/test_security.py +++ b/tests/validators/test_security.py @@ -68,9 +68,25 @@ def _skillspector_json_report( "issues": normalized_issues, "suppressed_count": 0, "suppressed": [], + "execution_successful": True, + "analysis_completeness": { + "total_components": 0, + "scanned_components": 0, + "coverage_percent": 100.0, + "is_complete": True, + "status": "complete", + "execution_successful": True, + "fully_inspected_files": 0, + "partially_inspected_files": 0, + "entirely_uninspected_files": 0, + "ledger_exceptions": [], + "scope_exclusions": [], + "analyzer_statuses": [], + "limitations": [], + }, "metadata": { "has_executable_scripts": False, - "skillspector_version": "1.0.0", + "skillspector_version": "2.10.0", "llm_requested": llm_requested, "llm_available": llm_available, "meta_analysis_applied": False, @@ -1225,6 +1241,89 @@ def test_unexpected_skillspector_suppression_fails_closed(self, mock_tools, samp assert result.status == "incomplete" assert any("unexpected suppressed findings" in error.lower() for error in result.errors) + @patch("skillevaluator.validators.security.Tools") + def test_skillspector_2_10_partial_scan_reports_incomplete_before_recommendation( + self, + mock_tools, + sample_skill_dir: Path, + ) -> None: + """LOW/CAUTION is the documented projection of an incomplete 2.10 scan.""" + payload = _skillspector_json_report() + payload["risk_assessment"].update( + { + "recommendation": "CAUTION", + "max_issue_severity": "NONE", + } + ) + payload["analysis_completeness"].update( + { + "total_components": 4, + "scanned_components": 4, + "is_complete": False, + "status": "partial", + "fully_inspected_files": 4, + "ledger_exceptions": [ + {"reason_code": "reference_unresolved", "fatal": False} for _ in range(12) + ], + } + ) + mock_tools.skillspector.is_available = True + mock_tools.skillspector.run.return_value = ToolResult( + success=True, + stdout=json.dumps(payload), + stderr="", + exit_code=0, + ) + + result = SecurityValidator(use_llm=False).validate_security_only(sample_skill_dir) + + assert result.status == "incomplete" + assert any("analysis_completeness" in error and "partial" in error for error in result.errors) + assert not any("recommendation" in error for error in result.errors) + + @pytest.mark.parametrize( + ("completeness_detail", "missing_detail"), + ( + pytest.param({"coverage_percent": 0}, None, id="zero-coverage"), + pytest.param({"partially_inspected_files": 1}, None, id="partially-inspected-file"), + pytest.param({"entirely_uninspected_files": 1}, None, id="uninspected-file"), + pytest.param({"ledger_exceptions": [{"fatal": True}]}, None, id="fatal-ledger-exception"), + pytest.param({"limitations": ["Analyzer failed."]}, None, id="limitation"), + pytest.param(None, "coverage_percent", id="coverage_percent"), + pytest.param(None, "partially_inspected_files", id="partially_inspected_files"), + pytest.param(None, "entirely_uninspected_files", id="entirely_uninspected_files"), + pytest.param(None, "ledger_exceptions", id="ledger_exceptions"), + pytest.param(None, "limitations", id="limitations"), + ), + ) + @patch("skillevaluator.validators.security.Tools") + def test_skillspector_complete_summary_rejects_invalid_details( + self, + mock_tools, + sample_skill_dir: Path, + completeness_detail: dict | None, + missing_detail: str | None, + ) -> None: + payload = _skillspector_json_report() + if missing_detail is None: + assert completeness_detail is not None + payload["analysis_completeness"].update(completeness_detail) + else: + payload["analysis_completeness"].pop(missing_detail) + mock_tools.skillspector.is_available = True + mock_tools.skillspector.run.return_value = ToolResult( + success=True, + stdout=json.dumps(payload), + stderr="", + exit_code=0, + ) + + result = SecurityValidator(use_llm=False).validate_security_only(sample_skill_dir) + + assert result.status == "incomplete" + assert any("analysis_completeness" in error for error in result.errors) + assert not any(detail.check_name == "skillspector" for detail in result.success_details) + @pytest.mark.parametrize( ("payload", "expected_error"), ( @@ -1365,11 +1464,34 @@ def test_unexpected_skillspector_suppression_fails_closed(self, mock_tools, samp ), pytest.param( { - "risk_assessment": {"score": 0, "severity": "LOW", "recommendation": "DO_NOT_INSTALL"}, + "risk_assessment": {"score": 0, "severity": "LOW", "recommendation": "CAUTION"}, "issues": [], }, "risk_assessment.recommendation", - id="contradictory-risk-recommendation", + id="complete-low-caution-recommendation", + ), + pytest.param( + { + "execution_successful": False, + "risk_assessment": {"score": 0, "severity": "LOW", "recommendation": "SAFE"}, + "issues": [], + }, + "execution_successful=false", + id="unsuccessful-execution", + ), + pytest.param( + { + "execution_successful": True, + "analysis_completeness": { + "is_complete": True, + "status": "complete", + "execution_successful": "true", + }, + "risk_assessment": {"score": 0, "severity": "LOW", "recommendation": "SAFE"}, + "issues": [], + }, + "analysis_completeness.execution_successful", + id="invalid-completeness-execution-marker", ), pytest.param( { @@ -1467,12 +1589,22 @@ def test_deterministic_skillspector_stage_rejects_llm_metadata( assert result.status == "incomplete" assert any("--no-llm" in error for error in result.errors) + @pytest.mark.parametrize("legacy", [False, True], ids=["current", "legacy"]) @patch("skillevaluator.validators.security.Tools") - def test_skillspector_accepts_valid_clean_report(self, mock_tools, sample_skill_dir: Path) -> None: + def test_skillspector_accepts_valid_clean_report( + self, + mock_tools, + sample_skill_dir: Path, + legacy: bool, + ) -> None: + payload = _skillspector_json_report() + if legacy: + payload.pop("execution_successful") + payload.pop("analysis_completeness") mock_tools.skillspector.is_available = True mock_tools.skillspector.run.return_value = ToolResult( success=True, - stdout=json.dumps(_skillspector_json_report()), + stdout=json.dumps(payload), stderr="", exit_code=0, )