fix(cli): prevent handled errors from producing duplicate output [CLI-1765] - #7130
fix(cli): prevent handled errors from producing duplicate output [CLI-1765]#7130bdemeo12 wants to merge 7 commits into
Conversation
…-1765] `processError` combines the command exit error with collected network errors via `errors.Join`, both in `FindMostRelevantError` and `createErrorWithExitCode`. A joined error does not satisfy the direct type assertions in `displayError`, so handled auxiliary errors were printed after valid command output - emitting a second JSON object and breaking `JSON.parse(stdout)`. Decide displayability at the top of `processError`, while the concrete error type is still intact, and pass that decision through to teardown. The combined error is still used for exit-code derivation and analytics, so both are unchanged. Also pins snyk-docker-plugin to 9.20.0 so CI exercises the failure this fixes: 9.20.0 performs a best-effort provenance fetch whose handled failure is what triggered the duplicate output (see CLIA-1576). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
FindMostRelevantError returns promoted errors (e.g. maintenance windows) uncombined, so judge those on their own merits rather than on the original error. Without this, a maintenance notice was suppressed whenever the command also exited non-zero. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
This PR originates from: #7047 |
…I-1765] Errors are combined with errors.Join before reaching displayError, which hides their concrete type from its direct type assertions. Handled errors therefore slipped past the guard and were printed after the command's real output, emitting a second JSON object and breaking JSON.parse(stdout). displayError now inspects a combined error member by member. When the command exited with a non-failure code it produced valid output, so anything collected alongside it is auxiliary and stays hidden. When the command failed, errors are suppressed only if every one of them is individually suppressible, so genuine failures such as authentication errors still reach the user. Pins snyk-docker-plugin to 9.20.0 so CI exercises the failure being fixed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
125499b to
cf9231f
Compare
Resolved package.json / package-lock.json in favour of snyk-docker-plugin 9.20.0. main pinned 9.19.0 to avoid the duplicate-output bug; this branch is the fix for that bug, so it keeps 9.20.0 to exercise it in CI.
…snyk/cli into CLI-1765/prevent-duplicate-error-output
|
|
||
| for _, err := range unwrappedErrs { | ||
| var exitErr *exec.ExitError | ||
| if errors.As(err, &exitErr) && exitErr.ExitCode() < constants.SNYK_EXIT_CODE_ERROR { |
There was a problem hiding this comment.
An exit code below SNYK_EXIT_CODE_ERROR means the command worked and anything alongside it is noise
| // | ||
| // Joined errors match no direct type assertion, so they are unwrapped and checked | ||
| // one at a time. | ||
| func shouldSuppressDisplay(err error) bool { |
There was a problem hiding this comment.
All errors are combined here:
Line 771 in cbc275e
shouldSuppressDisplay returns true when there's nothing to show:
Line 504 in cbc275e
Previously displayError checked the error's type directly. it didnt account for a combined error, so it fell through to the printing code and emitted a second JSON object.:
Line 462 in 26f0543
Now this check suppresses the display err by returning early, before the printing
A merge reintroduced an earlier approach alongside the current one, leaving shouldSuppressDisplay and its test declared twice, so the package did not compile. Removes the superseded design: processError returns to two values, tearDown calls displayError directly, and the isCombined helper and duplicate declarations are gone. displayError alone decides what to print. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Pull Request Submission Checklist
are release-note ready, emphasizing
what was changed, not how.
What does this PR do?
Fixes:
json output being unparseable because an extra error was printed after the result. This happens when the CLI receives an error (aside from exit status 1), it prints it — so once teardown joins the exit error together with the handled network errors, the result is no longer exit status 1 and gets printed after the real output.
Also pins
snyk-docker-pluginto9.20.0so CI exercises the failure being fixed. (Original PR: #7047 + discussion: https://snyksec.atlassian.net/servicedesk/customer/portal/64/CLIA-1576)Where should the reviewer start?
cliv2/pkg/core/main.go—shouldSuppressDisplayand the third return value fromprocessError, then the guardeddisplayErrorcall intearDown.Then
cliv2/pkg/core/main_test.gofor the two new cases inTest_processErrorand the newTest_shouldSuppressDisplay.How should this be manually tested?
Against an image whose provenance fetch fails (SDP version 9.20.0 +):
The
container.spec.tsacceptance job is the automated equivalent — it fails on 9.20.0 without this fix, which is why the pin is included here.What's the product update that needs to be communicated to CLI users?
n/a