Skip to content

feat(cli): add fail-on-findings option - #469

Merged
rng1995 merged 3 commits into
NVIDIA:mainfrom
deepujain:feat/448-fail-on-findings
Sep 16, 2026
Merged

rng1995 merged 3 commits into
NVIDIA:mainfrom
deepujain:feat/448-fail-on-findings

Conversation

@deepujain

Copy link
Copy Markdown
Contributor

Summary

  • add an opt-in --fail-on-findings gate for active findings below the aggregate risk threshold
  • apply the gate to single and recursive scans after reports are written
  • cover the low-risk single-scan and recursive cases

Validation

  • ruff format --check src/skillspector/cli.py tests/unit/test_cli.py
  • ruff check src/skillspector/cli.py tests/unit/test_cli.py
  • git diff --check
  • python3 -m py_compile src/skillspector/cli.py tests/unit/test_cli.py
  • focused pytest could not collect locally because the temporary Python environment did not retain package metadata; CI is the remaining test surface

Fixes #448

Signed-off-by: Deepak Jain deepujain@gmail.com

@deepujain
deepujain force-pushed the feat/448-fail-on-findings branch 3 times, most recently from f568fed to 3b3e81a Compare September 9, 2026 21:54

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

Reviewed current head d4e5942508223db05163b02c1dfc38d6d3bd733f. --fail-on-findings is applied after report output, aggregates recursive child findings, and preserves execution-failure exit-code precedence. The regressions cover both single-scan and recursive low-score modes. I found no required code or test changes.

All hosted required checks pass. The branch is behind main, so it must be updated and revalidated before merging.

@deepujain
deepujain force-pushed the feat/448-fail-on-findings branch 2 times, most recently from 1b64afd to de383ec Compare September 12, 2026 08:23

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

Corrective re-review of current head 768ff663d2fde2813c5501b9fb6f8cbc9df4cfa7 after the force-push/rebase and latest main synchronization. The ordinary single-skill and recursive gates remain correctly placed after report generation and below execution-failure precedence, but the new public flag is silently ignored in the supported MCP Registry scan path. The required correction and regression are detailed inline; please also update the documented stable exit-code contract.

changes, lint, DCO, and docker-smoke pass; test-unit is still running. This is not merge-ready.

Comment thread src/skillspector/cli.py
@deepujain
deepujain force-pushed the feat/448-fail-on-findings branch from 768ff66 to ca2ce8b Compare September 12, 2026 16:14

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

Re-reviewed current head ca2ce8b8391476dc87ba6ebbf0033859e6d1b121 after the author force-push. The PR-owned cli.py and test_cli.py blobs are byte-identical to previously reviewed head 768ff663d2fde2813c5501b9fb6f8cbc9df4cfa7; the existing inline thread remains unresolved and has no reply. The required correction therefore remains: --fail-on-findings is accepted with --mcp-registry, but that branch returns before applying the gate, so a below-threshold registry finding can still exit 0. Apply the gate or reject the option combination, add the registry regression, and update the README's stable exit-code contract.

All hosted checks pass, but this head is not merge-ready.

@deepujain
deepujain force-pushed the feat/448-fail-on-findings branch 2 times, most recently from 3a6de37 to 9bd7bd5 Compare September 14, 2026 15:39

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

Re-reviewed current head 9bd7bd500d0eb07d41310f08d507d32ca64383e6 after the force-push. The two PR-owned blobs are byte-for-byte identical to the last marked changes-requested head.

The existing blocker remains: scan accepts --fail-on-findings with --mcp-registry, but the registry path returns at lines 561-588 before applying the new gate. A below-threshold registry finding can therefore still exit 0. Apply the gate or reject the option combination, add the registry regression, and update README's stable exit-code contract.

All hosted checks pass, but the current review thread remains unresolved and GitHub reports mergeStateStatus=BLOCKED.

deepujain and others added 2 commits September 15, 2026 13:10
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
@rng1995
rng1995 force-pushed the feat/448-fail-on-findings branch from 9bd7bd5 to 6296329 Compare September 15, 2026 20:28

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

Re-reviewed exact head 6296329 after rebase. MCP-registry findings now honor --fail-on-findings with a below-threshold exit-code regression, and the stable README contract matches the implementation. The actionable thread is addressed and all review threads are resolved. Full local non-provider/integration suite passed (4,834 tests), and all five exact-head hosted checks are green. Approved.

@rng1995
rng1995 enabled auto-merge (squash) September 16, 2026 04:07
@rng1995
rng1995 merged commit d678603 into NVIDIA:main Sep 16, 2026
5 checks passed
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.

[Feature Request] An easy method to have skillspector to exit with an error if theres findings

2 participants