Repository navigation
Conversation
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
|
Ready for review at All six hosted CI checks have passed. |
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
@chrisknvidia thank you for tracking down #839 and fixing it with a narrow change. Using Click's parameter source to reject an explicit --transitive-depth 1 while still allowing the omitted default was careful work, and the tests cover exactly that edge.
Value and readiness
Before this change, scan --mcp-registry accepted --use-shipped-baseline, --transitive, --transitive-depth, --transitive-allow-prefix and --transitive-deny-prefix, then returned without using any of them. Users got a report with exit 0 that looked as if those controls had been applied. This PR adds the five options to the existing registry-mode rejection in scan() (src/skillspector/cli.py:701-706 on the head). The scan now exits 2 before it acquires the registry, reads any snapshots or writes a report. The depth check uses ctx.get_parameter_source("transitive_depth"). An omitted depth is still accepted. An explicit --transitive-depth 1, --transitive-depth=1, --transitive-depth 01 or a depth set through a default_map is rejected.
The fix works. On current main, every combination from the issue exits 0 and overwrites --output. With this PR merged onto main, each one exits 2 with empty stdout, the existing --output file is left as it was, and scan_registry is never called. There is also a side benefit. On main, a missing local path combined with --transitive fell through to fetching the official registry by name and then timed out. With this PR it fails right away.
Ready to merge.
Material findings
None.
Verification and gaps
- Focused tests on the PR merged with current main:
tests/unit/test_cli.py,tests/unit/test_cli_subprocess.pyandtests/unit/test_mcp_registry.pygave 453 passed. The same files on the PR head gave 412 passed.tests/unit/test_mcp_registry.pytogether withtests/nodes/test_explicit_exclusions.pyalso passed on the merged tree (96 passed). - I ran the 12 new
test_cli.pycases against main's source and all 12 fail there, so they exercise the new behavior and do not pass vacuously. - I ran a CLI matrix through the real
skillspectorconsole script on main and on the merged tree. These cases all go from exit 0 to exit 2, with the output file left untouched: each of the five options, depth values1,=1,01and2, an empty allow prefix, the options placed before--mcp-registry, and combinations with--mcp-registry-compare. These behave the same on both trees: plain registry scans, snapshot comparison,--fail-on-findings(exit 1), the existing--exclude,--recursiveand--formaterrors, and skill-mode use of--transitive-depth 1,--use-shipped-baselineand allow prefixes. scanhas the same options at the branch base and on current main, so main's newer commits add nothing else that needs rejecting. The branch is 314 commits behind main but merges cleanly. It also has no conflicts with the opencli.pychanges in #836 and #823.ParameterSourceneeds Click 8.0 or later, which the pinned Typer 0.23 already requires (click 8.4.2 resolves locally).- All 6 CI checks passed on
f7ad068a. - Not verified: the live, paginated fetch from the official registry. You noted the same gap in the description. The new check runs before the registry is fetched, so the fix does not depend on that path.
Tradeoffs
Callers that pass the same skill flags in both modes, including an explicit --transitive-depth 1, will now get exit 2 in registry mode instead of a silent no-op. That is the intended behavior, and the new README paragraph documents it. Nothing in src/, contrib/, scripts/ or CI passes these flags in registry mode.
`scan --mcp-registry` accepted --use-shipped-baseline and the --transitive* controls and then ignored them. Reject them with exit code 2 before registry acquisition or report writes, using Click parameter provenance so an explicit or configured --transitive-depth 1 is rejected while the omitted default still scans. Release-line adaptations: the registry check keeps the release flag set (no --min-coverage), and the README note goes in a new short "MCP Registry scans" section because the snapshot-comparison section it extends on main is not on this line. tests/unit/test_cli_subprocess.py is new here with only the _run_python/_run_cli helpers; the main test that uses --mcp-registry-compare (not on this line) is ported as a plain default-options registry scan. (cherry picked from commit ca3067a) Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Complete recursive scans reported a MEDIUM aggregate (score 21-50) as SAFE even though each MEDIUM child report said CAUTION. The aggregate now uses the same band mapping as single-skill reports through a new shared report.risk_severity_and_recommendation() helper. Execution failures still force DO_NOT_INSTALL, incomplete analysis still raises SAFE to CAUTION, and the >50 exit threshold is unchanged. cli.py, report.py and test_cli.py applied cleanly. The only conflict was in tests/unit/test_cli_subprocess.py, which on this line has only the #840 registry tests. It now also gets main's small _write_skill helper and the new recursive MEDIUM subprocess test. The other main-only tests in that hunk are left out. (cherry picked from commit 11c8b48) Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…1 notes List the five analyzer, report and CLI fixes backported onto the v2.13.0 base (#820, #823, #836, #837, #840) in the CHANGELOG and release notes, and raise the intro count to eight backported fixes. Document the compatibility notes: scan --mcp-registry now exits 2 with --use-shipped-baseline or any --transitive* option, including an explicit --transitive-depth 1; recursive multi-skill scans exit 2 with --use-shipped-baseline; a complete recursive MEDIUM aggregate reports CAUTION instead of SAFE in JSON and SARIF with exit codes unchanged; and the TM1 and MP3 detection changes can change findings and scores. main has no CHANGELOG entries for these PRs, so the text is written from each backport. Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 participants
Fixes #839.
--mcp-registrycurrently accepts shipped-baseline and transitive controls, then returns before applying them. Reject these combinations with exit code 2 before registry acquisition, comparison reads or report writes. Use Click parameter provenance to distinguish omitted depth=1 from an explicitly supplied or configured depth=1.Preserve default registry scans, snapshot comparison, output protection and ordinary skill-mode baseline/transitive behavior. Document the unsupported combinations.
Validation: the full default suite passed on this head (13,696 passed, 14 skipped, four expected failures; provider/integration-marked tests excluded). Also passed: 412 affected tests, 28 installed-wheel CLI cases and 12 independent CLI review cases. The matrix covers explicit defaults, combined options, empty prefixes, output preservation, changed/unchanged snapshot comparisons, strict exits and ordinary skill controls. Source/test Ruff lint and formatting passed. The live official-registry fetch exceeded a 90-second limit; local registry parsing/comparison passed. All six hosted checks passed. Additional independent code/security reviews and installed-console checks passed, including output symlink preservation, repeated/explicit options, supported comparisons, and a record fetched from the official public registry. The full paginated remote-registry CLI path remains unverified.