Skip to content

fix(report): keep missing references as non-blocking caveats - #805

Open
efegokdemir wants to merge 3 commits into
NVIDIA:mainfrom
efegokdemir:codex/804-cli-reference-caveats
Open

efegokdemir wants to merge 3 commits into
NVIDIA:mainfrom
efegokdemir:codex/804-cli-reference-caveats

Conversation

@efegokdemir

@efegokdemir efegokdemir commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

What

Treat scans with only missing-reference ledger caveats as complete when every discovered file was inspected and no other limitations exist. Preserve the reference and exception details in reports, and keep --fail-on-incomplete from rejecting these scans.

Why

The MCP safety gate already allowed a missing-reference-only result, but CLI and SARIF completeness still reported it as partial and raised the recommendation to CAUTION. This made the same fully inspected workspace appear incomplete depending on the output path. Ambiguous references and actual coverage gaps still need to block completeness.

How

  • Centralize the missing-reference-only predicate and use it when finalizing the shared completeness ledger.
  • Represent that state as complete_with_caveats, with is_complete=true, only when there are no limitations or uninspected files.
  • Preserve the caveat status in SARIF and keep the MCP gate aligned with the shared predicate.

Testing

  • Reproduced on pristine main: a no-findings scan with only missing workspace references returned partial, CAUTION, and exit code 1 with --fail-on-incomplete.
  • Added CLI JSON, terminal, and SARIF regression coverage; verified missing-reference details remain visible and the strict gate succeeds. Ambiguous-reference and opaque/incomplete coverage cases remain blocking.
  • pytest -q tests/test_reference_destinations.py tests/test_reference_destination_kinds.py tests/nodes/test_finalize_inspection_ledger.py tests/nodes/test_analysis_completeness.py tests/nodes/test_security_remediation.py — 755 passed.
  • make test-unit — 10,540 passed, 17 skipped, 133 deselected, 4 xfailed.
  • make lint — passed.
  • make format-check — passed.
  • uv build --out-dir /tmp/skillspector-805-audit-dist — passed.

Compatibility

Single-key and normal complete-scan behavior is unchanged. Missing references remain visible; only their effect on completeness/recommendation changes when there is no actual inspection gap. Ambiguous references and incomplete coverage remain blocking.

Checklist

  • Added regression tests for the reported behavior.
  • Ran targeted and full non-integration/non-provider tests.
  • Ran lint, formatting, and package build.
  • Prepared with AI assistance.

Fixes #804

@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 taking on #804, centralizing the missing-reference predicate, and keeping ambiguous references blocking.

Value and readiness

The noise #804 describes is real, and aligning CLI/SARIF with the MCP gate is a reasonable goal. However, reference_missing does not only mean "absent". Referenced content that exists but was never inspected gets the same reason, and this PR turns those scans into complete/SAFE results that pass --fail-on-incomplete. Not ready yet.

Material findings

  1. Blocker, src/skillspector/inspection_ledger.py:1056: the caveat-only rule also covers present but uninspected targets.
    • Resolution checks only the discovery inventory (references.py:508-529). Any existing target outside that inventory therefore becomes missing.
    • Repro A, in-bundle symlink: SKILL.md says "Follow the steps in docs/setup.md", and docs/setup.md is a symlink to a file outside the scan root. On main b07f181d with --no-llm --fail-on-incomplete, the only ledger exception is reference_missing, and the symlink appears only in scope_exclusions as not_regular_file. There are 0 partial and 0 uninspected files and no findings; the result is CAUTION with exit 1.
    • Repro B, single-file target: skillspector scan skill/SKILL.md, where SKILL.md says "Run scripts/setup.sh first" and skill/scripts/setup.sh exists and pipes curl to sh. Main gives the same result: only reference_missing, CAUTION, exit 1.
    • With this PR, both match reference_caveat_only and become complete_with_caveats, is_complete=true, SAFE, exit 0. The uninspected instruction or script is indistinguishable from a workspace path. The MCP gate already allows these cases; its comment at mcp_server.py:196 assumes the path is not carried by the bundle.
    • Expected: establish that the target is absent before treating it as a caveat. One option is an lstat (no read) of the normalized target relative to the skill directory or the selected file's parent, with a distinct blocking reason for present-but-uninspected targets. At minimum, exclude targets that match a scope exclusion. Please add regressions where A and B stay partial and a truly absent workspace path becomes complete_with_caveats.
  2. Blocker: the new status is not handled where existing code compares against "complete".
    • src/skillspector/nodes/report.py:1736: transitive truncation sets is_complete=False but rewrites the status only when it equals "complete". JSON can then show status: complete_with_caveats together with is_complete: false, the kind of contradiction downstream validators reject (#800).
    • src/skillspector/nodes/finalize_inspection_ledger.py:821: the coverage-finding downgrade likewise checks only "complete".
    • Expected: handle both complete statuses at these sites, or derive the status from is_complete, and test the truncation path.

PIC tradeoffs

  • Policy: this changes the default CLI/SARIF verdict and --fail-on-incomplete, not only MCP. An opt-in flag is the alternative.
  • Schema: complete_with_caveats is a new public status. Consumers that allow only complete|partial|failed may reject it. The alternative is status: complete plus a caveats flag. Either way it needs a release note.
  • The recommendation for these scans changes from CAUTION to SAFE.

Verification and gaps

  • I read the full diff against 3c8e4b95 and traced finalize_ledger → report() → _build_sarif → the CLI gate (cli.py:866, which reads is_complete) and the MCP gate.
  • I ran trusted origin/main (b07f181d) in a scratch worktree for both repros (results above). I did not run PR code; the PR's behavior comes from the predicate, since there are no other exceptions, no limitations and zero partial/uninspected counts.
  • User exclusions, excluded directories and executable symlinks are in known_paths, resolve with partial dispositions, and stay blocking. The hole covers non-executable symlinks and special files, plus files outside a single-file target.
  • Main is 49 commits ahead, including #796 and #806, which touch report.py and mcp_server.py; git merge-tree is clean and those sites are unchanged.
  • CI: all 6 checks are green on f6bdc08.

Comment thread src/skillspector/inspection_ledger.py
efegokdemir and others added 2 commits October 14, 2026 06:39
Signed-off-by: Efe <efe@rexcode.co.uk>
Signed-off-by: Efe Gökdemir <gokdemirefe1903@gmail.com>
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.

CLI still reports zero-finding scans as partial/CAUTION when the only exceptions are reference_missing (#510 fixed MCP only)

2 participants