fix: address second-round SkillEvaluator review feedback
Review feedback from NVIDIA (Nir Paz), minus the LLM items (declined on the thread: cost-by-default + prompt-injection surface; static-only also keeps the timeout moot at ~1.5s vs the 120s ceiling): - Incomplete-validator findings are now PRESERVED as partial evidence; only the validator's pass/fail verdict is excluded from the advisory verdict. A report with findings from an incomplete check no longer reads as clean. - Clean-report wording is now "no findings from completed checks" whenever any validator was incomplete. - Pinned both scanner binaries to known releases in code comments, config guidance, and docs: SkillEvaluator v0.1.0, SkillSpector v2.9.5. - Tests: 29 (was 28) — partial-evidence preservation flips the old discard-pinning test, plus the completed-checks wording case.
This commit is contained in:
@@ -2004,7 +2004,7 @@ DEFAULT_CONFIG = {
|
||||
# guard (which stays the enforcement layer) and only when the
|
||||
# optional `skillevaluator` binary is on PATH:
|
||||
# uv tool install --python 3.13 \
|
||||
# "skillevaluator @ git+https://github.com/NVIDIA/SkillEvaluator.git"
|
||||
# "skillevaluator @ git+https://github.com/NVIDIA/SkillEvaluator.git@v0.1.0"
|
||||
# Findings are informational — shown with file/line before the
|
||||
# install confirmation, never blocking. Secrets-class findings
|
||||
# (private keys, tokens, credentialed connection strings) are
|
||||
|
||||
@@ -102,7 +102,9 @@ class TestParseReport:
|
||||
assert report.findings == []
|
||||
assert report.incomplete_checks == ["Security Scan"]
|
||||
|
||||
def test_incomplete_check_findings_not_collected(self):
|
||||
def test_incomplete_check_findings_preserved(self):
|
||||
"""Partial evidence from an incomplete validator is kept as findings
|
||||
(Nir Paz review) — only the validator's pass/fail verdict is excluded."""
|
||||
raw = _report_json([])
|
||||
raw["results"].append({
|
||||
"validator": "Security Scan",
|
||||
@@ -111,7 +113,23 @@ class TestParseReport:
|
||||
"findings": [_finding("hardcoded_secrets", severity="critical")],
|
||||
})
|
||||
report = _parse_report(raw)
|
||||
assert report.findings == []
|
||||
assert len(report.findings) == 1
|
||||
assert report.findings[0].is_secrets_class
|
||||
assert report.incomplete_checks == ["Security Scan"]
|
||||
# findings present -> report is not clean, even though the only
|
||||
# failing validator was incomplete
|
||||
assert not report.passed
|
||||
|
||||
def test_incomplete_check_without_findings_stays_passed(self):
|
||||
raw = _report_json([])
|
||||
raw["results"].append({
|
||||
"validator": "Security Scan",
|
||||
"passed": False,
|
||||
"status": "incomplete",
|
||||
"findings": [],
|
||||
})
|
||||
report = _parse_report(raw)
|
||||
assert report.passed
|
||||
|
||||
def test_complete_failed_check_still_fails(self):
|
||||
report = _parse_report(_report_json([_finding("emails")]))
|
||||
|
||||
@@ -28,7 +28,7 @@ Design contract (deliberate):
|
||||
The scanner binary is optional::
|
||||
|
||||
uv tool install --python 3.13 \
|
||||
"skillevaluator @ git+https://github.com/NVIDIA/SkillEvaluator.git"
|
||||
"skillevaluator @ git+https://github.com/NVIDIA/SkillEvaluator.git@v0.1.0"
|
||||
|
||||
Enable/disable via ``skills.tier1_advisory`` in config.yaml (default: on;
|
||||
a no-op unless the binary is installed).
|
||||
@@ -57,7 +57,7 @@ SCANNER_NAME = "skillevaluator-tier1"
|
||||
#
|
||||
# `security` invokes NVIDIA SkillSpector (a second optional binary,
|
||||
# pinned separately: uv tool install
|
||||
# "git+https://github.com/NVIDIA/SkillSpector.git") in its static-rules
|
||||
# "git+https://github.com/NVIDIA/SkillSpector.git@v2.9.5") in its static-rules
|
||||
# mode — still keyless, no LLM calls. When SkillSpector is absent or its
|
||||
# report fails SkillEvaluator's internal consistency checks, the check
|
||||
# reports status="incomplete" and is treated as "no opinion" here.
|
||||
@@ -143,22 +143,22 @@ def tier1_advisory_enabled() -> bool:
|
||||
def _parse_report(report: dict) -> Tier1Report:
|
||||
"""Reduce a SkillEvaluator JSON report to install-relevant findings.
|
||||
|
||||
A validator whose ``status`` is ``"incomplete"`` produced no usable
|
||||
evidence (e.g. SkillSpector missing, or its report failed
|
||||
SkillEvaluator's internal consistency checks) — its fail verdict
|
||||
carries no findings, so it is recorded as an incomplete check and
|
||||
excluded from the pass/fail signal rather than rendered as an
|
||||
unexplained failure.
|
||||
A validator whose ``status`` is ``"incomplete"`` produced partial
|
||||
evidence at best (e.g. SkillSpector missing, or its report failed
|
||||
SkillEvaluator's internal consistency checks). Its findings ARE
|
||||
kept — partial evidence is still evidence — but the validator is
|
||||
excluded from the pass/fail signal, so an evidence-free fail
|
||||
verdict can't render as an unexplained failure.
|
||||
"""
|
||||
findings: List[Tier1Finding] = []
|
||||
incomplete: List[str] = []
|
||||
any_complete_failed = False
|
||||
for res in report.get("results", []) or []:
|
||||
validator = str(res.get("validator", "unknown"))
|
||||
if str(res.get("status", "")).lower() == "incomplete":
|
||||
is_incomplete = str(res.get("status", "")).lower() == "incomplete"
|
||||
if is_incomplete:
|
||||
incomplete.append(validator)
|
||||
continue
|
||||
if not res.get("passed", True):
|
||||
elif not res.get("passed", True):
|
||||
any_complete_failed = True
|
||||
for f in res.get("findings", []) or []:
|
||||
if not isinstance(f, dict):
|
||||
@@ -219,7 +219,10 @@ def format_tier1_report(report: Tier1Report, limit: int = 10) -> str:
|
||||
return ""
|
||||
lines: List[str] = []
|
||||
if not report.findings:
|
||||
lines.append("SkillEvaluator Tier 1: no findings.")
|
||||
if report.incomplete_checks:
|
||||
lines.append("SkillEvaluator Tier 1: no findings from completed checks.")
|
||||
else:
|
||||
lines.append("SkillEvaluator Tier 1: no findings.")
|
||||
else:
|
||||
lines.append(
|
||||
f"SkillEvaluator Tier 1 (advisory): "
|
||||
|
||||
@@ -355,8 +355,8 @@ the `security` check; without it that check simply reports "not run"):
|
||||
|
||||
```bash
|
||||
uv tool install --python 3.13 \
|
||||
"skillevaluator @ git+https://github.com/NVIDIA/SkillEvaluator.git"
|
||||
uv tool install "git+https://github.com/NVIDIA/SkillSpector.git"
|
||||
"skillevaluator @ git+https://github.com/NVIDIA/SkillEvaluator.git@v0.1.0"
|
||||
uv tool install "git+https://github.com/NVIDIA/SkillSpector.git@v2.9.5"
|
||||
```
|
||||
|
||||
Without the binary on PATH the scan is silently skipped. To turn it off
|
||||
|
||||
Reference in New Issue
Block a user