[linter-miner] linter: add errorfwrapv — flag fmt.Errorf using %v to wrap errors instead of %w#39263
Conversation
…of %w The errorfwrapv linter detects fmt.Errorf calls that format an error argument using %v instead of %w. Using %v breaks error-chain inspection: errors.Is and errors.As cannot unwrap the error, which causes subtle bugs when callers check for specific error types or sentinel values. The analyzer: - Finds all fmt.Errorf(format, args...) calls - Parses the format string to map positional verb indices to verb chars - For each %v verb, checks if the corresponding argument implements error - Reports a diagnostic if so, skipping test files and nolint directives Evidence from GitHub Discussions (#39228, #39232) and a real occurrence in pkg/workflow/compiler_orchestrator_frontmatter.go confirms this pattern appears in the codebase. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Adds a new custom Go analysis linter (errorfwrapv) to detect fmt.Errorf calls that format an error argument with %v (breaking error unwrapping) instead of %w.
Changes:
- Introduces the
pkg/linters/errorfwrapvanalyzer that inspectsfmt.Errorfcalls with string-literal formats and reports%vapplied toerror-typed arguments. - Adds analysistest-based coverage and fixtures for expected diagnostics.
- Registers the new analyzer in
cmd/linters/main.goso it runs with the linter driver.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/errorfwrapv/errorfwrapv.go | Implements the analyzer and a custom format-string verb parser to map verbs to argument positions. |
| pkg/linters/errorfwrapv/errorfwrapv_test.go | Adds analysistest harness for the new analyzer. |
| pkg/linters/errorfwrapv/testdata/src/errorfwrapv/errorfwrapv.go | Adds fixture cases covering “bad” %v on error and “good” non-error %v / %w usage. |
| cmd/linters/main.go | Registers errorfwrapv.Analyzer in the multichecker driver. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
| if s[i] == '[' { | ||
| for i < len(s) && s[i] != ']' { | ||
| i++ | ||
| } | ||
| i++ |
| multichecker.Main( | ||
| contextcancelnotdeferred.Analyzer, | ||
| ctxbackground.Analyzer, | ||
| errormessage.Analyzer, | ||
| fprintlnsprintf.Analyzer, | ||
| errstringmatch.Analyzer, | ||
| errorfwrapv.Analyzer, | ||
| execcommandwithoutcontext.Analyzer, |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (>100 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Test Classification Details
Testdata coverage (
Language SupportTests analyzed:
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 100/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). The single TestErrorfWrapV function uses analysistest.Run() with testdata covering 2 positive-detection cases and 3 true-negative (no-false-positive) cases. Build tag present, no mock libraries, no test inflation.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on two correctness bugs in parseFormatVerbs plus a few test coverage gaps.
📋 Key Themes & Highlights
Blocking issues
%[n]explicit-index not decoded (errorfwrapv.go:108): the bracket block is skipped without extractingn, soargIdxcontinues its sequential count.%[2]vwith an out-of-order error arg is silently missed (false negative).%*vdynamic width not tracked (errorfwrapv.go:127):*is recorded as the verb rather than consuming oneargIdxslot for the width, shifting the argument mapping for all subsequent verbs (false negatives in the presence of dynamic widths).
Test coverage gaps
- No fixture exercises the
//nolint:errorfwrapvsuppression path. - No fixture covers
%[n]vexplicit-index format verbs. - No fixture tests a concrete
*T(pointer receiver) error type.
Positive highlights
- ✅
types.Implements(tv.Type, errorIface)is the correct approach for static-type checking; correctly handleserrorinterface variables and pointer types. - ✅ Single-report-per-call design (
returnafter first hit) avoids noisy duplicate diagnostics. - ✅
%%escape is correctly handled (theif s[i] == '%' { continue }branch consumes it beforeargIdxcan be corrupted). - ✅ Real-world evidence in the PR description (the existing
nolintoncompiler_orchestrator_frontmatter.go:50) confirms the linter targets a genuine pattern.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 250.3 AIC · ⌖ 13.9 AIC · ⊞ 27.9K
| } | ||
| i++ | ||
| if i >= len(s) { | ||
| break |
There was a problem hiding this comment.
[/tdd] parseFormatVerbs scans past [n] but never extracts the numeric index — argIdx still increments sequentially. For fmt.Errorf("%[2]v occurred: %[1]s", name, err), the verb %[2]v is recorded at argIdx=0 → callArgIdx=1 = name (not an error), silently missing the real error arg — a false negative.
💡 Suggested fix
Extract the integer between [ and ] and use it (0-based) as the argument index for this verb, and reset the sequential counter:
if s[i] == '[' {
start := i + 1
for i < len(s) && s[i] != ']' {
i++
}
n, _ := strconv.Atoi(s[start:i]) // 1-based explicit index
i++ // skip ']'
if i >= len(s) {
break
}
// ... parse flags/width/precision, then:
verbs[n-1] = rune(s[verbPos]) // use explicit 0-based index
argIdx = n // reset sequential counter per Go fmt spec
continue
}Add a test fixture for the explicit-index case:
// BadExplicitIndexV uses a positional index with %v on an error arg.
func BadExplicitIndexV(err error) error {
return fmt.Errorf("%[1]v: failed", err) // want `fmt\.Errorf formats an error argument with %v`
}| goto width | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
[/tdd] %*v (width taken from an argument, e.g. fmt.Errorf("%-*v: %v", 10, name, err)) is not handled. The parser exits the width loop on * and records * as the verb, consuming argIdx=0. The actual v verb is then skipped. For the second %v, the code maps argIdx=1 → call.Args[2] = name — missing err at call.Args[3] — false negative.
💡 Suggested fix
Handle * in the width (and precision) slot: treat it as consuming one argument index before reading the verb.
// width:
if i < len(s) && s[i] == '*' {
argIdx++ // '*' consumes one call.Args slot
i++
} else {
for i < len(s) && s[i] >= '0' && s[i] <= '9' {
i++
}
}
// precision:
if i < len(s) && s[i] == '.' {
i++
if i < len(s) && s[i] == '*' {
argIdx++
i++
} else {
for i < len(s) && s[i] >= '0' && s[i] <= '9' {
i++
}
}
}Add a test fixture:
// BadDynamicWidthWrap uses dynamic width and %v for the error.
func BadDynamicWidthWrap(err error) error {
return fmt.Errorf("%-*v", 10, err) // want `fmt\.Errorf formats an error argument with %v`
}| // GoodMixedVerbs uses %w for the error and %v for a non-error. | ||
| func GoodMixedVerbs(name string, err error) error { | ||
| return fmt.Errorf("operation %v failed: %w", name, err) | ||
| } |
There was a problem hiding this comment.
[/tdd] The nolint suppression path (analyzer lines 79–81) has no fixture. If nolint.HasDirective were accidentally removed or inverted, no test would catch it.
💡 Suggested fixture to add
// SuppressedByNolint should emit no diagnostic due to the directive.
func SuppressedByNolint(err error) error {
return fmt.Errorf("operation failed: %v", err) (nolint/redacted):errorfwrapv
}|
|
||
| // BadVWrap uses %v to format an error — should be %w. | ||
| func BadVWrap(err error) error { | ||
| return fmt.Errorf("operation failed: %v", err) // want `fmt\.Errorf formats an error argument with %v` |
There was a problem hiding this comment.
[/tdd] Good that BadVWrapExtra shows %v alongside a non-error arg — that's a useful positive case. Consider also adding a fixture for a concrete error-implementing type passed directly (not via the error interface variable) to confirm types.Implements handles pointer receivers correctly.
💡 Suggested fixture
type myError struct{ msg string }
func (e *myError) Error() string { return e.msg }
// BadConcretePointerWrap passes *myError directly with %v.
func BadConcretePointerWrap(err *myError) error {
return fmt.Errorf("wrapped: %v", err) // want `fmt\.Errorf formats an error argument with %v`
}
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated in |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in |
Summary
Adds a new
errorfwrapvcustomgo/analysislinter that detectsfmt.Errorfcalls where a%vverb is applied to anerror-interface argument. These calls silently swallow the error chain;%wshould be used instead to allow callers to inspect wrapped errors viaerrors.Is/errors.As. The linter ships with suppression support (//nolint:errorfwrapv), test-file skipping, and is documented by a new ADR.Changes
New files
pkg/linters/errorfwrapv/errorfwrapv.gogo/analysisanalyzer. Usestypes.Implementsto identifyerror-interface arguments paired with%vformat verbs insidefmt.Errorfcalls. Supports//nolint:errorfwrapvline suppression and skips test files.pkg/linters/errorfwrapv/(test fixtures)%vwith error args) and good-case functions (%w, non-error args, suppressed violations) that drive the analyzer's tests.docs/adr/39263-custom-linter-for-errorf-percent-v-error-wrapping.mderrorfwrapvas a custom linter.Modified files
cmd/linters/main.goerrorfwrapvinto the multichecker analyzer slice.pkg/linters/spec_test.goerrorfwrapv,httpnoctx,timeafterleak, andtimesleepnocontextto the documented-analyzer inventory; updates expected count from 25 → 29.pkg/workflow/compiler_orchestrator_frontmatter.go//nolint:errorlintdirective to also carry//nolint:errorfwrapv, suppressing the intentionalfmt.Errorf %vcall that deliberately avoids exposingos.PathErrordetails to callers.Implementation notes
types.Implementsrather than string matching, ensuring it correctly identifies values that satisfy theerrorinterface regardless of their concrete type.%vvs%wdistinction — onlyfmt.Errorfcalls are checked (notfmt.Sprintfetc.); the diagnostic fires specifically when the verb is%vand the corresponding argument implementserror.//nolint:errorfwrapvon the same line silences the diagnostic for deliberate, justified non-wrapping calls (e.g. where exposing the wrapped type would leak internal details)._test.gofiles to avoid noise from test helpers that intentionally use%vfor message formatting.Commit history
e746b8ab8linter: add errorfwrapv — flag fmt.Errorf %v wrapping errors instead of %w3b21d268cdocs(adr): add draft ADR-39263 for errorfwrapv linterce189fdaefix errorfwrapv parsing and docsbdda0945ffix: suppress intentional errorfwrapv violation7db30f89afix: avoid unchecked error interface assertion1eafd1af7refactor: avoid errorfwrapv variable shadowing551cfee5efix: return static error for nil error interface3b971b9efchore: clarify errorfwrapv init errorChecklist
cmd/linters/main.go)pkg/linters/spec_test.go, count 25 → 29)//nolintsuppression%vcall incompiler_orchestrator_frontmatter.gosuppressed with rationaledocs/adr/39263-...)drafttoaccepted(post-review)