feat(cli): opt-in discovery of an author-shipped baseline (#278) - #286
Conversation
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Approved. Shipped suppressions remain inert and unparsed by default, require explicit consumer opt-in, preserve explicit --baseline precedence, and keep machine-readable stdout clean. Provenance, malformed input, recursive behavior, and suppression semantics have adequate focused coverage.
|
@rodboev - Please address review comments and resolve merge conflicts. |
Discover a co-located .skillspector-baseline.yaml and apply it only when the consumer opts in with --use-shipped-baseline, reporting provenance on stderr. Detection without opt-in leaves findings and the risk score untouched and never parses the file. Explicit --baseline still wins. Closes NVIDIA#278 Signed-off-by: Rod Boev <rod.boev@gmail.com>
Signed-off-by: Rod Boev <rod.boev@gmail.com>
f90cf4b to
45d7d58
Compare
|
Addressed the requested maintenance on this PR.
|
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
rng1995
left a comment
There was a problem hiding this comment.
Re-review approved at the current head. The previously reviewed opt-in shipped-baseline behavior remains acceptable and there are no unresolved review threads. CI is green except for the mechanical DCO failure on the unsigned merge commit.
7f6b515 to
f5f1367
Compare
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
03566f9 to
3fa3b77
Compare
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
04a30a9 to
2c44cdc
Compare
Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
Both conflicts were purely additive and both sides are kept. - `cli.py`: main's NVIDIA#286 added `discover_baseline` to the same sorted import block this branch added `effective_findings` to. Union of the two. - `tests/unit/test_suppression.py`: main appended its `discover_baseline` tests where this branch appended the `effective_findings` tests. Both sets retained. Checked that NVIDIA#286 does not undermine this branch. `discover_baseline` is called only from `scan()` behind `--use-shipped-baseline`; the `baseline` generation command still calls `_scan_state()` without a baseline, so `state["baseline"]` is never set there and its `suppressed_findings` stays empty. The opt-in shipped-baseline path actually widens where suppression is active, which is more surface for this fix, not less. All four call sites remain routed through `effective_findings`, and no falsy `filtered_findings or findings` fallback was reintroduced by the merge. Merged tree: 2208 passed, 17 skipped, 4 xfailed. Ruff clean across 175 files. The two substantive mutants still die on the merged tree. Signed-off-by: Werner Kasselman <145896621+wernerkasselman-au@users.noreply.github.com>
Brings the branch up to `5680c2c`. Both earlier conflicts were purely additive and both sides were kept: `cli.py` (main's NVIDIA#286 `discover_baseline` alongside this branch's `effective_findings` in the same sorted import block) and `tests/unit/test_suppression.py` (main's `discover_baseline` tests alongside this branch's `effective_findings` tests). Checked that NVIDIA#286 does not undermine this branch. `discover_baseline` is called only from `scan()` behind `--use-shipped-baseline`; the `baseline` generation command still calls `_scan_state()` without a baseline, so `state["baseline"]` is never set there and its `suppressed_findings` stays empty. The opt-in shipped-baseline path widens where suppression is active, which is more surface for this fix, not less. All four call sites remain routed through `effective_findings`, and no falsy `filtered_findings or findings` fallback was reintroduced. Merged tree: 2210 passed, 17 skipped, 4 xfailed. Ruff clean across 175 files. Signed-off-by: Werner Kasselman <145896621+wernerkasselman-au@users.noreply.github.com>
Summary
A skill distributed with a vetted
.skillspector-baseline.yamlgets no benefit from it today:skillspector scan <skill>ignores the file unless every consumer finds it and passes--baselineby hand, so consumers of distributed skills see the author's known false positives raw. Loading it automatically would be worse: a baseline shipped by the skill author is third-party configuration from the party being audited, and applying it silently would let it suppress findings, lower the risk score, and flip the non-zero-exit install gate in a consumer's scan.This makes discovery explicit-trust:
scandetects a co-located baseline and reports it, applies it only when the consumer opts in with--use-shipped-baseline, and prints provenance when it does.Closes #278
Behavior
.skillspector-baseline.yamlat the top level of the scanned directory produces a stderr notice naming the file and the opt-in flag. Findings, risk score, and exit code are unchanged, and the file is never parsed; discovery is a pure existence check.--use-shipped-baseline: the discovered baseline goes through the same loading and validation as--baselineand is applied; a stderr provenance line records that an author-shipped baseline at that path was applied.--show-suppressedlists what it suppressed and why through the existing rendering. A malformed shipped baseline exits 2, same as an explicit one; it is never silently skipped.--baseline PATHalways wins: discovery is skipped entirely when an explicit baseline is given, including the missing-file exit-2 path.--format jsonand--format sarifstdout stays machine-parseable.Discovery is limited to the canonical
.skillspector-baseline.yaml, the nameskillspector baselinewrites by default..ymland.jsonbaselines stay usable through explicit--baseline; keeping auto-discovery to one filename keeps the author-controlled trust surface small and avoids any precedence question.Diff Notes
src/skillspector/suppression.py: newdiscover_baseline()that returns the co-located.skillspector-baseline.yamlpath orNonewithout reading the file. Top level of the scanned directory only.src/skillspector/cli.py:--use-shipped-baselineflag onscan, a discovery gate in the single-directory path, and a stderr console for the detection notice, provenance line, and a note when opt-in is given but nothing is shipped. The discovered path flows through the existing baseline plumbing, so suppression semantics and report rendering are unchanged.tests/unit/test_suppression.py: discovery unit tests (canonical file, absence, non-directory input, directory-named-like-the-baseline, noncanonical.yml/.jsonsiblings ignored, nested files ignored).tests/unit/test_cli.py: end-to-end tests (all--no-llm) for detect-without-apply, opt-in apply with provenance andsuppressed: trueentries, explicit--baselineprecedence over a shipped suppress-all baseline, JSON and SARIF stdout parseability with a discovered baseline in a directory whose name has a space and a non-ASCII character, no-op opt-in when nothing canonical is shipped,--show-suppressedreason rendering, exit 2 on a malformed shipped baseline under opt-in, and an untouched recursive path.Scope
--baselinethreading there is tracked separately ([BUG] Recursive multi-skill scans silently ignore --baseline and --show-suppressed #201/fix(cli): baseline suppression in recursive multi-skill scans (#201) #205).Verification
uv run pytest tests/unit/test_cli.py tests/unit/test_suppression.py -q→ 89 passeduv run ruff check src/ tests/→ All checks passed.uv run ruff format --check src/ tests/→ 165 files already formatted