diff --git a/cmd/linters/main.go b/cmd/linters/main.go index 05edcf6f0d8..ca2d88d226c 100644 --- a/cmd/linters/main.go +++ b/cmd/linters/main.go @@ -36,6 +36,7 @@ import ( "github.com/github/gh-aw/pkg/linters/lenstringzero" "github.com/github/gh-aw/pkg/linters/manualmutexunlock" "github.com/github/gh-aw/pkg/linters/osexitinlibrary" + "github.com/github/gh-aw/pkg/linters/osgetenvlibrary" "github.com/github/gh-aw/pkg/linters/ossetenvlibrary" panicinlibrarycode "github.com/github/gh-aw/pkg/linters/panic-in-library-code" "github.com/github/gh-aw/pkg/linters/rawloginlib" @@ -73,6 +74,7 @@ func main() { largefunc.Analyzer, manualmutexunlock.Analyzer, osexitinlibrary.Analyzer, + osgetenvlibrary.Analyzer, ossetenvlibrary.Analyzer, panicinlibrarycode.Analyzer, rawloginlib.Analyzer, diff --git a/docs/adr/42115-add-osgetenvlibrary-linter-flag-env-reads-in-libraries.md b/docs/adr/42115-add-osgetenvlibrary-linter-flag-env-reads-in-libraries.md new file mode 100644 index 00000000000..3f0b1825137 --- /dev/null +++ b/docs/adr/42115-add-osgetenvlibrary-linter-flag-env-reads-in-libraries.md @@ -0,0 +1,44 @@ +# ADR-42115: Add osgetenvlibrary Linter to Flag os.Getenv/LookupEnv in Library Packages + +**Date**: 2026-06-28 +**Status**: Draft +**Deciders**: Unknown (automated PR by linter-miner) + +--- + +### Context + +Library packages (non-main, non-test Go packages) that call `os.Getenv` or `os.LookupEnv` directly couple themselves to the process environment, making configuration invisible to callers, hiding dependency paths from function signatures, and requiring tests to manipulate environment variables as side effects. A code scan of `pkg/` found six existing call sites in non-main library packages. The repository already enforces a complementary rule (`ossetenvlibrary`) that prevents library code from *writing* to the process environment; this PR extends that boundary to cover *reading* from it. + +### Decision + +We will add a new `osgetenvlibrary` static analysis linter that flags any call to `os.Getenv` or `os.LookupEnv` in non-main, non-test Go packages. The analyzer uses type-aware `*types.Func` matching (the same approach as `ossetenvlibrary`) and supports `//nolint:osgetenvlibrary` escape hatches for exceptional cases. Library authors must pass configuration through explicit parameters, constructor arguments, or config structs instead of reading from the process environment. + +### Alternatives Considered + +#### Alternative 1: Documentation Only — Accept Environment Coupling and Publish a Convention + +Acknowledge `os.Getenv` usage in library code as acceptable and document the pattern as a team convention rather than enforcing it via a linter. This preserves short-term development velocity and requires no refactoring of existing call sites. It was not chosen because undocumented conventions drift over time, the six existing violations demonstrate the pattern is already spreading, and documentation alone provides no enforcement or discoverability in the IDE/CI loop. + +#### Alternative 2: Extend the Existing ossetenvlibrary Linter Rather Than Creating a New Package + +Add `Getenv`/`LookupEnv` detection directly into the existing `ossetenvlibrary` analyzer to keep the two concerns in one place. This was not chosen because combining read and write checks in a single analyzer conflates two distinct concerns (environment pollution vs. hidden reads), makes the linter name misleading, and complicates targeted suppression—callers who need to suppress a write check but not a read check (or vice versa) would have no granular escape hatch. + +### Consequences + +#### Positive +- Library packages become independently testable: callers can supply configuration through explicit parameters without setting environment variables. +- Configuration dependencies are made visible in function and constructor signatures, improving API discoverability and reducing hidden coupling. +- Completes the environment-boundary enforcement story alongside `ossetenvlibrary`, covering both reads and writes. + +#### Negative +- Six existing `pkg/` call sites must be refactored to thread configuration explicitly through call stacks, representing near-term churn. +- Library authors who currently rely on `os.Getenv` for optional defaults must update their APIs, which may involve adding new parameters or config structs to public interfaces. + +#### Neutral +- The `//nolint:osgetenvlibrary` escape hatch is available for cases where environment reads are genuinely appropriate (e.g., environment-inspection utilities), but each suppression requires explicit opt-out rather than opt-in. +- Main packages (`cmd/` paths and packages named `main`) and test files are exempt from the rule, consistent with the `ossetenvlibrary` scoping. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/linters/osgetenvlibrary/osgetenvlibrary.go b/pkg/linters/osgetenvlibrary/osgetenvlibrary.go new file mode 100644 index 00000000000..ff3a3a4a9a8 --- /dev/null +++ b/pkg/linters/osgetenvlibrary/osgetenvlibrary.go @@ -0,0 +1,91 @@ +// Package osgetenvlibrary implements a Go analysis linter that flags +// os.Getenv and os.LookupEnv calls in non-main, non-test packages. +package osgetenvlibrary + +import ( + "go/ast" + "go/types" + "strings" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + + "github.com/github/gh-aw/pkg/linters/internal/astutil" + "github.com/github/gh-aw/pkg/linters/internal/filecheck" + "github.com/github/gh-aw/pkg/linters/internal/nolint" +) + +// Analyzer is the os-getenv-in-library analysis pass. +var Analyzer = &analysis.Analyzer{ + Name: "osgetenvlibrary", + Doc: "reports calls to os.Getenv or os.LookupEnv in non-main, non-test packages", + URL: "https://github.com/github/gh-aw/tree/main/pkg/linters/osgetenvlibrary", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (any, error) { + pkgPath := pass.Pkg.Path() + if pass.Pkg.Name() == "main" || strings.HasSuffix(pkgPath, "/main") || strings.Contains(pkgPath, "/cmd/") { + return nil, nil + } + + insp, err := astutil.Inspector(pass) + if err != nil { + return nil, err + } + noLintLinesByFile := nolint.BuildLineIndex(pass, "osgetenvlibrary") + + nodeFilter := []ast.Node{ + (*ast.CallExpr)(nil), + } + + insp.Preorder(nodeFilter, func(n ast.Node) { + call, ok := n.(*ast.CallExpr) + if !ok { + return + } + + if strings.HasSuffix(pkgPath, ".test") || filecheck.IsTestFile(pass.Fset.PositionFor(call.Pos(), false).Filename) { + return + } + + fn, ok := calledOSFunc(pass, call) + if !ok { + return + } + position := pass.Fset.PositionFor(call.Pos(), false) + if nolint.HasDirective(position, noLintLinesByFile) { + return + } + 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") + } + }) + + return nil, nil +} + +func calledOSFunc(pass *analysis.Pass, call *ast.CallExpr) (*types.Func, bool) { + var obj types.Object + switch fun := call.Fun.(type) { + case *ast.SelectorExpr: + obj = pass.TypesInfo.Uses[fun.Sel] + case *ast.Ident: + obj = pass.TypesInfo.Uses[fun] + default: + return nil, false + } + + fn, ok := obj.(*types.Func) + if !ok || fn.Pkg() == nil || fn.Pkg().Path() != "os" { + return nil, false + } + if fn.Name() != "Getenv" && fn.Name() != "LookupEnv" { + return nil, false + } + return fn, true +} diff --git a/pkg/linters/osgetenvlibrary/osgetenvlibrary_test.go b/pkg/linters/osgetenvlibrary/osgetenvlibrary_test.go new file mode 100644 index 00000000000..69f897b6442 --- /dev/null +++ b/pkg/linters/osgetenvlibrary/osgetenvlibrary_test.go @@ -0,0 +1,15 @@ +//go:build !integration + +package osgetenvlibrary_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + + "github.com/github/gh-aw/pkg/linters/osgetenvlibrary" +) + +func TestAnalyzer(t *testing.T) { + analysistest.Run(t, analysistest.TestData(), osgetenvlibrary.Analyzer, "osgetenvlibrary", "mainpkg") +} diff --git a/pkg/linters/osgetenvlibrary/testdata/src/mainpkg/main.go b/pkg/linters/osgetenvlibrary/testdata/src/mainpkg/main.go new file mode 100644 index 00000000000..98808551970 --- /dev/null +++ b/pkg/linters/osgetenvlibrary/testdata/src/mainpkg/main.go @@ -0,0 +1,9 @@ +package main + +import "os" + +func main() { + // os.Getenv and os.LookupEnv are allowed in main packages. + _ = os.Getenv("KEY") + _, _ = os.LookupEnv("KEY") +} diff --git a/pkg/linters/osgetenvlibrary/testdata/src/osgetenvlibrary/osgetenvlibrary.go b/pkg/linters/osgetenvlibrary/testdata/src/osgetenvlibrary/osgetenvlibrary.go new file mode 100644 index 00000000000..df2d8d26077 --- /dev/null +++ b/pkg/linters/osgetenvlibrary/testdata/src/osgetenvlibrary/osgetenvlibrary.go @@ -0,0 +1,33 @@ +package osgetenvlibrary + +import "os" + +// 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" +} + +// BadLookupEnv calls os.LookupEnv and should be flagged. +func BadLookupEnv() (string, bool) { + return os.LookupEnv("CONFIG_KEY") // want "os.LookupEnv couples the library to the process environment" +} + +// OkSetenv calls os.Setenv (not our concern here) and should NOT be flagged. +func OkSetenv() error { + return os.Setenv("KEY", "val") +} + +type fakeOS struct{} + +func (fakeOS) Getenv(_ string) string { return "" } + +// LocalVarNamedOS should not be flagged just because the variable is named os. +func LocalVarNamedOS() string { + os := fakeOS{} + return os.Getenv("KEY") +} + +// SuppressedGetenv uses a nolint directive and should not be flagged. +func SuppressedGetenv() string { + return os.Getenv("CONFIG_KEY") //nolint:osgetenvlibrary +}