From 08d49779a4d282a56c1ce4391be81cdedd7e5f20 Mon Sep 17 00:00:00 2001 From: Tomas Date: Thu, 27 Aug 2026 19:28:21 +0300 Subject: [PATCH 1/4] fix(security): honor SkillSpector completeness Signed-off-by: Tomas --- CHANGELOG.md | 4 + src/skillevaluator/validators/security.py | 64 ++++++++++++++++ tests/validators/test_security.py | 92 ++++++++++++++++++++++- 3 files changed, 157 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a2ad6515..53687d54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,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. - Tier 3 paired pass@k evidence now respects Python's active integer-string conversion limit, preserves nonzero Wilson interval widths and paired-effect directions at large case counts, and documents exact-rational omission diff --git a/src/skillevaluator/validators/security.py b/src/skillevaluator/validators/security.py index a173faba..a3e7e520 100644 --- a/src/skillevaluator/validators/security.py +++ b/src/skillevaluator/validators/security.py @@ -609,6 +609,70 @@ 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 + 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 57172f28..ae881fe7 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, @@ -911,6 +927,53 @@ 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["metadata"]["skillspector_version"] = "2.10.0" + payload["execution_successful"] = True + payload["analysis_completeness"] = { + "total_components": 4, + "scanned_components": 4, + "coverage_percent": 100.0, + "is_complete": False, + "status": "partial", + "execution_successful": True, + "fully_inspected_files": 4, + "partially_inspected_files": 0, + "entirely_uninspected_files": 0, + "ledger_exceptions": [ + {"reason_code": "reference_unresolved", "fatal": False} for _ in range(12) + ], + "scope_exclusions": [], + "analyzer_statuses": [], + "limitations": [], + } + 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( ("payload", "expected_error"), ( @@ -1051,11 +1114,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( { From 6fd3a5d22c67e74777707b20961878ef109d0059 Mon Sep 17 00:00:00 2001 From: Tomas <180413002+Tomauskasz@users.noreply.github.com> Date: Tue, 1 Sep 2026 08:09:37 +0300 Subject: [PATCH 2/4] fix(security): validate completeness details Context: SkillSpector complete-summary flags could contradict authoritative coverage, inspection-count, exception, or limitation details while SkillEvaluator still accepted the scan. Changes: - Require present completeness objects to report numeric 100% coverage, zero integer partial and uninspected counts, and empty exception and limitation lists. - Reject missing authoritative detail fields when the completeness object exists. - Add regressions for contradictory and missing details plus the legacy report path without completeness metadata. Impact: Contradictory or malformed claimed-complete reports fail closed. Older reports that omit the completeness object retain their existing validation path. Validation: - Contradictory-detail regression: 5 passed. - Missing-detail regression: 5 passed. - Focused current, contradictory, missing, and legacy cases: 12 passed. - Validator test suite: 847 passed. - Ruff and `git diff --check`: passed. Notes: The full repository suite was not run locally. Signed-off-by: Tomas <180413002+Tomauskasz@users.noreply.github.com> --- src/skillevaluator/validators/security.py | 25 +++++++ tests/validators/test_security.py | 89 +++++++++++++++++++++++ 2 files changed, 114 insertions(+) diff --git a/src/skillevaluator/validators/security.py b/src/skillevaluator/validators/security.py index a3e7e520..e0e007ab 100644 --- a/src/skillevaluator/validators/security.py +++ b/src/skillevaluator/validators/security.py @@ -672,6 +672,31 @@ def _validate_skillspector_report( 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): diff --git a/tests/validators/test_security.py b/tests/validators/test_security.py index ae881fe7..337edc98 100644 --- a/tests/validators/test_security.py +++ b/tests/validators/test_security.py @@ -974,6 +974,72 @@ def test_skillspector_2_10_partial_scan_reports_incomplete_before_recommendation 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", + ( + pytest.param({"coverage_percent": 0}, id="zero-coverage"), + pytest.param({"partially_inspected_files": 1}, id="partially-inspected-file"), + pytest.param({"entirely_uninspected_files": 1}, id="uninspected-file"), + pytest.param({"ledger_exceptions": [{"fatal": True}]}, id="fatal-ledger-exception"), + pytest.param({"limitations": ["Analyzer failed."]}, id="limitation"), + ), + ) + @patch("skillevaluator.validators.security.Tools") + def test_skillspector_complete_summary_rejects_contradictory_details( + self, + mock_tools, + sample_skill_dir: Path, + completeness_detail: dict, + ) -> None: + payload = _skillspector_json_report() + payload["analysis_completeness"].update(completeness_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( + "missing_detail", + ( + "coverage_percent", + "partially_inspected_files", + "entirely_uninspected_files", + "ledger_exceptions", + "limitations", + ), + ) + @patch("skillevaluator.validators.security.Tools") + def test_skillspector_completeness_rejects_missing_authoritative_details( + self, + mock_tools, + sample_skill_dir: Path, + missing_detail: str, + ) -> None: + payload = _skillspector_json_report() + 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"), ( @@ -1255,6 +1321,29 @@ def test_skillspector_accepts_valid_clean_report(self, mock_tools, sample_skill_ assert not result.errors assert any(detail.check_name == "skillspector" for detail in result.success_details) + @patch("skillevaluator.validators.security.Tools") + def test_skillspector_accepts_legacy_clean_report_without_completeness( + self, + mock_tools, + sample_skill_dir: Path, + ) -> None: + payload = _skillspector_json_report() + 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(payload), + stderr="", + exit_code=0, + ) + + result = SecurityValidator(use_llm=False).validate_security_only(sample_skill_dir) + + assert result.passed + assert not result.errors + assert any(detail.check_name == "skillspector" for detail in result.success_details) + @patch("skillevaluator.validators.security.Tools") def test_skillspector_accepts_valid_findings_report(self, mock_tools, sample_skill_dir: Path) -> None: issue = { From bdd69c182c747848a583925357f1fa57a9fd6e82 Mon Sep 17 00:00:00 2001 From: Tomas <180413002+Tomauskasz@users.noreply.github.com> Date: Tue, 1 Sep 2026 09:18:05 +0300 Subject: [PATCH 3/4] test(security): consolidate completeness cases Context: Contradictory and missing SkillSpector completeness details used separate parameterized tests with identical scanner setup, execution, and assertions. Use one ten-case matrix for the shared fail-closed contract. Changes: - Pair each invalid detail mutation with an optional missing-field selector. - Preserve all five contradictory-value cases and all five missing-field cases. - Keep one public validation call and one assertion body for every case. Impact: The validator behavior and coverage remain unchanged while the test removes 23 net lines of duplicated mechanics. Validation: - Focused completeness and legacy contract set: 12 passed. - `uv run pytest -q`: 5482 passed, 17 skipped, 4 deselected. - `uv run ruff check src tests`: passed. - `uv build`: source archive and wheel built successfully. - `git diff --check`: passed. Notes: The repository-wide formatter check remains red on pre-existing files and is not part of the configured Makefile lint target. --- tests/validators/test_security.py | 61 ++++++++++--------------------- 1 file changed, 19 insertions(+), 42 deletions(-) diff --git a/tests/validators/test_security.py b/tests/validators/test_security.py index ca6edf52..cf1e7412 100644 --- a/tests/validators/test_security.py +++ b/tests/validators/test_security.py @@ -1289,57 +1289,34 @@ def test_skillspector_2_10_partial_scan_reports_incomplete_before_recommendation assert not any("recommendation" in error for error in result.errors) @pytest.mark.parametrize( - "completeness_detail", + ("completeness_detail", "missing_detail"), ( - pytest.param({"coverage_percent": 0}, id="zero-coverage"), - pytest.param({"partially_inspected_files": 1}, id="partially-inspected-file"), - pytest.param({"entirely_uninspected_files": 1}, id="uninspected-file"), - pytest.param({"ledger_exceptions": [{"fatal": True}]}, id="fatal-ledger-exception"), - pytest.param({"limitations": ["Analyzer failed."]}, id="limitation"), + 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_contradictory_details( + def test_skillspector_complete_summary_rejects_invalid_details( self, mock_tools, sample_skill_dir: Path, - completeness_detail: dict, + completeness_detail: dict | None, + missing_detail: str | None, ) -> None: payload = _skillspector_json_report() - payload["analysis_completeness"].update(completeness_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( - "missing_detail", - ( - "coverage_percent", - "partially_inspected_files", - "entirely_uninspected_files", - "ledger_exceptions", - "limitations", - ), - ) - @patch("skillevaluator.validators.security.Tools") - def test_skillspector_completeness_rejects_missing_authoritative_details( - self, - mock_tools, - sample_skill_dir: Path, - missing_detail: str, - ) -> None: - payload = _skillspector_json_report() - payload["analysis_completeness"].pop(missing_detail) + 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, From 1f25501459590e9e99d8ca6c4468b5f00f6fdca7 Mon Sep 17 00:00:00 2001 From: Tomas <180413002+Tomauskasz@users.noreply.github.com> Date: Tue, 1 Sep 2026 10:56:02 +0300 Subject: [PATCH 4/4] test(security): simplify report fixtures Context: The current and legacy SkillSpector report-contract tests repeated complete fixture values and duplicated the validator execution and assertions. Changes: - Update only the completeness fields that differ in the partial-report case. - Parameterize the shared clean-report acceptance path for current and legacy metadata. Impact: Preserve the current, legacy, and incomplete report assertions while removing 20 lines of repeated fixture and mock setup. Validation: - Full suite: 5482 passed, 17 skipped, 4 deselected. - Lint: All checks passed. - Build: source distribution and wheel built successfully. - Focused report-contract cases: passed. - git diff --check: passed. Notes: None. Signed-off-by: Tomas <180413002+Tomauskasz@users.noreply.github.com> --- tests/validators/test_security.py | 56 ++++++++++--------------------- 1 file changed, 18 insertions(+), 38 deletions(-) diff --git a/tests/validators/test_security.py b/tests/validators/test_security.py index cf1e7412..11330848 100644 --- a/tests/validators/test_security.py +++ b/tests/validators/test_security.py @@ -1255,25 +1255,18 @@ def test_skillspector_2_10_partial_scan_reports_incomplete_before_recommendation "max_issue_severity": "NONE", } ) - payload["metadata"]["skillspector_version"] = "2.10.0" - payload["execution_successful"] = True - payload["analysis_completeness"] = { - "total_components": 4, - "scanned_components": 4, - "coverage_percent": 100.0, - "is_complete": False, - "status": "partial", - "execution_successful": True, - "fully_inspected_files": 4, - "partially_inspected_files": 0, - "entirely_uninspected_files": 0, - "ledger_exceptions": [ - {"reason_code": "reference_unresolved", "fatal": False} for _ in range(12) - ], - "scope_exclusions": [], - "analyzer_statuses": [], - "limitations": [], - } + 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, @@ -1596,31 +1589,18 @@ 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: - mock_tools.skillspector.is_available = True - mock_tools.skillspector.run.return_value = ToolResult( - success=True, - stdout=json.dumps(_skillspector_json_report()), - stderr="", - exit_code=0, - ) - - result = SecurityValidator(use_llm=False).validate_security_only(sample_skill_dir) - - assert result.passed - assert not result.errors - assert any(detail.check_name == "skillspector" for detail in result.success_details) - - @patch("skillevaluator.validators.security.Tools") - def test_skillspector_accepts_legacy_clean_report_without_completeness( + def test_skillspector_accepts_valid_clean_report( self, mock_tools, sample_skill_dir: Path, + legacy: bool, ) -> None: payload = _skillspector_json_report() - payload.pop("execution_successful") - payload.pop("analysis_completeness") + 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,