Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cmd/linters/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -73,6 +74,7 @@ func main() {
largefunc.Analyzer,
manualmutexunlock.Analyzer,
osexitinlibrary.Analyzer,
osgetenvlibrary.Analyzer,
ossetenvlibrary.Analyzer,
panicinlibrarycode.Analyzer,
rawloginlib.Analyzer,
Expand Down
Original file line number Diff line number Diff line change
@@ -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.*
91 changes: 91 additions & 0 deletions pkg/linters/osgetenvlibrary/osgetenvlibrary.go
Original file line number Diff line number Diff line change
@@ -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/") {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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
}

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")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

case "LookupEnv":
pass.ReportRangef(call, "os.LookupEnv couples the library to the process environment; pass configuration explicitly instead")
Comment on lines +61 to +65

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
})

return nil, nil
}

func calledOSFunc(pass *analysis.Pass, call *ast.CallExpr) (*types.Func, bool) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

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
}
15 changes: 15 additions & 0 deletions pkg/linters/osgetenvlibrary/osgetenvlibrary_test.go
Original file line number Diff line number Diff line change
@@ -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")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

}
Comment on lines +13 to +15
9 changes: 9 additions & 0 deletions pkg/linters/osgetenvlibrary/testdata/src/mainpkg/main.go
Original file line number Diff line number Diff line change
@@ -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")
}
Original file line number Diff line number Diff line change
@@ -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"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

}

// 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
}
Loading