fix(lint): eliminate env-coupling in workflow_data, add_interactive_engine, outcomes_command#43506
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR removes direct os.Getenv / os.LookupEnv calls from library code to avoid coupling core logic to ambient process environment state, aligning with the existing injectable env-lookup pattern already used in pkg/workflow.
Changes:
- Switched
pkg/workflow/workflow_data.goto use the existinglookupProcessEnv("GH_HOST")helper instead ofos.Getenv. - Added a new injectable environment lookup facility in
pkg/cli/process_env_lookup.goand updated CLI code to use it. - Updated CLI code paths in
add_interactive_engine.goandoutcomes_command.goto use the new lookup helpers.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/workflow_data.go | Uses lookupProcessEnv for GH_HOST to avoid direct process env dependency. |
| pkg/cli/process_env_lookup.go | Introduces injectable env lookup for CLI code (lookupEnv, lookupEnvOk, and an exported setter). |
| pkg/cli/add_interactive_engine.go | Replaces os.Getenv with injected lookupEnv for engine env-var detection. |
| pkg/cli/outcomes_command.go | Replaces os.LookupEnv with injected lookupEnvOk for outcomes dir env-var fallback. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Low
| func lookupEnv(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 | ||
| } | ||
|
|
||
| func lookupEnvOk(key string) (string, bool) { | ||
| processEnvLookupMu.RLock() | ||
| defer processEnvLookupMu.RUnlock() | ||
| return processEnvLookup(key) | ||
| } |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (49 additions across 4 files in default business logic paths). |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Review: fix(lint): eliminate env-coupling in workflow_data, add_interactive_engine, outcomes_command
The approach is sound — mirroring the existing pkg/workflow/process_env_lookup.go pattern into pkg/cli is the right architectural move, and the mechanics (mutex-guarded injection, nil-reset, semantic wrappers) are implemented correctly.
Two items need addressing before merge:
1. Naming inconsistency (blocking)
SetEnvLookup in pkg/cli should match SetProcessEnvLookup in pkg/workflow. Both packages use the same concept; diverging names create discovery friction for callers and reviewers. See inline comment.
2. Missing test coverage (blocking)
pkg/workflow/process_env_lookup.go is exercised by existing tests that call SetProcessEnvLookup. The new pkg/cli equivalent ships with no tests, leaving the injectable override path — and the thread-safety guarantee — unverified. See inline comment.
What looks good
- Correctly drops the
"os"import fromworkflow_data.gosince the lastos.Getenvcall there was the one being replaced. add_interactive_engine.goandoutcomes_command.goretain their"os"imports legitimately (used foros.Stderr/os.Stdout).- The
lookupEnv/lookupEnvOksplit faithfully preservesos.Getenvvsos.LookupEnvsemantics with a clear comment explaining the intentional discard of the existence flag. - File naming (
process_env_lookup.go) is consistent with the existing package.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 39.9 AIC · ⌖ 6.13 AIC · ⊞ 4.9K
|
|
||
| // SetEnvLookup configures how CLI helpers resolve environment values. | ||
| // Passing nil restores the default process environment lookup. | ||
| func SetEnvLookup(lookup func(string) (string, bool)) { |
There was a problem hiding this comment.
Naming inconsistency with pkg/workflow counterpart
The exported setter here is named SetEnvLookup, but its mirror in pkg/workflow/process_env_lookup.go is named SetProcessEnvLookup. This asymmetry makes the two injection points harder to discover and use consistently.
Consider renaming to SetProcessEnvLookup to match:
// SetProcessEnvLookup configures how CLI helpers resolve environment values.
func SetProcessEnvLookup(lookup func(string) (string, bool)) {@copilot please address this.
| processEnvLookupMu.RLock() | ||
| defer processEnvLookupMu.RUnlock() | ||
| return processEnvLookup(key) | ||
| } |
There was a problem hiding this comment.
No test coverage for the injectable override
pkg/workflow/process_env_lookup.go is exercised in github_cli_test.go and features_test.go (using SetProcessEnvLookup). This new pkg/cli counterpart has no equivalent tests, leaving the thread-safe injection path untested.
Please add at least a simple test similar to the workflow package pattern — for example, verifying that lookupEnv and lookupEnvOk delegate to an injected function, and that passing nil restores the default.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design to this lint-fix PR. The change is well-structured and the pattern mirrors the established pkg/workflow/process_env_lookup.go idiom. Three observations worth addressing before merge.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap:
SetEnvLookupis wired as a test hook but no_test.gofile covers the override path or the nil-reset behaviour. This is the highest-priority gap. - Logger necessity: The logger dependency in
process_env_lookup.gois heavier than thepkg/workflowequivalent. Consider whether the twoPrintcalls add enough value to justify the extra dependency. lookupEnvsemantics test: The discardedboolflag inlookupEnvis intentional and well-commented, but a test pinning the "empty string ≠ missing" behaviour would protect against future regressions.
Positive Highlights
- ✅ Thread-safe design using
sync.RWMutex— reads lock correctly withRLock/RUnlock - ✅
SetEnvLookup(nil)restores the default — good nil-guard hygiene - ✅ Clean call-site replacements — minimal diff, no logic change, easy to audit
- ✅
pkg/workflow/workflow_data.gofix is a single-line swap using the already-established helper — no risk
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 43.6 AIC · ⌖ 9.35 AIC · ⊞ 6.6K
Comment /matt to run again
|
|
||
| // SetEnvLookup configures how CLI helpers resolve environment values. | ||
| // Passing nil restores the default process environment lookup. | ||
| func SetEnvLookup(lookup func(string) (string, bool)) { |
There was a problem hiding this comment.
[/tdd] SetEnvLookup is advertised as a test hook but no tests exercise the override path. Without a test, there is no regression safety if the injection mechanism changes.
💡 Suggested test skeleton
func TestSetEnvLookup_override(t *testing.T) {
t.Cleanup(func() { SetEnvLookup(nil) })
SetEnvLookup(func(key string) (string, bool) {
if key == "SENTINEL" {
return "injected", true
}
return "", false
})
if got := lookupEnv("SENTINEL"); got != "injected" {
t.Fatalf("lookupEnv: got %q, want %q", got, "injected")
}
val, ok := lookupEnvOk("SENTINEL")
if val != "injected" || !ok {
t.Fatalf("lookupEnvOk: got (%q, %v), want (\"injected\", true)", val, ok)
}
}Also add a test passing nil to verify the default (os.LookupEnv) is restored.
@copilot please address this.
| "github.com/github/gh-aw/pkg/logger" | ||
| ) | ||
|
|
||
| var processEnvLookupLog = logger.New("cli:process_env_lookup") |
There was a problem hiding this comment.
[/codebase-design] The processEnvLookupLog logger is imported from pkg/logger but the two log lines it emits (Print("Restoring...") and Print("Installing...")) are at default level and will appear in production output on every test that calls SetEnvLookup. In similar packages (e.g., pkg/workflow/process_env_lookup.go) check whether a logger is even needed here — if these messages are only diagnostic during development, consider removing the logger dependency entirely to keep the module shallow.
💡 Alternative without the logger
If the project convention is that injection-setup logging is not needed at runtime, the import and processEnvLookupLog variable can be dropped. The SetEnvLookup body becomes just the mutex + assignment, matching the simpler style of pkg/workflow/process_env_lookup.go.
@copilot please address this.
| 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.
[/tdd] lookupEnv silently discards the bool existence flag. The inline comment explains the intent (match os.Getenv semantics), which is correct — but it is worth a test that verifies an explicitly-set empty string ("") is returned as "" (not treated as missing), so the semantics stay locked in.
💡 Suggested edge-case test
func TestLookupEnv_emptyStringIsNotMissing(t *testing.T) {
t.Cleanup(func() { SetEnvLookup(nil) })
SetEnvLookup(func(key string) (string, bool) {
return "", true // key exists but is empty
})
if got := lookupEnv("ANY"); got != "" {
t.Fatalf("want empty string, got %q", got)
}
}@copilot please address this.
There was a problem hiding this comment.
One medium correctness issue, otherwise a clean migration
The new pkg/cli/process_env_lookup.go infrastructure is well-structured and mirrors the pkg/workflow pattern correctly. The two call-site migrations (workflow_data.go and add_interactive_engine.go) are correct. One blocking issue in the third site below.
📋 Review detail
Blocking: outcomes_command.go line 176 calls lookupEnvOk but immediately discards the boolean with _. The point of having two separate helpers (lookupEnv vs lookupEnvOk) is to signal call-site intent; using the wrong one here teaches readers that lookupEnvOk(x), _ is acceptable. lookupEnv is the right choice here since the downstream check is if outcomesDir != "" regardless of whether the key exists in the environment. See inline comment.
Non-blocking (already in existing review threads):
- Naming inconsistency between
SetEnvLookup(this PR) andSetProcessEnvLookup(workflow package). - No unit tests for
SetEnvLookup/lookupEnv/lookupEnvOk— the workflow package's existing tests (features_test.go,github_cli_test.go) demonstrate the requireddefer SetProcessEnvLookup(nil)cleanup discipline thatpkg/cliwill also need.
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.
🔎 Code quality review by PR Code Quality Reviewer · 162.7 AIC · ⌖ 5.39 AIC · ⊞ 5.4K
Comment /review to run again
| outcomesDir := config.OutcomesDir | ||
| if outcomesDir == "" { | ||
| outcomesDir, _ = os.LookupEnv("GH_AW_OUTCOMES_DIR") | ||
| outcomesDir, _ = lookupEnvOk("GH_AW_OUTCOMES_DIR") |
There was a problem hiding this comment.
Wrong helper used: lookupEnvOk is called but ok is always discarded, making lookupEnv the semantically correct choice here.
💡 Explanation and suggested fix
The original code was outcomesDir, _ = os.LookupEnv(...) — the _ blank already showed that the "was this key set at all" distinction did not matter. The migration keeps that blank:
outcomesDir, _ = lookupEnvOk("GH_AW_OUTCOMES_DIR")But lookupEnvOk exists precisely to expose the (string, bool) pair. Discarding the bool here undermines the intent of the API split. lookupEnv already encodes "return empty string whether the key is absent or explicitly set to empty" — which is exactly what the downstream if outcomesDir != "" check relies on.
Suggested fix:
outcomesDir = lookupEnv("GH_AW_OUTCOMES_DIR")Semantically identical in this case (the empty-string check follows regardless), but correctly signals that the set/unset distinction is irrelevant at this call site, and prevents lookupEnvOk with _ from becoming a copy-paste anti-pattern.
|
@copilot please run the
|
…ctable env lookup Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed both reviewer threads in commit
|
|
@copilot please run the
|
All items are addressed. No further changes needed:
CI needs a human re-trigger (close/reopen or |
🤖 PR Triage — Run §28741077056
Rationale: Eliminates env-coupling
|
|
@copilot please run the Unresolved review feedback to address:
|
…outcomes_command Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all
|
|
🎉 This pull request is included in a new release. Release: |
osgetenvlibrarylint violations in three library files whereos.Getenv/os.LookupEnvwere called directly, coupling business logic to ambient process environment state.Changes
pkg/workflow/workflow_data.go— replaceos.Getenv("GH_HOST")with the existinglookupProcessEnvhelper (ADR-43104 pattern); drop unusedosimportpkg/cli/process_env_lookup.go(new) — introduce apkg/cli-scoped injectable env lookup, mirroringpkg/workflow/process_env_lookup.go:lookupEnv(key)—os.GetenvsemanticslookupEnvOk(key)—os.LookupEnvsemanticsSetEnvLookup(fn)— thread-safe override for tests, protected bysync.RWMutexpkg/cli/add_interactive_engine.go—os.Getenv(envVar)→lookupEnv(envVar)in engine env-var fallback detectionpkg/cli/outcomes_command.go—os.LookupEnv("GH_AW_OUTCOMES_DIR")→lookupEnvOk("GH_AW_OUTCOMES_DIR")in outcomes JSONL export path