From a5902858a73da7eacfe6247e62b586d392250b7c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 5 Sep 2026 04:25:29 +0000 Subject: [PATCH 1/4] Initial plan From 8ff6b3d3fe752fba5c5a85ac07e9f2f765085d51 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 5 Sep 2026 04:47:36 +0000 Subject: [PATCH 2/4] Fix cacheRecoveryError %v->%w and harden format-string linters for concatenation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/audit_run_pipeline.go | 2 +- pkg/cli/audit_run_pipeline_test.go | 5 +- pkg/linters/errorfwrapv/errorfwrapv.go | 13 ++-- .../testdata/src/errorfwrapv/errorfwrapv.go | 17 +++++ .../fmterrorfnoverbs/fmterrorfnoverbs.go | 23 +++--- .../src/fmterrorfnoverbs/fmterrorfnoverbs.go | 12 +++ pkg/linters/internal/astutil/astutil.go | 60 +++++++++++++++ pkg/linters/internal/astutil/astutil_test.go | 73 +++++++++++++++++++ 8 files changed, 185 insertions(+), 20 deletions(-) diff --git a/pkg/cli/audit_run_pipeline.go b/pkg/cli/audit_run_pipeline.go index 678aed62108..ee471d5ed7d 100644 --- a/pkg/cli/audit_run_pipeline.go +++ b/pkg/cli/audit_run_pipeline.go @@ -321,7 +321,7 @@ func cacheRecoveryError(message string, runID int64, runOutputDir string, err er " - 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", runID, runOutputDir, err) } func prepareRunForAnalysis(run WorkflowRun, cfg auditRunConfig, useLocalCache bool) WorkflowRun { diff --git a/pkg/cli/audit_run_pipeline_test.go b/pkg/cli/audit_run_pipeline_test.go index 6a1b5afdf91..2664181b298 100644 --- a/pkg/cli/audit_run_pipeline_test.go +++ b/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") + 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") } func TestPrepareRunForAnalysis(t *testing.T) { diff --git a/pkg/linters/errorfwrapv/errorfwrapv.go b/pkg/linters/errorfwrapv/errorfwrapv.go index eb7affe3c5f..9e880c83092 100644 --- a/pkg/linters/errorfwrapv/errorfwrapv.go +++ b/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++ { diff --git a/pkg/linters/errorfwrapv/testdata/src/errorfwrapv/errorfwrapv.go b/pkg/linters/errorfwrapv/testdata/src/errorfwrapv/errorfwrapv.go index be27979dfa5..f3c7c5bbb70 100644 --- a/pkg/linters/errorfwrapv/testdata/src/errorfwrapv/errorfwrapv.go +++ b/pkg/linters/errorfwrapv/testdata/src/errorfwrapv/errorfwrapv.go @@ -94,6 +94,23 @@ 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"+ + "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 { + return fmt.Errorf(prefix+"\n\n"+ + "context: %d\n"+ + "Original error: %w", 42, err) +} + // SuppressedByNolint is intentionally suppressed. func SuppressedByNolint(err error) error { return fmt.Errorf("operation failed: %v", err) //nolint:errorfwrapv diff --git a/pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs.go b/pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs.go index 0aee6acfde8..1aaee0544b8 100644 --- a/pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs.go +++ b/pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs.go @@ -5,7 +5,6 @@ package fmterrorfnoverbs import ( "go/ast" - "go/token" "golang.org/x/tools/go/analysis" @@ -42,18 +41,12 @@ 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 @@ -61,7 +54,15 @@ func run(pass *analysis.Pass) (any, error) { 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") } }) } diff --git a/pkg/linters/fmterrorfnoverbs/testdata/src/fmterrorfnoverbs/fmterrorfnoverbs.go b/pkg/linters/fmterrorfnoverbs/testdata/src/fmterrorfnoverbs/fmterrorfnoverbs.go index e85ef4abc3e..44a6b6e2cd5 100644 --- a/pkg/linters/fmterrorfnoverbs/testdata/src/fmterrorfnoverbs/fmterrorfnoverbs.go +++ b/pkg/linters/fmterrorfnoverbs/testdata/src/fmterrorfnoverbs/fmterrorfnoverbs.go @@ -63,3 +63,15 @@ 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 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` +} + +// 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) +} diff --git a/pkg/linters/internal/astutil/astutil.go b/pkg/linters/internal/astutil/astutil.go index 2051e25335d..b1f9fa1914d 100644 --- a/pkg/linters/internal/astutil/astutil.go +++ b/pkg/linters/internal/astutil/astutil.go @@ -706,6 +706,66 @@ 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 (e.g. +// `message + "...\n" + "..."`), which is common when a format string is built +// from a caller-supplied prefix followed by literal boilerplate. +// +// 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. +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) + right, rightOK := ResolveFormatString(bin.Y) + if !leftOK && !rightOK { + return "", false + } + if !leftOK { + left = formatOpaquePlaceholder + } + if !rightOK { + right = formatOpaquePlaceholder + } + 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 diff --git a/pkg/linters/internal/astutil/astutil_test.go b/pkg/linters/internal/astutil/astutil_test.go index 7a8cc1b60b1..0bde998ad9e 100644 --- a/pkg/linters/internal/astutil/astutil_test.go +++ b/pkg/linters/internal/astutil/astutil_test.go @@ -1078,6 +1078,79 @@ 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", + expr: concat(ast.NewIdent("message"), strLit(`"\n\nOriginal error: %v"`)), + want: "\x00\n\nOriginal error: %v", + wantOK: true, + }, + { + // 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", + expr: concat(strLit(`"abc%"`), ast.NewIdent("errStr"), strLit(`"v..."`)), + want: "abc%\x00v...", + wantOK: true, + }, + { + 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() From a86269c9838e5f56ebfa9cfef43b515c484be19a Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sat, 5 Sep 2026 05:03:48 +0000 Subject: [PATCH 3/4] Add ADR for concatenated fmt.Errorf analysis --- ...15-fix-concatenated-fmt-errorf-analysis.md | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 docs/adr/58715-fix-concatenated-fmt-errorf-analysis.md diff --git a/docs/adr/58715-fix-concatenated-fmt-errorf-analysis.md b/docs/adr/58715-fix-concatenated-fmt-errorf-analysis.md new file mode 100644 index 00000000000..d2125c60e0e --- /dev/null +++ b/docs/adr/58715-fix-concatenated-fmt-errorf-analysis.md @@ -0,0 +1,50 @@ +# 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 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. + +## 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 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. + +### Neutral + +- The new analysis intentionally treats non-literal operands as opaque boundaries rather than attempting full 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.* From 94b2883ff84273105ca80d43ec9c71923150afeb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 5 Sep 2026 05:36:52 +0000 Subject: [PATCH 4/4] 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> --- ...15-fix-concatenated-fmt-errorf-analysis.md | 11 ++-- pkg/cli/audit_run_pipeline.go | 4 +- pkg/cli/audit_run_pipeline_test.go | 4 +- .../testdata/src/errorfwrapv/errorfwrapv.go | 25 ++++++---- .../src/fmterrorfnoverbs/fmterrorfnoverbs.go | 22 +++++--- pkg/linters/internal/astutil/astutil.go | 50 ++++--------------- pkg/linters/internal/astutil/astutil_test.go | 15 +++--- 7 files changed, 55 insertions(+), 76 deletions(-) diff --git a/docs/adr/58715-fix-concatenated-fmt-errorf-analysis.md b/docs/adr/58715-fix-concatenated-fmt-errorf-analysis.md index d2125c60e0e..81290494a1f 100644 --- a/docs/adr/58715-fix-concatenated-fmt-errorf-analysis.md +++ b/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. --- diff --git a/pkg/cli/audit_run_pipeline.go b/pkg/cli/audit_run_pipeline.go index ee471d5ed7d..78bc050c404 100644 --- a/pkg/cli/audit_run_pipeline.go +++ b/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 { diff --git a/pkg/cli/audit_run_pipeline_test.go b/pkg/cli/audit_run_pipeline_test.go index 2664181b298..e2712b11dcc 100644 --- a/pkg/cli/audit_run_pipeline_test.go +++ b/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) { diff --git a/pkg/linters/errorfwrapv/testdata/src/errorfwrapv/errorfwrapv.go b/pkg/linters/errorfwrapv/testdata/src/errorfwrapv/errorfwrapv.go index f3c7c5bbb70..33cc6595e69 100644 --- a/pkg/linters/errorfwrapv/testdata/src/errorfwrapv/errorfwrapv.go +++ b/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. diff --git a/pkg/linters/fmterrorfnoverbs/testdata/src/fmterrorfnoverbs/fmterrorfnoverbs.go b/pkg/linters/fmterrorfnoverbs/testdata/src/fmterrorfnoverbs/fmterrorfnoverbs.go index 44a6b6e2cd5..6366b9e69c3 100644 --- a/pkg/linters/fmterrorfnoverbs/testdata/src/fmterrorfnoverbs/fmterrorfnoverbs.go +++ b/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) } diff --git a/pkg/linters/internal/astutil/astutil.go b/pkg/linters/internal/astutil/astutil.go index b1f9fa1914d..bd7b1fe8f50 100644 --- a/pkg/linters/internal/astutil/astutil.go +++ b/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 diff --git a/pkg/linters/internal/astutil/astutil_test.go b/pkg/linters/internal/astutil/astutil_test.go index 0bde998ad9e..dd4e4738e68 100644 --- a/pkg/linters/internal/astutil/astutil_test.go +++ b/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",