Improve the output of violations

This improves upon the output of violations. It does so in a couple of
ways. First it removes the inline suggestions - which become clutter for
many results - in favor of recommending the use of `ades -explain` at
the end of the human-readable report (also resolving the no-op of the
`-suggestions` flag when `-json` is used). Second, it now groups the
violations by the job in which they occur, reducing the amount of
duplicated text in the output.

Additionally this makes the order of the results in the human readable
report deterministic, which is slightly nicer for humans because it is
somewhat sensibly ordered, but notably resolves some flakiness in the
tests.

Signed-off-by: Eric Cornelissen <[email protected]>

RULES.md

@@ -7,8 +7,8 @@ to address it.
 
 ## <a id="ADES100"></a> ADES100 - Expression in `run:` directive
 
-When an expression appears in a `run:` directive you can avoid any potential attacks by extracting
-the expression into an environment variable and using the environment variable instead.
+When an expression appears in a `run:` directive you can avoid potential attacks by extracting the
+expression into an environment variable and using the environment variable instead.
 
 For example, given the workflow snippet:
 
@@ -34,9 +34,8 @@ it can be made safer by converting it into:
 
 ## <a id="ADES101"></a> ADES101 - Expression in `actions/github-script` script
 
-When an expression appears in a `actions/github-script` script you can avoid any potential attacks
-by extracting the expression into an environment variable and using the environment variable
-instead.
+When an expression appears in a `actions/github-script` script you can avoid potential attacks by
+extracting the expression into an environment variable and using the environment variable instead.
 
 For example, given the workflow snippet:
 
@@ -65,8 +64,8 @@ it can be made safer by converting it into:
 ## <a id="ADES102"></a> ADES102 - Expression in `roots/issue-closer` issue close message
 
 When an expression appears in the issue close message of `roots/issue-closer` it is interpreted as
-an ES6-style template literal. You can avoid any potential attacks by extracting the expression into
-an environment variable and using the environment variable instead.
+an ES6-style template literal. You can avoid potential attacks by extracting the expression into an
+environment variable and using the environment variable instead.
 
 For example, given the workflow snippet:
 
@@ -93,7 +92,7 @@ it can be made safer by converting it into:
 ## <a id="ADES103"></a> ADES103 - Expression in `roots/issue-closer` pull request close message
 
 When an expression appears in the pull request close message of `roots/issue-closer` it is
-interpreted as an ES6-style template literal. You can avoid any potential attacks by extracting the
+interpreted as an ES6-style template literal. You can avoid potential attacks by extracting the
 expression into an environment variable and using the environment variable instead.
 
 For example, given the workflow snippet:

cmd/ades/main.go

