Refactor workflow env access in github_cli/features to injected lookup#43104
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR reduces direct coupling to the process environment in pkg/workflow helpers by routing environment lookups through an injectable lookup function, enabling more controllable tests and future dependency-injection work while preserving existing runtime behavior.
Changes:
- Introduced a package-level, configurable process environment lookup (
SetProcessEnvLookup+ internallookupProcessEnv) with a default backed byos.LookupEnv. - Updated
setupGHCommandto readGH_TOKEN,GITHUB_TOKEN, andGH_HOSTvia the injected lookup boundary rather than directos.Getenvcalls. - Updated feature-flag environment detection to read
GH_AW_FEATURESvia the injected lookup boundary and added tests validating the injection path.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/process_env_lookup.go | Adds injectable environment lookup used by workflow helpers. |
| pkg/workflow/github_cli.go | Switches GH token/host environment reads to the injected lookup helper. |
| pkg/workflow/github_cli_test.go | Adds coverage ensuring GH CLI helpers honor the configured lookup behavior. |
| pkg/workflow/features.go | Switches feature env reads (GH_AW_FEATURES) to the injected lookup helper. |
| pkg/workflow/features_test.go | Adds coverage ensuring feature detection honors the configured lookup behavior. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Low
| func lookupProcessEnv(key string) string { | ||
| processEnvLookupMu.RLock() | ||
| defer processEnvLookupMu.RUnlock() | ||
| // Intentionally ignore the existence flag to preserve os.Getenv semantics: | ||
| // missing variables and explicitly empty variables are both treated as "". | ||
| value, _ := processEnvLookup(key) | ||
| return value | ||
| } |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (118 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. 🔍 Note on relation to ADR-42984A prior ADR (ADR-42984) decided to use constructor injection ( ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (2 tests)
|
Review Summary 🧠Applied 📋 Issues found (7 inline comments)
@copilot please address the review comments above.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /improve-codebase-architecture, and /tdd — requesting changes on a correctness issue in the locking implementation and weak test assertions.
📋 Key Themes & Highlights
Key Issues
- Potential deadlock (
process_env_lookup.goL32):RLockis held while calling the user-supplied function. If that function callsSetProcessEnvLookup, a write-lock deadlock follows. Fix: copy the pointer under the lock, then release before calling. - Incomplete injection boundary (
github_cli.goL62):os.Environ()still snapshots the real process environment for the base command env slice — worth documenting or closing the gap in a follow-up. - Weak test assertions (
github_cli_test.goL139, L153): Tworequire.NotNil(t, cmd.Env)guards are always true given the test setup; they should be replaced with targeted content assertions. - Missing edge case (
features_test.goL95): No test forGH_AW_FEATURESset-but-blank, which is the key semantic parity check between oldos.Getenvand newlookupProcessEnv. - Global mutable state (
process_env_lookup.goL17):SetProcessEnvLookupis package-level and unsafe for parallel tests. Documenting this limitation on the function would prevent silent test pollution.
Positive Highlights
- ✅ Clean, minimal new file —
process_env_lookup.gois a well-scoped abstraction - ✅ Nil reset to default (
SetProcessEnvLookup(nil)) is a thoughtful API decision - ✅ All call sites correctly use
t.Cleanupfor teardown — no leaked state - ✅ Both positive and negative injection paths are tested
- ✅ PR description accurately describes scope and preserves existing semantics
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 90 AIC · ⌖ 6.8 AIC · ⊞ 6.6K
Comment /matt to run again
| defer processEnvLookupMu.RUnlock() | ||
| // Intentionally ignore the existence flag to preserve os.Getenv semantics: | ||
| // missing variables and explicitly empty variables are both treated as "". | ||
| value, _ := processEnvLookup(key) |
There was a problem hiding this comment.
[/codebase-design] The RLock is held for the full duration of the user-supplied processEnvLookup(key) call — if that function ever calls SetProcessEnvLookup (acquiring the write lock) you have a self-deadlock. The standard fix is to copy the function pointer under the lock, release it, then call the copy.
💡 Suggested fix
func lookupProcessEnv(key string) string {
processEnvLookupMu.RLock()
fn := processEnvLookup
processEnvLookupMu.RUnlock()
value, _ := fn(key)
return value
}This is the standard copy-then-call pattern for guarded function-pointer fields. It also shortens the critical section and avoids arbitrary work inside the injected function from holding the lock.
@copilot please address this.
|
|
||
| // SetProcessEnvLookup configures how workflow helpers resolve environment values. | ||
| // Passing nil restores the default process environment lookup. | ||
| func SetProcessEnvLookup(lookup func(string) (string, bool)) { |
There was a problem hiding this comment.
[/codebase-design] SetProcessEnvLookup is package-level mutable state — callers across goroutines (production code plus concurrent tests) can race on it without realising. Consider whether the injection point belongs on a context value or a per-call struct rather than a global.
💡 Why this matters
Global test helpers like SetProcessEnvLookup pair well with t.Cleanup (which this PR does correctly), but package-level state still means sub-tests or parallel tests that call the same workflow helpers share the same slot. If two parallel t.Parallel() sub-tests each call SetProcessEnvLookup, they race. A context.WithValue or an explicit env-lookup parameter threaded through each helper would be fully parallel-safe and would make the dependency visible at each call site.
The current approach is a pragmatic first step, but documenting the known limitation ("not safe for parallel tests") on the function would prevent surprises.
@copilot please address this.
| cmd.Env = append(os.Environ(), "GH_TOKEN="+githubToken) | ||
| } | ||
| if os.Getenv("GH_HOST") == "" { | ||
| if lookupProcessEnv("GH_HOST") == "" { |
There was a problem hiding this comment.
[/codebase-design] GH_HOST is read via lookupProcessEnv (injectable) but os.Environ() on line 62 still snapshots the real process environment — so the token injection path (GH_TOKEN=...) bypasses the injection boundary. Test environments that stub SetProcessEnvLookup will still inherit real GH_TOKEN/GITHUB_TOKEN from the process if those are set, which can cause test pollution.
💡 Suggested approach
Either:
- Also build the env slice from the injected lookup (fuller DI), or
- Explicitly document that
cmd.Envis seeded fromos.Environ()and the injected lookup only controls the token-selection logic, not the base environment.
Option 2 is fine for now, but worth a comment so the next slice of this work doesn't silently re-couple.
@copilot please address this.
| }) | ||
|
|
||
| cmd := ExecGH("auth", "status") | ||
| require.NotNil(t, cmd.Env) |
There was a problem hiding this comment.
[/tdd] require.NotNil(t, cmd.Env) is weaker than it looks: cmd.Env will be non-nil precisely because the test injected a token, so this assertion is always trivially true and never catches a regression where the env slice is constructed but empty. Assert on the length or specific contents only.
💡 Suggested replacement
// Instead of require.NotNil(t, cmd.Env)
assert.Contains(t, cmd.Env, "GH_TOKEN=lookup-token")
assert.Contains(t, cmd.Env, "GH_HOST=configured.ghe.com")The two assert.Contains calls below already cover the interesting invariants — the require.NotNil guard only triggers if cmd.Env is literally nil, which can't happen once tokens are appended. Removing it keeps the test specification clean.
@copilot please address this.
| }) | ||
|
|
||
| cmd := ExecGH("auth", "status") | ||
| require.NotNil(t, cmd.Env) |
There was a problem hiding this comment.
[/tdd] The "both tokens absent" sub-test asserts cmd.Env is non-nil, but when neither token is set setupGHCommand doesn't append to cmd.Env — it stays nil unless SetGHHostEnv sets it. The test should assert explicitly that cmd.Env contains no GH_TOKEN= entry rather than a NotNil guard.
💡 Why the current assertion may pass for the wrong reason
cmd.Env is non-nil here because SetGHHostEnv appends GH_HOST=... even when tokens are absent. The require.NotNil therefore passes, but it says nothing about the absence of an injected token. Replace it with the exact negative assertion that is already on line 154 and remove the redundant guard.
@copilot please address this.
| SetProcessEnvLookup(nil) | ||
| }) | ||
|
|
||
| result := isFeatureEnabled(constants.FeatureFlag("firewall"), nil) |
There was a problem hiding this comment.
[/tdd] There is no test for the case where GH_AW_FEATURES returns an empty string from the lookup (i.e. the variable exists but is blank). lookupProcessEnv preserves os.Getenv semantics and treats this as "not set", which is arguably the most important edge case to lock down now that the lookup is injectable.
💡 Suggested additional test
t.Run("empty GH_AW_FEATURES disables feature", func(t *testing.T) {
SetProcessEnvLookup(func(key string) (string, bool) {
if key == "GH_AW_FEATURES" {
return "", true // set but blank
}
return "", false
})
t.Cleanup(func() { SetProcessEnvLookup(nil) })
require.False(t, isFeatureEnabled(constants.FeatureFlag("firewall"), nil))
})This exercises the branch in isFeatureInEnvironment where features == "" is treated the same as absent, ensuring the semantic parity with the old os.Getenv path is maintained.
@copilot please address this.
| "sync" | ||
| ) | ||
|
|
||
| type envLookupFunc func(string) (string, bool) |
There was a problem hiding this comment.
[/improve-codebase-architecture] envLookupFunc is an unexported type alias. As follow-up slices extend the injection boundary, this type will likely appear in more packages (or in test helpers). Consider whether it belongs as a named exported type in a small envlookup or envutil package so consumers don't have to redeclare the function signature.
💡 Future-proofing note
Right now the type is only used within the workflow package, so unexported is correct. But if the PR description's "follow-up slices" plan to apply the same pattern to sibling packages, each will have to duplicate the signature. Exporting the type (or providing it via a shared internal package) would keep the injection contract in one place.
This is a forward-looking suggestion — no change needed now, but worth noting before the pattern proliferates.
@copilot please address this.
There was a problem hiding this comment.
Review: Refactor workflow env access to injected lookup
The abstraction design is clean and the injection pattern is well-structured. process_env_lookup.go correctly uses sync.RWMutex and restores the default on nil reset — no issues there.
Two related test-isolation bugs in the new tests need fixing before merge.
Root cause (shared)
When cmd.Env is nil (because neither token injection block ran), SetGHHostEnv seeds cmd.Env via append(os.Environ(), ...). This copies the real process environment into cmd.Env, so any GH_TOKEN present in the actual runner process bleeds in. Both new sub-tests in TestExecGHUsesConfiguredProcessEnvLookup fail to isolate from this.
Fix (both sub-tests): Add t.Setenv("GH_TOKEN", "") and t.Setenv("GITHUB_TOKEN", "") before calling ExecGH in each sub-test. t.Setenv automatically restores on cleanup.
The production code change itself (lines 44–45, 64 in github_cli.go and line 92 in features.go) is correct.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 107 AIC · ⌖ 5.79 AIC · ⊞ 4.9K
|
|
||
| cmd := ExecGH("auth", "status") | ||
| require.NotNil(t, cmd.Env) | ||
| assert.False(t, slices.ContainsFunc(cmd.Env, func(e string) bool { |
There was a problem hiding this comment.
The assertion on line 154 has a subtle but real false-negative risk.
When both tokens are absent via the injected lookup, the cmd.Env injection at setupGHCommand line 62 is skipped — so cmd.Env is nil going into SetGHHostEnv. Inside SetGHHostEnv, when cmd.Env == nil, it calls append(os.Environ(), "GH_HOST="+host), seeding cmd.Env from the real process environment.
Any environment where GH_TOKEN is set in the process (CI runners, developer machines with a token) will include GH_TOKEN=<real_value> in cmd.Env, causing this assertion to pass falsely — or fail unexpectedly — depending on the token's presence.
Fix: isolate this sub-test from the real process environment:
t.Run("does not inject gh token when both tokens are absent", func(t *testing.T) {
t.Setenv("GH_TOKEN", "") // isolate from real process env
t.Setenv("GITHUB_TOKEN", "")
SetProcessEnvLookup(...)
...
})@copilot please address this.
|
|
||
| cmd := ExecGH("auth", "status") | ||
| require.NotNil(t, cmd.Env) | ||
| assert.Contains(t, cmd.Env, "GH_TOKEN=lookup-token") |
There was a problem hiding this comment.
The same process-environment isolation issue applies here. When ghToken == "" and githubToken != "", line 62 does:
cmd.Env = append(os.Environ(), "GH_TOKEN="+githubToken)os.Environ() captures the real process env. If GH_TOKEN is already set in the real environment, both GH_TOKEN=<real> and GH_TOKEN=lookup-token end up in cmd.Env. The test passes (Contains finds the injected value), but the subprocess would use the earlier (real) token, not the injected one.
Add t.Setenv("GH_TOKEN", "") before calling ExecGH to ensure the injected token is actually the only one present:
t.Setenv("GH_TOKEN", "")
t.Setenv("GITHUB_TOKEN", "")
SetProcessEnvLookup(...)@copilot please address this.
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed in commit |
|
/souschef |
|
🚀 pr-sous-chef has started processing this pull request comment |
|
@copilot run pr-finisher |
PR is in a merge-ready state:
Hand-off: CI re-trigger required before merge. |
|
🎉 This pull request is included in a new release. Release: |
This PR addresses a focused subset of process-environment coupling findings by removing direct environment reads from lower-level workflow helpers. The change targets
pkg/workflow/github_cli.goandpkg/workflow/features.go, and threads access through an injectable lookup boundary.Scope and intent
os.Getenvusage in selected workflow library code with explicit, injectable environment lookup.Workflow env lookup abstraction
pkg/workflow/process_env_lookup.gowith:SetProcessEnvLookup(func(string) (string, bool))for caller/test injection.lookupProcessEnv(string) stringused by workflow helpers.os.LookupEnv), but no longer hardwired at call sites.github_clidecouplingsetupGHCommandnow resolvesGH_TOKEN,GITHUB_TOKEN, andGH_HOSTvialookupProcessEnv(...)instead of direct process reads.featuresdecouplingisFeatureInEnvironmentnow resolvesGH_AW_FEATURESvialookupProcessEnv(...).Targeted tests for injection path
github_clitests to validate configured lookup behavior (token fallback present/absent cases).featurestests to validate configured lookup behavior (feature present/missing cases).