fix: ensure nested legacycli invocations forward errors

[CatalinSnyk]

Pull Request Submission Checklist

• Follows CONTRIBUTING guidelines
• Includes detailed description of changes
• Contains risk assessment (Low | Medium | High)
• Highlights breaking API changes (if applicable)
• Links to automated tests covering new functionality
• Includes manual testing instructions (if necessary)
• Updates relevant GitBook documentation (PR link: ___)

What does this PR do?

Following the work from CLI-646 that added the capability of capturing errors from TS CLI invocations, this adds PR aims to ensure nested invocations of the TS CLI are forwarded properly to debug logs, analytics and output.

One current gap left from CLI-646 is the fact that if the --sarif or --json flags are set, we don't send this errors to the IPC. We can retrieve error messages from the JSON payload, but in the case of SARIF, there is no error message present.
Extracting the error message from the JSON payload is just about parsing and accessing the "error" field. I added a fallback case and in case parsing fails, we default back to the normal Error.message field.

Here's an example of an error caught when the --json flag is set:

Error{
    message: "{\"ok\":false,\"error\":\"This is the actual error message\",\"path":\"./\"}"
    ...otherFields
}

Where should the reviewer start?

• main.ts - Removed JSON from the list of IPC exceptions, and moved the output case above the rest, just to simplify the conditions a bit and since the TS CLI will always output the errors in JSON format, while also sending them to the IPC.
• ipc.ts - Added a new argument to the sendError function: isJson is set when the --json flag is set, this will add a meta field "suppressJsonOutput" to the error in order to signal the Golang CLI to now show this error. Other things added was just around extracting the error message from the stringified JSON payload from the messsage field of the caught error.
• main.go - The output workflow configuration values were not set correctly in some cases because the flags were not added as persistent. The other thing added here was generating the right error message for the JSON output; if the error is Error Catalog, the proper message is the detail field. This also takes into account the suppressJsonOutput field.

How should this be manually tested?

Using the snyk-macos-arm64 sbom --format cyclonedx1.4+json --file=./foldernotthere command, the expected error message would surface the fact that the specified file is not present, instead of the "An error occurred while running the underlying analysis needed to generate the SBOM." message. Other commands that make use of TS CLI workflow invocations should also surface the errors captured though the IPC.

Related

• CLI-696

[github-actions]

	Warnings
⚠️	Since the CLI is unifying on a standard and improved tooling, we're starting to migrate old-style imports and exports to ES6 ones. A file you've modified is using either module.exports or require(). If you can, please update them to ES6 import syntax and export syntax. Files found: src/cli/ipc.ts src/cli/main.ts test/setup-jest.ts

Generated by 🚫 dangerJS against f48415e

[thisislawatts]

issue: Use imports here rather than commonjs require format.

[thisislawatts]

question: Why remove the ? character from this string?

[CatalinSnyk]

I added that in the previous PR when I was reading about the instance parameter (and I was confused about what it did), and I missed removing that 😅

[thisislawatts]

Can we remove that as part of this change?

[thisislawatts]

question: The comment here mentions: Only set if the invocation uses the --json flag. But below it looks like we are checking, configuration.WORKFLOW_USE_STDIO, how does that relate to the --json flag?

[CatalinSnyk]

I was referring to the "suppressJsonOutput" field being set, will update this to be a bit more expressive about that.

[thisislawatts]

Question: What is the incorrect behaviour you were seeing which required this change? I am not sure I understand how it is related to the change you are looking to make here?

[CatalinSnyk]

The configuration values were not being populated properly (eg: even if the --json flag was in the args, the configuration flag for the JSON output would have the default value of false).
This would result in incorrect output for errors sent thought IPC: we would get the normal human readable output even if JSON formatted output was expected.