fix(scan): surface backend scanner-skip warnings to users - #311
Merged
Conversation
Previously the CLI only checked artifact scan results when SBOM/VEX generation was explicitly requested, so a backend-reported skip_reason (e.g. appsec-v2 skipped for exceeding a file-count threshold) never reached users in the common case of a plain `scan repo`/`scan image`. Now the artifact results are always fetched, and a skip_reason short- circuits SBOM/VEX download to print a "Scanner skipped: ..." warning.
|
Test Coverage Reporttotal: (statements) 72.0% Coverage by function |
There was a problem hiding this comment.
🟡 Changes recommended
The new stderr capture helper can leak global os.Stderr on FailNow/panic and the unconditional artifact-results fetch currently treats 404 as an error, which can cause flaky tests and noisy warnings.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the scan flows to surface backend “scanner skipped” warnings (via skip_reason) to users consistently, instead of only when SBOM/VEX generation flags are used, improving debuggability when a backend scanner is skipped.
Changes:
- Parse
error(debug-only) andskip_reason(user-safe) inArtifactScanResultsResponse, and exposeSkipMessage()helper. - Always fetch artifact scan results across repo/image/SBOM paths and emit
Scanner skipped: ...warnings to stderr when present. - Add/extend unit + integration tests, including a new stderr-capture test helper.
File summaries
| File | Description |
|---|---|
| internal/testutil/httptest.go | Adds CaptureStderr helper used by new tests. |
| internal/scan/sbom/sbom.go | Surfaces skip_reason (and debug error) during SBOM scan flow; avoids artifact downloads when skipped. |
| internal/scan/sbom/sbom_integration_test.go | Adds integration coverage that skip reasons are displayed to users. |
| internal/scan/sbom_vex.go | Adds debug logging + skip warning handling and nil-options no-op behavior in downloader. |
| internal/scan/sbom_vex_test.go | Adds unit coverage for skip-warning output in downloader. |
| internal/scan/repo/repo.go | Calls SBOM/VEX downloader unconditionally to surface skip warnings even without SBOM/VEX flags. |
| internal/scan/repo/repo_test.go | Adds regression coverage for skip-warning behavior when SBOM/VEX flags are absent. |
| internal/scan/repo/githints_test.go | Updates mock server routing to account for new artifact-results fetch. |
| internal/scan/image/image.go | Calls SBOM/VEX downloader unconditionally (mirrors repo scan behavior). |
| internal/scan/image/image_test.go | Adds regression coverage and updates mock server routing for new artifact-results fetch. |
| internal/api/client.go | Extends artifact results response struct with error and skip_reason, adds SkipMessage(). |
| internal/api/client_test.go | Adds parsing tests for the new response fields. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
hetpatel-armis
approved these changes
Sep 2, 2026
Download() is now called unconditionally to surface scanner-skip warnings, so a 404/"not yet available" result should only be treated as an error when SBOM/VEX was actually requested. Also hardens CaptureStderr to restore os.Stderr via defer and drain the pipe concurrently, avoiding a leak on t.Fatal/panic and a deadlock if the buffer fills. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
CaptureStderr currently doesn’t guarantee closing its pipe fds on t.Fatal/panic exit paths, which can leak/block goroutines and make the test binary flaky over time.
Review details
Suppressed comments (1)
internal/testutil/httptest.go:63
- CaptureStderr restores os.Stderr via defer even if f calls t.Fatal (Goexit), but the pipe fds are only closed after f returns. If f triggers FailNow/panic, the writer/reader stay open and the io.Copy goroutine can leak/block for the rest of the test binary. Add a defer that also closes the pipe so cleanup happens on all exit paths (errors can be ignored in the defer).
os.Stderr = w
defer func() { os.Stderr = oldStderr }()
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
Propagate the scanner-skip message (e.g. AI-scan file-count limits) into `Summary.ScannerSkipReason` from all three scanner paths (repo, image, sbom) so it renders in the human summary dashboard even after the transient stderr warning has scrolled away. `SBOMVEXDownloader.Download` now returns the skip reason alongside its error.
There was a problem hiding this comment.
🟡 Changes recommended
The new CaptureStderr helper redirects a process-global os.Stderr without robust cleanup/serialization, which can leak resources and cause flaky tests under parallel package execution.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/testutil/httptest.go:62
- CaptureStderr restores os.Stderr via defer, but if f() exits via t.Fatal/runtime.Goexit (or panics), the pipe ends are never closed. That can leave the reader goroutine blocked on io.Copy and leak file descriptors. Also, since os.Stderr is process-global and Go runs package tests in parallel, this helper can still cause cross-package test flakiness unless it’s guarded by a global mutex (even when individual tests don’t call t.Parallel).
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
… docstring Copilot review flagged that a t.Fatal (runtime.Goexit) or panic inside CaptureStderr's callback left the pipe writer open, leaking the drain goroutine. Also update Download's docstring, which still claimed missing results always error out after the skip-reason no-op was added.
There was a problem hiding this comment.
🔵 Needs a closer look
The new CaptureStderr test helper can leak the pipe reader FD when f() aborts (t.Fatal/panic), and its doc comment is misleading about concurrency safety.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
internal/testutil/httptest.go:53
- The doc comment says this is only unsafe with t.Parallel, but go test runs packages in parallel by default, and CaptureStderr is already used in multiple packages in this repo. The comment should warn that redirecting os.Stderr is unsafe when any other tests (including other packages) run concurrently.
internal/testutil/httptest.go:61
- CaptureStderr can leak the read end of the pipe if f() calls t.Fatal/FailNow (runtime.Goexit) or panics: execution skips the explicit r.Close() at the end of the function, so the file descriptor may remain open for the rest of the test process. Add a defer to always close r right after the pipe is created (after the error check).
r, w, err := os.Pipe()
if err != nil {
t.Fatalf("failed to create pipe: %v", err)
}
os.Stderr = w
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
…oncurrency doc Copilot's re-review caught that only the write end was deferred-closed; the read end could still leak on t.Fatal/panic. Also the doc comment undersold the hazard — go test runs different packages' tests concurrently by default, not just t.Parallel within one package.
There was a problem hiding this comment.
🟡 Changes recommended
The new CaptureStderr test helper has correctness/documentation issues (misleading concurrency notes and a window where os.Stderr can point at a closed pipe), which can cause flaky or confusing tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Lite
…urrency doc Copilot's re-review caught two more issues: os.Stderr pointed at the already-closed writer between the explicit Close() and the deferred restore, and the prior doc edit wrongly blamed cross-package test concurrency (packages run in separate processes) instead of the real hazard — parallel subtests in the same process.
There was a problem hiding this comment.
🟡 Changes recommended
The new CaptureStderr helper mutates process-global os.Stderr and can interfere with parallel tests in the same package, creating avoidable flakiness/race risk.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Lite
Copilot flagged that concurrent t.Parallel tests in the same binary could clobber each other's os.Stderr swap. A package-level mutex fully closes that out instead of relying on caller discipline.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive and well-tested across scan flows, with only a minor robustness improvement suggested for whitespace-only skip messages.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/api/client.go:860
- SkipMessage treats a whitespace-only skip_reason (e.g. " \n") as a real skip message, which can produce a confusing "Scanner skipped:" warning with no visible reason. Trimming whitespace here makes the helper more robust and keeps downstream output clean.
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
…message
SkipMessage() only checked for an empty string, so a whitespace-only
skip_reason (" \n") would still surface a "Scanner skipped:" warning with
no visible reason.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive, preserve existing error-handling expectations, and include targeted tests covering both the new warning behavior and the new API response parsing.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When the backend skips a scanner (e.g. appsec-v2 skipped for exceeding a file-count threshold), the CLI silently ignored this. Users got no findings from that scanner and no explanation why, and the warning was only ever checked when
--generate-sbom/--generate-vexwas requested.Solution
ArtifactScanResultsResponsenow parseserror(technical detail, debug-only) andskip_reason(user-safe message) fields from the backend, exposed via a newSkipMessage()helper. The repo, image, and SBOM scan paths now always fetch artifact scan results and print aScanner skipped: ...warning to stderr viacli.PrintWarningfwhen present, instead of only doing so when SBOM/VEX generation was requested.SBOMVEXDownloader.Downloadshort-circuits after printing the skip warning, and no-ops cleanly when SBOM/VEX options are nil.Type of Change
Testing
Automated Tests
Manual Testing
TODO
Reviewer Notes
No behavior change when
skip_reasonis absent; the artifact-results fetch is now unconditional (previously gated on SBOM/VEX flags), so it adds one extra API call per scan even when no SBOM/VEX flags are passed.Checklist
Screenshots (if applicable)
N/A