@@ -21,9 +21,12 @@ import (
 	"fmt"
 	"io"
 	"io/fs"
+	"maps"
 	"os"
 	"path/filepath"
 	"regexp"
+	"slices"
+	"sort"
 	"strings"
 
 	"github.com/ericcornelissen/ades"
@@ -64,7 +67,7 @@ var (
 	flagSuggestions = flag.Bool(
 		"suggestions",
 		false,
-		"Show suggested fixes inline",
+		"No effect (deprecated)",
 	)
 	flagVersion = flag.Bool(
 		"version",
@@ -91,6 +94,11 @@ func run() int {
 		return exitSuccess
 	}
 
+	if *flagSuggestions {
+		fmt.Fprintln(os.Stderr, "The -suggestions flag is deprecated and will be removed in the future")
+		fmt.Fprintln(os.Stderr, "")
+	}
+
 	if *flagExplain != "" {
 		explanation, err := ades.Explain(*flagExplain)
 		if err != nil {
@@ -139,23 +147,33 @@ func run() int {
 	if *flagJson {
 		fmt.Println(printJson(report))
 	} else {
+		targets := slices.Collect(maps.Keys(report))
+		sort.Strings(targets)
+
 		separator := false
-		for target, violations := range report {
+		for _, target := range targets {
+			violations := report[target]
+
 			if separator {
 				fmt.Println( /* empty line between targets */ )
 			}
 			if len(targets) > 1 {
 				fmt.Printf("[%s]\n", target)
 			}
 
-			fmt.Print(printViolations(violations, *flagSuggestions))
+			fmt.Print(printProjectViolations(violations))
 			separator = true
 		}
 	}
 
 	for _, targetViolations := range report {
 		for _, fileViolations := range targetViolations {
 			if len(fileViolations) > 0 {
+				if !(*flagJson) {
+					fmt.Println()
+					fmt.Println("Use -explain for more details and fix suggestions (example: 'ades -explain ADES100')")
+				}
+
 				return exitViolations
 			}
 		}
@@ -417,7 +435,7 @@ Flags:
   -help               Show this help message and exit.
   -json               Output results in JSON format.
   -legal              Show legal information and exit.
-  -suggestions        Show suggested fixes inline.
+  -suggestions        No effect (deprecated).
   -version            Show the program version and exit.
   -                   Read workflow or manifest from stdin.
 

cmd/ades/output.go

@@ -18,6 +18,8 @@ package main
 import (
 	"encoding/json"
 	"fmt"
+	"maps"
+	"slices"
 	"sort"
 	"strings"
 
@@ -60,19 +62,20 @@ func printJson(rawViolations map[string]map[string][]ades.Violation) string {
 	return string(jsonBytes)
 }
 
-func printViolations(violations map[string][]ades.Violation, suggestions bool) string {
+func printProjectViolations(violations map[string][]ades.Violation) string {
 	clean := true
 
+	files := slices.Collect(maps.Keys(violations))
+	sort.Strings(files)
+
 	var sb strings.Builder
-	for file, fileViolations := range violations {
+	for _, file := range files {
+		fileViolations := violations[file]
 		if cnt := len(fileViolations); cnt > 0 {
 			clean = false
 			sb.WriteString(fmt.Sprintf("Detected %d violation(s) in %q:", cnt, file))
 			sb.WriteRune('\n')
-			for _, violation := range fileViolations {
-				sb.WriteString(printViolation(&violation, suggestions))
-				sb.WriteRune('\n')
-			}
+			sb.WriteString(printFileViolations(fileViolations))
 		}
 	}
 
@@ -83,21 +86,35 @@ func printViolations(violations map[string][]ades.Violation, suggestions bool) s
 	}
 }
 
-func printViolation(violation *ades.Violation, suggestions bool) string {
-	var sb strings.Builder
-	if violation.JobId == "" {
-		sb.WriteString(fmt.Sprintf("  step %q has %q", violation.StepId, violation.Problem))
-	} else {
-		sb.WriteString(fmt.Sprintf("  job %q, step %q has %q", violation.JobId, violation.StepId, violation.Problem))
+func printFileViolations(violations []ades.Violation) string {
+	byJob := make(map[string][]ades.Violation, len(violations))
+	for _, violation := range violations {
+		jobId := violation.JobId
+		if _, ok := byJob[jobId]; !ok {
+			byJob[jobId] = make([]ades.Violation, 0)
+		}
+
+		byJob[jobId] = append(byJob[jobId], violation)
 	}
 
-	if suggestions {
-		suggestion, _ := ades.Suggestion(violation)
+	jobs := slices.Collect(maps.Keys(byJob))
+	sort.Strings(jobs)
 
-		sb.WriteString(", suggestion:\n")
-		sb.WriteString(suggestion)
-	} else {
-		sb.WriteString(fmt.Sprintf(" (%s)", violation.RuleId))
+	var sb strings.Builder
+	for _, job := range jobs {
+		violations := byJob[job]
+
+		if job != "" {
+			sb.WriteString("  ")
+			sb.WriteString(fmt.Sprintf("%d in job %q:", len(violations), job))
+			sb.WriteRune('\n')
+		}
+
+		for _, violation := range violations {
+			sb.WriteString("    ")
+			sb.WriteString(fmt.Sprintf("step %q contains %q (%s)", violation.StepId, violation.Problem, violation.RuleId))
+			sb.WriteRune('\n')
+		}
 	}
 
 	return sb.String()