diff --git a/Makefile b/Makefile index 04df77d164d..b3e6675b343 100644 --- a/Makefile +++ b/Makefile @@ -1023,6 +1023,16 @@ lint-cjs: fmt-check-cjs validate-cjs-syntax check-node-version lint-json: fmt-check-json @echo "✓ JSON formatting validated" +# Full-repository error-message audit (non-blocking report). +# Reports pre-existing error-message violations across the repo so the debt can +# be tracked as a metric over time. Always exits 0. +.PHONY: lint-error-messages-report +lint-error-messages-report: + @echo "Building custom linters..." + @env -u GOOS -u GOARCH go build -o /tmp/gh-aw-linters ./cmd/linters + @echo "Auditing error messages across $(LINTER_PACKAGES) (non-blocking)..." + @/tmp/gh-aw-linters -errormessage -errormessage.full-repo $(LINTER_PACKAGES) || true + # Lint error messages for quality compliance .PHONY: lint-errors lint-errors: @@ -1399,6 +1409,7 @@ help: @echo " validate-cjs-syntax - Syntax-check all non-test .cjs files (catches module-load SyntaxErrors)" @echo " lint-json - Lint JSON files in pkg directory (excluding actions/setup/js)" @echo " lint-errors - Lint error messages for quality compliance" + @echo " lint-error-messages-report - Non-blocking full-repo error message audit" @echo " validate-otel-contract - Validate the gh-aw OpenTelemetry compatibility contract" @echo " lint-action-sh - Lint action shell scripts for python/python3 invocations" @echo " shellcheck-setup-sh - Run shellcheck on actions/setup/sh scripts" diff --git a/pkg/linters/README.md b/pkg/linters/README.md index 60fe1ff1a20..30f7ea61b4d 100644 --- a/pkg/linters/README.md +++ b/pkg/linters/README.md @@ -15,7 +15,7 @@ This package currently provides custom Go analyzers in the following subpackages - `deferinloop` — reports `defer` statements placed directly inside `for`/`range` loop bodies, which execute when the enclosing function returns rather than each iteration and can cause resource leaks. - `errorfwrapv` — reports `fmt.Errorf` calls that pass error arguments without `%w` wrapping. - `excessivefuncparams` — reports function declarations that exceed a configurable parameter-count threshold. -- `errormessage` — reports non-actionable error-message patterns in changed files. +- `errormessage` — reports non-actionable error-message patterns in changed files; pass `-errormessage.full-repo` (or `-errormessage.changed-files=all`) to audit the whole repository. - `errortypeassertion` — reports type assertions from `error` to concrete types and recommends `errors.As`. - `errstringmatch` — reports `strings.Contains(err.Error(), "...")` patterns and recommends `errors.Is` / `errors.As`. - `fileclosenotdeferred` — reports non-deferred file `Close()` calls that can leak resources. @@ -85,7 +85,7 @@ This package currently provides custom Go analyzers in the following subpackages | `deferinloop` | Custom `go/analysis` analyzer that flags `defer` statements inside `for`/`range` loop bodies that execute when the enclosing function returns rather than each iteration | | `errorfwrapv` | Custom `go/analysis` analyzer that flags `fmt.Errorf` calls that pass error arguments without `%w` wrapping | | `excessivefuncparams` | Custom `go/analysis` analyzer that flags function declarations with too many positional parameters | -| `errormessage` | Custom `go/analysis` analyzer that flags non-actionable error message patterns in changed files | +| `errormessage` | Custom `go/analysis` analyzer that flags non-actionable error message patterns in changed files (or all files with `-errormessage.full-repo`) | | `errortypeassertion` | Custom `go/analysis` analyzer that flags type assertions from `error` to concrete types and recommends `errors.As` | | `errstringmatch` | Custom `go/analysis` analyzer that flags brittle `strings.Contains(err.Error(), "...")` checks | | `execcommandwithoutcontext` | Custom `go/analysis` analyzer that flags `exec.Command(...)` calls that should use `exec.CommandContext(...)` in context-receiving functions | diff --git a/pkg/linters/errormessage/errormessage.go b/pkg/linters/errormessage/errormessage.go index 757012b3af5..bc54c18b7a4 100644 --- a/pkg/linters/errormessage/errormessage.go +++ b/pkg/linters/errormessage/errormessage.go @@ -24,16 +24,29 @@ var ( // changedFilesCSV allows CI to scope linting to changed files only, // preventing legacy violations from blocking incremental adoption. changedFilesCSV string + // fullRepo enables auditing every analyzed file instead of only the + // changed ones, so pre-existing violations can be tracked as a metric. + fullRepo bool ) +// fullRepoSentinel is the -changed-files value that enables full-repository +// auditing, equivalent to passing -full-repo. +const fullRepoSentinel = "all" + // Analyzer is the errormessage analysis pass. -var Analyzer = analyzerutil.New("errormessage", "reports non-actionable error message patterns in changed files", run) +var Analyzer = analyzerutil.New("errormessage", "reports non-actionable error message patterns in changed files (or all files with -full-repo)", run) func init() { - Analyzer.Flags.StringVar(&changedFilesCSV, "changed-files", "", "comma-separated list of changed file paths to lint (when empty, analyzer is a no-op)") + Analyzer.Flags.StringVar(&changedFilesCSV, "changed-files", "", "comma-separated list of changed file paths to lint (when empty, analyzer is a no-op; use \"all\" to audit every file)") + Analyzer.Flags.BoolVar(&fullRepo, "full-repo", false, "audit every analyzed file instead of only the changed ones") } func run(pass *analysis.Pass) (any, error) { + if fullRepo || isFullRepoSentinel(changedFilesCSV) { + pkgLog.Printf("analyzing package %s in full-repo mode", pass.Pkg.Path()) + return runOnFiles(pass, nil) + } + changed := parseChangedFiles(changedFilesCSV) if len(changed) == 0 { pkgLog.Printf("no changed files provided for %s, skipping", pass.Pkg.Path()) @@ -41,6 +54,12 @@ func run(pass *analysis.Pass) (any, error) { } pkgLog.Printf("analyzing package %s (%d changed files)", pass.Pkg.Path(), len(changed)) + return runOnFiles(pass, changed) +} + +// runOnFiles analyzes the package. When changed is nil every file is checked +// (full-repo audit mode); otherwise only files present in changed are checked. +func runOnFiles(pass *analysis.Pass, changed map[string]struct{}) (any, error) { noLintIndex, err := nolint.Index(pass) if err != nil { return nil, err @@ -92,7 +111,18 @@ func parseChangedFiles(csv string) map[string]struct{} { return changed } +// isFullRepoSentinel reports whether the -changed-files value requests a +// full-repository audit. +func isFullRepoSentinel(csv string) bool { + return strings.EqualFold(strings.TrimSpace(csv), fullRepoSentinel) +} + +// shouldCheckFile reports whether filename is in scope. A nil changed set means +// full-repo audit mode, where every file is in scope. func shouldCheckFile(filename string, changed map[string]struct{}) bool { + if changed == nil { + return true + } path := filepath.ToSlash(filename) for changedPath := range changed { if path == changedPath || strings.HasSuffix(path, "/"+changedPath) { diff --git a/pkg/linters/errormessage/errormessage_test.go b/pkg/linters/errormessage/errormessage_test.go index 7d928b43b0c..7bde75c9bee 100644 --- a/pkg/linters/errormessage/errormessage_test.go +++ b/pkg/linters/errormessage/errormessage_test.go @@ -22,3 +22,25 @@ func TestErrorMessage(t *testing.T) { analysistest.Run(t, analysistest.TestData(), errormessage.Analyzer, "errormessage") } + +func TestErrorMessageFullRepoFlag(t *testing.T) { + if err := errormessage.Analyzer.Flags.Set("full-repo", "true"); err != nil { + t.Fatalf("failed to set full-repo flag: %v", err) + } + t.Cleanup(func() { + _ = errormessage.Analyzer.Flags.Set("full-repo", "false") + }) + + analysistest.Run(t, analysistest.TestData(), errormessage.Analyzer, "errormessage") +} + +func TestErrorMessageFullRepoSentinel(t *testing.T) { + if err := errormessage.Analyzer.Flags.Set("changed-files", "all"); err != nil { + t.Fatalf("failed to set changed-files flag: %v", err) + } + t.Cleanup(func() { + _ = errormessage.Analyzer.Flags.Set("changed-files", "") + }) + + analysistest.Run(t, analysistest.TestData(), errormessage.Analyzer, "errormessage") +}