Address PR review feedback: require full resolution for concatenated format string analysis

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>

Author: Copilot

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

@@ -12,7 +12,11 @@ The `cacheRecoveryError` helper in the audit pipeline built its `fmt.Errorf` for
 
 ## Decision
 
-We will add a shared AST utility, `astutil.ResolveFormatString`, that resolves `fmt.Errorf`-style format-string expressions from string literals and `+`-concatenated fragments while preserving opaque non-literal boundaries with a placeholder byte. We will update `errorfwrapv` and `fmterrorfnoverbs` to analyze the resolved format string instead of requiring a single `*ast.BasicLit`, and we will change `cacheRecoveryError` to wrap its causal error with `%w`. For concatenated no-verb cases, `fmterrorfnoverbs` will emit a generic `errors.New` suggestion instead of synthesizing a replacement literal that did not exist in source.
+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
 
@@ -37,12 +41,11 @@ A more aggressive option would be to resolve identifiers, function calls, or con
 
 ### Negative
 
-- The shared AST utility adds more nuanced format-string reconstruction logic, which increases maintenance burden and requires careful regression coverage.
-- Placeholder-based reconstruction produces conservative generic diagnostics for some concatenated expressions, so the linter cannot always suggest an exact source replacement.
+- The shared AST utility adds concatenation tree traversal, which requires test coverage for nested `+` literal expressions.
 
 ### Neutral
 
-- The new analysis intentionally treats non-literal operands as opaque boundaries rather than attempting full expression evaluation.
+- 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.
 
 ---

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: %w", 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

@@ -131,8 +131,8 @@ func TestCacheRecoveryError(t *testing.T) {
 	assert.Contains(t, msg, "1234")
 	assert.Contains(t, msg, "/tmp/run-1234")
 	assert.Contains(t, msg, "boom")
-	assert.ErrorIs(t, err, sentinel, "cacheRecoveryError must wrap its cause with %%w so errors.Is can match it")
-	assert.True(t, errors.Unwrap(err) != nil, "cacheRecoveryError result must be unwrappable")
+	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/testdata/src/errorfwrapv/errorfwrapv.go

@@ -94,21 +94,26 @@ func GoodMixedVerbs(name string, err error) error {
 	return fmt.Errorf("operation %v failed: %w", name, err)
 }
 
-// BadConcatVWrap builds its format string via string concatenation with a
-// caller-supplied prefix, mirroring cacheRecoveryError. The trailing error
-// argument is formatted with %v instead of %w.
-func BadConcatVWrap(prefix string, err error) error {
-	return fmt.Errorf(prefix+"\n\n"+ // want `fmt\.Errorf formats an error argument with %v`
-		"context: %d\n"+
+// 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 with a
-// caller-supplied prefix but correctly wraps the trailing error with %w.
-func GoodConcatWWrap(prefix string, err error) error {
+// 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: %w", 42, err)
+		"Original error: %v", 42, err)
 }
 
 // SuppressedByNolint is intentionally suppressed.

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

@@ -64,14 +64,20 @@ 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 with a
-// caller-supplied prefix; the literal portion has no format verbs.
-func concatNoVerbs(prefix string) error {
-	return fmtalias.Errorf(prefix + " occurred with no additional context") // 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`
 }
 
-// concatWithVerb builds its format string via string concatenation and the
-// literal portion contains a real verb; must NOT be flagged.
-func concatWithVerb(prefix string, n int) error {
-	return fmtalias.Errorf(prefix+" occurred %d times", n)
+// 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

@@ -708,35 +708,13 @@ func StringLitValue(expr ast.Expr) (string, bool) {
 
 // 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 (e.g.
-// `message + "...\n" + "..."`), which is common when a format string is built
-// from a caller-supplied prefix followed by literal boilerplate.
+// as well as string concatenation via the `+` operator where all operands
+// are string literals (e.g. `"header\n" + "body %w"`).
 //
-// Non-literal operands (identifiers, calls, etc.) are treated as opaque and
-// their unknowable runtime content is never counted as a %-verb: they are
-// replaced with formatOpaquePlaceholder rather than dropped outright, so a
-// literal segment ending in '%' can never merge across an opaque boundary
-// with the next literal segment's leading character to fabricate a spurious
-// verb (e.g. `"abc%" + errStr + "v..."` cannot be mistaken for `"abc%v..."`).
-// Verbs that occur entirely within a single literal segment are unaffected
-// and keep their relative order, so they still line up positionally with the
-// call's trailing (fmt) arguments.
-//
-// Each returned literal segment is unquoted independently (via
-// StringLitValue), so the result is equivalent to unquoting every literal
-// operand and concatenating them (with opaque placeholders in between) — not
-// to unquoting the full original expression as a single token. This only
-// matters for callers that care about raw escape-sequence boundaries.
-//
-// ok is false unless expr resolves to at least one string literal, so a bare
-// non-literal expression (e.g. a lone identifier) is correctly rejected
-// rather than treated as an empty, verb-free format string.
-//
-// Recursion depth tracks the nesting of `+` operands in expr, which for
-// realistic source code (a chain of concatenated string operands) is
-// bounded by the number of operands and shallow in practice; it is not
-// guarded against pathological or generated expressions with extreme
-// nesting depth.
+// 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
@@ -746,26 +724,16 @@ func ResolveFormatString(expr ast.Expr) (value string, ok bool) {
 		return "", false
 	}
 	left, leftOK := ResolveFormatString(bin.X)
-	right, rightOK := ResolveFormatString(bin.Y)
-	if !leftOK && !rightOK {
-		return "", false
-	}
 	if !leftOK {
-		left = formatOpaquePlaceholder
+		return "", false
 	}
+	right, rightOK := ResolveFormatString(bin.Y)
 	if !rightOK {
-		right = formatOpaquePlaceholder
+		return "", false
 	}
 	return left + right, true
 }
 
-// formatOpaquePlaceholder stands in for a non-literal (opaque) operand of a
-// concatenated format-string expression in ResolveFormatString. It is a
-// single character that is neither '%' nor a recognized format verb letter,
-// so it can never combine with an adjacent literal segment to fabricate a
-// spurious %-verb sequence across the boundary of a dropped operand.
-const formatOpaquePlaceholder = "\x00"
-
 // 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

@@ -1109,19 +1109,16 @@ func TestResolveFormatString(t *testing.T) {
 			wantOK: true,
 		},
 		{
-			name:   "concatenation with a leading non-literal identifier",
+			name:   "concatenation with a leading non-literal identifier returns ok=false",
 			expr:   concat(ast.NewIdent("message"), strLit(`"\n\nOriginal error: %v"`)),
-			want:   "\x00\n\nOriginal error: %v",
-			wantOK: true,
+			want:   "",
+			wantOK: false,
 		},
 		{
-			// A literal ending in '%' followed by an opaque operand followed
-			// by a literal starting with 'v' must not merge into a
-			// fabricated "%v" verb across the dropped operand's boundary.
-			name:   "opaque operand between a trailing percent and a verb letter does not fabricate a verb",
+			name:   "opaque operand between literal segments returns ok=false",
 			expr:   concat(strLit(`"abc%"`), ast.NewIdent("errStr"), strLit(`"v..."`)),
-			want:   "abc%\x00v...",
-			wantOK: true,
+			want:   "",
+			wantOK: false,
 		},
 		{
 			name:   "concatenation of only non-literal identifiers",