Fix cacheRecoveryError's %v-not-%w bug and extend errorfwrapv/fmterrorfnoverbs to concatenated format strings

docs/adr/58715-fix-concatenated-fmt-errorf-analysis.md

@@ -0,0 +1,53 @@
+# ADR-58715: Fix concatenated fmt.Errorf analysis in linters
+
+**Date**: 2026-09-05
+**Status**: Draft
+**Deciders**: gh-aw maintainers
+
+---
+
+## Context
+
+The `cacheRecoveryError` helper in the audit pipeline built its `fmt.Errorf` format string from concatenated string fragments and formatted the causal error with `%v`, which prevented callers from using `errors.Is` and `errors.As` on the returned error. The existing `errorfwrapv` and `fmterrorfnoverbs` linters only inspected plain string literals, so they missed this bug pattern when the format string was assembled with `+` concatenation. This pull request fixes the production bug and extends the shared linter infrastructure so the same class of mistake is detected in concatenated format strings. The implementation must avoid inventing false format verbs when opaque non-literal operands appear between literal fragments.
+
+## Decision
+
+We will refactor `cacheRecoveryError` to use a literal format string with `%s` for its message argument and `%w` for its error argument (`fmt.Errorf("%s\n\n...", message, runID, runOutputDir, err)`), avoiding format string concatenation in production while wrapping the causal error with `%w`.
+
+We will add a shared AST utility, `astutil.ResolveFormatString`, that resolves `fmt.Errorf`-style format-string expressions built from string literals and `+`-concatenated literal trees. When an expression contains non-literal (opaque) operands, `ResolveFormatString` returns `ok = false` because format verbs and positional argument indices cannot be proven at compile time.
+
+We will update `errorfwrapv` and `fmterrorfnoverbs` to analyze format strings resolved by `astutil.ResolveFormatString`. This allows detecting mistakes in multi-line or concatenated string literal format strings without risking false positives or incorrect argument indexing when opaque operands are present.
+
+## Alternatives Considered
+
+### Keep linter analysis limited to plain string literals
+
+This was the previous behavior and would have minimized implementation change in the linter stack. It was rejected because the production bug in `cacheRecoveryError` demonstrates that real code in this repository already constructs `fmt.Errorf` format strings through concatenation, so literal-only analysis leaves an important blind spot.
+
+### Special-case only `cacheRecoveryError`
+
+The PR could have changed `%v` to `%w` in the helper and added a regression test without touching shared linter code. It was rejected because the PR evidence shows the underlying issue is broader than one helper: both `errorfwrapv` and `fmterrorfnoverbs` missed concatenated format strings, so a one-off fix would not prevent recurrence elsewhere.
+
+### Fully evaluate arbitrary non-literal string expressions
+
+A more aggressive option would be to resolve identifiers, function calls, or constant propagation across all string-producing expressions. It was rejected because the current PR only justifies support for concatenated expressions with literal segments, and broader evaluation would add complexity and risk without evidence it is needed for this bug class.
+
+## Consequences
+
+### Positive
+
+- `cacheRecoveryError` now preserves the wrapped error chain, so callers can use `errors.Is` and `errors.As` on permission and cache-recovery failures.
+- `errorfwrapv` and `fmterrorfnoverbs` now detect concatenated `fmt.Errorf` patterns that previously escaped linting, reducing the chance of similar regressions.
+
+### Negative
+
+- The shared AST utility adds concatenation tree traversal, which requires test coverage for nested `+` literal expressions.
+
+### Neutral
+
+- The new analysis intentionally skips format strings with non-literal (opaque) operands, choosing safety over unprovable expression evaluation.
+- Additional unit and testdata coverage is required to lock in behavior around concatenation, verb detection, and false-positive prevention.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/cli/audit_run_pipeline.go

