fix(cli): prevent handled errors from producing duplicate output [CLI-1765]

[bdemeo12]

Pull Request Submission Checklist

• Follows CONTRIBUTING guidelines
• Commit messages
  are release-note ready, emphasizing
  what was changed, not how.
• Includes detailed description of changes
• Contains risk assessment (Low | Medium | High)
• Highlights breaking API changes (if applicable) — none
• Links to automated tests covering new functionality
• Includes manual testing instructions (if necessary)
• Updates relevant GitBook documentation (PR link: ___) — n/a, no user-facing docs change
• Includes product update to be announced in the next stable release notes

What does this PR do?

CLI Ticket: https://snyksec.atlassian.net/browse/CLI-1765?actionerId=712020%3Aed5c88da-ffa1-47ff-9ea7-b4345eaadf9f&sourceType=assign

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-plugin to 9.20.0 so 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 — shouldSuppressDisplay and the third return value from processError, then the guarded displayError call in tearDown.

Then cliv2/pkg/core/main_test.go for the two new cases in Test_processError and the new Test_shouldSuppressDisplay.

How should this be manually tested?

Against an image whose provenance fetch fails (SDP version 9.20.0 +):

snyk container test <image> --json > out.json
jq . out.json

The container.spec.ts acceptance 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

[snyk-io]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[bdemeo12]

This PR originates from: #7047

[bdemeo12]

All errors are combined here:

cli/cliv2/pkg/core/main.go

Line 771 in cbc275e

resultErrorList = append([]error{resultError}, resultErrorList...)

shouldSuppressDisplay returns true when there's nothing to show:

cli/cliv2/pkg/core/main.go

Line 504 in cbc275e

if shouldSuppressDisplay(err) {

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

cli/cliv2/pkg/core/main.go

Line 462 in 26f0543

if isExitError || isErrorWithCode || errorHasBeenShown(err) {

Now this check suppresses the display err by returning early, before the printing

[CatalinSnyk]

The Code reproduction also makes me think the original error is the better place to make this decision. Before processError, we know whether the command returned exec.ExitError or ErrorWithExitCode; after joining, we have to infer that recursively and risk suppressing unrelated sibling errors.

[CatalinSnyk]

I think we could calculate suppression from the original error in tearDown func, before processError joins anything?

suppressDisplay := shouldSuppressDisplay(err)
if err != nil {
    allErrors, outputError = processError(err, errorList)
    // If processError selected a different error, such as maintenance or the
    // captured cause of TS_CLI_TERMINATED, that error still needs displaying.
    suppressDisplay = suppressDisplay && errors.Is(outputError, err)
}
if !suppressDisplay {
    displayError(outputError, ...)
}

shouldSuppressDisplay could then retain the simple direct checks for exec.ExitError, ErrorWithExitCode, and already-displayed errors. This covers both cases while avoiding recursive unwrapping.

[bdemeo12]

bumped SDP to use as an acceptance test

[bdemeo12]

#7047

[github-actions]

	Warnings
⚠️	There are multiple commits on your branch, please squash them locally before merging!
⚠️	"[fix(cli): prevent handled errors from producing duplicate output CLI-1765](#7130)" is too long. Keep the first line of your commit message under 72 characters.
⚠️	"[fix(cli): decide error suppression from the original exit code CLI-1765](https://api.github.com/repos/snyk/cli/git/commits/5958a11905ae98d26c61e2ed757e1e2a5bfcdd11)" is too long. Keep the first line of your commit message under 72 characters.

Generated by 🚫 dangerJS against a858775

[CatalinSnyk]

suggestion: Could we also use the ticket mentioned tests here? They assert that both exec.ExitError and ErrorWithExitCode remain the returned outputError, while the auxiliary error remains in allErrors. I think it better shows the separate concern for the user facing output and analytics error list.

[bdemeo12]

Yes! Added!

[CatalinSnyk]

suggestion: I think we can keep this responsibility inside processError instead of returning a joined display error and having carrying a suppression state suppression?

processError already returns both allErrors and outputError. We can keep the joined errors in allErrors for the analytics payload, but return the original command error as outputError when the selected result still contains it. Something like this is what I'm thinking currently:

mostRelevant := cli_errors.FindMostRelevantError(allErrors)
if commandOwnsOutput(originalError) &&
    errors.Is(mostRelevant, originalError) {
    outputError = originalError
} else {
    outputError = mostRelevant
}

commandOwnsOutput could match the two original command-result types: exec.ExitError and ErrorWithExitCode.

[bdemeo12]

Yes! processError now owns this! Thanks for the help!

[robertolopezlopez]

@bdemeo12 please have a look to the failing pipeline 👍

[bdemeo12]

@bdemeo12 please have a look to the failing pipeline 👍

I think this pipeline is flakey! I reran it and its succeeded!

it looks good to me, but @CatalinSnyk has broader context to judge. I would rather wait for his opinion (thus removing my approval)

[snyk-pr-review-bot]

PR Reviewer Guide 🔍

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Silent Failures 🟡 [minor] In processError, the new logic returns early if commandOwnsOutput(resultError) is true and it matches mostRelevant. This prevents the subsequent mapErrorToExitCode and createErrorWithExitCode logic from running for that specific error. While commandOwnsOutput currently targets errors that likely already have codes, if a new error type is added that 'owns output' but requires mapping (like a custom exit code from a plugin), this early return would bypass the mapping table. if commandOwnsOutput(resultError) && errors.Is(mostRelevant, resultError) { return resultErrorList, resultError }

📚 Repository Context Analyzed This review considered 11 relevant code sections from 7 files (average relevance: 0.90) cliv2/pkg/core/errorhandling.go (lines 1-110) package.json (lines 1-236) cliv2/pkg/core/main.go (4 segments, lines 1-188) cliv2/pkg/core/exitcode.go (lines 1-197) cliv2/pkg/core/logheaderfooter.go (lines 1-251) src/cli/main.ts (lines 1-236) package-lock.json (2 segments, lines 6-266)

🤖 Repository instructions applied (from AGENTS.md)

[PeterSchafer]

Closing this PR as it was replaced by #7191