Add walkfuncerrshadow analyzer for filepath.Walk err shadowing - #49633
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Adds the walkfuncerrshadow Go analyzer to detect confusing filepath.Walk and WalkDir error-parameter shadowing.
Changes:
- Implements, registers, tests, and documents the analyzer.
- Adds
nolintsuppression support, with a multiline-call gap. - Includes unrelated generated workflow lock changes.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/walkfuncerrshadow/walkfuncerrshadow.go |
Implements analyzer logic. |
pkg/linters/walkfuncerrshadow/walkfuncerrshadow_test.go |
Runs analyzer fixtures. |
pkg/linters/walkfuncerrshadow/testdata/src/walkfuncerrshadow/walkfuncerrshadow.go |
Covers positive and negative cases. |
pkg/linters/spec_test.go |
Adds analyzer to documentation checks. |
pkg/linters/registry.go |
Registers the analyzer. |
pkg/linters/README.md |
Documents the analyzer package. |
pkg/linters/doc.go |
Updates package documentation. |
.github/workflows/smoke-checkout-pr-dispatch.lock.yml |
Contains unrelated timeout/model drift. |
.github/workflows/daily-arxiv-researcher.lock.yml |
Contains unrelated model-routing drift. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Balanced
| # --allow-tool shell(yq) | ||
| # --allow-tool write | ||
| timeout-minutes: 10 | ||
| timeout-minutes: 20 |
| pos := pass.Fset.PositionFor(callbackErr.Pos(), false) | ||
| if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { | ||
| return | ||
| } | ||
| if nolint.HasDirectiveForLinter(pos, noLintIndex, analyzerName) { | ||
| return | ||
| } | ||
|
|
||
| pkgLog.Printf("flagging filepath.%s callback err shadow at %s", sel.Sel.Name, pos) |
| -- /bin/bash -c 'set +o histexpand; : "${RUNNER_TOOL_CACHE:?RUNNER_TOOL_CACHE must be set}"; GH_AW_TOOL_CACHE="$RUNNER_TOOL_CACHE"; export PATH="$(find "$GH_AW_TOOL_CACHE" -maxdepth 5 -type d -name bin 2>/dev/null | tr '\''\n'\'' '\'':'\'')$PATH"; [ -n "$GOROOT" ] && export PATH="$GOROOT/bin:$PATH" || true; [ -n "$ERLANG_HOME" ] && export PATH="$ERLANG_HOME/bin:$PATH" || true && GH_AW_NODE_EXEC="${GH_AW_NODE_BIN:-}"; if [ -z "$GH_AW_NODE_EXEC" ] || [ ! -x "$GH_AW_NODE_EXEC" ]; then GH_AW_NODE_EXEC="$(command -v node 2>/dev/null || true)"; fi; if [ -z "$GH_AW_NODE_EXEC" ]; then echo "node runtime missing on this runner — check runtimes.node in workflow YAML" >&2; exit 127; fi; GH_AW_NPM_GLOBAL_ROOT="$(npm root -g 2>/dev/null || true)"; if [ -n "$GH_AW_NPM_GLOBAL_ROOT" ]; then export NODE_PATH="${GH_AW_NPM_GLOBAL_ROOT}${NODE_PATH:+:${NODE_PATH}}"; fi; "$GH_AW_NODE_EXEC" ${RUNNER_TEMP}/gh-aw/actions/claude_harness.cjs claude --print --no-chrome --allowed-tools '\''Bash,BashOutput,Edit(/tmp/*),ExitPlanMode,Glob,Grep,KillBash,LS,MultiEdit(/tmp/*),NotebookRead,Read,Read(/tmp/*),Task,TodoWrite,Write(/tmp/*)'\'' --debug-file /tmp/gh-aw/threat-detection/detection.log --verbose --permission-mode acceptEdits --output-format stream-json --prompt-file /tmp/gh-aw/aw-prompts/prompt.txt' 2>&1 | tee -a /tmp/gh-aw/threat-detection/detection.log | ||
| env: | ||
| ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} | ||
| ANTHROPIC_MODEL: detection |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
Quality Highlights✅ Excellent behavioral test design:
✅ Spec compliance: Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on correctness gaps and test coverage asymmetry.
📋 Key Themes & Highlights
Key Themes
- Silent false-negatives: The single-LHS guard (
len(assign.Lhs) != 1) misses valid multi-return patterns likeerr, _ = filepath.Walk(...) - nolint placement: The directive check runs at the callback-parameter position, not the assignment line where users would naturally place
//nolint - Test coverage gaps:
BadWalkDirusesvar err; err =but there is noerr :=fixture for WalkDir; multi-return shape is not tested at all
Positive Highlights
- ✅ Clean analyzer structure following existing
go/analysispatterns in the repo - ✅ Good use of
nolint.ImplementsErrorto verify the third param is actuallyerror - ✅ Well-structured test fixtures covering the non-flagged cases
- ✅ Registry and doc wiring is complete and consistent
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 43.6 AIC · ⌖ 11.2 AIC · ⊞ 7.1K
Comment /matt to run again
| return | ||
| } | ||
| if sel.Sel.Name != "Walk" && sel.Sel.Name != "WalkDir" { | ||
| return |
There was a problem hiding this comment.
[/codebase-design] The guard len(assign.Lhs) != 1 silently skips multi-return assignments like err, _ = filepath.Walk(...). This is a valid Go pattern and the analyzer will miss it.
💡 Suggested fix
Search all LHS identifiers for one named err rather than requiring exactly 1:
var outerErr *ast.Ident
for _, lhs := range assign.Lhs {
if id, ok := lhs.(*ast.Ident); ok && id.Name == "err" {
outerErr = id
break
}
}
if outerErr == nil {
return
}Add a corresponding test fixture covering err, _ = filepath.Walk(...).
@copilot please address this.
| callbackErr := callbackErrParam(pass, callback) | ||
| if callbackErr == nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
[/tdd] The nolint suppression check is applied at the position of the callback parameter, not at the assignment statement. A developer writing //nolint:walkfuncerrshadow on the outer err := line won't be suppressed here.
💡 Details
The nolint directive is conventionally placed on the line that triggers the diagnostic — which a user would naturally place on the err := filepath.Walk(...) line. Consider checking pos against both the callback parameter position and the assignment's position, or document clearly which line the directive must appear on.
Add a test fixture with the directive on the assignment line (not the callback) to confirm it either works or fails intentionally.
@copilot please address this.
| sel.Sel.Name, | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
[/tdd] callbackErrParam hardcodes len(params) != 3 to identify the Walk signature, but this is a structural assumption that isn't verified against the actual type. A function literal with three parameters where the third is an error but is not a filepath.WalkFunc could also match.
The type check in the outer checkAssign (via IsPkgSelector) guards against this, but a comment here explaining why the 3-param check is safe would prevent future maintenance confusion.
@copilot please address this.
| } | ||
|
|
||
| func BadWalkDir(root string) error { | ||
| var err error |
There was a problem hiding this comment.
[/tdd] The BadWalkDir fixture uses var err error; err = ... (an assign statement =) rather than the short-variable-declaration err := ... used in BadWalk. This is a good additional shape to cover, but there's no fixture testing err := filepath.WalkDir(...) with :=. Add one to make coverage symmetric.
@copilot please address this.
Documents the architectural decision to add a focused go/analysis analyzer for filepath.Walk/WalkDir err parameter shadowing.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (218 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
|
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · auto · 53.3 AIC · ⌖ 9.18 AIC · ⊞ 7.8K
Comment /review to run again
Comments that could not be inline-anchored
pkg/linters/walkfuncerrshadow/walkfuncerrshadow.go:356
nolint suppression is checked against the callback err parameter's line, not the outer assignment's line — a //nolint:walkfuncerrshadow placed above a multi-line filepath.Walk(...) call won't suppress the diagnostic if the func-literal signature wraps onto its own line.
<details>
<summary>💡 Details</summary>
pos is derived from callbackErr.Pos() (the third param identifier), and nolint.HasDirectiveForLinter only checks that line and the line immediately above it (`position.Line-1…
There was a problem hiding this comment.
Review: walkfuncerrshadow analyzer
The implementation is well-structured, follows existing linter conventions, and covers the core detection logic correctly. One blocking correctness issue with the nolint suppression anchor.
Blocking
Nolint position anchor uses callbackErr.Pos() instead of assign.Pos() — see inline comment on walkfuncerrshadow.go:96. When the callback spans multiple lines, the //nolint:walkfuncerrshadow directive placed above filepath.Walk(...) falls outside the ±1 line window relative to the err param identifier. Suppression silently breaks. Fix: use assign.Pos() as the nolint check anchor (consistent with httprespbodyclose, contextcancelnotdeferred), and add a multi-line callback test case to lock the behaviour.
Non-blocking notes
- The
len(assign.Lhs) != 1guard is correct sincefilepath.Walkreturns a single value; no change needed. - Test coverage for all happy/sad paths is solid; just the multi-line nolint gap.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 92.2 AIC · ⌖ 10.6 AIC · ⊞ 5.4K
| if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) { | ||
| return | ||
| } | ||
| if nolint.HasDirectiveForLinter(pos, noLintIndex, analyzerName) { |
There was a problem hiding this comment.
Nolint position anchor is fragile for multi-line callbacks.
The suppression check uses callbackErr.Pos() — the err identifier inside the callback signature. For single-line callbacks (as in the testdata), this works because the callback err and the filepath.Walk(...) call share the same line, so //nolint:walkfuncerrshadow on the preceding line matches via pos.Line-1.
However, when the callback spans multiple lines (e.g. the func( literal is on a separate line from the err param), the nolint directive placed on the line before filepath.Walk(...) will no longer be one line above callbackErr.Pos(), and suppression silently stops working.
The consistent pattern in this codebase (httprespbodyclose, contextcancelnotdeferred) is to anchor the nolint check to the assign statement position — the line users actually annotate:
pos := pass.Fset.PositionFor(assign.Pos(), false)
if filecheck.ShouldSkipFilename(pos.Filename, generatedFiles) {
return
}
if nolint.HasDirectiveForLinter(pos, noLintIndex, analyzerName) {
return
}
// Keep callbackErr for the diagnostic range
pass.ReportRangef(callbackErr, ...)Also add a multi-line callback test case to testdata to lock this behaviour.
@copilot please address this.
|
|
||
| func GoodNolint(root string) error { | ||
| //nolint:walkfuncerrshadow | ||
| err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { |
There was a problem hiding this comment.
Testdata has no multi-line callback nolint suppression test.
The existing GoodNolint test case uses a single-line callback signature, meaning the nolint directive and the err param are at most 2 lines apart. This does not exercise suppression for multi-line callbacks where err appears on a line far from the directive.
Please add a test case like:
func GoodNolintMultiLine(root string) error {
(nolint/redacted):walkfuncerrshadow
err := filepath.Walk(root, func(
path string,
info os.FileInfo,
err error,
) error {
return err
})
return err
}This would expose the position-anchor bug described in the implementation comment.
@copilot please address this.
|
@copilot quick triage for this PR:
|
|
@copilot quick triage for this PR:
Run details: https://github.com/github/gh-aw/actions/runs/30726226343
|
Re-ran PR-finisher triage on current HEAD Current status:
Remaining blockers are maintainer-only:
Validation note: I re-ran local checks on current head; |
|
@copilot quick triage for this PR:
Run details: https://github.com/github/gh-aw/actions/runs/30727625239
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Re-ran PR-finisher triage on current head ( Actionable requested-review feedback is already addressed in the branch (nolint assignment anchor, multiline nolint fixture, and WalkDir Remaining blockers are maintainer-only:
Branch refresh is already applied (head includes latest Yes — the stale |
🔍 Triage Summary
Adds walkfuncerrshadow linter analyzer. CHANGES_REQUESTED twice from automated reviewer — needs rework, no CI recorded. Automated triage — see the triage report for full context.
|
|
@copilot quick triage for this PR:
|
Triaged and fixed via branch refresh on current head The failing job ( Current head includes the upstream fix (through merge commit |
Re-ran PR-finisher triage on current head Current requested-review status:
Failed check investigation (
Branch refresh is already applied on this run (head includes latest |
This adds a focused custom analyzer for a recurring
filepath.Walk/filepath.WalkDirpattern where the callback’serrparameter shadows the outererrassigned from the walk call. The result is clearer diagnostics around a confusing pattern that is easy to misuse during refactors.What the analyzer flags
errfilepath.Walk/filepath.WalkDircallbacks whose third parameter is also namederrwalkError distinct outer variable namesAnalyzer wiring
pkg/linters/walkfuncerrshadow/with a newgo/analysisanalyzerCoverage and docs
analysistestfixtures for:WalkWalkDirfilepathwalkernolintsuppressionThis pattern is now reported and the diagnostic points at the callback parameter, recommending a distinct name such as
walkErr.Run URL: https://github.com/github/gh-aw/actions/runs/30719603092
branch refresh requested from
Run details: https://github.com/github/gh-aw/actions/runs/30726226343