Skip to content

fix(batch): score gap-fill findings - #772

Open
efegokdemir wants to merge 2 commits into
NVIDIA:mainfrom
efegokdemir:fix/batch-gap-fill-risk-assessment
Open

efegokdemir wants to merge 2 commits into
NVIDIA:mainfrom
efegokdemir:fix/batch-gap-fill-risk-assessment

Conversation

@efegokdemir

@efegokdemir efegokdemir commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #771.

Batch scans now recalculate risk from gap-fill findings using the shared scoring policy, so the score, recommendation, Markdown details and exit status agree. The failure threshold remains the existing score > 50 rule. In the pre-fix code, the reported high-risk case scored 0/100 and returned SAFE; the regression now covers the corrected verdict and the incomplete-scan CAUTION case.

  • Focused regression: test_gap_fill_preserves_caution_for_incomplete_scan (3 passed in focused selection).
  • Ruff check, Ruff format check and git diff --check passed.

Signed-off-by: Efe <efe@rexcode.co.uk>

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

[SkillSpector Review]

@efegokdemir thank you for closing the gap where multilingual gap-fill findings appeared in the batch report but never counted toward its verdict.

Value and readiness

The fix is correct and valuable. On main, run_one builds entry["risk_assessment"] from the graph result and only then appends gap-fill issues. A non-English skill with three HIGH gap-fill findings therefore still reports 0/100 LOW SAFE, is left out of the Markdown HIGH/CRITICAL section, and exits 0. The PR recomputes the score with the report node's own _compute_risk_score, over active_findings + gap_findings with the graph's has_executable_scripts and component_metadata. Score, band, recommendation and the batch's score > 50 exit rule now agree with the issues list. One test gap on the fail-closed path must be closed before merge.

Material findings

  1. Blocker: contrib/batch_scan/runner.py:801. The one piece of new decision logic, keeping CAUTION when report() already failed closed (an incomplete or degraded scan upgraded from SAFE to CAUTION), has no test. Both new tests start from SAFE or use an empty gap-fill result, so deleting this branch would still pass CI. An incomplete scan plus one LOW gap-fill finding would then be relabeled SAFE. Please add a case where the graph result has risk_recommendation="CAUTION" with a SAFE-band score (as report() produces for an incomplete scan), gap-fill returns one LOW finding, and the test asserts the recommendation stays CAUTION while the score includes the finding.

Optional:

  • The PR description says --fail-on-findings exit status now reflects gap-fill findings. contrib.batch_scan has no such flag; its exit 1 comes from score > 50. The code is right, but please correct the description so release notes do not overstate it.
  • _compute_risk_score is a private report-node helper. Contrib already imports private internals, but a one-line comment naming report() as the policy source would help keep the two in sync.

PIC tradeoffs

The gap-fill rule IDs (P5–P8, MP1–MP3, RA1–RA2) are core static rule IDs, so the shared per-rule diminishing-returns policy applies across static and gap-fill findings. LLM gap-fill findings with confidence ≥ 0.7 can now move a skill to DO NOT INSTALL and exit 1. I believe that is the intended outcome of #771, but it is a verdict-policy change for batch users.

Verification and gaps

  • Read entry_from_result, run_one (merged with main, including #796), run_gap_fill, _compute_risk_score and the report node's fail-closed block. The graph state carries active_findings, component_metadata and has_executable_scripts, and report() only ever upgrades SAFE to CAUTION, which this PR mirrors.
  • The first new test asserts 75 / HIGH / DO NOT INSTALL, the Markdown HIGH/CRITICAL details and exit 1. The second asserts that an empty gap-fill result leaves the core assessment unchanged. Both would catch a revert of the recompute.
  • git merge-tree against main b07f181d is clean. #796 changed runner.py imports and patches, but not this block. CI is green. I did not run the tests (static review).

Decision: Changes Requested (reviewed head f259d3aeb8e8d6b0cf4f5b2129d4e0a7ab4f81f7)

Comment thread contrib/batch_scan/runner.py
Signed-off-by: Efe <efe@rexcode.co.uk>
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.

Batch gap-fill findings do not update risk scores, verdicts, or exit status

2 participants