Skip to content

fix(cli): thread --baseline and --show-suppressed through recursive scans (#201)#205

Open
wernerkasselman-au wants to merge 2 commits into
NVIDIA:mainfrom
wernerkasselman-au:fix/recursive-baseline-suppression
Open

fix(cli): thread --baseline and --show-suppressed through recursive scans (#201)#205
wernerkasselman-au wants to merge 2 commits into
NVIDIA:mainfrom
wernerkasselman-au:fix/recursive-baseline-suppression

Conversation

@wernerkasselman-au

Copy link
Copy Markdown
Contributor

What this fixes

Closes the bug I filed as #201. The suppression work (#88) and the multi-skill work (#136) did not compose: in scan --recursive, the --baseline and --show-suppressed options were accepted on the command line and then silently dropped, so a baselined finding was not actually suppressed. It still counted toward each sub-skill's risk score, and it could still flip the exit code to 1. The single-skill path honoured both options, so only the recursive path regressed, which is exactly the mode you reach for to scan a whole skill collection in one pass.

The change

_scan_multi_skill() did not accept the two options, the recursive dispatch in scan() did not pass them, and the per-skill _scan_state(...) call left them out. I added baseline and show_suppressed parameters to _scan_multi_skill(), passed them from the recursive dispatch, and forwarded them into the per-skill _scan_state(...) call, so the recursive path now mirrors the single-skill path (cli.py around the single-skill _scan_state call).

Tests

Added test_cli_recursive_forwards_baseline_and_show_suppressed. It builds a two-sub-skill collection (no root SKILL.md), generates a real baseline against one sub-skill, then captures the state handed to the graph for each sub-skill and asserts the baseline was loaded into it and show_suppressed is set. On the old code the state carried neither key, so the test pins the regression rather than just exercising the happy path. I kept it at the plumbing level on purpose, so it does not couple to the suppression-matching internals.

uv run ruff check src/ tests/           # clean
uv run ruff format --check src/ tests/  # clean
uv run pytest tests/unit/test_cli.py tests/test_multi_skill.py -q  # 20 passed

Refs #201

wernerkasselman-au added a commit to wernerkasselman-au/SkillSpector that referenced this pull request Jun 24, 2026
…port

The recursive summary table and combined JSON report computed finding_count as
`filtered_findings or findings`, which falls through to the unfiltered findings
when suppression empties filtered_findings, since an empty list is falsy. A fully
baselined sub-skill then showed score 0 but a non-zero finding_count.

Add _result_finding_count(), which falls back to raw findings only when
filtered_findings is absent (the report node always sets it, possibly empty after
suppression), and use it at both the summary table and the combined JSON report.
Add a regression test asserting the combined JSON finding_count is 0 for a fully
suppressed sub-skill.

Surfaced by an independent review of NVIDIA#205.

Refs NVIDIA#201

Signed-off-by: Werner Kasselman <145896621+wernerkasselman-au@users.noreply.github.com>

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

Approving — fixes #201 by threading --baseline/--show-suppressed into the recursive multi-skill path (they were silently dropped). Also a real correctness fix: _result_finding_count() replaces filtered_findings or findings, which mis-counted a fully-suppressed skill (empty list is falsy, so the or fell through to the unfiltered findings). Both behaviors are covered by new tests.

Non-blocking: this and #209 both edit _scan_multi_skill — whichever merges second will need a small rebase.

@rng1995

rng1995 commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Please resolve merge conflicts @wernerkasselman-au

…cans

_scan_multi_skill did not accept or forward the suppression options, and the
recursive dispatch in scan() did not pass them, so scan --recursive silently
ignored --baseline and --show-suppressed. Suppressed findings still counted
toward each sub-skill's score and could flip the exit code to 1, breaking the
incremental baseline workflow precisely in the mode meant for skill collections.

Add baseline and show_suppressed parameters to _scan_multi_skill, pass them from
the recursive dispatch, and forward them into the per-skill _scan_state call,
mirroring the single-skill path. Add a regression test that captures the per
sub-skill graph state and asserts the baseline and show_suppressed are present.

Refs NVIDIA#201

Signed-off-by: Werner Kasselman <145896621+wernerkasselman-au@users.noreply.github.com>
…port

The recursive summary table and combined JSON report computed finding_count as
`filtered_findings or findings`, which falls through to the unfiltered findings
when suppression empties filtered_findings, since an empty list is falsy. A fully
baselined sub-skill then showed score 0 but a non-zero finding_count.

Add _result_finding_count(), which falls back to raw findings only when
filtered_findings is absent (the report node always sets it, possibly empty after
suppression), and use it at both the summary table and the combined JSON report.
Add a regression test asserting the combined JSON finding_count is 0 for a fully
suppressed sub-skill.

Surfaced by an independent review of NVIDIA#205.

Refs NVIDIA#201

Signed-off-by: Werner Kasselman <145896621+wernerkasselman-au@users.noreply.github.com>
@wernerkasselman-au wernerkasselman-au force-pushed the fix/recursive-baseline-suppression branch from b45b4ee to ccfbdf4 Compare July 10, 2026 21:29
@wernerkasselman-au

Copy link
Copy Markdown
Contributor Author

Rebased onto current main and resolved the merge conflict in tests/unit/test_cli.py. The conflict was purely additive: main added test_scan_multi_skill_markdown_output_to_file / test_scan_multi_skill_json_output_unchanged, and this branch added test_cli_recursive_forwards_baseline_and_show_suppressed; all three are kept. The cli.py change (threading baseline / show_suppressed through _scan_multi_skill and _scan_state) merged cleanly and matches _scan_state's current signature on main.

Local verification: tests/unit/test_cli.py 11 passed, ruff format --check and ruff check both clean. @rng1995

@wernerkasselman-au

Copy link
Copy Markdown
Contributor Author

Hi @rng1995,

Both are sorted on my side.

For #205, I have rebased onto current main and resolved the one conflict in tests/unit/test_cli.py. It was purely additive (main had added test_scan_multi_skill_markdown_output_to_file and test_scan_multi_skill_json_output_unchanged, and this branch added test_cli_recursive_forwards_baseline_and_show_suppressed), so I kept all three rather than dropping either side. Two commits sit on the branch: the first threads baseline and show_suppressed through _scan_multi_skill and _scan_state (that merged cleanly and lines up with _scan_state's current signature on main), and the second is a small follow-on fix so the recursive multi-skill report counts post-suppression findings, via a _result_finding_count helper, so a fully suppressed sub-skill now reports 0 rather than its raw pre-baseline count (covered by test_cli_recursive_json_finding_count_excludes_suppressed). Locally the CLI suite is green (11 passed) and both ruff format --check and ruff check are clean. It reads as MERGEABLE now, though the CI run is sitting at action_required, so it will need one of you to approve the fork workflow before the checks go green (nothing I can trigger from my side).

For #207, I have closed it as obsolete. The ruff 0.15.2 formatting it applied is already on main, ruff format --check src/ tests/ passes there across all 144 files, and the branch was based on an older main (7bc9c0f) whose head predates newer commits to static_runner.py and test_llm_analyzer_base.py (69ac3ba, 1c796a5), so merging it as-is would actually have reverted those. Rebasing onto main drops its only commit as already upstream, which leaves nothing to merge. Happy to reopen and reformat against current main if you would rather keep a dedicated formatting PR, just let me know.

Thanks,
Werner

@wernerkasselman-au

Copy link
Copy Markdown
Contributor Author

@rng1995 when you have a moment, could you approve the workflow run on this PR? It is a fork PR, so the CI is sitting at action_required and needs a maintainer to kick it off before the checks can go green. Everything is green locally on my side (11 CLI tests passing, ruff clean), just need the gate opened. Thanks.

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.

2 participants