Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
89 changes: 89 additions & 0 deletions src/skillevaluator/validators/security.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 (

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These three flags do not cover the full completeness contract. A report with is_complete=true, status=complete, and both execution flags true is accepted as SAFE even when coverage_percent=0, entirely_uninspected_files=1, or ledger_exceptions contains a fatal entry. Validate those authoritative fields and reject contradictions so an incomplete SkillSpector report cannot fail open; add regressions for the contradictory cases.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 6fd3a5d and updated onto current main in merge commit 4ffb55c. A claimed-complete report now requires present authoritative details with numeric 100% coverage, zero integer partial/uninspected counts, and empty ledger-exception/limitation lists. Missing or contradictory details fail closed; reports that omit analysis_completeness entirely retain the legacy path.

Verification after merging current main: 1,053 validator tests passed; the full suite passed with 5,482 passed, 17 skipped, and 4 deselected; Ruff passed; source and wheel builds passed; git diff --check passed. The merge also incorporates main's Gitleaks workflow repair.

Fresh CI, DCO, and Security runs were created, but GitHub marked them action_required before running. The contributor account cannot approve fork workflows, so they are waiting for maintainer approval.

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")
Expand Down
142 changes: 137 additions & 5 deletions tests/validators/test_security.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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"),
(
Expand Down Expand Up @@ -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(
{
Expand Down Expand Up @@ -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,
)
Expand Down