Fix cacheRecoveryError's %v-not-%w bug and extend errorfwrapv/fmterrorfnoverbs to concatenated format strings#58715
Conversation
…ncatenation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Opaque format segments can contain directives, causing both linters to emit incorrect diagnostics and potentially behavior-changing recommendations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes #58712 by preserving cache-recovery error chains and extending format-string linting to concatenated expressions.
Changes:
- Replaces
%vwith%wincacheRecoveryError. - Adds shared concatenated format-string resolution.
- Extends both linters and their regression coverage.
File summaries
| File | Description |
|---|---|
pkg/cli/audit_run_pipeline.go |
Wraps the recovery error. |
pkg/cli/audit_run_pipeline_test.go |
Verifies error-chain preservation. |
pkg/linters/internal/astutil/astutil.go |
Adds format-string resolution. |
pkg/linters/internal/astutil/astutil_test.go |
Tests literal and opaque segments. |
pkg/linters/errorfwrapv/errorfwrapv.go |
Analyzes concatenated formats. |
pkg/linters/errorfwrapv/testdata/src/errorfwrapv/errorfwrapv.go |
Adds concatenation fixtures. |
pkg/linters/fmterrorfnoverbs/fmterrorfnoverbs.go |
Checks concatenated formats for verbs. |
pkg/linters/fmterrorfnoverbs/testdata/src/fmterrorfnoverbs/fmterrorfnoverbs.go |
Adds concatenation fixtures. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| formatStr, ok := astutil.ResolveFormatString(call.Args[0]) | ||
| if !ok { | ||
| return | ||
| } | ||
|
|
||
| verbs := parseFormatVerbs(lit.Value) | ||
| verbs := parseFormatVerbs(formatStr) |
| 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) { |
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Reviewed PR #58715 and submitted review comments; no additional GitHub write was needed beyond the review actions. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
✅ Ponytail Reviewer completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The format-string resolver is still making claims it cannot prove once a non-literal operand is involved, so these linters will produce false positives on valid fmt.Errorf calls.
The new `ResolveFormatString` helper flattens opaque operands into placeholders, but both analyzers still treat the remaining verbs as if argument indexing stayed intact. That breaks on concatenations where the unknown prefix already contains verbs or width/precision operands, and `fmterrorfnoverbs` now also reports "no verbs" for calls whose runtime prefix may contain them. Until the analysis tracks opaque segments explicitly, the new support is not reliable enough to merge.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 20.6 AIC · ⌖ 7.49 AIC · ⊞ 23.5K
Comment /review to run again
| } | ||
|
|
||
| if !hasRealFormatVerb(val) { | ||
| if !hasRealFormatVerb(formatStr) { |
There was a problem hiding this comment.
This change makes fmterrorfnoverbs report errors.New for concatenated format strings even when the dynamic prefix may contain real verbs, so it will now suggest a broken rewrite for valid fmt.Errorf(prefix+" suffix") calls.
💡 A non-literal prefix means "no verbs in the literal tail" is not the same as "no verbs at runtime."
ResolveFormatString only proves facts about the literal pieces. If prefix contains %d, the existing call still has formatting semantics and errors.New is not equivalent. The current generic message avoids a bad auto-fix string, but the diagnostic itself is still unsound. Limit this rule to fully literal format strings, or teach the helper to surface an "unknown verbs present" state that suppresses the report.
| } | ||
|
|
||
| verbs := parseFormatVerbs(lit.Value) | ||
| verbs := parseFormatVerbs(formatStr) |
There was a problem hiding this comment.
This linter still misclassifies error arguments after an opaque prefix because ResolveFormatString preserves later verbs but throws away how many runtime verbs the prefix may have consumed. fmt.Errorf(prefix+" suffix: %v", err) is valid when prefix already contains %w, yet this code will still flag the trailing %v path incorrectly.
💡 The analyzer can only reason about argument positions when every earlier verb is known.
Once a non-literal segment appears before a literal verb, nextArgIdx is no longer trustworthy: the runtime prefix may consume zero, one, or several arguments, including an existing %w. That means classifyErrorArgs can attach the later %v to the wrong argument and emit a bogus diagnostic. A safer fix is to stop analyzing any call whose unresolved segment appears before the verb you want to judge, or carry structured segments plus an "unknown arg consumption" state instead of flattening to a plain string.
🏗️ ADR Required — draft added for PR #58715I added a draft ADR at Evidence reviewed
Draft ADR scopeThe draft captures the architectural decision to support concatenated Next actionPlease review and refine the ADR rationale/trade-offs in Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd to the %v→%w fix and the new astutil.ResolveFormatString helper.
📋 Key Themes & Highlights
Key Themes
- Root cause well-addressed:
cacheRecoveryErrornow wraps with%w, verified viaerrors.Isin the updated test. ResolveFormatStringis a solid, well-documented shared abstraction, with the opaque-placeholder design specifically preventing verb fabrication across concatenation boundaries — good defensive test coverage inastutil_test.go.- Two existing Copilot review threads (argument-index misattribution when an opaque prefix's runtime value contains a
%-verb) remain unresolved in this PR; I did not duplicate them but flagged one related gap: no explicit regression test documents this known limitation for future contributors. - Minor: the
fmterrorfnoverbsconcatenated-format fallback message isn't asserted precisely by its testdatawantregex, so wording drift wouldn't be caught by CI.
Positive Highlights
- ✅
cacheRecoveryErrortest now asserts botherrors.Isanderrors.Unwrap != nil— solid regression coverage for the actual bug. - ✅ New testdata added per-linter mirrors the real-world
cacheRecoveryErrorshape (prefix + literal...), directly closing the gap that let the original bug through undetected. - ✅
formatOpaquePlaceholderdesign is a thoughtful, well-commented safeguard against a subtle false-negative/false-positive class of bugs.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 38.5 AIC · ⌖ 14.8 AIC · ⊞ 10.3K
Comment /matt to run again
| @@ -1078,6 +1078,79 @@ func TestStringLitValue(t *testing.T) { | |||
| } | |||
There was a problem hiding this comment.
[/tdd] Good coverage of the opaque-placeholder verb-fabrication boundary, but there's no regression test for the argument-index shift when an opaque operand's runtime value itself contains a %-verb (e.g. prefix := "%d: "). ResolveFormatString correctly can't resolve that content, but downstream consumers (parseFormatVerbs in errorfwrapv) still count implicit arg indices as if the opaque segment consumed zero verbs, which can silently misattribute later verbs to the wrong call.Args index.
💡 Suggested regression test
A case like the reviewer's own example is worth adding as an explicit testdata scenario in errorfwrapv/testdata (or a targeted parseFormatVerbs/classifyErrorArgs unit test) that documents the current known-limitation, so a future contributor doesn't need to rediscover it from the existing PR review comment thread.
@copilot please address this.
| 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 { |
There was a problem hiding this comment.
[/diagnosing-bugs] This branch's report message differs (errors.New(%q) for plain literals vs. a bare generic errors.New for concatenated ones) but neither test asserts the exact suggested message text for the concatenated case — concatNoVerbs's want regex only anchors on the prefix, so a future edit to the fallback wording wouldn't be caught by CI.
💡 Suggestion
Consider tightening the want regex in concatNoVerbs (fmterrorfnoverbs testdata) to match the full generic message, so the two code paths (plain-literal vs concatenated) stay independently verified as the message wording evolves.
@copilot please address this.
There was a problem hiding this comment.
I found one over-engineering issue in the new format-string handling: the shared recursive resolver and opaque-placeholder machinery are broader than the two linters need, and a smaller local helper would be easier to follow. net: -60 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #58715 · codex · mai10 · 5.11 AIC · ⌖ 10.6 AIC · ⊞ 14K
Comment /ponytail to run again
| return s, true | ||
| } | ||
|
|
||
| // ResolveFormatString resolves a fmt.Errorf-style format-string argument |
There was a problem hiding this comment.
pkg/linters/internal/astutil/astutil.go:L709: yagni: recursive ResolveFormatString helper with opaque-placeholder logic and supporting docs. Inline a tiny local helper in each linter and keep the AST handling scoped to the specific cases they need.
@copilot Please take the next forward-progress pass on this PR.
|
…format string analysis Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed review feedback on current HEAD:
|
cacheRecoveryErrorformats itserrargument with%vinstead of%w, breakingerrors.Is/errors.Aschain inspection for callers of the permission/cache-recovery path. Theerrorfwrapvandfmterrorfnoverbslinters couldn't catch this because they only extract format strings from a plain*ast.BasicLit, bailing out on themessage+"..."concatenationcacheRecoveryErroractually uses.Production fix
pkg/cli/audit_run_pipeline.go:cacheRecoveryErrornow wrapserrwith%w.Shared format-string resolution
astutil.ResolveFormatString(expr ast.Expr)to resolve a format-string argument built from a chain of+-concatenated string literals and non-literal operands (e.g.message + "\n\n" + "..." + "%v").%can't merge across a dropped operand into a fabricated verb (e.g."abc%" + errStr + "v..."won't be mistaken for"abc%v...").ok=falsewhen no literal segment is found at all, so bare non-literal format strings remain correctly ignored.Linter updates
errorfwrapvandfmterrorfnoverbsboth use the new helper instead of duplicating*ast.BasicLit-only extraction, so they now analyze concatenated format strings.fmterrorfnoverbsonly proposes a concreteerrors.New("...")replacement when the format string is a single literal; for concatenated expressions it falls back to a generic suggestion instead of synthesizing a misleading literal.Tests
message + "literal"-style format string (trailing%verror arg / no-verb string), plus a regression case for the verb-fabrication boundary.TestCacheRecoveryErrornow assertserrors.Issucceeds through the returned error.