Skip to content

Honor SkillSpector analysis completeness - #112

Open
Tomauskasz wants to merge 5 commits into
NVIDIA:mainfrom
Tomauskasz:fix/skillspector-report-contract
Open

Honor SkillSpector analysis completeness#112
Tomauskasz wants to merge 5 commits into
NVIDIA:mainfrom
Tomauskasz:fix/skillspector-report-contract

Conversation

@Tomauskasz

@Tomauskasz Tomauskasz commented Aug 27, 2026

Copy link
Copy Markdown

Summary

  • Validate SkillSpector 2.10 execution_successful and analysis_completeness fields when present.
  • Report incomplete analysis before checking the recommendation/severity invariant.
  • Preserve compatibility with older SkillSpector reports that omit the new completeness fields.

Root cause

SkillSpector deliberately upgrades a LOW report from SAFE to CAUTION when analysis is incomplete. SkillEvaluator ignored the authoritative completeness fields and therefore rejected a valid incomplete LOW/CAUTION report as a false recommendation/severity contradiction.

The validator now fails closed on unsuccessful, malformed, contradictory, or incomplete markers. A complete LOW/CAUTION report remains invalid.

Verification

  • Full suite: 5482 passed, 17 skipped, 4 deselected
  • Focused current, legacy, and incomplete report-contract cases: 3 passed
  • Current and legacy clean-report fixtures share one parameterized test path
  • make lint: passed
  • make build: passed
  • git diff --check: passed

Compatibility

Older reports that omit both execution_successful and analysis_completeness retain the existing validation path.

AI assistance disclosure

Codex inspected the SkillEvaluator and SkillSpector report contracts, reproduced the failure, drafted the implementation and tests, and ran the verification commands. I reviewed the root cause, fail-closed behavior, compatibility boundary, diff, and final pull-request text.

Signed-off-by: Tomas <tomas.bitvinskas@copla.com>
@Tomauskasz

Copy link
Copy Markdown
Author

End-to-end follow-up:

  • Installed this fix with SkillEvaluator PR fix(tier3): decode native Codex exec calls #111, the pinned Harbor provenance backport, and SkillSpector PR #451.
  • Direct SkillSpector scan: LOW/SAFE, 100% coverage, complete analysis, zero ledger exceptions.
  • Installed Tier 1 security validation passed and exited 0.
  • One four-case paired Codex/Terra full run completed with 8 expected and 8 scored attempts, no execution errors, and a complete report.
  • All three tiers passed; with-skill score was 0.9471 versus 0.6613 baseline.

The full source suite for this PR also passed: 5160 passed, 17 skipped, 4 deselected. Lint and build passed.

@rng1995 rng1995 left a comment

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.

The new contract checks still accept contradictory incomplete evidence.

"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.

@rng1995

rng1995 commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

@Tomauskasz Gentle ping when you have a chance: the remaining completeness-contract review thread is still waiting for an update or response. Thank you for your patience throughout the review and for your contribution.

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>
Resolve the Unreleased changelog conflict by retaining both the SkillSpector
completeness fix and the independent fixes already published on main.

Validation:
- Full suite with all and dev extras: 5,482 passed, 17 skipped, 4 deselected.
- Validator suite: 1,053 passed.
- Ruff: passed.
- Source distribution and wheel build: passed.
- Conflict-marker and diff checks: passed.

Signed-off-by: Tomas <180413002+Tomauskasz@users.noreply.github.com>
@wuzhenhua24

Copy link
Copy Markdown

Independent reproduction — this matches the root cause described here exactly, and I bisected the SkillSpector side.

Minimal reproducer. A skill with no findings at all is enough; the only trigger is a / in a heading, which the reference resolver records as an unresolved local reference:

---
name: repro
description: Minimal reproducer for the SkillSpector completeness contract mismatch. Use when reproducing the reported validation failure.
---

# Repro

## Overview

Prose only. No executable content.

### Input/Output Separation

Keep inputs and outputs distinct.

Behaviour across SkillSpector versions (same skill, same SkillEvaluator commit, --no-llm):

SkillSpector score severity recommendation is_complete skillevaluator security-scan
2.9.6 0 LOW SAFE false PASS
2.10.0 0 LOW CAUTION false (status: partial) INCOMPLETE
2.11.0 0 LOW CAUTION false (status: partial) INCOMPLETE

Failure message on 2.10.0+:

skillspector JSON field 'risk_assessment.recommendation' does not match the risk severity;
security scan did not complete

Three things worth noting:

  1. The skill has zero findings and score: 0, yet the entire security scan result is discarded. The rejection is not scoped to one questionable claim in the report.
  2. 2.9.6 already reports is_complete: false for this skill — it just doesn't escalate the recommendation. So the behavioural change in 2.10.0 is the fail-closed escalation, not completeness detection. That supports reading analysis_completeness before the recommendation/severity invariant, as this PR does.
  3. The trigger surface is wide. ledger_exceptions is a single reference_unresolved from the reference_resolution phase, raised by a slash inside a Markdown heading. Prose-heavy skills hit this readily — the first real skill where I saw it had two such exceptions, one from a heading and one from a table cell.

Environment: SkillEvaluator 3bfba44 (main, reports 0.2.1), SkillSpector installed via uv tool install, macOS / Python 3.13.

Until this lands, we are pinning SkillSpector to v2.9.6 downstream, with a regression test that fails on 2.10.0+ so we can tell when the pin is safe to drop.

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.
@Tomauskasz

Copy link
Copy Markdown
Author

Follow-up from the complete three-lane review is in bdd69c1.

The ten contradictory/missing completeness-detail cases now share one parameter matrix and one scanner/assertion body. Runtime behavior and case coverage are unchanged; the test removes 23 net lines of duplicated mechanics.

Verification:

  • focused completeness and legacy contract set: 12 passed;
  • full serial suite: 5,482 passed, 17 skipped, 4 deselected;
  • repository Ruff check passed;
  • source and wheel builds passed;
  • git diff --check passed.

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>
@Tomauskasz

Copy link
Copy Markdown
Author

Final test-only polish is in 1f25501.

The partial-report regression now overrides only fields that differ from the shared fixture. Current and legacy clean-report coverage now shares one parameterized test path. Production behavior and contract coverage are unchanged; the test file is 20 lines smaller.

Final verification: 5,482 passed, 17 skipped, 4 deselected; Ruff passed; source and wheel builds passed; git diff --check passed. The independent follow-up review found no issue.

Fresh CI, DCO, and Security runs for this head are action_required and are waiting for maintainer approval of the fork workflows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants