From 9137650f87a72ed1246ecb4e675dd600921ca961 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 7 Jul 2026 18:29:32 +0000 Subject: [PATCH 01/11] Initial plan From 02ff4e5371ad49dd64948854ac882b19e150c835 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 7 Jul 2026 18:43:43 +0000 Subject: [PATCH 02/11] feat: add envutil bool and string helpers Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_interactive_orchestrator.go | 3 +- pkg/cli/add_interactive_workflow.go | 2 +- pkg/cli/add_wizard_command.go | 3 +- pkg/cli/codespace.go | 4 +- pkg/cli/interactive.go | 3 +- pkg/envutil/README.md | 59 +++++++++++- pkg/envutil/envutil.go | 73 +++++++++++++-- pkg/envutil/envutil_test.go | 118 ++++++++++++++++++++++++ pkg/envutil/spec_test.go | 88 ++++++++++++++++++ pkg/parser/github.go | 5 +- pkg/parser/github_wasm.go | 7 +- 11 files changed, 338 insertions(+), 27 deletions(-) diff --git a/pkg/cli/add_interactive_orchestrator.go b/pkg/cli/add_interactive_orchestrator.go index 8e22194c7ab..a23ac5e6053 100644 --- a/pkg/cli/add_interactive_orchestrator.go +++ b/pkg/cli/add_interactive_orchestrator.go @@ -10,6 +10,7 @@ import ( "charm.land/huh/v2" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/envutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/workflow" ) @@ -71,7 +72,7 @@ func RunAddInteractive(ctx context.Context, config *AddInteractiveConfig) error addInteractiveLog.Print("Starting interactive add workflow") // Assert this function is not running in automated unit tests or CI - if os.Getenv("GO_TEST_MODE") == "true" || os.Getenv("CI") != "" { //nolint:osgetenvlibrary + if envutil.GetBoolFromEnv("GO_TEST_MODE", false, addInteractiveLog) || IsRunningInCI() { return errors.New("interactive add cannot be used in automated tests or CI environments") } diff --git a/pkg/cli/add_interactive_workflow.go b/pkg/cli/add_interactive_workflow.go index 3d21d45448a..cb011f1dc2a 100644 --- a/pkg/cli/add_interactive_workflow.go +++ b/pkg/cli/add_interactive_workflow.go @@ -87,7 +87,7 @@ func (c *AddInteractiveConfig) checkStatusAndOfferRun(ctx context.Context) error } // In Codespaces, don't offer to trigger - provide link to Actions page instead - if os.Getenv("CODESPACES") == "true" { //nolint:osgetenvlibrary + if isRunningInCodespace() { addInteractiveLog.Print("Running in Codespaces, skipping run offer and showing Actions link") fmt.Fprintln(os.Stderr, "") fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Running in GitHub Codespaces - please trigger the workflow manually from the Actions page")) diff --git a/pkg/cli/add_wizard_command.go b/pkg/cli/add_wizard_command.go index 07fc827cce6..fe5441dcf5c 100644 --- a/pkg/cli/add_wizard_command.go +++ b/pkg/cli/add_wizard_command.go @@ -2,7 +2,6 @@ package cli import ( "errors" - "os" "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/logger" @@ -84,7 +83,7 @@ Note: To create a new workflow from scratch, use the 'new' command instead.`, // add-wizard requires an interactive terminal isTerminal := tty.IsStdoutTerminal() - isCIEnv := os.Getenv("CI") != "" //nolint:osgetenvlibrary + isCIEnv := IsRunningInCI() addWizardLog.Printf("Terminal check: is_terminal=%v, is_ci=%v", isTerminal, isCIEnv) if !isTerminal || isCIEnv { return errors.New("add-wizard requires an interactive terminal; use 'add' for non-interactive environments") diff --git a/pkg/cli/codespace.go b/pkg/cli/codespace.go index 5794a49121a..69c4cf2df1a 100644 --- a/pkg/cli/codespace.go +++ b/pkg/cli/codespace.go @@ -1,10 +1,10 @@ package cli import ( - "os" "strings" "github.com/github/gh-aw/pkg/console" + "github.com/github/gh-aw/pkg/envutil" "github.com/github/gh-aw/pkg/logger" ) @@ -14,7 +14,7 @@ var codespaceLog = logger.New("cli:codespace") // by checking for the CODESPACES environment variable func isRunningInCodespace() bool { // GitHub Codespaces sets CODESPACES=true environment variable - isCodespace := strings.EqualFold(os.Getenv("CODESPACES"), "true") //nolint:osgetenvlibrary + isCodespace := envutil.GetBoolFromEnv("CODESPACES", false, codespaceLog) codespaceLog.Printf("Codespace detection: is_codespace=%v", isCodespace) return isCodespace } diff --git a/pkg/cli/interactive.go b/pkg/cli/interactive.go index c4b967a2b76..2549ddd03de 100644 --- a/pkg/cli/interactive.go +++ b/pkg/cli/interactive.go @@ -15,6 +15,7 @@ import ( "charm.land/huh/v2" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/envutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/setutil" "github.com/github/gh-aw/pkg/sliceutil" @@ -64,7 +65,7 @@ func CreateWorkflowInteractively(ctx context.Context, workflowName string, verbo interactiveLog.Printf("Starting interactive workflow creation: workflowName=%s, force=%v", workflowName, force) // Assert this function is not running in automated unit tests - if os.Getenv("GO_TEST_MODE") == "true" || os.Getenv("CI") != "" { //nolint:osgetenvlibrary + if envutil.GetBoolFromEnv("GO_TEST_MODE", false, interactiveLog) || IsRunningInCI() { return errors.New("interactive workflow creation cannot be used in automated tests or CI environments") } diff --git a/pkg/envutil/README.md b/pkg/envutil/README.md index c5b88d20027..570e74966be 100644 --- a/pkg/envutil/README.md +++ b/pkg/envutil/README.md @@ -1,12 +1,12 @@ # envutil Package -> Reads and validates integer-valued environment variables with bounds checking. +> Reads and validates environment variables with consistent default handling. ## Overview -The `envutil` package centralizes the pattern of reading integer-valued environment variables, validating them against configured minimum and maximum bounds, and falling back to a default value when the variable is absent or out of range. It emits warning messages to stderr when an invalid value is encountered, following the console formatting conventions of the rest of the codebase. +The `envutil` package centralizes the pattern of reading environment variables, validating typed values where needed, and falling back to a default value when the variable is absent or invalid. It emits warning messages to stderr when an invalid typed value is encountered, following the console formatting conventions of the rest of the codebase. -The package exposes a single generic helper, `GetIntFromEnv`, which encapsulates the repetitive read-parse-validate-default logic required wherever configurable integer parameters are exposed via environment variables. +The package exposes helpers for integer, boolean, and string values so callers can avoid scattered `os.Getenv` checks while preserving consistent defaulting and debug logging behavior. ## Public API @@ -15,6 +15,8 @@ The package exposes a single generic helper, `GetIntFromEnv`, which encapsulates | Function | Signature | Description | |----------|-----------|-------------| | `GetIntFromEnv` | `func(envVar string, defaultValue, minValue, maxValue int, debugLog *logger.Logger) int` | Reads an integer-valued environment variable, validates it, and returns a default when absent or invalid | +| `GetBoolFromEnv` | `func(envVar string, defaultValue bool, debugLog *logger.Logger) bool` | Reads a boolean-valued environment variable and returns a default when absent or invalid | +| `GetStringFromEnv` | `func(envVar, defaultValue string, debugLog *logger.Logger) string` | Reads a string-valued environment variable and returns a default when absent | #### `GetIntFromEnv` @@ -40,6 +42,46 @@ Reads `envVar` from the process environment, parses it as an integer, validates - SHOULD emit warnings formatted via `console.FormatWarningMessage` to `os.Stderr` when `debugLog` is `nil`. - MAY route warnings through `debugLog.Printf` when `debugLog` is non-nil instead of writing directly to stderr. +#### `GetBoolFromEnv` + +```go +func GetBoolFromEnv(envVar string, defaultValue bool, debugLog *logger.Logger) bool +``` + +Reads `envVar` from the process environment, parses it as a boolean using Go's standard `strconv.ParseBool` rules, and returns `defaultValue` when the variable is absent or unparseable. + +| Parameter | Type | Description | +|-----------|------|-------------| +| `envVar` | `string` | Environment variable name (e.g. `"CI"`) | +| `defaultValue` | `bool` | Value returned when env var is absent or invalid | +| `debugLog` | `*logger.Logger` | Optional logger for debug output; pass `nil` to disable | + +**Behavioral contract**: +- MUST return `defaultValue` when the environment variable is not set (empty string). +- MUST return `defaultValue` and emit a warning when the value cannot be parsed as a boolean. +- MUST log the accepted value via `debugLog` when `debugLog` is non-nil. +- SHOULD emit warnings formatted via `console.FormatWarningMessage` to `os.Stderr` when `debugLog` is `nil`. +- MAY route warnings through `debugLog.Printf` when `debugLog` is non-nil instead of writing directly to stderr. + +#### `GetStringFromEnv` + +```go +func GetStringFromEnv(envVar, defaultValue string, debugLog *logger.Logger) string +``` + +Reads `envVar` from the process environment and returns `defaultValue` when the variable is absent or empty. + +| Parameter | Type | Description | +|-----------|------|-------------| +| `envVar` | `string` | Environment variable name (e.g. `"GITHUB_TOKEN"`) | +| `defaultValue` | `string` | Value returned when env var is absent or empty | +| `debugLog` | `*logger.Logger` | Optional logger for debug output; pass `nil` to disable | + +**Behavioral contract**: +- MUST return `defaultValue` when the environment variable is not set (empty string). +- MUST return the environment variable value unchanged when it is non-empty. +- SHOULD log only the variable name, not the resolved value, via `debugLog` when `debugLog` is non-nil. + ## Usage Examples ```go @@ -53,19 +95,26 @@ var log = logger.New("mypackage:config") // Read GH_AW_MAX_CONCURRENT_DOWNLOADS, constrained to [1, 20], default 5 concurrency := envutil.GetIntFromEnv("GH_AW_MAX_CONCURRENT_DOWNLOADS", 5, 1, 20, log) +// Read CI, defaulting to false when unset or invalid +isCI := envutil.GetBoolFromEnv("CI", false, log) + +// Read GITHUB_TOKEN, defaulting to empty string when unset +token := envutil.GetStringFromEnv("GITHUB_TOKEN", "", log) + // Suppress debug output by passing nil logger timeout := envutil.GetIntFromEnv("GH_AW_TIMEOUT", 60, 1, 3600, nil) ``` ## Thread Safety -`GetIntFromEnv` is safe for concurrent use. It holds no shared mutable state; each invocation reads the process environment via `os.Getenv` and operates on function-local variables only. +`GetIntFromEnv`, `GetBoolFromEnv`, and `GetStringFromEnv` are safe for concurrent use. They hold no shared mutable state; each invocation reads the process environment via `os.Getenv` and operates on function-local variables only. ## Design Decisions - Warning messages use `console.FormatWarningMessage` so they render consistently in terminals. - All warnings go to `os.Stderr` to avoid polluting structured stdout output. -- The function handles integers only; floating-point or string env vars should be read directly via `os.Getenv`. +- Typed helpers use Go standard library parsing rules (`strconv.Atoi`, `strconv.ParseBool`) for predictable behavior. +- `GetStringFromEnv` logs only the variable name so secret-like values can be read without echoing their contents. - When `debugLog` is non-nil, warnings are routed through the logger rather than written directly to stderr, allowing callers to control output formatting. ## Dependencies diff --git a/pkg/envutil/envutil.go b/pkg/envutil/envutil.go index 50237c1bfcd..5f47f14f103 100644 --- a/pkg/envutil/envutil.go +++ b/pkg/envutil/envutil.go @@ -10,6 +10,14 @@ import ( "github.com/github/gh-aw/pkg/logger" ) +func warn(debugLog *logger.Logger, msg string) { + if debugLog != nil { + debugLog.Printf("WARNING: %s", msg) + return + } + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(msg)) +} + // GetIntFromEnv is a generic helper that reads an integer value from an environment variable, // validates it against min/max bounds, and returns a default value if invalid. // This follows the configuration helper pattern from pkg/workflow/config_helpers.go. @@ -28,14 +36,6 @@ import ( // // Invalid values trigger warning messages to stderr, or through the logger if provided. func GetIntFromEnv(envVar string, defaultValue, minValue, maxValue int, debugLog *logger.Logger) int { - warn := func(msg string) { - if debugLog != nil { - debugLog.Printf("WARNING: %s", msg) - } else { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(msg)) - } - } - envValue := os.Getenv(envVar) //nolint:osgetenvlibrary if envValue == "" { return defaultValue @@ -43,12 +43,12 @@ func GetIntFromEnv(envVar string, defaultValue, minValue, maxValue int, debugLog val, err := strconv.Atoi(envValue) if err != nil { - warn(fmt.Sprintf("Invalid %s value '%s' (must be a number), using default %d", envVar, envValue, defaultValue)) + warn(debugLog, fmt.Sprintf("Invalid %s value '%s' (must be a number), using default %d", envVar, envValue, defaultValue)) return defaultValue } if val < minValue || val > maxValue { - warn(fmt.Sprintf("%s value %d is out of bounds (must be %d-%d), using default %d", envVar, val, minValue, maxValue, defaultValue)) + warn(debugLog, fmt.Sprintf("%s value %d is out of bounds (must be %d-%d), using default %d", envVar, val, minValue, maxValue, defaultValue)) return defaultValue } @@ -57,3 +57,56 @@ func GetIntFromEnv(envVar string, defaultValue, minValue, maxValue int, debugLog } return val } + +// GetBoolFromEnv reads a boolean value from an environment variable and returns +// defaultValue if the variable is not set or cannot be parsed as a boolean. +// +// Parameters: +// - envVar: The environment variable name (e.g., "CI") +// - defaultValue: The default value to return if env var is not set or invalid +// - log: Optional logger for debug output +// +// Returns the parsed boolean value, or defaultValue if: +// - Environment variable is not set +// - Value cannot be parsed as a boolean +// +// Invalid values trigger warning messages to stderr, or through the logger if provided. +func GetBoolFromEnv(envVar string, defaultValue bool, debugLog *logger.Logger) bool { + envValue := os.Getenv(envVar) //nolint:osgetenvlibrary + if envValue == "" { + return defaultValue + } + + val, err := strconv.ParseBool(envValue) + if err != nil { + warn(debugLog, fmt.Sprintf("Invalid %s value '%s' (must be a boolean), using default %t", envVar, envValue, defaultValue)) + return defaultValue + } + + if debugLog != nil { + debugLog.Printf("Using %s=%t", envVar, val) + } + return val +} + +// GetStringFromEnv reads a string value from an environment variable and +// returns defaultValue if the variable is not set. +// +// Parameters: +// - envVar: The environment variable name (e.g., "GITHUB_TOKEN") +// - defaultValue: The default value to return if env var is not set +// - log: Optional logger for debug output +// +// Returns the environment variable value, or defaultValue when the variable is +// not set. Empty string values are treated the same as unset variables. +func GetStringFromEnv(envVar, defaultValue string, debugLog *logger.Logger) string { + envValue := os.Getenv(envVar) //nolint:osgetenvlibrary + if envValue == "" { + return defaultValue + } + + if debugLog != nil { + debugLog.Printf("Using %s from environment", envVar) + } + return envValue +} diff --git a/pkg/envutil/envutil_test.go b/pkg/envutil/envutil_test.go index bc16e727097..9820fea044e 100644 --- a/pkg/envutil/envutil_test.go +++ b/pkg/envutil/envutil_test.go @@ -377,6 +377,124 @@ func TestGetIntFromEnv_EmptyString(t *testing.T) { } } +func TestGetBoolFromEnv(t *testing.T) { + const testEnvVar = "GH_AW_TEST_BOOL_VALUE" + originalValue := os.Getenv(testEnvVar) + defer func() { + if originalValue != "" { + os.Setenv(testEnvVar, originalValue) + } else { + os.Unsetenv(testEnvVar) + } + }() + + tests := []struct { + name string + envValue string + defaultValue bool + expected bool + }{ + { + name: "default when env var not set", + envValue: "", + defaultValue: true, + expected: true, + }, + { + name: "valid true value", + envValue: "true", + defaultValue: false, + expected: true, + }, + { + name: "valid false value", + envValue: "false", + defaultValue: true, + expected: false, + }, + { + name: "numeric true value", + envValue: "1", + defaultValue: false, + expected: true, + }, + { + name: "invalid value", + envValue: "invalid", + defaultValue: true, + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.envValue != "" { + os.Setenv(testEnvVar, tt.envValue) + } else { + os.Unsetenv(testEnvVar) + } + + result := GetBoolFromEnv(testEnvVar, tt.defaultValue, nil) + if result != tt.expected { + t.Errorf("Expected %v, got %v", tt.expected, result) + } + }) + } +} + +func TestGetStringFromEnv(t *testing.T) { + const testEnvVar = "GH_AW_TEST_STRING_VALUE" + originalValue := os.Getenv(testEnvVar) + defer func() { + if originalValue != "" { + os.Setenv(testEnvVar, originalValue) + } else { + os.Unsetenv(testEnvVar) + } + }() + + tests := []struct { + name string + envValue string + defaultValue string + expected string + }{ + { + name: "default when env var not set", + envValue: "", + defaultValue: "fallback", + expected: "fallback", + }, + { + name: "default when env var empty", + envValue: "", + defaultValue: "fallback", + expected: "fallback", + }, + { + name: "returns env value", + envValue: "value", + defaultValue: "fallback", + expected: "value", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.name == "default when env var not set" { + os.Unsetenv(testEnvVar) + } else { + os.Setenv(testEnvVar, tt.envValue) + } + + result := GetStringFromEnv(testEnvVar, tt.defaultValue, nil) + if result != tt.expected { + t.Errorf("Expected %q, got %q", tt.expected, result) + } + }) + } +} + // Benchmark tests func BenchmarkGetIntFromEnv_ValidValue(b *testing.B) { diff --git a/pkg/envutil/spec_test.go b/pkg/envutil/spec_test.go index 3b4a31ad378..c9a41ca40ed 100644 --- a/pkg/envutil/spec_test.go +++ b/pkg/envutil/spec_test.go @@ -169,3 +169,91 @@ func TestSpec_PublicAPI_GetIntFromEnv_DocExample(t *testing.T) { assert.Equal(t, 8, result, "documented example: returns valid value within [1, 20]") } + +// TestSpec_PublicAPI_GetBoolFromEnv validates the documented behavior of +// GetBoolFromEnv as described in the package README.md. +func TestSpec_PublicAPI_GetBoolFromEnv_ReturnsDefault_WhenNotSet(t *testing.T) { + const envVar = "GH_AW_SPEC_TEST_BOOL_UNSET" + os.Unsetenv(envVar) + defer os.Unsetenv(envVar) + + result := GetBoolFromEnv(envVar, true, nil) + assert.True(t, result, + "GetBoolFromEnv should return defaultValue when env var is not set") +} + +func TestSpec_PublicAPI_GetBoolFromEnv_ReturnsDefault_WhenInvalid(t *testing.T) { + const envVar = "GH_AW_SPEC_TEST_BOOL_INVALID" + os.Setenv(envVar, "not-a-bool") + defer os.Unsetenv(envVar) + + result := GetBoolFromEnv(envVar, true, nil) + assert.True(t, result, + "GetBoolFromEnv should return defaultValue when value cannot be parsed as boolean") +} + +func TestSpec_PublicAPI_GetBoolFromEnv_ReturnsParsedValue(t *testing.T) { + const envVar = "GH_AW_SPEC_TEST_BOOL_VALID" + os.Setenv(envVar, "false") + defer os.Unsetenv(envVar) + + result := GetBoolFromEnv(envVar, true, nil) + assert.False(t, result, + "GetBoolFromEnv should return the parsed boolean value") +} + +func TestSpec_PublicAPI_GetBoolFromEnv_AcceptsNonNilLogger(t *testing.T) { + const envVar = "GH_AW_SPEC_TEST_BOOL_LOGGER" + os.Setenv(envVar, "true") + defer os.Unsetenv(envVar) + + log := logger.New("envutil:spec_test") + + assert.NotPanics(t, func() { + assert.True(t, GetBoolFromEnv(envVar, false, log)) + }, "GetBoolFromEnv should not panic when a non-nil logger is passed") +} + +// TestSpec_PublicAPI_GetStringFromEnv validates the documented behavior of +// GetStringFromEnv as described in the package README.md. +func TestSpec_PublicAPI_GetStringFromEnv_ReturnsDefault_WhenNotSet(t *testing.T) { + const envVar = "GH_AW_SPEC_TEST_STRING_UNSET" + os.Unsetenv(envVar) + defer os.Unsetenv(envVar) + + result := GetStringFromEnv(envVar, "fallback", nil) + assert.Equal(t, "fallback", result, + "GetStringFromEnv should return defaultValue when env var is not set") +} + +func TestSpec_PublicAPI_GetStringFromEnv_ReturnsDefault_WhenEmpty(t *testing.T) { + const envVar = "GH_AW_SPEC_TEST_STRING_EMPTY" + os.Setenv(envVar, "") + defer os.Unsetenv(envVar) + + result := GetStringFromEnv(envVar, "fallback", nil) + assert.Equal(t, "fallback", result, + "GetStringFromEnv should return defaultValue when env var is empty") +} + +func TestSpec_PublicAPI_GetStringFromEnv_ReturnsValue(t *testing.T) { + const envVar = "GH_AW_SPEC_TEST_STRING_VALUE" + os.Setenv(envVar, "value") + defer os.Unsetenv(envVar) + + result := GetStringFromEnv(envVar, "fallback", nil) + assert.Equal(t, "value", result, + "GetStringFromEnv should return the env var value when it is non-empty") +} + +func TestSpec_PublicAPI_GetStringFromEnv_AcceptsNonNilLogger(t *testing.T) { + const envVar = "GH_AW_SPEC_TEST_STRING_LOGGER" + os.Setenv(envVar, "token") + defer os.Unsetenv(envVar) + + log := logger.New("envutil:spec_test") + + assert.NotPanics(t, func() { + assert.Equal(t, "token", GetStringFromEnv(envVar, "", log)) + }, "GetStringFromEnv should not panic when a non-nil logger is passed") +} diff --git a/pkg/parser/github.go b/pkg/parser/github.go index e82ad0ae45c..9054f3592cb 100644 --- a/pkg/parser/github.go +++ b/pkg/parser/github.go @@ -10,6 +10,7 @@ import ( "strings" "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/envutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/stringutil" ) @@ -62,11 +63,11 @@ func GetGitHubToken() (string, error) { githubLog.Print("Getting GitHub token") // First try environment variable - if token := os.Getenv("GITHUB_TOKEN"); token != "" { //nolint:osgetenvlibrary + if token := envutil.GetStringFromEnv("GITHUB_TOKEN", "", githubLog); token != "" { githubLog.Print("Found GITHUB_TOKEN environment variable") return token, nil } - if token := os.Getenv("GH_TOKEN"); token != "" { //nolint:osgetenvlibrary + if token := envutil.GetStringFromEnv("GH_TOKEN", "", githubLog); token != "" { githubLog.Print("Found GH_TOKEN environment variable") return token, nil } diff --git a/pkg/parser/github_wasm.go b/pkg/parser/github_wasm.go index a91bd83a40f..44b011b8129 100644 --- a/pkg/parser/github_wasm.go +++ b/pkg/parser/github_wasm.go @@ -4,14 +4,15 @@ package parser import ( "errors" - "os" + + "github.com/github/gh-aw/pkg/envutil" ) func GetGitHubToken() (string, error) { - if token := os.Getenv("GITHUB_TOKEN"); token != "" { + if token := envutil.GetStringFromEnv("GITHUB_TOKEN", "", nil); token != "" { return token, nil } - if token := os.Getenv("GH_TOKEN"); token != "" { + if token := envutil.GetStringFromEnv("GH_TOKEN", "", nil); token != "" { return token, nil } return "", errors.New("GitHub token not available in Wasm (set GITHUB_TOKEN or GH_TOKEN environment variable)") From 10b0a85986a4b8bd61e141ddf9d42e03e83de291 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 7 Jul 2026 18:48:59 +0000 Subject: [PATCH 03/11] fix: preserve env flag semantics in cli helpers Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_interactive_orchestrator.go | 2 +- pkg/cli/codespace.go | 2 +- pkg/cli/interactive.go | 2 +- pkg/envutil/envutil_test.go | 10 +++++++--- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/pkg/cli/add_interactive_orchestrator.go b/pkg/cli/add_interactive_orchestrator.go index a23ac5e6053..5874a3f0bea 100644 --- a/pkg/cli/add_interactive_orchestrator.go +++ b/pkg/cli/add_interactive_orchestrator.go @@ -72,7 +72,7 @@ func RunAddInteractive(ctx context.Context, config *AddInteractiveConfig) error addInteractiveLog.Print("Starting interactive add workflow") // Assert this function is not running in automated unit tests or CI - if envutil.GetBoolFromEnv("GO_TEST_MODE", false, addInteractiveLog) || IsRunningInCI() { + if envutil.GetStringFromEnv("GO_TEST_MODE", "", addInteractiveLog) == "true" || IsRunningInCI() { return errors.New("interactive add cannot be used in automated tests or CI environments") } diff --git a/pkg/cli/codespace.go b/pkg/cli/codespace.go index 69c4cf2df1a..e74c6580a01 100644 --- a/pkg/cli/codespace.go +++ b/pkg/cli/codespace.go @@ -14,7 +14,7 @@ var codespaceLog = logger.New("cli:codespace") // by checking for the CODESPACES environment variable func isRunningInCodespace() bool { // GitHub Codespaces sets CODESPACES=true environment variable - isCodespace := envutil.GetBoolFromEnv("CODESPACES", false, codespaceLog) + isCodespace := strings.EqualFold(envutil.GetStringFromEnv("CODESPACES", "", codespaceLog), "true") codespaceLog.Printf("Codespace detection: is_codespace=%v", isCodespace) return isCodespace } diff --git a/pkg/cli/interactive.go b/pkg/cli/interactive.go index 2549ddd03de..d4df05cfb46 100644 --- a/pkg/cli/interactive.go +++ b/pkg/cli/interactive.go @@ -65,7 +65,7 @@ func CreateWorkflowInteractively(ctx context.Context, workflowName string, verbo interactiveLog.Printf("Starting interactive workflow creation: workflowName=%s, force=%v", workflowName, force) // Assert this function is not running in automated unit tests - if envutil.GetBoolFromEnv("GO_TEST_MODE", false, interactiveLog) || IsRunningInCI() { + if envutil.GetStringFromEnv("GO_TEST_MODE", "", interactiveLog) == "true" || IsRunningInCI() { return errors.New("interactive workflow creation cannot be used in automated tests or CI environments") } diff --git a/pkg/envutil/envutil_test.go b/pkg/envutil/envutil_test.go index 9820fea044e..f8264060641 100644 --- a/pkg/envutil/envutil_test.go +++ b/pkg/envutil/envutil_test.go @@ -455,24 +455,28 @@ func TestGetStringFromEnv(t *testing.T) { tests := []struct { name string + setEnv bool envValue string defaultValue string expected string }{ { name: "default when env var not set", + setEnv: false, envValue: "", defaultValue: "fallback", expected: "fallback", }, { name: "default when env var empty", + setEnv: true, envValue: "", defaultValue: "fallback", expected: "fallback", }, { name: "returns env value", + setEnv: true, envValue: "value", defaultValue: "fallback", expected: "value", @@ -481,10 +485,10 @@ func TestGetStringFromEnv(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if tt.name == "default when env var not set" { - os.Unsetenv(testEnvVar) - } else { + if tt.setEnv { os.Setenv(testEnvVar, tt.envValue) + } else { + os.Unsetenv(testEnvVar) } result := GetStringFromEnv(testEnvVar, tt.defaultValue, nil) From 8ed6cc287066670d1abc2304a96d4c758e0f410f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 7 Jul 2026 18:53:54 +0000 Subject: [PATCH 04/11] docs: clarify envutil warning helpers Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/envutil/README.md | 2 +- pkg/envutil/envutil.go | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/envutil/README.md b/pkg/envutil/README.md index 570e74966be..2ecc74c023b 100644 --- a/pkg/envutil/README.md +++ b/pkg/envutil/README.md @@ -114,7 +114,7 @@ timeout := envutil.GetIntFromEnv("GH_AW_TIMEOUT", 60, 1, 3600, nil) - Warning messages use `console.FormatWarningMessage` so they render consistently in terminals. - All warnings go to `os.Stderr` to avoid polluting structured stdout output. - Typed helpers use Go standard library parsing rules (`strconv.Atoi`, `strconv.ParseBool`) for predictable behavior. -- `GetStringFromEnv` logs only the variable name so secret-like values can be read without echoing their contents. +- `GetStringFromEnv` logs only that the variable was found, not its value, so secret-like values can be read without echoing their contents. - When `debugLog` is non-nil, warnings are routed through the logger rather than written directly to stderr, allowing callers to control output formatting. ## Dependencies diff --git a/pkg/envutil/envutil.go b/pkg/envutil/envutil.go index 5f47f14f103..e08af4f7353 100644 --- a/pkg/envutil/envutil.go +++ b/pkg/envutil/envutil.go @@ -10,6 +10,8 @@ import ( "github.com/github/gh-aw/pkg/logger" ) +// warn routes environment parsing warnings through the provided logger when +// available, or to stderr using the standard console warning format otherwise. func warn(debugLog *logger.Logger, msg string) { if debugLog != nil { debugLog.Printf("WARNING: %s", msg) From 707d2baef43ee69ad8456c480f2e27d4bc630faf Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 7 Jul 2026 18:58:06 +0000 Subject: [PATCH 05/11] docs: align envutil helper docs with code Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/envutil/README.md | 2 +- pkg/envutil/envutil.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/envutil/README.md b/pkg/envutil/README.md index 2ecc74c023b..38115352895 100644 --- a/pkg/envutil/README.md +++ b/pkg/envutil/README.md @@ -114,7 +114,7 @@ timeout := envutil.GetIntFromEnv("GH_AW_TIMEOUT", 60, 1, 3600, nil) - Warning messages use `console.FormatWarningMessage` so they render consistently in terminals. - All warnings go to `os.Stderr` to avoid polluting structured stdout output. - Typed helpers use Go standard library parsing rules (`strconv.Atoi`, `strconv.ParseBool`) for predictable behavior. -- `GetStringFromEnv` logs only that the variable was found, not its value, so secret-like values can be read without echoing their contents. +- `GetStringFromEnv` logs only the variable name when a value is found, never the value itself, so secret-like values can be read without echoing their contents. - When `debugLog` is non-nil, warnings are routed through the logger rather than written directly to stderr, allowing callers to control output formatting. ## Dependencies diff --git a/pkg/envutil/envutil.go b/pkg/envutil/envutil.go index e08af4f7353..5fa2a6cf6c0 100644 --- a/pkg/envutil/envutil.go +++ b/pkg/envutil/envutil.go @@ -29,7 +29,7 @@ func warn(debugLog *logger.Logger, msg string) { // - defaultValue: The default value to return if env var is not set or invalid // - minValue: Minimum allowed value (inclusive) // - maxValue: Maximum allowed value (inclusive) -// - log: Optional logger for debug output +// - debugLog: Optional logger for debug output // // Returns the parsed integer value, or defaultValue if: // - Environment variable is not set @@ -66,7 +66,7 @@ func GetIntFromEnv(envVar string, defaultValue, minValue, maxValue int, debugLog // Parameters: // - envVar: The environment variable name (e.g., "CI") // - defaultValue: The default value to return if env var is not set or invalid -// - log: Optional logger for debug output +// - debugLog: Optional logger for debug output // // Returns the parsed boolean value, or defaultValue if: // - Environment variable is not set @@ -97,7 +97,7 @@ func GetBoolFromEnv(envVar string, defaultValue bool, debugLog *logger.Logger) b // Parameters: // - envVar: The environment variable name (e.g., "GITHUB_TOKEN") // - defaultValue: The default value to return if env var is not set -// - log: Optional logger for debug output +// - debugLog: Optional logger for debug output // // Returns the environment variable value, or defaultValue when the variable is // not set. Empty string values are treated the same as unset variables. From eb0563db8adbde82d25df646a70d54c224bb9e35 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 7 Jul 2026 19:02:17 +0000 Subject: [PATCH 06/11] refactor: use bool helper for GO_TEST_MODE Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_interactive_orchestrator.go | 2 +- pkg/cli/interactive.go | 2 +- pkg/envutil/README.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/cli/add_interactive_orchestrator.go b/pkg/cli/add_interactive_orchestrator.go index 5874a3f0bea..a23ac5e6053 100644 --- a/pkg/cli/add_interactive_orchestrator.go +++ b/pkg/cli/add_interactive_orchestrator.go @@ -72,7 +72,7 @@ func RunAddInteractive(ctx context.Context, config *AddInteractiveConfig) error addInteractiveLog.Print("Starting interactive add workflow") // Assert this function is not running in automated unit tests or CI - if envutil.GetStringFromEnv("GO_TEST_MODE", "", addInteractiveLog) == "true" || IsRunningInCI() { + if envutil.GetBoolFromEnv("GO_TEST_MODE", false, addInteractiveLog) || IsRunningInCI() { return errors.New("interactive add cannot be used in automated tests or CI environments") } diff --git a/pkg/cli/interactive.go b/pkg/cli/interactive.go index d4df05cfb46..2549ddd03de 100644 --- a/pkg/cli/interactive.go +++ b/pkg/cli/interactive.go @@ -65,7 +65,7 @@ func CreateWorkflowInteractively(ctx context.Context, workflowName string, verbo interactiveLog.Printf("Starting interactive workflow creation: workflowName=%s, force=%v", workflowName, force) // Assert this function is not running in automated unit tests - if envutil.GetStringFromEnv("GO_TEST_MODE", "", interactiveLog) == "true" || IsRunningInCI() { + if envutil.GetBoolFromEnv("GO_TEST_MODE", false, interactiveLog) || IsRunningInCI() { return errors.New("interactive workflow creation cannot be used in automated tests or CI environments") } diff --git a/pkg/envutil/README.md b/pkg/envutil/README.md index 38115352895..2f06a915108 100644 --- a/pkg/envutil/README.md +++ b/pkg/envutil/README.md @@ -80,7 +80,7 @@ Reads `envVar` from the process environment and returns `defaultValue` when the **Behavioral contract**: - MUST return `defaultValue` when the environment variable is not set (empty string). - MUST return the environment variable value unchanged when it is non-empty. -- SHOULD log only the variable name, not the resolved value, via `debugLog` when `debugLog` is non-nil. +- SHOULD log only that the variable was found, without logging its value, via `debugLog` when `debugLog` is non-nil. ## Usage Examples From 0bc8407911af7be3a1365161d2d39dc7b08ff375 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 7 Jul 2026 19:07:43 +0000 Subject: [PATCH 07/11] docs: clarify envutil helper intent Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_interactive_orchestrator.go | 2 ++ pkg/cli/interactive.go | 2 ++ pkg/envutil/README.md | 2 +- pkg/envutil/envutil.go | 3 ++- 4 files changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/cli/add_interactive_orchestrator.go b/pkg/cli/add_interactive_orchestrator.go index a23ac5e6053..c9c4309deb6 100644 --- a/pkg/cli/add_interactive_orchestrator.go +++ b/pkg/cli/add_interactive_orchestrator.go @@ -72,6 +72,8 @@ func RunAddInteractive(ctx context.Context, config *AddInteractiveConfig) error addInteractiveLog.Print("Starting interactive add workflow") // Assert this function is not running in automated unit tests or CI + // GO_TEST_MODE intentionally uses GetBoolFromEnv so common boolean spellings + // are treated consistently across test and automation environments. if envutil.GetBoolFromEnv("GO_TEST_MODE", false, addInteractiveLog) || IsRunningInCI() { return errors.New("interactive add cannot be used in automated tests or CI environments") } diff --git a/pkg/cli/interactive.go b/pkg/cli/interactive.go index 2549ddd03de..cf2920f7185 100644 --- a/pkg/cli/interactive.go +++ b/pkg/cli/interactive.go @@ -65,6 +65,8 @@ func CreateWorkflowInteractively(ctx context.Context, workflowName string, verbo interactiveLog.Printf("Starting interactive workflow creation: workflowName=%s, force=%v", workflowName, force) // Assert this function is not running in automated unit tests + // GO_TEST_MODE intentionally uses GetBoolFromEnv so common boolean spellings + // are treated consistently across test and automation environments. if envutil.GetBoolFromEnv("GO_TEST_MODE", false, interactiveLog) || IsRunningInCI() { return errors.New("interactive workflow creation cannot be used in automated tests or CI environments") } diff --git a/pkg/envutil/README.md b/pkg/envutil/README.md index 2f06a915108..56667a399c3 100644 --- a/pkg/envutil/README.md +++ b/pkg/envutil/README.md @@ -114,7 +114,7 @@ timeout := envutil.GetIntFromEnv("GH_AW_TIMEOUT", 60, 1, 3600, nil) - Warning messages use `console.FormatWarningMessage` so they render consistently in terminals. - All warnings go to `os.Stderr` to avoid polluting structured stdout output. - Typed helpers use Go standard library parsing rules (`strconv.Atoi`, `strconv.ParseBool`) for predictable behavior. -- `GetStringFromEnv` logs only the variable name when a value is found, never the value itself, so secret-like values can be read without echoing their contents. +- `GetStringFromEnv` logs only that the variable was found from the environment, never the value itself, so secret-like values can be read without echoing their contents. - When `debugLog` is non-nil, warnings are routed through the logger rather than written directly to stderr, allowing callers to control output formatting. ## Dependencies diff --git a/pkg/envutil/envutil.go b/pkg/envutil/envutil.go index 5fa2a6cf6c0..1b3a07ce573 100644 --- a/pkg/envutil/envutil.go +++ b/pkg/envutil/envutil.go @@ -10,7 +10,8 @@ import ( "github.com/github/gh-aw/pkg/logger" ) -// warn routes environment parsing warnings through the provided logger when +// warn is an internal helper shared by GetIntFromEnv and GetBoolFromEnv. It +// routes environment parsing warnings through the provided logger when // available, or to stderr using the standard console warning format otherwise. func warn(debugLog *logger.Logger, msg string) { if debugLog != nil { From 17e0b79491d8d926074cb90f32c8a1432a7a71d0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 7 Jul 2026 19:12:08 +0000 Subject: [PATCH 08/11] docs: clarify string env logging behavior Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/envutil/README.md | 2 +- pkg/parser/github_wasm.go | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/envutil/README.md b/pkg/envutil/README.md index 56667a399c3..0275de17c2e 100644 --- a/pkg/envutil/README.md +++ b/pkg/envutil/README.md @@ -114,7 +114,7 @@ timeout := envutil.GetIntFromEnv("GH_AW_TIMEOUT", 60, 1, 3600, nil) - Warning messages use `console.FormatWarningMessage` so they render consistently in terminals. - All warnings go to `os.Stderr` to avoid polluting structured stdout output. - Typed helpers use Go standard library parsing rules (`strconv.Atoi`, `strconv.ParseBool`) for predictable behavior. -- `GetStringFromEnv` logs only that the variable was found from the environment, never the value itself, so secret-like values can be read without echoing their contents. +- `GetStringFromEnv` logs only the variable name when a value is found from the environment, never the value itself, so secret-like values can be read without echoing their contents. - When `debugLog` is non-nil, warnings are routed through the logger rather than written directly to stderr, allowing callers to control output formatting. ## Dependencies diff --git a/pkg/parser/github_wasm.go b/pkg/parser/github_wasm.go index 44b011b8129..a2f260a40ac 100644 --- a/pkg/parser/github_wasm.go +++ b/pkg/parser/github_wasm.go @@ -9,6 +9,8 @@ import ( ) func GetGitHubToken() (string, error) { + // Wasm callers do not use the package logger, so pass nil to suppress debug + // logging while still centralizing environment variable access. if token := envutil.GetStringFromEnv("GITHUB_TOKEN", "", nil); token != "" { return token, nil } From a3ea34f01a7efa1a2a589b96be032e67a2b7d94d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 7 Jul 2026 19:16:27 +0000 Subject: [PATCH 09/11] docs: explain env helper log semantics Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/add_interactive_orchestrator.go | 5 +++-- pkg/cli/interactive.go | 5 +++-- pkg/envutil/README.md | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/pkg/cli/add_interactive_orchestrator.go b/pkg/cli/add_interactive_orchestrator.go index c9c4309deb6..7becae55270 100644 --- a/pkg/cli/add_interactive_orchestrator.go +++ b/pkg/cli/add_interactive_orchestrator.go @@ -71,9 +71,10 @@ type AddInteractiveConfig struct { func RunAddInteractive(ctx context.Context, config *AddInteractiveConfig) error { addInteractiveLog.Print("Starting interactive add workflow") - // Assert this function is not running in automated unit tests or CI + // Assert this function is not running in automated unit tests or CI. // GO_TEST_MODE intentionally uses GetBoolFromEnv so common boolean spellings - // are treated consistently across test and automation environments. + // are treated consistently across test and automation environments, while + // IsRunningInCI centralizes the broader CI environment detection logic. if envutil.GetBoolFromEnv("GO_TEST_MODE", false, addInteractiveLog) || IsRunningInCI() { return errors.New("interactive add cannot be used in automated tests or CI environments") } diff --git a/pkg/cli/interactive.go b/pkg/cli/interactive.go index cf2920f7185..ed0c8a38d9d 100644 --- a/pkg/cli/interactive.go +++ b/pkg/cli/interactive.go @@ -64,9 +64,10 @@ func (b *InteractiveWorkflowBuilder) ensureNonTTYScanner(r io.Reader) *bufio.Sca func CreateWorkflowInteractively(ctx context.Context, workflowName string, verbose bool, force bool) error { interactiveLog.Printf("Starting interactive workflow creation: workflowName=%s, force=%v", workflowName, force) - // Assert this function is not running in automated unit tests + // Assert this function is not running in automated unit tests or CI. // GO_TEST_MODE intentionally uses GetBoolFromEnv so common boolean spellings - // are treated consistently across test and automation environments. + // are treated consistently across test and automation environments, while + // IsRunningInCI centralizes the broader CI environment detection logic. if envutil.GetBoolFromEnv("GO_TEST_MODE", false, interactiveLog) || IsRunningInCI() { return errors.New("interactive workflow creation cannot be used in automated tests or CI environments") } diff --git a/pkg/envutil/README.md b/pkg/envutil/README.md index 0275de17c2e..226097d9423 100644 --- a/pkg/envutil/README.md +++ b/pkg/envutil/README.md @@ -114,7 +114,7 @@ timeout := envutil.GetIntFromEnv("GH_AW_TIMEOUT", 60, 1, 3600, nil) - Warning messages use `console.FormatWarningMessage` so they render consistently in terminals. - All warnings go to `os.Stderr` to avoid polluting structured stdout output. - Typed helpers use Go standard library parsing rules (`strconv.Atoi`, `strconv.ParseBool`) for predictable behavior. -- `GetStringFromEnv` logs only the variable name when a value is found from the environment, never the value itself, so secret-like values can be read without echoing their contents. +- `GetStringFromEnv` logs a message of the form `Using ENV_VAR from environment` when a value is found, never the value itself, so secret-like values can be read without echoing their contents. - When `debugLog` is non-nil, warnings are routed through the logger rather than written directly to stderr, allowing callers to control output formatting. ## Dependencies From f0eb966b304a5f56df5f42d8bab8ad23a2763236 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 8 Jul 2026 01:58:24 +0000 Subject: [PATCH 10/11] docs(adr): add draft ADR-44097 for extending envutil with typed env helpers Co-Authored-By: Claude Sonnet 4.6 --- ...097-extend-envutil-for-typed-env-access.md | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 docs/adr/44097-extend-envutil-for-typed-env-access.md diff --git a/docs/adr/44097-extend-envutil-for-typed-env-access.md b/docs/adr/44097-extend-envutil-for-typed-env-access.md new file mode 100644 index 00000000000..a4dfdb541da --- /dev/null +++ b/docs/adr/44097-extend-envutil-for-typed-env-access.md @@ -0,0 +1,47 @@ +# ADR-44097: Extend envutil with Typed Helpers for Boolean and String Environment Variables + +**Date**: 2026-07-08 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +The codebase has a convention of reading environment variables through `pkg/envutil.GetIntFromEnv`, which centralizes defaulting, bounds-validation, and debug logging for integer-typed env vars. However, boolean and string env vars in `pkg/cli` and `pkg/parser` were still read via direct `os.Getenv` calls, suppressed with `//nolint:osgetenvlibrary` annotations. This produced inconsistent behavior: CI detection was replicated inline across three call sites in `pkg/cli` (`os.Getenv("CI") != ""`), `GO_TEST_MODE` was compared as a raw string rather than using Go's canonical boolean parsing, and `CODESPACES` used `strings.EqualFold` on a raw string rather than a shared accessor. Sensitive tokens (`GITHUB_TOKEN`, `GH_TOKEN`) were read directly without debug-safe logging. The growing number of exemptions signalled that the existing helper surface was incomplete for the types of env vars the codebase actually uses. + +### Decision + +We will add `GetBoolFromEnv` and `GetStringFromEnv` helpers to `pkg/envutil`, consistent in signature and behavior with the existing `GetIntFromEnv`, and migrate all direct `os.Getenv` calls in `pkg/cli` and `pkg/parser` to use these helpers or the existing `IsRunningInCI()` / `isRunningInCodespace()` wrapper functions that now delegate to them. The shared internal `warn` function extracted in this PR eliminates duplicated warning routing code across all three typed helpers. + +### Alternatives Considered + +#### Alternative 1: Retain Direct `os.Getenv` with Nolint Suppressions + +Continue using raw `os.Getenv` at each call site with `//nolint:osgetenvlibrary` to satisfy the linter. Each call site would keep its own string-comparison, defaulting, and logging logic. This avoids introducing a new abstraction, but perpetuates inconsistency: `GO_TEST_MODE == "true"` rejects `"1"`, `"TRUE"`, and `"yes"` as truthy, while the rest of the codebase would gain consistent boolean parsing via `strconv.ParseBool`. It also makes debug-safe logging of sensitive env values (never log the value itself) opt-in per call site rather than the default. + +#### Alternative 2: Dependency-Injection of an Environment Reader Interface + +Introduce an `EnvReader` interface and inject it into all consumers, allowing env access to be mocked in tests without `os.Setenv`. This would improve test isolation for packages that currently must manipulate real process environment state. However, it is a significantly larger change, requires threading the interface through many function signatures, and conflicts with the existing `envutil` "static helper" design that callers already adopt. The present PR's test coverage uses `os.Setenv`/`os.Unsetenv` with deferred cleanup, which is adequate for the current test surface. + +### Consequences + +#### Positive +- `GetBoolFromEnv` accepts all spellings recognised by `strconv.ParseBool` (`"1"`, `"t"`, `"TRUE"`, etc.), making boolean env var handling consistent with Go conventions across the codebase. +- `GetStringFromEnv` logs `Using ENV_VAR from environment` rather than the value itself, making it safe to use with secret-like variables (`GITHUB_TOKEN`, `GH_TOKEN`) without risk of echoing credentials in debug output. +- Removing `//nolint:osgetenvlibrary` suppressions from `pkg/cli` and `pkg/parser` means the linter now enforces the centralized pattern without human-reviewed exceptions. +- The shared `warn` helper in `envutil.go` eliminates a duplicated closure that was previously inlined inside `GetIntFromEnv`, reducing surface area for divergence. + +#### Negative +- `GetStringFromEnv` treats an empty string value identically to an unset variable and returns `defaultValue` in both cases. Callers that need to distinguish between `VAR=""` (explicitly blank) and `VAR` unset cannot use this helper and must call `os.Getenv` directly. +- `GetStringFromEnv` does not emit a warning when the env var is absent (unlike `GetIntFromEnv` and `GetBoolFromEnv` for invalid values), so there is no diagnostic signal when an expected variable is missing. +- Adding two new exported functions to `pkg/envutil` slightly grows the stable API surface that future refactors must maintain. + +#### Neutral +- The `warn` helper is package-private; callers outside `pkg/envutil` cannot reuse it for their own typed-parsing scenarios. +- Boolean env vars that previously used non-canonical comparisons (e.g., `os.Getenv("CI") != ""`) now use `strconv.ParseBool`, which does *not* treat a non-empty non-boolean string as truthy — a subtle behavioral change that is intentional and documented in the PR. +- All three new and existing helpers share the same `debugLog *logger.Logger` optional parameter convention; passing `nil` suppresses structured logging and falls back to `os.Stderr` warnings. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* From 8d278fe9d3d01c322adf81f3cd15fc6dd6419e3f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 8 Jul 2026 02:10:30 +0000 Subject: [PATCH 11/11] fix review follow-up on env helpers Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/cli/codespace.go | 2 +- pkg/envutil/envutil_test.go | 8 ++++---- pkg/parser/github.go | 2 -- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/pkg/cli/codespace.go b/pkg/cli/codespace.go index e74c6580a01..718ab38e2c1 100644 --- a/pkg/cli/codespace.go +++ b/pkg/cli/codespace.go @@ -14,7 +14,7 @@ var codespaceLog = logger.New("cli:codespace") // by checking for the CODESPACES environment variable func isRunningInCodespace() bool { // GitHub Codespaces sets CODESPACES=true environment variable - isCodespace := strings.EqualFold(envutil.GetStringFromEnv("CODESPACES", "", codespaceLog), "true") + isCodespace := strings.EqualFold(envutil.GetStringFromEnv("CODESPACES", "", nil), "true") codespaceLog.Printf("Codespace detection: is_codespace=%v", isCodespace) return isCodespace } diff --git a/pkg/envutil/envutil_test.go b/pkg/envutil/envutil_test.go index f8264060641..bff708c4858 100644 --- a/pkg/envutil/envutil_test.go +++ b/pkg/envutil/envutil_test.go @@ -379,9 +379,9 @@ func TestGetIntFromEnv_EmptyString(t *testing.T) { func TestGetBoolFromEnv(t *testing.T) { const testEnvVar = "GH_AW_TEST_BOOL_VALUE" - originalValue := os.Getenv(testEnvVar) + originalValue, hadOriginalValue := os.LookupEnv(testEnvVar) defer func() { - if originalValue != "" { + if hadOriginalValue { os.Setenv(testEnvVar, originalValue) } else { os.Unsetenv(testEnvVar) @@ -444,9 +444,9 @@ func TestGetBoolFromEnv(t *testing.T) { func TestGetStringFromEnv(t *testing.T) { const testEnvVar = "GH_AW_TEST_STRING_VALUE" - originalValue := os.Getenv(testEnvVar) + originalValue, hadOriginalValue := os.LookupEnv(testEnvVar) defer func() { - if originalValue != "" { + if hadOriginalValue { os.Setenv(testEnvVar, originalValue) } else { os.Unsetenv(testEnvVar) diff --git a/pkg/parser/github.go b/pkg/parser/github.go index 9054f3592cb..0140dfd31cd 100644 --- a/pkg/parser/github.go +++ b/pkg/parser/github.go @@ -64,11 +64,9 @@ func GetGitHubToken() (string, error) { // First try environment variable if token := envutil.GetStringFromEnv("GITHUB_TOKEN", "", githubLog); token != "" { - githubLog.Print("Found GITHUB_TOKEN environment variable") return token, nil } if token := envutil.GetStringFromEnv("GH_TOKEN", "", githubLog); token != "" { - githubLog.Print("Found GH_TOKEN environment variable") return token, nil }