Skip to content

[PLUTO-1396] Handle tool errors gracefully #75

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zhamborova merged 6 commits into from
Apr 15, 2025
Merged

[PLUTO-1396] Handle tool errors gracefully #75

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f225ba3
[PLUTO-1396] Handle tool errors gracefully
zhamborova Apr 14, 2025
f9d4372
[PLUTO-1396] Handle sarif err
zhamborova Apr 14, 2025
235225c
[PLUTO-1396] Do not gather fatal failures in sarif
zhamborova Apr 15, 2025
2bc5e47
[PLUTO-1396] consistent error log
zhamborova Apr 15, 2025
011554f
[PLUTO-1396] rem continue
zhamborova Apr 15, 2025
484c72c
[PLUTO-1396] rem continue
zhamborova Apr 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 40 additions & 24 deletions cmd/analyze.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,42 +188,33 @@ func getToolName(toolName string, version string) string {
return toolName
}

func runEslintAnalysis(workDirectory string, pathsToCheck []string, autoFix bool, outputFile string, outputFormat string) {
func runEslintAnalysis(workDirectory string, pathsToCheck []string, autoFix bool, outputFile string, outputFormat string) error {
eslint := config.Config.Tools()["eslint"]
eslintInstallationDirectory := eslint.InstallDir
nodeRuntime := config.Config.Runtimes()["node"]
nodeBinary := nodeRuntime.Binaries["node"]

tools.RunEslint(workDirectory, eslintInstallationDirectory, nodeBinary, pathsToCheck, autoFix, outputFile, outputFormat)
return tools.RunEslint(workDirectory, eslintInstallationDirectory, nodeBinary, pathsToCheck, autoFix, outputFile, outputFormat)
}

func runTrivyAnalysis(workDirectory string, pathsToCheck []string, outputFile string, outputFormat string) {
func runTrivyAnalysis(workDirectory string, pathsToCheck []string, outputFile string, outputFormat string) error {
trivy := config.Config.Tools()["trivy"]
trivyBinary := trivy.Binaries["trivy"]

err := tools.RunTrivy(workDirectory, trivyBinary, pathsToCheck, outputFile, outputFormat)
if err != nil {
log.Fatalf("Error running Trivy: %v", err)
}
return tools.RunTrivy(workDirectory, trivyBinary, pathsToCheck, outputFile, outputFormat)
}

func runPmdAnalysis(workDirectory string, pathsToCheck []string, outputFile string, outputFormat string) {
func runPmdAnalysis(workDirectory string, pathsToCheck []string, outputFile string, outputFormat string) error {
pmd := config.Config.Tools()["pmd"]
pmdBinary := pmd.Binaries["pmd"]

err := tools.RunPmd(workDirectory, pmdBinary, pathsToCheck, outputFile, outputFormat, config.Config)
if err != nil {
log.Fatalf("Error running PMD: %v", err)
}
return tools.RunPmd(workDirectory, pmdBinary, pathsToCheck, outputFile, outputFormat, config.Config)
}

func runPylintAnalysis(workDirectory string, pathsToCheck []string, outputFile string, outputFormat string) {
func runPylintAnalysis(workDirectory string, pathsToCheck []string, outputFile string, outputFormat string) error {
pylint := config.Config.Tools()["pylint"]

err := tools.RunPylint(workDirectory, pylint, pathsToCheck, outputFile, outputFormat)
if err != nil {
log.Fatalf("Error running Pylint: %v", err)
}
return tools.RunPylint(workDirectory, pylint, pathsToCheck, outputFile, outputFormat)
}

var analyzeCmd = &cobra.Command{
Expand Down Expand Up @@ -262,13 +253,22 @@ var analyzeCmd = &cobra.Command{
defer os.RemoveAll(tmpDir)

var sarifOutputs []string
failedTools := make(map[string]error)
for toolName := range toolsToRun {
log.Printf("Running %s...\n", toolName)
tmpFile := filepath.Join(tmpDir, fmt.Sprintf("%s.sarif", toolName))
runTool(workDirectory, toolName, args, tmpFile)
if err := runTool(workDirectory, toolName, args, tmpFile); err != nil {
log.Printf("Warning: Tool %s failed: %v\n", toolName, err)
failedTools[toolName] = err
continue
Copy link
Contributor

@machadoit machadoit Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would see us not having these continues. I thing that the 'append' of sarifs, should be ready to receive a file that does not exist or something.

But would like not to miss any possible generated sarif from the tool

Copy link
Contributor Author

@zhamborova zhamborova Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, will remove 👍

}
sarifOutputs = append(sarifOutputs, tmpFile)
Copy link
Contributor

@machadoit machadoit Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming that this line works even if tmpFile foes not exist, LGTM!

Copy link
Contributor Author

@zhamborova zhamborova Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in the mergesarif function non existent files should be handled 👍

		data, err := os.ReadFile(file)
		if err != nil {
			if os.IsNotExist(err) {
				// Skip if file doesn't exist (tool might have failed)
				continue
			}
			return fmt.Errorf("failed to read SARIF file %s: %w", file, err)
		}```

}

if len(sarifOutputs) == 0 && len(failedTools) > 0 {
log.Fatal("All tools failed to run. No analysis results available.")
}

// create output file tmp file
tmpOutputFile := filepath.Join(tmpDir, "merged.sarif")

Expand All @@ -277,6 +277,22 @@ var analyzeCmd = &cobra.Command{
log.Fatalf("Failed to merge SARIF outputs: %v", err)
}

// Add error runs to the merged SARIF
if len(failedTools) > 0 {
mergedSarif, err := utils.ReadSarifFile(tmpOutputFile)
if err != nil {
log.Fatalf("Failed to read merged SARIF: %v", err)
}

for toolName, err := range failedTools {
utils.AddErrorRun(&mergedSarif, toolName, err.Error())
Copy link
Contributor

@machadoit machadoit Apr 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my head, each tool has the responsability of returning its error and output to its SARIF if that is the format. Meaning, that on this method, I would not expect to have to mess with their SARIF outputs, does it make sense? 🤔

So the error that the tool propagates, is the error that we show on the STDOUT, but the SARIF would not necessarily be an extra one with that error.

I guess the question/concern is, if ESLint still creates a SARIF with an error, it would be nice to use that SARIF. Do you know if ESLint or other tools create a SARIF with information in case of failures?

Copy link
Contributor Author

@zhamborova zhamborova Apr 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The eslint does indeed capture some errors and outputs it in sarif, and current implementation lets eslint handle its own errors and report them in SARIF format but if it was unable to run, we would handle the catastrophic failures where the tool couldnt't run at all.

Definitely open to refining this logic to maybe separate "tool-reported" vs "wrapper-reported" errors, or do the intention was not to even capture those? 🤔

Copy link
Contributor

@machadoit machadoit Apr 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah let's sync tomorrow, for me, especially for tools that already handle some things well on their own SARIF, would avoid creating generic error SARIF. Would aim to share the errors on the STDOUT for those 'fatal problems' where the tool didn't even ran.

}

if err := utils.WriteSarifFile(mergedSarif, tmpOutputFile); err != nil {
log.Fatalf("Failed to write updated SARIF: %v", err)
}
}

if outputFile != "" {
// copy tmpOutputFile to outputFile
content, err := os.ReadFile(tmpOutputFile)
Expand All @@ -302,17 +318,17 @@ var analyzeCmd = &cobra.Command{
},
}

func runTool(workDirectory string, toolName string, args []string, outputFile string) {
func runTool(workDirectory string, toolName string, args []string, outputFile string) error {
switch toolName {
case "eslint":
runEslintAnalysis(workDirectory, args, autoFix, outputFile, outputFormat)
return runEslintAnalysis(workDirectory, args, autoFix, outputFile, outputFormat)
case "trivy":
runTrivyAnalysis(workDirectory, args, outputFile, outputFormat)
return runTrivyAnalysis(workDirectory, args, outputFile, outputFormat)
case "pmd":
runPmdAnalysis(workDirectory, args, outputFile, outputFormat)
return runPmdAnalysis(workDirectory, args, outputFile, outputFormat)
case "pylint":
runPylintAnalysis(workDirectory, args, outputFile, outputFormat)
return runPylintAnalysis(workDirectory, args, outputFile, outputFormat)
default:
log.Printf("Warning: Unsupported tool: %s\n", toolName)
return fmt.Errorf("unsupported tool: %s", toolName)
}
}
19 changes: 12 additions & 7 deletions tools/eslintRunner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import (
"codacy/cli-v2/config"
"fmt"
"os"
"os/exec"
"path/filepath"
Expand All @@ -10,7 +11,7 @@
// * Run from the root of the repo we want to analyse
// * NODE_PATH="<the installed eslint path>/node_modules"
// * The local installed ESLint should have the @microsoft/eslint-formatter-sarif installed
func RunEslint(repositoryToAnalyseDirectory string, eslintInstallationDirectory string, nodeBinary string, pathsToCheck []string, autoFix bool, outputFile string, outputFormat string) {
func RunEslint(repositoryToAnalyseDirectory string, eslintInstallationDirectory string, nodeBinary string, pathsToCheck []string, autoFix bool, outputFile string, outputFormat string) error {

Check warning on line 14 in tools/eslintRunner.go

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

tools/eslintRunner.go#L14 

Method RunEslint has 7 parameters (limit is 5)

Check warning on line 14 in tools/eslintRunner.go

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

tools/eslintRunner.go#L14 

Method RunEslint has a cyclomatic complexity of 9 (limit is 7)
eslintInstallationNodeModules := filepath.Join(eslintInstallationDirectory, "node_modules")
eslintJsPath := filepath.Join(eslintInstallationNodeModules, ".bin", "eslint")

Expand Down Expand Up @@ -50,10 +51,14 @@
nodePathEnv := "NODE_PATH=" + eslintInstallationNodeModules
cmd.Env = append(cmd.Env, nodePathEnv)

// DEBUG
// fmt.Println(cmd.Env)
// fmt.Println(cmd)

// TODO eslint returns 1 when it finds errors, so we're not propagating it
cmd.Run()
// Run the command and handle errors
err := cmd.Run()
if err != nil {
// ESLint returns 1 when it finds errors, which is not a failure
if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 {
return nil
}
return fmt.Errorf("failed to run ESLint: %w", err)
}
return nil
}
135 changes: 130 additions & 5 deletions utils/sarif.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,9 @@
}

type Run struct {
Tool Tool `json:"tool"`
Results []Result `json:"results"`
Tool Tool `json:"tool"`
Results []Result `json:"results"`
Invocations []Invocation `json:"invocations,omitempty"`
}

type Tool struct {
Expand Down Expand Up @@ -80,18 +81,89 @@
type Region struct {
StartLine int `json:"startLine"`
StartColumn int `json:"startColumn"`
EndLine int `json:"endLine"`
EndColumn int `json:"endColumn"`
}

type Invocation struct {

Check notice on line 88 in utils/sarif.go

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

utils/sarif.go#L88 

exported type Invocation should have comment or be unexported
ExecutionSuccessful bool `json:"executionSuccessful"`
ExitCode int `json:"exitCode"`
ExitSignalName string `json:"exitSignalName"`
ExitSignalNumber int `json:"exitSignalNumber"`
Stderr Artifact `json:"stderr"`
}

type Artifact struct {

Check notice on line 96 in utils/sarif.go

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

utils/sarif.go#L96 

exported type Artifact should have comment or be unexported
Text string `json:"text"`
}

type MessageText struct {
Text string `json:"text"`
}

// ReadSarifFile reads a SARIF file and returns its contents
func ReadSarifFile(file string) (SarifReport, error) {
data, err := os.ReadFile(file)
if err != nil {
return SarifReport{}, fmt.Errorf("failed to read SARIF file: %w", err)
}

var sarif SarifReport
if err := json.Unmarshal(data, &sarif); err != nil {
return SarifReport{}, fmt.Errorf("failed to parse SARIF file: %w", err)
}

return sarif, nil
}

// WriteSarifFile writes a SARIF report to a file
func WriteSarifFile(sarif SarifReport, outputFile string) error {
out, err := os.Create(outputFile)
if err != nil {
return fmt.Errorf("failed to create output file: %w", err)
}
defer out.Close()

encoder := json.NewEncoder(out)
encoder.SetIndent("", " ")
if err := encoder.Encode(sarif); err != nil {
return fmt.Errorf("failed to write SARIF: %w", err)
}

return nil
}

// AddErrorRun adds an error run to an existing SARIF report
func AddErrorRun(sarif *SarifReport, toolName string, errorMessage string) {
errorRun := Run{
Tool: Tool{
Driver: Driver{
Name: toolName,
Version: "1.0.0",
},
},
Invocations: []Invocation{
{
ExecutionSuccessful: false,
ExitCode: 1,
ExitSignalName: "error",
ExitSignalNumber: 1,
Stderr: Artifact{
Text: errorMessage,
},
},
},
Results: []Result{},
}
sarif.Runs = append(sarif.Runs, errorRun)
}

// ConvertPylintToSarif converts Pylint JSON output to SARIF format
func ConvertPylintToSarif(pylintOutput []byte) []byte {
var issues []PylintIssue
if err := json.Unmarshal(pylintOutput, &issues); err != nil {
// If parsing fails, return empty SARIF report
return createEmptySarifReport()
// If parsing fails, return empty SARIF report with error
return createEmptySarifReportWithError(err.Error())
}

// Create SARIF report
Expand All @@ -108,6 +180,17 @@
},
},
Results: make([]Result, 0, len(issues)),
Invocations: []Invocation{
{
ExecutionSuccessful: true, // Pylint ran successfully if we got here
ExitCode: 0,
ExitSignalName: "",
ExitSignalNumber: 0,
Stderr: Artifact{
Text: "",
},
},
},
},
},
}
Expand Down Expand Up @@ -161,6 +244,33 @@

// createEmptySarifReport creates an empty SARIF report in case of errors
func createEmptySarifReport() []byte {
emptyReport := SarifReport{
Version: "2.1.0",
Schema: "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
Runs: []Run{
{
Tool: Tool{
Driver: Driver{
Name: "Pylint",
Version: "3.3.6",
InformationURI: "https://pylint.org",
},
},
Results: []Result{},
Invocations: []Invocation{},
},
},
}
sarifData, err := json.MarshalIndent(emptyReport, "", " ")
if err != nil {
fmt.Fprintf(os.Stderr, "Error marshaling empty SARIF report: %v\n", err)
return []byte("{}")
}
return sarifData
}

// createEmptySarifReportWithError creates an empty SARIF report with error information
func createEmptySarifReportWithError(errorMessage string) []byte {
emptyReport := SarifReport{
Version: "2.1.0",
Schema: "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
Expand All @@ -174,10 +284,25 @@
},
},
Results: []Result{},
Invocations: []Invocation{
{
ExecutionSuccessful: false,
ExitCode: 1,
ExitSignalName: "error",
ExitSignalNumber: 1,
Stderr: Artifact{
Text: errorMessage,
},
},
},
},
},
}
sarifData, _ := json.MarshalIndent(emptyReport, "", " ")
sarifData, err := json.MarshalIndent(emptyReport, "", " ")
if err != nil {
fmt.Fprintf(os.Stderr, "Error marshaling SARIF report with error: %v\n", err)
return []byte("{}")
}
return sarifData
}

Expand Down