test: guard process/gitlab env scrubs against drift (#2693)

Co-authored-by: ayattara <alassane.yattara@savoirfairelinux.com> Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>
2026-08-19 09:02:52 -04:00
parent 33a8e71f6d
commit 898ec63857
7 changed files with 989 additions and 157 deletions
@@ -0,0 +1,59 @@
+package process
+
+import (
+	"os"
+	"testing"
+
+	"github.com/kandev/kandev/internal/testutil"
+)
+
+// ambientGitLabEnvVars lists every environment variable this package reads in
+// non-test code for GitLab host/token resolution. TestMain clears all of them
+// before any test runs; TestMainScrubsAmbientGitLabEnvironment guards that the
+// scrub actually ran.
+var ambientGitLabEnvVars = []string{
+	gitLabHostEnv,
+	legacyGitLabHostEnv,
+	gitLabTokenEnv,
+}
+
+// ambientEnvNotScrubbed lists environment variables this package legitimately
+// reads in non-test code that TestMain must NOT clear.
+var ambientEnvNotScrubbed = []string{
+	// SHELL selects the interactive shell in shell_unix.go; shell_test.go sets
+	// it deliberately to assert that selection, so clearing it here would
+	// break that test rather than protect it.
+	"SHELL",
+	// COMSPEC is the Windows analogue of SHELL, read in shell_windows.go. Both
+	// files are parsed by the guard below regardless of build tags (only one
+	// compiles per platform), so both names must stay exempt.
+	"COMSPEC",
+}
+
+// clearAmbientGitLabEnv removes the inherited GitLab host/token values so
+// tests observe an unconfigured environment. Individual tests that need one
+// of these variables still set it explicitly with t.Setenv.
+func clearAmbientGitLabEnv() {
+	for _, name := range ambientGitLabEnvVars {
+		if err := os.Unsetenv(name); err != nil {
+			panic("process tests: unset " + name + ": " + err.Error())
+		}
+	}
+}
+
+// TestAmbientEnvCoverageIncludesEveryPackageEnvRead fails when non-test code
+// grows a new environment read that is neither in ambientGitLabEnvVars nor
+// ambientEnvNotScrubbed, so the TestMain scrub cannot silently fall behind the
+// code it protects.
+//
+// environmentValue is declared as an extra reader because it, not os.Getenv,
+// is how this package reads the environment almost everywhere: six of the
+// eight GitLab host/token reads go through (*GitOperator).environmentValue,
+// the other two being the package-level os.Getenv fallbacks in
+// git_pr_providers.go. environmentValue falls back to
+// os.Environ() when the operator was built with a nil provider (git.go:119).
+// That is the exact path the reported failure came in on, so a guard watching
+// only os.Getenv would have watched the wrong two call sites.
+func TestAmbientEnvCoverageIncludesEveryPackageEnvRead(t *testing.T) {
+	testutil.AssertEnvReadsCovered(t, ambientGitLabEnvVars, ambientEnvNotScrubbed, "environmentValue")
+}
@@ -440,7 +440,7 @@ exit 8
 // loudly instead of letting the parent shell's GitLab host/token variables
 // silently re-arm GitLab host-trust test failures.
 func TestMainScrubsAmbientGitLabEnvironment(t *testing.T) {
-	for _, name := range []string{gitLabHostEnv, legacyGitLabHostEnv, gitLabTokenEnv} {
+	for _, name := range ambientGitLabEnvVars {
 		if _, ok := os.LookupEnv(name); ok {
 			t.Fatalf("%s is set during tests; TestMain must clear it", name)
 		}
@@ -44,9 +44,7 @@ func TestMain(m *testing.M) {
 	// assume these are unset. Scrub them here so the suite is hermetic regardless
 	// of the parent shell's environment; tests that need a specific value set it
 	// explicitly via t.Setenv.
-	_ = os.Unsetenv(gitLabHostEnv)
-	_ = os.Unsetenv(legacyGitLabHostEnv)
-	_ = os.Unsetenv(gitLabTokenEnv)
+	clearAmbientGitLabEnv()
 	goleak.VerifyTestMain(m)
 }

@@ -1,14 +1,10 @@
 package gitlab

 import (
-	"go/ast"
-	"go/parser"
-	"go/token"
 	"os"
-	"path/filepath"
-	"strconv"
-	"strings"
 	"testing"
+
+	"github.com/kandev/kandev/internal/testutil"
 )

 // envMockGitLab mirrors the mock switch that factory.go and service_config.go
@@ -95,128 +91,5 @@ func TestClearAmbientGitLabEnvNeutralizesInheritedHosts(t *testing.T) {
 // a new os.Getenv/os.LookupEnv call that ambientGitLabEnvVars does not cover,
 // so the scrub cannot silently fall behind the code it protects.
 func TestAmbientGitLabEnvCoversEveryPackageEnvRead(t *testing.T) {
-	fileSet := token.NewFileSet()
-	files := parseNonTestSources(t, fileSet)
-	constants := stringConstants(files)
-	covered := make(map[string]bool, len(ambientGitLabEnvVars))
-	for _, name := range ambientGitLabEnvVars {
-		covered[name] = true
-	}
-	for _, file := range files {
-		for _, call := range envReadCalls(file) {
-			name, resolved := resolveEnvName(call, constants)
-			if !resolved {
-				t.Errorf("%s: environment variable name is not a string literal or a constant of this "+
-					"package; a constant imported from elsewhere needs resolveEnvName taught about it",
-					fileSet.Position(call.Pos()))
-				continue
-			}
-			if !covered[name] {
-				t.Errorf("%s: %s is read in non-test code but missing from ambientGitLabEnvVars",
-					fileSet.Position(call.Pos()), name)
-			}
-		}
-	}
-}
-
-// parseNonTestSources parses every production file of the package, which is
-// the code whose environment reads the scrub has to keep up with.
-func parseNonTestSources(t *testing.T, fileSet *token.FileSet) []*ast.File {
-	t.Helper()
-	paths, err := filepath.Glob("*.go")
-	if err != nil {
-		t.Fatalf("list sources: %v", err)
-	}
-	var files []*ast.File
-	for _, path := range paths {
-		if strings.HasSuffix(path, "_test.go") {
-			continue
-		}
-		file, parseErr := parser.ParseFile(fileSet, path, nil, 0)
-		if parseErr != nil {
-			t.Fatalf("parse %s: %v", path, parseErr)
-		}
-		files = append(files, file)
-	}
-	if len(files) == 0 {
-		t.Fatal("no non-test sources found in the package directory")
-	}
-	return files
-}
-
-// stringConstants maps package-level string constant names to their values so
-// env reads written as os.Getenv(envGitLabHost) resolve to the variable name.
-func stringConstants(files []*ast.File) map[string]string {
-	constants := make(map[string]string)
-	for _, file := range files {
-		for _, decl := range file.Decls {
-			genDecl, ok := decl.(*ast.GenDecl)
-			if !ok || genDecl.Tok != token.CONST {
-				continue
-			}
-			for _, spec := range genDecl.Specs {
-				valueSpec, ok := spec.(*ast.ValueSpec)
-				if !ok {
-					continue
-				}
-				for i, name := range valueSpec.Names {
-					if i >= len(valueSpec.Values) {
-						continue
-					}
-					if value, ok := literalString(valueSpec.Values[i]); ok {
-						constants[name.Name] = value
-					}
-				}
-			}
-		}
-	}
-	return constants
-}
-
-// envReadCalls collects every os.Getenv / os.LookupEnv call in a file.
-func envReadCalls(file *ast.File) []*ast.CallExpr {
-	var calls []*ast.CallExpr
-	ast.Inspect(file, func(node ast.Node) bool {
-		call, ok := node.(*ast.CallExpr)
-		if !ok || len(call.Args) != 1 {
-			return true
-		}
-		selector, ok := call.Fun.(*ast.SelectorExpr)
-		if !ok {
-			return true
-		}
-		pkgIdent, ok := selector.X.(*ast.Ident)
-		if !ok || pkgIdent.Name != "os" {
-			return true
-		}
-		if selector.Sel.Name == "Getenv" || selector.Sel.Name == "LookupEnv" {
-			calls = append(calls, call)
-		}
-		return true
-	})
-	return calls
-}
-
-func resolveEnvName(call *ast.CallExpr, constants map[string]string) (string, bool) {
-	if value, ok := literalString(call.Args[0]); ok {
-		return value, true
-	}
-	ident, ok := call.Args[0].(*ast.Ident)
-	if !ok {
-		return "", false
-	}
-	value, ok := constants[ident.Name]
-	return value, ok
-}
-
-func literalString(expr ast.Expr) (string, bool) {
-	lit, ok := expr.(*ast.BasicLit)
-	if !ok || lit.Kind != token.STRING {
-		return "", false
-	}
-	value, err := strconv.Unquote(lit.Value)
-	if err != nil {
-		return "", false
-	}
-	return value, true
+	testutil.AssertEnvReadsCovered(t, ambientGitLabEnvVars, nil)
 }
@@ -240,10 +240,7 @@ func (s *Service) reconcileExistingSummary(
 	activityObserved bool,
 ) (*statussummary.TaskStatusSummary, error) {
 	for attempt := 0; attempt < maxSummaryReconcileAttempts && current != nil; attempt++ {
-		activityNeedsRepair := activityObserved &&
-			(authoritativeActivity.After(time.Time{}) &&
-				(current.LastActivityAt == nil || authoritativeActivity.After(*current.LastActivityAt)))
-		if current.PendingAction == pendingAction && !activityNeedsRepair {
+		if !summaryNeedsReconcile(current, pendingAction, authoritativeActivity, activityObserved) {
 			return current, nil
 		}
 		if err := prepareSummaryReconcileAttempt(ctx, attempt, current.Revision); err != nil {
@@ -271,37 +268,63 @@ func (s *Service) reconcileExistingSummary(
 			s.publishReconciledSummary(ctx, task, next)
 			return &next, nil
 		}
-		rows, err := s.statusSummaries.LoadTaskStatusSummaries(ctx, []string{task.ID})
+		current, pendingAction, err = s.reloadSummaryReconcileState(ctx, task.ID)
 		if err != nil {
-			return nil, fmt.Errorf("reload after compare-and-set rejection: %w", err)
+			return nil, err
 		}
-		current = rows[task.ID]
 		if current == nil {
 			return nil, nil
 		}
-		if s.sessions == nil {
-			return nil, errors.New("reload sessions: session repository unavailable")
-		}
-		refreshedSessions, err := s.sessions.ListTaskSessions(ctx, task.ID)
-		if err != nil {
-			return nil, fmt.Errorf("reload sessions: %w", err)
-		}
-		pendingBySession, err := s.GetPendingActionsForSessions(ctx, taskSessionIDs(refreshedSessions))
-		if err != nil {
-			return nil, fmt.Errorf("reload pending actions: %w", err)
-		}
-		pendingAction = pendingActionForTask(refreshedSessions, pendingBySession)
 	}
-	activityNeedsRepair := activityObserved &&
-		(authoritativeActivity.After(time.Time{}) &&
-			(current != nil && (current.LastActivityAt == nil || authoritativeActivity.After(*current.LastActivityAt))))
-	if current != nil && current.PendingAction == pendingAction && !activityNeedsRepair {
+	if !summaryNeedsReconcile(current, pendingAction, authoritativeActivity, activityObserved) {
 		return current, nil
 	}
 	s.logSummaryReconcileExhaustion(task.ID, current)
 	return nil, errors.New("exhausted compare-and-set retries")
 }

+func summaryNeedsReconcile(
+	current *statussummary.TaskStatusSummary,
+	pendingAction string,
+	authoritativeActivity time.Time,
+	activityObserved bool,
+) bool {
+	if current == nil {
+		return false
+	}
+	if current.PendingAction != pendingAction {
+		return true
+	}
+	return activityObserved && authoritativeActivity.After(time.Time{}) &&
+		(current.LastActivityAt == nil || authoritativeActivity.After(*current.LastActivityAt))
+}
+
+func (s *Service) reloadSummaryReconcileState(
+	ctx context.Context,
+	taskID string,
+) (*statussummary.TaskStatusSummary, string, error) {
+	rows, err := s.statusSummaries.LoadTaskStatusSummaries(ctx, []string{taskID})
+	if err != nil {
+		return nil, "", fmt.Errorf("reload after compare-and-set rejection: %w", err)
+	}
+	current := rows[taskID]
+	if current == nil {
+		return nil, "", nil
+	}
+	if s.sessions == nil {
+		return nil, "", errors.New("reload sessions: session repository unavailable")
+	}
+	refreshedSessions, err := s.sessions.ListTaskSessions(ctx, taskID)
+	if err != nil {
+		return nil, "", fmt.Errorf("reload sessions: %w", err)
+	}
+	pendingBySession, err := s.GetPendingActionsForSessions(ctx, taskSessionIDs(refreshedSessions))
+	if err != nil {
+		return nil, "", fmt.Errorf("reload pending actions: %w", err)
+	}
+	return current, pendingActionForTask(refreshedSessions, pendingBySession), nil
+}
+
 func maxSummaryActivity(current *time.Time, candidate time.Time) *time.Time {
 	if candidate.IsZero() {
 		if current == nil {
@@ -0,0 +1,355 @@
+package testutil
+
+import (
+	"fmt"
+	"go/ast"
+	"go/parser"
+	"go/token"
+	"path/filepath"
+	"strconv"
+	"strings"
+	"testing"
+)
+
+// AssertEnvReadsCovered parses every non-test *.go file in the caller's
+// package directory (the current working directory when the test runs) and
+// fails t for any environment read whose argument name is not present in
+// scrubbed or exempt, or cannot be resolved to a string literal or
+// same-package constant. Callers use it to guard a hermetic-environment test
+// scrub against silently falling behind new environment reads.
+//
+// os.Getenv and os.LookupEnv are always treated as environment reads. They are
+// matched on the literal identifier os, so a file that imports os under an
+// alias or through a dot import is not scanned; no file in this repository
+// does either, and a review of one that did would be the place to catch it.
+// Names are likewise resolved against package-level constants with no scope
+// analysis, so a local variable shadowing one of those constants resolves to
+// the constant's value rather than its own.
+//
+// Only reads that name a variable are classified. An argument-less bulk read
+// such as os.Environ, or an accessor wrapping one (filterGitEnv(g.environmentValues())
+// in the process package), hands out the whole ambient environment with no name
+// to check, so this guard says nothing about it; a scrub list is not the tool
+// for that channel. Reads through an API other than os.Getenv/os.LookupEnv,
+// syscall.Getenv among them, are likewise not scanned.
+//
+// A package that funnels its reads through its own accessor (for example
+// (*GitOperator).environmentValue, which falls back to os.Environ) must name
+// that accessor in extraReaders, or the guard sees only the minority of its
+// reads and the assurance is hollow. Names in extraReaders match on the
+// selector alone, so any receiver counts, as does a plain package-level
+// function of the same name. Only the first argument is read, so an accessor
+// that later grows a second parameter stays covered rather than silently
+// dropping every one of its call sites out of the scan. For the same reason an
+// extraReader matching no call at all is an error rather than a no-op: the
+// match is by name, so renaming the accessor would otherwise leave the guard
+// compiling, passing, and watching nothing.
+func AssertEnvReadsCovered(t testing.TB, scrubbed, exempt []string, extraReaders ...string) {
+	t.Helper()
+	fileSet := token.NewFileSet()
+	files := parseNonTestSources(t, fileSet)
+	for _, message := range uncoveredEnvReads(fileSet, files, scrubbed, exempt, extraReaders...) {
+		t.Error(message)
+	}
+}
+
+// envScan carries the classification inputs shared by every file in one pass.
+type envScan struct {
+	constants    packageConstants
+	covered      map[string]bool
+	extraReaders map[string]bool
+}
+
+// packageConstants holds every package-level string constant, plus the names no
+// single value can be attributed to. Build-tagged platform variants (foo_unix.go
+// and foo_windows.go) may legally declare one name twice, and every file is
+// parsed regardless of its tags, so a plain name-to-value map would silently
+// keep whichever file was parsed last and classify the other file's read against
+// the wrong variable.
+//
+// Two spellings of that hazard are tracked separately because they need
+// different advice. conflicts holds names declared with two different string
+// literals. unresolved holds names declared somewhere this scanner cannot read a
+// literal from at all — a reference to another constant, a concatenation, an
+// untyped iota, or an implicit repetition of the previous expression list. The
+// second kind matters for the same reason as the first: without it a literal
+// declaration in one build-tagged file lends its value to a same-named
+// declaration in another that this scanner cannot read, and the read resolves to
+// the wrong platform's variable just as silently.
+type packageConstants struct {
+	values     map[string]string
+	conflicts  map[string]bool
+	unresolved map[string]bool
+}
+
+func newPackageConstants() packageConstants {
+	return packageConstants{
+		values:     make(map[string]string),
+		conflicts:  make(map[string]bool),
+		unresolved: make(map[string]bool),
+	}
+}
+
+// record keeps the first value seen for name and marks the name conflicting if
+// a later file declares it with a different one. Re-declaring the same value is
+// not a conflict: both spellings resolve to the same variable.
+func (c packageConstants) record(name, value string) {
+	if previous, seen := c.values[name]; seen && previous != value {
+		c.conflicts[name] = true
+		return
+	}
+	c.values[name] = value
+}
+
+// recordUnresolvable marks a name this scanner cannot read a string literal
+// from, so no other file's declaration of the same name can stand in for it.
+func (c packageConstants) recordUnresolvable(name string) {
+	c.unresolved[name] = true
+}
+
+// resolve refuses to guess for a name that is conflicting or unreadable, so the
+// read is reported as unclassifiable rather than silently resolved to whichever
+// declaration this scanner happened to understand.
+func (c packageConstants) resolve(name string) (string, bool) {
+	if c.conflicts[name] || c.unresolved[name] {
+		return "", false
+	}
+	value, ok := c.values[name]
+	return value, ok
+}
+
+// uncoveredEnvReads returns one message per environment read in files whose
+// argument cannot be resolved to a string literal or same-package constant,
+// or whose resolved name is in neither scrubbed nor exempt. Kept independent
+// of testing.TB so it can be driven directly from inline source snippets in
+// tests.
+func uncoveredEnvReads(fileSet *token.FileSet, files []*ast.File, scrubbed, exempt []string, extraReaders ...string) []string {
+	scan := envScan{
+		constants:    stringConstants(files),
+		covered:      make(map[string]bool, len(scrubbed)+len(exempt)),
+		extraReaders: make(map[string]bool, len(extraReaders)),
+	}
+	for _, name := range scrubbed {
+		scan.covered[name] = true
+	}
+	for _, name := range exempt {
+		scan.covered[name] = true
+	}
+	for _, name := range extraReaders {
+		scan.extraReaders[name] = true
+	}
+	messages := staleExtraReaders(files, extraReaders)
+	for _, file := range files {
+		messages = append(messages, uncoveredEnvReadsInFile(fileSet, file, scan)...)
+	}
+	return messages
+}
+
+// staleExtraReaders returns one message per caller-declared extra reader that
+// no scanned call in the package targets. Extra readers are matched by name, so
+// the compiler cannot follow a rename of the accessor: the string keeps
+// compiling, nothing fails, and the guard silently stops watching every read
+// that went through it — the blind spot extraReaders exists to close, reopened
+// by an ordinary refactor. A reader that matches nothing is therefore an error.
+func staleExtraReaders(files []*ast.File, extraReaders []string) []string {
+	var messages []string
+	for _, name := range extraReaders {
+		if callsExtraReader(files, name) {
+			continue
+		}
+		messages = append(messages, fmt.Sprintf("extra reader %q passed to AssertEnvReadsCovered matches no call in "+
+			"this package; if the accessor was renamed, rename it here too, because the guard stops watching "+
+			"the reads that went through it", name))
+	}
+	return messages
+}
+
+// callsExtraReader reports whether any call the scan would look at targets
+// name, in either the receiver.method(...) or the bare function(...) spelling.
+// Argument-less calls are ignored to match envReadCalls, so "used" means
+// "actually scanned" rather than merely "mentioned".
+func callsExtraReader(files []*ast.File, name string) bool {
+	found := false
+	for _, file := range files {
+		ast.Inspect(file, func(node ast.Node) bool {
+			call, ok := node.(*ast.CallExpr)
+			if !ok || len(call.Args) == 0 {
+				return true
+			}
+			switch target := call.Fun.(type) {
+			case *ast.SelectorExpr:
+				found = found || target.Sel.Name == name
+			case *ast.Ident:
+				found = found || target.Name == name
+			}
+			return true
+		})
+		if found {
+			return true
+		}
+	}
+	return false
+}
+
+func uncoveredEnvReadsInFile(fileSet *token.FileSet, file *ast.File, scan envScan) []string {
+	var messages []string
+	for _, call := range envReadCalls(file, scan.extraReaders) {
+		name, resolved := resolveEnvName(call, scan.constants)
+		pos := fileSet.Position(call.Pos())
+		if !resolved {
+			messages = append(messages, unresolvedMessage(pos, call.Args[0], scan.constants))
+			continue
+		}
+		if !scan.covered[name] {
+			messages = append(messages, fmt.Sprintf("%s: %s is read in non-test code but missing from the "+
+				"scrubbed/exempt lists passed to AssertEnvReadsCovered", pos, name))
+		}
+	}
+	return messages
+}
+
+// parseNonTestSources parses every production file in the current directory,
+// which is the code whose environment reads a scrub has to keep up with.
+func parseNonTestSources(t testing.TB, fileSet *token.FileSet) []*ast.File {
+	t.Helper()
+	paths, err := filepath.Glob("*.go")
+	if err != nil {
+		t.Fatalf("list sources: %v", err)
+	}
+	var files []*ast.File
+	for _, path := range paths {
+		if strings.HasSuffix(path, "_test.go") {
+			continue
+		}
+		file, parseErr := parser.ParseFile(fileSet, path, nil, 0)
+		if parseErr != nil {
+			t.Fatalf("parse %s: %v", path, parseErr)
+		}
+		files = append(files, file)
+	}
+	if len(files) == 0 {
+		t.Fatal("no non-test sources found in the package directory")
+	}
+	return files
+}
+
+// unresolvedMessage explains why an environment name could not be classified.
+// A name declared with different values in more than one file gets its own
+// message: the cause is build-tagged platform variants, all of which this guard
+// parses, and the generic "pass a literal or constant" advice would not lead an
+// author there.
+func unresolvedMessage(pos token.Position, arg ast.Expr, constants packageConstants) string {
+	if ident, ok := arg.(*ast.Ident); ok && constants.conflicts[ident.Name] {
+		return fmt.Sprintf("%s: constant %s is declared with different values in more than one file of this "+
+			"package, most likely build-tagged platform variants, and every file is scanned regardless of its "+
+			"tags, so the guard cannot tell which value applies here; read the variable through a string literal "+
+			"instead", pos, ident.Name)
+	}
+	return fmt.Sprintf("%s: environment variable name is not a string literal or a resolvable same-package "+
+		"constant; pass a literal or constant so AssertEnvReadsCovered can classify it", pos)
+}
+
+// stringConstants maps package-level string constant names to their values so
+// env reads written as os.Getenv(someConst) resolve to the variable name, and
+// records the names whose value would otherwise depend on which file you
+// believe: those declared twice with different literals, and those declared
+// anywhere in a form this scanner cannot read a literal from.
+func stringConstants(files []*ast.File) packageConstants {
+	constants := newPackageConstants()
+	for _, file := range files {
+		for _, decl := range file.Decls {
+			genDecl, ok := decl.(*ast.GenDecl)
+			if !ok || genDecl.Tok != token.CONST {
+				continue
+			}
+			recordConstSpecs(constants, genDecl.Specs)
+		}
+	}
+	return constants
+}
+
+// recordConstSpecs records every string-literal constant in one const block, and
+// marks every other name it declares unresolvable. Skipping those instead would
+// let a same-named literal in a build-tagged sibling file resolve this read.
+func recordConstSpecs(constants packageConstants, specs []ast.Spec) {
+	for _, spec := range specs {
+		valueSpec, ok := spec.(*ast.ValueSpec)
+		if !ok {
+			continue
+		}
+		for i, name := range valueSpec.Names {
+			if i < len(valueSpec.Values) {
+				if value, ok := literalString(valueSpec.Values[i]); ok {
+					constants.record(name.Name, value)
+					continue
+				}
+			}
+			constants.recordUnresolvable(name.Name)
+		}
+	}
+}
+
+// envReadCalls collects every os.Getenv / os.LookupEnv call in a file, plus
+// every call to one of the caller-declared extraReaders. Any call carrying at
+// least one argument qualifies, because resolveEnvName only ever reads the
+// first: pinning this to exactly one argument meant that adding a fallback
+// parameter to an accessor dropped all of its call sites from the scan without
+// failing a single test.
+func envReadCalls(file *ast.File, extraReaders map[string]bool) []*ast.CallExpr {
+	var calls []*ast.CallExpr
+	ast.Inspect(file, func(node ast.Node) bool {
+		call, ok := node.(*ast.CallExpr)
+		if !ok || len(call.Args) == 0 {
+			return true
+		}
+		if isEnvRead(call.Fun, extraReaders) {
+			calls = append(calls, call)
+		}
+		return true
+	})
+	return calls
+}
+
+// isEnvRead reports whether fun names os.Getenv, os.LookupEnv, or one of the
+// extraReaders. An extraReader matches on the trailing name alone, so both
+// receiver.environmentValue(x) and a bare environmentValue(x) count.
+func isEnvRead(fun ast.Expr, extraReaders map[string]bool) bool {
+	switch target := fun.(type) {
+	case *ast.SelectorExpr:
+		if extraReaders[target.Sel.Name] {
+			return true
+		}
+		pkgIdent, ok := target.X.(*ast.Ident)
+		if !ok || pkgIdent.Name != "os" {
+			return false
+		}
+		return target.Sel.Name == "Getenv" || target.Sel.Name == "LookupEnv"
+	case *ast.Ident:
+		return extraReaders[target.Name]
+	default:
+		return false
+	}
+}
+
+func resolveEnvName(call *ast.CallExpr, constants packageConstants) (string, bool) {
+	if value, ok := literalString(call.Args[0]); ok {
+		return value, true
+	}
+	ident, ok := call.Args[0].(*ast.Ident)
+	if !ok {
+		return "", false
+	}
+	return constants.resolve(ident.Name)
+}
+
+func literalString(expr ast.Expr) (string, bool) {
+	lit, ok := expr.(*ast.BasicLit)
+	if !ok || lit.Kind != token.STRING {
+		return "", false
+	}
+	value, err := strconv.Unquote(lit.Value)
+	if err != nil {
+		return "", false
+	}
+	return value, true
+}
@@ -0,0 +1,524 @@
+package testutil
+
+import (
+	"fmt"
+	"go/ast"
+	"go/parser"
+	"go/token"
+	"strings"
+	"testing"
+)
+
+func parseSnippet(t *testing.T, src string) (*token.FileSet, *ast.File) {
+	t.Helper()
+	fileSet := token.NewFileSet()
+	file, err := parser.ParseFile(fileSet, "snippet.go", src, 0)
+	if err != nil {
+		t.Fatalf("parse snippet: %v", err)
+	}
+	return fileSet, file
+}
+
+func TestUncoveredEnvReadsCoveredLiteral(t *testing.T) {
+	fileSet, file := parseSnippet(t, `package example
+
+import "os"
+
+func read() string { return os.Getenv("FOO") }
+`)
+
+	messages := uncoveredEnvReads(fileSet, []*ast.File{file}, []string{"FOO"}, nil)
+	if len(messages) != 0 {
+		t.Fatalf("covered literal reported as uncovered: %v", messages)
+	}
+}
+
+func TestUncoveredEnvReadsCoveredConstant(t *testing.T) {
+	fileSet, file := parseSnippet(t, `package example
+
+import "os"
+
+const fooEnv = "FOO"
+
+func read() string { return os.Getenv(fooEnv) }
+`)
+
+	messages := uncoveredEnvReads(fileSet, []*ast.File{file}, []string{"FOO"}, nil)
+	if len(messages) != 0 {
+		t.Fatalf("covered constant reported as uncovered: %v", messages)
+	}
+}
+
+func TestUncoveredEnvReadsUncoveredName(t *testing.T) {
+	fileSet, file := parseSnippet(t, `package example
+
+import "os"
+
+func read() string { return os.Getenv("BAR") }
+`)
+
+	messages := uncoveredEnvReads(fileSet, []*ast.File{file}, nil, nil)
+	if len(messages) != 1 {
+		t.Fatalf("expected exactly one uncovered-name message, got %v", messages)
+	}
+	if !strings.Contains(messages[0], "BAR") {
+		t.Fatalf("message %q does not name the uncovered variable", messages[0])
+	}
+}
+
+// TestUncoveredEnvReadsUncoveredLookupEnv pins the os.LookupEnv half of the
+// classifier. Neither guarded package reads the environment that way today, so
+// dropping LookupEnv from isEnvRead would leave every other test in this file
+// and both live package guards green while silently narrowing the scan.
+func TestUncoveredEnvReadsUncoveredLookupEnv(t *testing.T) {
+	fileSet, file := parseSnippet(t, `package example
+
+import "os"
+
+func read() (string, bool) { return os.LookupEnv("BAR") }
+`)
+
+	messages := uncoveredEnvReads(fileSet, []*ast.File{file}, nil, nil)
+	if len(messages) != 1 {
+		t.Fatalf("expected exactly one uncovered-name message, got %v", messages)
+	}
+	if !strings.Contains(messages[0], "BAR") {
+		t.Fatalf("message %q does not name the uncovered variable", messages[0])
+	}
+}
+
+func TestUncoveredEnvReadsUnresolvableIdentifier(t *testing.T) {
+	fileSet, file := parseSnippet(t, `package example
+
+import "os"
+
+func read(name string) string { return os.Getenv(name) }
+`)
+
+	messages := uncoveredEnvReads(fileSet, []*ast.File{file}, []string{"FOO"}, nil)
+	if len(messages) != 1 {
+		t.Fatalf("expected exactly one unresolvable-identifier message, got %v", messages)
+	}
+	if !strings.Contains(messages[0], "not a string literal") {
+		t.Fatalf("message %q does not explain the resolution failure", messages[0])
+	}
+}
+
+// TestUncoveredEnvReadsIgnoresExtraReaderWhenNotDeclared pins the blind spot
+// that makes the extraReaders parameter necessary: a package reading the
+// environment through its own accessor is invisible to a Getenv-only scan.
+func TestUncoveredEnvReadsIgnoresExtraReaderWhenNotDeclared(t *testing.T) {
+	fileSet, file := parseSnippet(t, `package example
+
+type op struct{}
+
+func (o *op) environmentValue(key string) string { return "" }
+
+func read(o *op) string { return o.environmentValue("BAR") }
+`)
+
+	messages := uncoveredEnvReads(fileSet, []*ast.File{file}, nil, nil)
+	if len(messages) != 0 {
+		t.Fatalf("undeclared accessor should not be scanned, got %v", messages)
+	}
+}
+
+func TestUncoveredEnvReadsUncoveredExtraReaderMethod(t *testing.T) {
+	fileSet, file := parseSnippet(t, `package example
+
+type op struct{}
+
+func (o *op) environmentValue(key string) string { return "" }
+
+func read(o *op) string { return o.environmentValue("BAR") }
+`)
+
+	messages := uncoveredEnvReads(fileSet, []*ast.File{file}, nil, nil, "environmentValue")
+	if len(messages) != 1 {
+		t.Fatalf("expected exactly one uncovered-name message, got %v", messages)
+	}
+	if !strings.Contains(messages[0], "BAR") {
+		t.Fatalf("message %q does not name the uncovered variable", messages[0])
+	}
+}
+
+func TestUncoveredEnvReadsCoveredExtraReaderConstant(t *testing.T) {
+	fileSet, file := parseSnippet(t, `package example
+
+type op struct{}
+
+const fooEnv = "FOO"
+
+func (o *op) environmentValue(key string) string { return "" }
+
+func read(o *op) string { return o.environmentValue(fooEnv) }
+`)
+
+	messages := uncoveredEnvReads(fileSet, []*ast.File{file}, []string{"FOO"}, nil, "environmentValue")
+	if len(messages) != 0 {
+		t.Fatalf("covered accessor read reported as uncovered: %v", messages)
+	}
+}
+
+// TestUncoveredEnvReadsExtraReaderAsPlainFunction covers the package-level
+// spelling of an extra reader, which matches on the name alone.
+func TestUncoveredEnvReadsExtraReaderAsPlainFunction(t *testing.T) {
+	fileSet, file := parseSnippet(t, `package example
+
+func environmentValue(key string) string { return "" }
+
+func read() string { return environmentValue("BAR") }
+`)
+
+	messages := uncoveredEnvReads(fileSet, []*ast.File{file}, nil, nil, "environmentValue")
+	if len(messages) != 1 {
+		t.Fatalf("expected exactly one uncovered-name message, got %v", messages)
+	}
+	if !strings.Contains(messages[0], "BAR") {
+		t.Fatalf("message %q does not name the uncovered variable", messages[0])
+	}
+}
+
+// TestUncoveredEnvReadsExtraReaderWithFallbackArgument pins the arity
+// tolerance of the extra-reader match. The scan used to require exactly one
+// argument, so giving an accessor a second parameter removed every one of its
+// call sites from the scan at once, silently and with no test failing. Proven
+// against the real package: with a variadic parameter added to
+// (*GitOperator).environmentValue and an uncovered name read through it, the
+// process guard reported ok while the exactly-one-argument rule stood, and
+// failed naming git_pr_providers.go only once it was relaxed. A covered name
+// cannot show this, since it reports ok whether it is scanned or skipped.
+func TestUncoveredEnvReadsExtraReaderWithFallbackArgument(t *testing.T) {
+	fileSet, file := parseSnippet(t, `package example
+
+type op struct{}
+
+func (o *op) environmentValue(key, fallback string) string { return fallback }
+
+func read(o *op) string { return o.environmentValue("BAR", "") }
+`)
+
+	messages := uncoveredEnvReads(fileSet, []*ast.File{file}, nil, nil, "environmentValue")
+	if len(messages) != 1 {
+		t.Fatalf("expected exactly one uncovered-name message, got %v", messages)
+	}
+	if !strings.Contains(messages[0], "BAR") {
+		t.Fatalf("message %q does not name the uncovered variable", messages[0])
+	}
+}
+
+// TestUncoveredEnvReadsIgnoresNonOsSelector pins the package qualifier: a
+// Getenv that belongs to some other package is not an environment read, and
+// reporting it would be a false alarm the author cannot silence.
+func TestUncoveredEnvReadsIgnoresNonOsSelector(t *testing.T) {
+	fileSet, file := parseSnippet(t, `package example
+
+import "example.com/cfg"
+
+func read() string { return cfg.Getenv("BAR") }
+`)
+
+	messages := uncoveredEnvReads(fileSet, []*ast.File{file}, nil, nil)
+	if len(messages) != 0 {
+		t.Fatalf("non-os Getenv should not be scanned, got %v", messages)
+	}
+}
+
+// TestUncoveredEnvReadsVarHeldNameIsUnresolvable pins the deliberate choice to
+// resolve only constants. A name held in a var could change at runtime, so the
+// guard refuses to classify it and says so instead of guessing.
+func TestUncoveredEnvReadsVarHeldNameIsUnresolvable(t *testing.T) {
+	fileSet, file := parseSnippet(t, `package example
+
+import "os"
+
+var fooEnv = "FOO"
+
+func read() string { return os.Getenv(fooEnv) }
+`)
+
+	messages := uncoveredEnvReads(fileSet, []*ast.File{file}, []string{"FOO"}, nil)
+	if len(messages) != 1 {
+		t.Fatalf("expected exactly one unresolvable-identifier message, got %v", messages)
+	}
+	if !strings.Contains(messages[0], "not a string literal") {
+		t.Fatalf("message %q does not explain the resolution failure", messages[0])
+	}
+}
+
+// TestUncoveredEnvReadsStaleExtraReaderIsReported pins the rename protection.
+// Extra readers are matched by name, so renaming the accessor leaves the string
+// here compiling and matching nothing, and the guard goes back to watching only
+// os.Getenv. Proven against the real package: renaming
+// (*GitOperator).environmentValue to envValue package-wide and adding an
+// uncovered read through it left the process guard reporting ok, which is
+// exactly the blind spot extraReaders was added to close.
+func TestUncoveredEnvReadsStaleExtraReaderIsReported(t *testing.T) {
+	fileSet, file := parseSnippet(t, `package example
+
+type op struct{}
+
+// Renamed away from environmentValue; the guard's string was not updated.
+func (o *op) envValue(key string) string { return "" }
+
+func read(o *op) string { return o.envValue("FOO") }
+`)
+
+	messages := uncoveredEnvReads(fileSet, []*ast.File{file}, []string{"FOO"}, nil, "environmentValue")
+	if len(messages) != 1 {
+		t.Fatalf("expected exactly one stale-extra-reader message, got %v", messages)
+	}
+	if !strings.Contains(messages[0], "environmentValue") {
+		t.Fatalf("message %q does not name the stale reader", messages[0])
+	}
+	if !strings.Contains(messages[0], "matches no call") {
+		t.Fatalf("message %q does not explain that the reader matched nothing", messages[0])
+	}
+}
+
+// TestUncoveredEnvReadsLiveExtraReaderIsNotReportedStale is the other half of
+// the pair: a reader that is genuinely called must not be reported, or the
+// staleness check would fire on every correct call site.
+func TestUncoveredEnvReadsLiveExtraReaderIsNotReportedStale(t *testing.T) {
+	fileSet, file := parseSnippet(t, `package example
+
+type op struct{}
+
+func (o *op) environmentValue(key string) string { return "" }
+
+func read(o *op) string { return o.environmentValue("FOO") }
+`)
+
+	messages := uncoveredEnvReads(fileSet, []*ast.File{file}, []string{"FOO"}, nil, "environmentValue")
+	if len(messages) != 0 {
+		t.Fatalf("live extra reader reported: %v", messages)
+	}
+}
+
+// parseSnippets parses several files into one file set, for the cases that only
+// arise across files of a package rather than inside one.
+func parseSnippets(t *testing.T, srcs ...string) (*token.FileSet, []*ast.File) {
+	t.Helper()
+	fileSet := token.NewFileSet()
+	files := make([]*ast.File, 0, len(srcs))
+	for i, src := range srcs {
+		file, err := parser.ParseFile(fileSet, fmt.Sprintf("snippet%d.go", i), src, 0)
+		if err != nil {
+			t.Fatalf("parse snippet %d: %v", i, err)
+		}
+		files = append(files, file)
+	}
+	return fileSet, files
+}
+
+// TestUncoveredEnvReadsConflictingConstantIsUnresolvable pins the build-tag
+// blind spot. Every file is parsed regardless of its tags, so a platform pair
+// may legally declare one constant name twice with different values. Flattening
+// those into a single map meant the later file's value won and the earlier
+// file's read was classified against the wrong variable — silently, since a
+// read of an unscrubbed name resolved to an exempt one reports ok. Proven
+// against the real package: a //go:build !windows file reading
+// os.Getenv(qaShellEnv) where qaShellEnv is "KANDEV_QA_UNIXONLY", paired with a
+// //go:build windows file declaring the same name as "SHELL", left the process
+// guard reporting ok even though the uncovered read is the one that compiles on
+// Linux. The same probe now fails.
+func TestUncoveredEnvReadsConflictingConstantIsUnresolvable(t *testing.T) {
+	fileSet, files := parseSnippets(t, `//go:build !windows
+
+package example
+
+import "os"
+
+const shellEnv = "UNCOVERED_ON_UNIX"
+
+func read() string { return os.Getenv(shellEnv) }
+`, `//go:build windows
+
+package example
+
+const shellEnv = "SHELL"
+`)
+
+	messages := uncoveredEnvReads(fileSet, files, nil, []string{"SHELL"})
+	if len(messages) != 1 {
+		t.Fatalf("expected exactly one conflicting-constant message, got %v", messages)
+	}
+	if !strings.Contains(messages[0], "shellEnv") {
+		t.Fatalf("message %q does not name the conflicting constant", messages[0])
+	}
+	if !strings.Contains(messages[0], "different values in more than one file") {
+		t.Fatalf("message %q does not explain the conflict", messages[0])
+	}
+}
+
+// TestUncoveredEnvReadsRepeatedConstantWithSameValueStillResolves is the other
+// half: a platform pair that declares one name twice with the SAME value is not
+// ambiguous, and rejecting it would make the conflict check fire on a correct
+// package.
+func TestUncoveredEnvReadsRepeatedConstantWithSameValueStillResolves(t *testing.T) {
+	fileSet, files := parseSnippets(t, `//go:build !windows
+
+package example
+
+import "os"
+
+const shellEnv = "SHELL"
+
+func read() string { return os.Getenv(shellEnv) }
+`, `//go:build windows
+
+package example
+
+const shellEnv = "SHELL"
+`)
+
+	messages := uncoveredEnvReads(fileSet, files, nil, []string{"SHELL"})
+	if len(messages) != 0 {
+		t.Fatalf("unambiguous repeated constant reported: %v", messages)
+	}
+}
+
+// TestUncoveredEnvReadsConstantResolvesAcrossFiles guards the capability the
+// conflict check must not break: constants are package-scoped, so a read in one
+// file resolves against a constant declared in another.
+func TestUncoveredEnvReadsConstantResolvesAcrossFiles(t *testing.T) {
+	fileSet, files := parseSnippets(t, `package example
+
+import "os"
+
+func read() string { return os.Getenv(fooEnv) }
+`, `package example
+
+const fooEnv = "FOO"
+`)
+
+	messages := uncoveredEnvReads(fileSet, files, []string{"FOO"}, nil)
+	if len(messages) != 0 {
+		t.Fatalf("cross-file constant reported as uncovered: %v", messages)
+	}
+}
+
+// TestUncoveredEnvReadsUnreadableConstantDoesNotBorrowSiblingValue pins the
+// other half of the build-tag collision, which the conflicting-literal check
+// alone does not catch. A platform pair may declare one constant name twice with
+// only ONE side written as a string literal — the other referring to a second
+// constant, concatenating, or repeating the previous expression list. Two
+// differing literals conflict, but a literal paired with an unreadable
+// declaration did not, so the literal file's value was lent to the read in the
+// file this scanner cannot read, exactly as silently. Proven against the real
+// package: a //go:build !windows file declaring qaEnv = qaOtherEnv (where
+// qaOtherEnv is the uncovered "KANDEV_QA_UNCOVERED") and reading os.Getenv(qaEnv),
+// paired with a //go:build windows file declaring qaEnv = "SHELL", left the
+// process guard reporting ok even though the uncovered read is the one that
+// compiles on Linux. The same probe now fails.
+func TestUncoveredEnvReadsUnreadableConstantDoesNotBorrowSiblingValue(t *testing.T) {
+	fileSet, files := parseSnippets(t, `//go:build !windows
+
+package example
+
+import "os"
+
+const otherEnv = "UNCOVERED_ON_UNIX"
+
+const shellEnv = otherEnv
+
+func read() string { return os.Getenv(shellEnv) }
+`, `//go:build windows
+
+package example
+
+const shellEnv = "SHELL"
+`)
+
+	messages := uncoveredEnvReads(fileSet, files, nil, []string{"SHELL"})
+	if len(messages) != 1 {
+		t.Fatalf("expected exactly one unresolvable-identifier message, got %v", messages)
+	}
+	// The generic message, not the conflicting-literals one: nothing here was
+	// declared with two different values, so pointing at that cause would send
+	// the author looking for a second literal that does not exist.
+	if !strings.Contains(messages[0], "not a string literal") {
+		t.Fatalf("message %q does not explain the resolution failure", messages[0])
+	}
+}
+
+// TestUncoveredEnvReadsImplicitRepetitionDoesNotBorrowSiblingValue covers the
+// valueless spelling of the same hazard: in a const block, a name with no
+// expression repeats the previous one, which this scanner does not evaluate.
+func TestUncoveredEnvReadsImplicitRepetitionDoesNotBorrowSiblingValue(t *testing.T) {
+	fileSet, files := parseSnippets(t, `//go:build !windows
+
+package example
+
+import "os"
+
+const (
+	firstEnv = "UNCOVERED_ON_UNIX"
+	shellEnv
+)
+
+func read() string { return os.Getenv(shellEnv) }
+`, `//go:build windows
+
+package example
+
+const shellEnv = "SHELL"
+`)
+
+	messages := uncoveredEnvReads(fileSet, files, nil, []string{"SHELL"})
+	if len(messages) != 1 {
+		t.Fatalf("expected exactly one unresolvable-identifier message, got %v", messages)
+	}
+	if !strings.Contains(messages[0], "not a string literal") {
+		t.Fatalf("message %q does not explain the resolution failure", messages[0])
+	}
+}
+
+// TestUncoveredEnvReadsUnreadableSpecDoesNotPoisonItsBlock bounds the rule
+// above: a name is marked unresolvable individually, not by association with a
+// const block or a multi-name spec that happens to contain one. Both guarded
+// packages declare their env names beside only other string literals —
+// git_pr_providers.go's block is literals throughout — so nothing live would
+// notice if a refactor hoisted the marking to spec or block level, and every
+// constant in a mixed block would start reporting unresolvable at once.
+//
+// Both hoists are covered, and each needs its own shape. fooEnv is a
+// single-name spec beside an unreadable sibling spec, which only a block-level
+// hoist poisons. bazEnv shares one spec with an unreadable name, which a
+// spec-level hoist poisons too — the level a first mutation attempt reached for
+// and found inert, because every other declaration here is one name per spec.
+func TestUncoveredEnvReadsUnreadableSpecDoesNotPoisonItsBlock(t *testing.T) {
+	fileSet, file := parseSnippet(t, `package example
+
+import "os"
+
+const timeout = 30
+
+const (
+	fooEnv       = "FOO"
+	scale        = timeout
+	bazEnv, unit = "BAZ", timeout
+)
+
+func read() string { return os.Getenv(fooEnv) + os.Getenv(bazEnv) }
+`)
+
+	messages := uncoveredEnvReads(fileSet, []*ast.File{file}, []string{"FOO", "BAZ"}, nil)
+	if len(messages) != 0 {
+		t.Fatalf("literal siblings of an unreadable spec reported as unresolvable: %v", messages)
+	}
+}
+
+func TestUncoveredEnvReadsExemptName(t *testing.T) {
+	fileSet, file := parseSnippet(t, `package example
+
+import "os"
+
+func shell() string { return os.Getenv("SHELL") }
+`)
+
+	messages := uncoveredEnvReads(fileSet, []*ast.File{file}, nil, []string{"SHELL"})
+	if len(messages) != 0 {
+		t.Fatalf("exempt name reported as uncovered: %v", messages)
+	}
+}