Add globwalkignorederror linter: flag discarded errors from filepath.Glob/os.ReadDir - #51185
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a custom analyzer detecting discarded errors from filepath.Glob and os.ReadDir.
Changes:
- Implements and registers
globwalkignorederror. - Adds analyzer fixtures and tests.
- Updates linter documentation and registry specifications.
Show a summary per file
| File | Description |
|---|---|
pkg/linters/globwalkignorederror/globwalkignorederror.go |
Implements the analyzer. |
pkg/linters/globwalkignorederror/globwalkignorederror_test.go |
Runs analyzer tests. |
pkg/linters/globwalkignorederror/testdata/src/globwalkignorederror/globwalkignorederror.go |
Covers detected, valid, and suppressed cases. |
pkg/linters/globwalkignorederror/testdata/src/globwalkignorederror/generated.go |
Verifies generated-file exclusion. |
pkg/linters/registry.go |
Registers the analyzer. |
pkg/linters/spec_test.go |
Adds specification coverage. |
pkg/linters/doc.go |
Documents the analyzer. |
pkg/linters/README.md |
Adds public documentation entries. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Files not reviewed (1)
- pkg/linters/globwalkignorederror/testdata/src/globwalkignorederror/generated.go: Generated file
- Files reviewed: 7/8 changed files
- Comments generated: 1
- Review effort level: Balanced
| if nolint.HasDirectiveForLinter(position, nolintIndex, "globwalkignorederror") { | ||
| return | ||
| } | ||
| pass.ReportRangef(call, "error return from %s.%s is discarded; malformed patterns or unreadable directories silently produce an empty result", pkgName.Imported().Name(), sel.Sel.Name) |
|
|
✅ 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 🏗️ failed during design decision gate check. 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.
|
|
Test Quality Sentinel Report 🧪PR: #51185 — Add globwalkignorederror linter Test Coverage Summary
Analyzed TestsTest Details (1 test)
Design Invariants Verified ✓
Spec Compliance
Quality Signals ✓
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 100/100. 100% design tests (threshold: 30% implementation). This PR demonstrates excellent test quality with a single behavioral test that comprehensively validates the new globwalkignorederror linter. The test uses the standard golang.org/x/tools/go/analysis framework and covers positive, negative, suppression, and generated-file edge cases with minimal test inflation (0.19:1 ratio).
There was a problem hiding this comment.
Review: globwalkignorederror linter
The linter structure, AST traversal, nolint support, and test coverage are solid. One blocking correctness issue with the diagnostic message (see inline comment).
Non-blocking note: spec_test.go bumps the documented analyzer count comment from 62 → 64, but only one analyzer is added here. The pre-existing count of 62 appears stale — worth correcting in the same commit.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 34.9 AIC · ⊞ 5.5K
| if nolint.HasDirectiveForLinter(position, nolintIndex, "globwalkignorederror") { | ||
| return | ||
| } | ||
| pass.ReportRangef(call, "error return from %s.%s is discarded; malformed patterns or unreadable directories silently produce an empty result", pkgName.Imported().Name(), sel.Sel.Name) |
There was a problem hiding this comment.
The diagnostic message conflates the two different failure modes of filepath.Glob and os.ReadDir:
filepath.GlobreturnsErrBadPatternfor malformed glob patterns; filesystem read errors are silently suppressed by design.os.ReadDirreturns filesystem errors but has no pattern concept.
The current message "malformed patterns or unreadable directories silently produce an empty result" is inaccurate for both. Consider per-function messages:
var msgs = map[string]map[string]string{
"path/filepath": {"Glob": "error return from filepath.Glob is discarded; ErrBadPattern is silently ignored"},
"os": {"ReadDir": "error return from os.ReadDir is discarded; filesystem errors are silently ignored"},
}@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — requesting changes on two test coverage gaps and one inaccurate diagnostic message.
📋 Key Themes & Highlights
Issues
- Inaccurate diagnostic message (line 85): the message mentions "malformed patterns" for both APIs, but
os.ReadDirhas no concept of patterns — onlyfilepath.Globdoes. An existing Copilot comment already flagged this. - spec_test comment count (spec_test.go:93): the comment jumped from
62to64, implying two analyzers were added. It should read63. - Test coverage gaps: the generated-file fixture covers only
filepath.Glob;os.ReadDiris untested there. Thesuppressed()fixture omits the preceding-line(nolint/redacted)case foros.ReadDir.
Positive Highlights
- ✅ Clean, idiomatic
go/analysisimplementation following the establishedstrconvparseignorederrorconventions - ✅
nolintsuppression and generated-file skip both wired correctly - ✅ Registry, doc.go, README, and spec_test all updated consistently
- ✅ Motivated by concrete real-world occurrences in the codebase
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 40.6 AIC · ⊞ 7.1K
Comment /matt to run again
|
|
||
| // documentedAnalyzers returns the analyzer subpackages documented in the README | ||
| // "Public API > Subpackages" table. The README documents 62 analyzers | ||
| // "Public API > Subpackages" table. The README documents 64 analyzers |
There was a problem hiding this comment.
[/tdd] Off-by-one in the count comment: it jumped from 62 to 64 (adds 2), but this PR adds exactly one analyzer.
💡 Fix
The comment on line 93 should read:
// "Public API > Subpackages" table. The README documents 63 analyzers
The doc.go bump (63 → 64) is correct. The spec_test comment that previously said 62 should become 63 after this PR.
@copilot please address this.
| if nolint.HasDirectiveForLinter(position, nolintIndex, "globwalkignorederror") { | ||
| return | ||
| } | ||
| pass.ReportRangef(call, "error return from %s.%s is discarded; malformed patterns or unreadable directories silently produce an empty result", pkgName.Imported().Name(), sel.Sel.Name) |
There was a problem hiding this comment.
[/grill-with-docs] The diagnostic message conflates the two APIs — os.ReadDir does not involve patterns at all, so "malformed patterns" does not apply to it.
💡 Suggested fix
An existing review comment already flags this (id 3738627680). The message should distinguish the two cases, e.g.:
// filepath.Glob path
pass.ReportRangef(call, "error return from filepath.Glob is discarded; a malformed pattern silently produces an empty result")
// os.ReadDir path
pass.ReportRangef(call, "error return from os.ReadDir is discarded; an unreadable directory silently produces an empty result")Or keep one message but drop the inaccurate half: "error return from %s.%s is discarded and will silently produce an empty result".
@copilot please address this.
| _ = files | ||
| entries, _ := os.ReadDir(".") //nolint:globwalkignorederror | ||
| _ = entries | ||
| } |
There was a problem hiding this comment.
[/tdd] Missing test case: os.ReadDir with a preceding-line (nolint/redacted) comment is not covered.
💡 Suggested addition
The suppressed() function covers filepath.Glob with a preceding-line nolint and os.ReadDir with a trailing-line nolint, but not os.ReadDir with a preceding-line nolint:
func suppressed() {
(nolint/redacted):globwalkignorederror
files, _ := filepath.Glob("*.go")
_ = files
entries, _ := os.ReadDir(".") (nolint/redacted):globwalkignorederror
_ = entries
// Missing: preceding-line nolint for os.ReadDir
(nolint/redacted):globwalkignorederror
entries2, _ := os.ReadDir(".")
_ = entries2
}This confirms the nolint index handles both placement styles for both APIs.
@copilot please address this.
| func generatedBad() { | ||
| files, _ := filepath.Glob("*.go") | ||
| _ = files | ||
| } |
There was a problem hiding this comment.
[/tdd] The generated-file fixture only tests filepath.Glob — add an os.ReadDir call to confirm both functions are suppressed in generated files.
💡 Suggested addition
func generatedBad() {
files, _ := filepath.Glob("*.go")
_ = files
entries, _ := os.ReadDir(".")
_ = entries
}Without this, a regression where only one function is skipped in generated files would go undetected.
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
filepath.Globandos.ReadDirerrors are sometimes discarded via the blank identifier, silently masking malformed patterns or unreadable directories and letting callers proceed as if an empty result were success (observed inpkg/cli/enable.go,pkg/cli/compile_pipeline.go,pkg/cli/audit_job.go).New linter
pkg/linters/globwalkignorederror/globwalkignorederror.go—go/analysispass flaggingx, _ := filepath.Glob(...)andx, _ := os.ReadDir(...)assignments, following thestrconvparseignorederrorconventions (nolint suppression + generated-file skip support).globwalkignorederror_test.go+testdata/—analysistestfixtures covering bad, good (error checked), suppressed (//nolint), and generated-file cases.Wiring & docs
pkg/linters/registry.go— registersglobwalkignorederror.AnalyzerinAll().pkg/linters/doc.go,pkg/linters/README.md,pkg/linters/spec_test.go— updated analyzer count (63 → 64) and documentation entries.