Fix errorfwrapv false positive on explicit arg index before dynamic * width - #51932
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
* width
There was a problem hiding this comment.
Pull request overview
Fixes explicit argument-index tracking in errorfwrapv and adjusts validation handling for analyzer fixtures.
Changes:
- Defers format value-index attribution until after width parsing.
- Adds an indexed dynamic-width
%wregression fixture. - Excludes
testdatapackages from changed-package validation.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/errorfwrapv/errorfwrapv.go |
Updates format argument parsing. |
pkg/linters/errorfwrapv/testdata/src/errorfwrapv/errorfwrapv.go |
Adds the regression fixture. |
scripts/agent-report-progress.sh |
Excludes fixtures from package linting. |
Makefile |
Excludes fixtures from impacted-test discovery. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Balanced
| valueArgIdx := 0 | ||
| hasExplicitValueArg := false |
| func GoodIndexedDynamicWidthWrap(width int, err error) error { | ||
| return fmt.Errorf("%[1]*w", width, err) | ||
| } |
| exit 1; \ | ||
| fi; \ | ||
| CHANGED_GO_FILES=$$({ git diff --name-only --diff-filter=ACDMR "$$BASE_COMMIT"; git ls-files --others --exclude-standard; } | sort -u | grep -E '\.go$$' || true); \ | ||
| CHANGED_GO_FILES=$$({ git diff --name-only --diff-filter=ACDMR "$$BASE_COMMIT"; git ls-files --others --exclude-standard; } | sort -u | grep -E '\.go$$' | grep -v -E '(^|/)testdata/' || true); \ |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. No over-engineering found: diff removes speculative code (net -6 lines) and adds minimal, non-abstracted testdata filtering. No unnecessary complexity to cut.
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
✅ PR Code Quality Reviewer completed the code quality review. 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. 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
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #51932 does not have the 'implementation' label and has only 6 new lines of code in business logic directories (threshold: 100).
|
There was a problem hiding this comment.
Skills-Based Review
Applied /diagnosing-bugs and /tdd — requesting changes on a regression risk and missing test coverage.
Key Themes
Issues
- Regression risk (
/diagnosing-bugs): Removing the pre-flag index parse breaks%[n]flag+verband%[n]widthverbpatterns (e.g.%[2]+v,%[2]10v). The subsequent index parse only fires when*is present, so explicit indexes before flags or fixed widths are silently dropped — a correctness regression. - Incomplete test coverage (
/tdd): The PR only adds aGoodcase for%[1]*w. The PR description claims the same change also fixes a false negative for%von the real error argument, but no test exercises that path. - testdata filter is too broad (Makefile/script): Excluding all
testdata/files from impacted-test detection means a fixture-only edit reports "No changed Go files" and skipstest-impacted-goentirely, even though analyzer tests depend on those fixtures.
Positive Highlights
- Root cause is correctly identified.
- PR description is clear and includes a minimal reproducer.
- Script change correctly guards
package_dir.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 18.6 AIC · ⌖ 6.53 AIC · ⊞ 7.1K
Comment /matt to run again
There was a problem hiding this comment.
The fix is correct. Moving parseFormatArgIndex to after consumeFormatWidthOrPrecision properly handles the %[1]*w case (explicit index before dynamic * width) without regressing other formats.
For %[2]v-style formats (index before verb with no width), the flags loop only consumes -+#0 characters and never consumes [, so the post-width parseFormatArgIndex call at line 212 correctly picks up the explicit index. The previous inline comment about %[2]+v regression was a false positive.
The Makefile and agent-report-progress.sh changes correctly exclude testdata/ from impacted-test and lint runs, avoiding treating analyzer fixtures as real packages.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 27.3 AIC · ⌖ 6.25 AIC · ⊞ 5.4K
|
This is a sharp fix to the ✅ What looks great:
This PR is ready for review and merge. Focused, well-tested, and addresses a real correctness bug.
|
|
@-
|
|
🎉 This pull request is included in a new release. Release: |
parseFormatVerbsconsumed an explicit[n]index immediately after%and marked it as the verb's value argument, before knowing whether that index actually feeds a following*width/precision. For%[1]*w, the index is re-consumed as the width source butvalueArgIdxstays pointed at the width argument, so the real error argument ends up with no verb recorded and gets flagged as missing%w— on code that already wraps correctly.Changes
pkg/linters/errorfwrapv/errorfwrapv.go: removed the speculative index parse before flag/width handling. The existing check afterconsumeFormatWidthOrPrecisionalready covers the "index directly before verb" case (it leavesiunchanged when there is no*/width), so an index is now only attributed to the value argument once it's known not to feed a*.pkg/linters/errorfwrapv/testdata/.../errorfwrapv.go: addedGoodIndexedDynamicWidthWrapexercisingfmt.Errorf("%[1]*w", width, err)with no expected diagnostic. ExistingGoodIndexedWidthWrap(%[2]*[1]w) andBadIndexedWidthNoW(%[2]*[1]s) are unchanged.scripts/agent-report-progress.sh,Makefile(test-impacted-go): excludetestdata/Go files from changed-file lint/impacted-test package selection. Analyzer fixtures intentionally containerrorlint/govetviolations and are not part of./cmd/... ./pkg/..., so editing them previously broke the local pre-PR gate with unrelated pre-existing findings.The same misattribution also caused a false negative for
%von the true error argument; that is fixed by the same change.Branch refresh requested by pr-sous-chef. Run: https://github.com/github/gh-aw/actions/runs/31461992299> Generated by 👨🍳 PR Sous Chef · gpt54 · 12.4 AIC · ⌖ 5.05 AIC · ⊞ 8.5K · ◷