[linter-miner] linter: add osgetenvlibrary — flag os.Getenv/LookupEnv in library packages#42115
Conversation
…ckages Introduces the `osgetenvlibrary` linter — the natural complement to the existing `ossetenvlibrary` linter. While `ossetenvlibrary` flags environment *writes* (os.Setenv/Unsetenv), this new linter flags environment *reads* (os.Getenv, os.LookupEnv) in non-main, non-test packages. Library packages that read environment variables directly couple themselves to the process environment, making them harder to test in isolation and surprising to callers that do not expect hidden configuration sources. Configuration should instead be passed explicitly through parameters. Evidence from static scan of pkg/: - pkg/logger/logger.go:31 - pkg/console/accessibility.go:16 - pkg/parser/github.go:84 - pkg/workflow/action_mode.go:40 - pkg/workflow/features.go:59 - pkg/constants/constants.go:382 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
automation
cookie
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel analysis for PR #42115 already completed in a prior session of this same workflow run (28331792274). add_comment and submit_pull_request_review were both successfully called and their limits (1 of 1 each) are exhausted. Score: 100/100 Excellent, verdict: APPROVE. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Adds a new go/analysis analyzer (osgetenvlibrary) to the gh aw linter suite to discourage library packages from directly reading process environment variables via os.Getenv / os.LookupEnv, complementing the existing ossetenvlibrary rule.
Changes:
- Introduces
pkg/linters/osgetenvlibraryanalyzer to reportos.Getenv/os.LookupEnvusage in non-main, non-test packages (withnolintsupport). - Adds
analysistest-based tests and initial test fixtures for library vsmainpackage behavior. - Registers the new analyzer in
cmd/linters/main.goso it runs with the linter driver.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/osgetenvlibrary/osgetenvlibrary.go | New analyzer implementation to detect os.Getenv / os.LookupEnv calls in library packages (type-aware matching + nolint support). |
| pkg/linters/osgetenvlibrary/osgetenvlibrary_test.go | Adds analysistest coverage for the new analyzer. |
| pkg/linters/osgetenvlibrary/testdata/src/osgetenvlibrary/osgetenvlibrary.go | Adds positive/negative fixture cases for library packages (including shadowing + nolint). |
| pkg/linters/osgetenvlibrary/testdata/src/mainpkg/main.go | Adds fixture verifying main packages are exempt. |
| cmd/linters/main.go | Registers osgetenvlibrary.Analyzer with the multichecker driver. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Low
| func TestAnalyzer(t *testing.T) { | ||
| analysistest.Run(t, analysistest.TestData(), osgetenvlibrary.Analyzer, "osgetenvlibrary", "mainpkg") | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 52.2 AIC · ⌖ 9.08 AIC · ⊞ 4.9K
| ) | ||
|
|
||
| func TestAnalyzer(t *testing.T) { | ||
| analysistest.Run(t, analysistest.TestData(), osgetenvlibrary.Analyzer, "osgetenvlibrary", "mainpkg") |
There was a problem hiding this comment.
The ossetenvlibrary test includes "fixtures/cmd/tool" to verify the /cmd/ path exclusion, but this linter is missing an equivalent fixture. Without it, the strings.Contains(pkgPath, "/cmd/") guard in run() is an untested code path.
Consider adding a testdata/src/fixtures/cmd/tool/main.go fixture and expanding the analysistest.Run call:
analysistest.Run(t, analysistest.TestData(), osgetenvlibrary.Analyzer, "osgetenvlibrary", "mainpkg", "fixtures/cmd/tool")@copilot please address this.
| switch fn.Name() { | ||
| case "Getenv": | ||
| pass.ReportRangef(call, "os.Getenv couples the library to the process environment; pass configuration explicitly instead") | ||
| case "LookupEnv": | ||
| pass.ReportRangef(call, "os.LookupEnv couples the library to the process environment; pass configuration explicitly instead") |
There was a problem hiding this comment.
Both Getenv and LookupEnv cases produce structurally identical messages differing only in the function name. The switch can be collapsed into a single ReportRangef call, removing the duplication:
pass.ReportRangef(call, "os.%s couples the library to the process environment; pass configuration explicitly instead", fn.Name())This also means that if a new function name is ever added to calledOSFunc, the report will still fire rather than falling through silently.
@copilot please address this.
Captures the architectural decision to add a static analysis linter that flags os.Getenv/LookupEnv calls in non-main, non-test Go library packages, complementing the existing ossetenvlibrary rule. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (148 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 MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Library boundary decisions like this one — choosing to prohibit 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)
Go: 1 ( Test inflation: 15 test lines vs 91 production lines (ratio ≈ 0.16). No inflation. Edge cases covered via testdata (six scenarios exercised by
Note on assertions: Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /grill-with-docs, and /improve-codebase-architecture — requesting changes on test coverage gaps before merge.
📋 Key Themes & Highlights
Key Themes
- Test coverage parity gap: The sibling
ossetenvlibraryshipsalias.go,dotimport.go, and afixtures/cmd/toolfixture; this linter is missing all three. The implementation should handle these cases correctly (type-aware*types.Funcmatching is import-alias-safe), but without fixtures there is no regression protection. - Untested exemption branch: The
/cmd/path guard at line 29 is never exercised by the currentanalysistestrun. - DRY:
calledOSFuncis a near-copy of the identically-named helper inossetenvlibrary; a shared internal helper would avoid future drift. - Minor: The two
switchcases produce identical messages except for the function name — collapsible to a singleReportRangefwithfn.Name().
Positive Highlights
- ✅ Correct use of type-aware
*types.Funcmatching — immune to import aliases by design - ✅
nolint:osgetenvlibraryescape hatch tested in fixture - ✅ Local-variable shadow case (
os := fakeOS{}) correctly handled and tested - ✅ Clean registration in
cmd/linters/main.go, alphabetically ordered - ✅ Diagnostic messages are actionable and consistent with the existing linter vocabulary
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 59.2 AIC · ⌖ 9.74 AIC · ⊞ 6.6K
| ) | ||
|
|
||
| func TestAnalyzer(t *testing.T) { | ||
| analysistest.Run(t, analysistest.TestData(), osgetenvlibrary.Analyzer, "osgetenvlibrary", "mainpkg") |
There was a problem hiding this comment.
[/tdd] The sibling ossetenvlibrary has alias.go and dotimport.go fixtures that confirm the linter catches calls through aliased (import o "os") and dot (import . "os") imports — this linter is missing both. Since calledOSFunc uses type-aware *types.Func matching these should work, but without fixtures there is no regression net.
💡 Suggested additions
Add testdata/src/osgetenvlibrary/alias.go:
package osgetenvlibrary
import o "os"
func BadAliasGetenv() string {
return o.Getenv("KEY") // want "os.Getenv couples the library"
}
func BadAliasLookupEnv() (string, bool) {
return o.LookupEnv("KEY") // want "os.LookupEnv couples the library"
}Add testdata/src/osgetenvlibrary/dotimport.go:
package osgetenvlibrary
import . "os"
func BadDotGetenv() string {
return Getenv("KEY") // want "os.Getenv couples the library"
}Mirrors ossetenvlibrary/testdata/src/ossetenvlibrary/alias.go and dotimport.go.
@copilot please address this.
|
|
||
| func run(pass *analysis.Pass) (any, error) { | ||
| pkgPath := pass.Pkg.Path() | ||
| if pass.Pkg.Name() == "main" || strings.HasSuffix(pkgPath, "/main") || strings.Contains(pkgPath, "/cmd/") { |
There was a problem hiding this comment.
[/tdd] The /cmd/ path exemption (strings.Contains(pkgPath, "/cmd/")) has no test fixture to exercise it. The sibling linter covers this with a fixtures/cmd/tool package — without a counterpart here, a future refactor could silently break this exemption.
💡 Suggested fix
Add a fixture at testdata/src/fixtures/cmd/tool/main.go:
package main
import "os"
func main() {
_ = os.Getenv("KEY")
_, _ = os.LookupEnv("KEY")
}And wire it into the test:
analysistest.Run(t, analysistest.TestData(), osgetenvlibrary.Analyzer,
"osgetenvlibrary", "mainpkg", "fixtures/cmd/tool")@copilot please address this.
| return nil, nil | ||
| } | ||
|
|
||
| func calledOSFunc(pass *analysis.Pass, call *ast.CallExpr) (*types.Func, bool) { |
There was a problem hiding this comment.
[/improve-codebase-architecture] calledOSFunc is nearly identical to the same-named helper in ossetenvlibrary — the only difference is the two function names it accepts. This duplication means bug-fixes or improvements (e.g., nil-safety on obj) have to be applied in both places.
💡 Refactoring idea
Consider extracting to a shared internal helper, e.g. pkg/linters/internal/osfunccheck:
// CalledOSFunc returns the *types.Func if the call targets one of the named
// os-package functions, or (nil, false) otherwise.
func CalledOSFunc(pass *analysis.Pass, call *ast.CallExpr, names ...string) (*types.Func, bool)Both osgetenvlibrary and ossetenvlibrary could then delegate to this, and the nil-guard on obj becomes a single, tested code path.
@copilot please address this.
| } | ||
| switch fn.Name() { | ||
| case "Getenv": | ||
| pass.ReportRangef(call, "os.Getenv couples the library to the process environment; pass configuration explicitly instead") |
There was a problem hiding this comment.
[/grill-with-docs] The two switch cases produce messages that are identical except for the function name — extracting the name via fn.Name() removes the duplication and ensures message consistency if the wording ever changes.
💡 Simplified form
// Before (two near-identical strings)
case "Getenv":
pass.ReportRangef(call, "os.Getenv couples the library...")
case "LookupEnv":
pass.ReportRangef(call, "os.LookupEnv couples the library...")
// After
pass.ReportRangef(call,
"os.%s couples the library to the process environment; pass configuration explicitly instead",
fn.Name())The switch can then be dropped entirely — calledOSFunc already guarantees the name is one of the two valid values.
@copilot please address this.
|
|
||
| // BadGetenv calls os.Getenv and should be flagged. | ||
| func BadGetenv() string { | ||
| return os.Getenv("CONFIG_KEY") // want "os.Getenv couples the library to the process environment" |
There was a problem hiding this comment.
[/tdd] os.ExpandEnv and os.Environ also read from the process environment but are not flagged. The fixture currently only confirms the two explicit API entries. If the scope is intentionally limited to Getenv/LookupEnv, it would be useful to add a // not flagged comment in the fixture documenting that decision explicitly so future contributors know it was intentional.
💡 Suggested documentation comment
// os.ExpandEnv and os.Environ are intentionally out of scope for this rule;
// only direct Getenv/LookupEnv calls are targeted.
func OkExpandEnv() string {
return os.ExpandEnv("${KEY}") // not flagged by this rule
}@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
Summary
Add a new
osgetenvlibraryGo static analysis linter that flagsos.Getenvandos.LookupEnvcalls in non-main, non-test library packages. This extends the existing environment-boundary enforcement story alongsideossetenvlibrary(which covers writes) to also cover reads.Motivation
Library packages that call
os.Getenv/os.LookupEnvdirectly couple themselves to the process environment. This hides configuration dependencies from function signatures, makes packages harder to test (tests must manipulate environment variables as side effects), and reduces caller visibility. A code scan ofpkg/identified six existing call sites in non-main library packages. This linter enforces the boundary at CI/IDE time rather than relying on convention.Changes
New:
pkg/linters/osgetenvlibrary/osgetenvlibrary.go— Go analysis pass using type-aware*types.Funcmatching to identifyos.Getenvandos.LookupEnvcalls.main, paths containing/cmd/, paths ending/main,.testpackages, and test files.//nolint:osgetenvlibraryinline suppression for exceptional cases.ossetenvlibrary:astutil.Inspector,filecheck.IsTestFile,nolint.BuildLineIndex,nolint.HasDirective.os.Getenv couples the library to the process environment; pass configuration explicitly insteados.LookupEnv couples the library to the process environment; pass configuration explicitly insteadosgetenvlibrary_test.go— Unit test usinganalysistest.Runcovering a library package (positive/negative/suppressed cases) and a main package (exemption).testdata/src/osgetenvlibrary/osgetenvlibrary.go— Test fixture: flagsos.Getenv,os.LookupEnv; does not flagos.Setenv, a method on a local struct namedos, or a call with//nolint:osgetenvlibrary.testdata/src/mainpkg/main.go— Test fixture:os.Getenv/os.LookupEnvin amainpackage are not flagged.Modified:
cmd/linters/main.goosgetenvlibrarypackage and registeredosgetenvlibrary.Analyzerin the linter binary, ordered betweenosexitinlibraryandossetenvlibrary.New:
docs/adr/42115-add-osgetenvlibrary-linter-flag-env-reads-in-libraries.mdossetenvlibrary), and consequences (positive: testability, explicit config APIs, complete boundary enforcement; negative: six existing call sites require refactoring).Scope and Exemptions
mainor path containing/cmd/_test.goor.testpackage)//nolint:osgetenvlibraryFollow-up
Six existing
os.Getenv/os.LookupEnvcall sites inpkg/library packages must be refactored to pass configuration through explicit parameters, constructor arguments, or config structs.