diff --git a/apps/backend/internal/agentctl/server/process/env_hermetic_test.go b/apps/backend/internal/agentctl/server/process/env_hermetic_test.go new file mode 100644 index 0000000000..f2bbb9ae79 --- /dev/null +++ b/apps/backend/internal/agentctl/server/process/env_hermetic_test.go @@ -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") +} diff --git a/apps/backend/internal/agentctl/server/process/git_pr_providers_test.go b/apps/backend/internal/agentctl/server/process/git_pr_providers_test.go index af94d7bd17..1fedcfe34a 100644 --- a/apps/backend/internal/agentctl/server/process/git_pr_providers_test.go +++ b/apps/backend/internal/agentctl/server/process/git_pr_providers_test.go @@ -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) } diff --git a/apps/backend/internal/agentctl/server/process/testmain_test.go b/apps/backend/internal/agentctl/server/process/testmain_test.go index e6ebfb8ded..df80e877c0 100644 --- a/apps/backend/internal/agentctl/server/process/testmain_test.go +++ b/apps/backend/internal/agentctl/server/process/testmain_test.go @@ -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) } diff --git a/apps/backend/internal/gitlab/env_hermetic_test.go b/apps/backend/internal/gitlab/env_hermetic_test.go index 6ab7e817d4..bc50d91243 100644 --- a/apps/backend/internal/gitlab/env_hermetic_test.go +++ b/apps/backend/internal/gitlab/env_hermetic_test.go @@ -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) } diff --git a/apps/backend/internal/task/service/service_status_summary_rebuild.go b/apps/backend/internal/task/service/service_status_summary_rebuild.go index 1441760983..5b895436ba 100644 --- a/apps/backend/internal/task/service/service_status_summary_rebuild.go +++ b/apps/backend/internal/task/service/service_status_summary_rebuild.go @@ -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 { diff --git a/apps/backend/internal/testutil/envscan.go b/apps/backend/internal/testutil/envscan.go new file mode 100644 index 0000000000..c905c38f82 --- /dev/null +++ b/apps/backend/internal/testutil/envscan.go @@ -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 +} diff --git a/apps/backend/internal/testutil/envscan_test.go b/apps/backend/internal/testutil/envscan_test.go new file mode 100644 index 0000000000..115be8f84c --- /dev/null +++ b/apps/backend/internal/testutil/envscan_test.go @@ -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) + } +}