@@ -315,13 +315,13 @@ func downloadLegacyEvalsArtifactIfNeeded(ctx context.Context, cfg auditRunConfig
 }
 
 func cacheRecoveryError(message string, runID int64, runOutputDir string, err error) error {
-	return fmt.Errorf(message+"\n\n"+
+	return fmt.Errorf("%s\n\n"+
 		"To download artifacts, use the GitHub MCP server:\n\n"+
 		"1. Use the github-mcp-server tool 'download_workflow_run_artifacts' with:\n"+
 		"   - run_id: %d\n"+
 		"   - output_directory: %s\n\n"+
 		"2. After downloading, run this audit command again to analyze the cached artifacts.\n\n"+
-		"Original error: %v", runID, runOutputDir, err)
+		"Original error: %w", message, runID, runOutputDir, err)
 }
 
 func prepareRunForAnalysis(run WorkflowRun, cfg auditRunConfig, useLocalCache bool) WorkflowRun {

pkg/cli/audit_run_pipeline_test.go

@@ -123,13 +123,16 @@ func TestAuditRunConfigAuditOptions(t *testing.T) {
 
 func TestCacheRecoveryError(t *testing.T) {
 	t.Parallel()
-	err := cacheRecoveryError("GitHub API access denied.", 1234, "/tmp/run-1234", errors.New("boom"))
+	sentinel := errors.New("boom")
+	err := cacheRecoveryError("GitHub API access denied.", 1234, "/tmp/run-1234", sentinel)
 	require.Error(t, err)
 	msg := err.Error()
 	assert.Contains(t, msg, "GitHub API access denied.")
 	assert.Contains(t, msg, "1234")
 	assert.Contains(t, msg, "/tmp/run-1234")
 	assert.Contains(t, msg, "boom")
+	require.ErrorIs(t, err, sentinel, "cacheRecoveryError must wrap its cause with %%w so errors.Is can match it")
+	require.Error(t, errors.Unwrap(err), "cacheRecoveryError result must be unwrappable")
 }
 
 func TestPrepareRunForAnalysis(t *testing.T) {

pkg/linters/errorfwrapv/errorfwrapv.go

@@ -7,7 +7,6 @@ package errorfwrapv
 import (
 	"errors"
 	"go/ast"
-	"go/token"
 	"go/types"
 	"strconv"
 
@@ -67,12 +66,12 @@ func analyzeFmtErrorfCall(pass *analysis.Pass, n ast.Node, generatedFiles filech
 	if len(call.Args) == 0 {
 		return
 	}
-	lit, ok := call.Args[0].(*ast.BasicLit)
-	if !ok || lit.Kind != token.STRING {
+	formatStr, ok := astutil.ResolveFormatString(call.Args[0])
+	if !ok {
 		return
 	}
 
-	verbs := parseFormatVerbs(lit.Value)
+	verbs := parseFormatVerbs(formatStr)
 	errorArgVerbs, wrappedErrorArgs, hasVerbV := classifyErrorArgs(pass, call, verbs)
 
 	if hasVerbV {
@@ -153,11 +152,11 @@ func needsWrapping(verbs []rune) bool {
 	return false
 }
 
+// parseFormatVerbs scans a format string produced by astutil.ResolveFormatString
+// — the unquoted literal segments of a fmt.Errorf format-string argument,
+// concatenated in order — for format verbs.
 func parseFormatVerbs(s string) []formatVerb {
 	var verbs []formatVerb
-	if len(s) >= 2 {
-		s = s[1 : len(s)-1]
-	}
 
 	nextArgIdx := 0
 	for i := 0; i < len(s); i++ {

pkg/linters/errorfwrapv/testdata/src/errorfwrapv/errorfwrapv.go

@@ -94,6 +94,28 @@ func GoodMixedVerbs(name string, err error) error {
 	return fmt.Errorf("operation %v failed: %w", name, err)
 }
 
+// BadConcatVWrap builds its format string via string concatenation of literals.
+// The trailing error argument is formatted with %v instead of %w.
+func BadConcatVWrap(err error) error {
+	return fmt.Errorf("context: %d\n"+ // want `fmt\.Errorf formats an error argument with %v`
+		"Original error: %v", 42, err)
+}
+
+// GoodConcatWWrap builds its format string via string concatenation of literals
+// and correctly wraps the trailing error with %w.
+func GoodConcatWWrap(err error) error {
+	return fmt.Errorf("context: %d\n"+
+		"Original error: %w", 42, err)
+}
+
+// OpaqueConcatVWrap builds its format string with a caller-supplied opaque prefix;
+// because the format string contains non-literal components, it cannot be safely analyzed.
+func OpaqueConcatVWrap(prefix string, err error) error {
+	return fmt.Errorf(prefix+"\n\n"+
+		"context: %d\n"+
+		"Original error: %v", 42, err)
+}
+
 // SuppressedByNolint is intentionally suppressed.
 func SuppressedByNolint(err error) error {
 	return fmt.Errorf("operation failed: %v", err) //nolint:errorfwrapv

pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs.go

@@ -5,7 +5,6 @@ package fmterrorfnoverbs
 
 import (
 	"go/ast"
-	"go/token"
 
 	"golang.org/x/tools/go/analysis"
 
@@ -42,26 +41,28 @@ func run(pass *analysis.Pass) (any, error) {
 			return
 		}
 
-		lit, ok := call.Args[0].(*ast.BasicLit)
-		if !ok || lit.Kind != token.STRING {
+		formatStr, ok := astutil.ResolveFormatString(call.Args[0])
+		if !ok {
 			return
 		}
 
-		// Unquote the string value
-		val := lit.Value
-		if len(val) >= 2 {
-			val = val[1 : len(val)-1]
-		}
-
-		if !hasRealFormatVerb(val) {
+		if !hasRealFormatVerb(formatStr) {
 			position := pass.Fset.PositionFor(call.Pos(), false)
 			if filecheck.ShouldSkipFilename(position.Filename, generatedFiles) {
 				return
 			}
 			if nolint.HasDirectiveForLinter(position, nolintIndex, "fmterrorfnoverbs") {
 				return
 			}
-			pass.ReportRangef(call, "fmt.Errorf called with no format verbs; use errors.New(%s) instead", lit.Value)
+			if _, isPlainLit := call.Args[0].(*ast.BasicLit); isPlainLit {
+				pass.ReportRangef(call, "fmt.Errorf called with no format verbs; use errors.New(%q) instead", formatStr)
+				return
+			}
+			// The format string is built from concatenated pieces (e.g. a
+			// caller-supplied prefix plus literal text), so formatStr doesn't
+			// correspond to a single source literal; suggest errors.New
+			// generically instead of proposing a synthetic replacement.
+			pass.ReportRangef(call, "fmt.Errorf called with no format verbs; use errors.New instead")
 		}
 	})
 }

pkg/linters/fmterrorfnoverbs/testdata/src/fmterrorfnoverbs/fmterrorfnoverbs.go

@@ -63,3 +63,21 @@ func realVerbWithEscapePercent(n int) error {
 func multipleEscapePercents() error {
 	return fmtalias.Errorf("between 50%% and 90%% utilised") // want `fmt\.Errorf called with no format verbs; use errors\.New`
 }
+
+// concatNoVerbs builds its format string via string concatenation of literals;
+// the literal portion has no format verbs.
+func concatNoVerbs() error {
+	return fmtalias.Errorf("operation failed: " + "occurred with no additional context") // want `fmt\.Errorf called with no format verbs; use errors\.New`
+}
+
+// opaqueConcatNoVerbs builds format string with an opaque prefix;
+// must NOT be flagged because prefix may contain verbs at runtime.
+func opaqueConcatNoVerbs(prefix string) error {
+	return fmtalias.Errorf(prefix + " occurred with no additional context")
+}
+
+// concatWithVerb builds its format string via string concatenation of literals and
+// contains a real verb; must NOT be flagged.
+func concatWithVerb(n int) error {
+	return fmtalias.Errorf("operation failed: "+"occurred %d times", n)
+}

pkg/linters/internal/astutil/astutil.go

@@ -706,6 +706,34 @@ func StringLitValue(expr ast.Expr) (string, bool) {
 	return s, true
 }
 
+// ResolveFormatString resolves a fmt.Errorf-style format-string argument
+// expression into its literal text content. It handles plain string literals
+// as well as string concatenation via the `+` operator where all operands
+// are string literals (e.g. `"header\n" + "body %w"`).
+//
+// If any operand in a concatenated expression is not a string literal (e.g.
+// an identifier or function call), the format string cannot be fully resolved
+// at compile time (its format verbs and argument counts are unprovable), so ok
+// is false.
+func ResolveFormatString(expr ast.Expr) (value string, ok bool) {
+	if s, litOK := StringLitValue(expr); litOK {
+		return s, true
+	}
+	bin, isBin := expr.(*ast.BinaryExpr)
+	if !isBin || bin.Op != token.ADD {
+		return "", false
+	}
+	left, leftOK := ResolveFormatString(bin.X)
+	if !leftOK {
+		return "", false
+	}
+	right, rightOK := ResolveFormatString(bin.Y)
+	if !rightOK {
+		return "", false
+	}
+	return left + right, true
+}
+
 // IsInInitFunction reports whether cur is inside a top-level init() function.
 // Only top-level (no receiver) init functions are recognized; methods named
 // init are ordinary methods and are not exempt. A node whose innermost

pkg/linters/internal/astutil/astutil_test.go

@@ -1078,6 +1078,76 @@ func TestStringLitValue(t *testing.T) {
 	}
 }
 
+func TestResolveFormatString(t *testing.T) {
+	t.Parallel()
+
+	strLit := func(v string) ast.Expr { return &ast.BasicLit{Kind: token.STRING, Value: v} }
+	concat := func(exprs ...ast.Expr) ast.Expr {
+		result := exprs[0]
+		for _, e := range exprs[1:] {
+			result = &ast.BinaryExpr{X: result, Op: token.ADD, Y: e}
+		}
+		return result
+	}
+
+	tests := []struct {
+		name   string
+		expr   ast.Expr
+		want   string
+		wantOK bool
+	}{
+		{
+			name:   "plain string literal",
+			expr:   strLit(`"operation failed: %w"`),
+			want:   "operation failed: %w",
+			wantOK: true,
+		},
+		{
+			name:   "concatenation of two literals",
+			expr:   concat(strLit(`"a"`), strLit(`"b: %v"`)),
+			want:   "ab: %v",
+			wantOK: true,
+		},
+		{
+			name:   "concatenation with a leading non-literal identifier returns ok=false",
+			expr:   concat(ast.NewIdent("message"), strLit(`"\n\nOriginal error: %v"`)),
+			want:   "",
+			wantOK: false,
+		},
+		{
+			name:   "opaque operand between literal segments returns ok=false",
+			expr:   concat(strLit(`"abc%"`), ast.NewIdent("errStr"), strLit(`"v..."`)),
+			want:   "",
+			wantOK: false,
+		},
+		{
+			name:   "concatenation of only non-literal identifiers",
+			expr:   concat(ast.NewIdent("a"), ast.NewIdent("b")),
+			wantOK: false,
+		},
+		{
+			name:   "non-ADD binary expression",
+			expr:   &ast.BinaryExpr{X: strLit(`"a"`), Op: token.SUB, Y: strLit(`"b"`)},
+			wantOK: false,
+		},
+		{
+			name:   "bare identifier",
+			expr:   ast.NewIdent("x"),
+			wantOK: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			got, ok := ResolveFormatString(tt.expr)
+			if ok != tt.wantOK || got != tt.want {
+				t.Fatalf("ResolveFormatString() = (%q, %v), want (%q, %v)", got, ok, tt.want, tt.wantOK)
+			}
+		})
+	}
+}
+
 func TestIsRegexpCompileCall(t *testing.T) {
 	t.Parallel()