Extend envutil for bool/string access and remove direct CI bypasses

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.*

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"
 )
@@ -70,8 +71,11 @@ 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
-	if os.Getenv("GO_TEST_MODE") == "true" || os.Getenv("CI") != "" { //nolint:osgetenvlibrary
+	// 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, 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")
 	}
 

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"))

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")

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 := strings.EqualFold(envutil.GetStringFromEnv("CODESPACES", "", nil), "true")
 	codespaceLog.Printf("Codespace detection: is_codespace=%v", isCodespace)
 	return isCodespace
 }

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"
@@ -63,8 +64,11 @@ 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
-	if os.Getenv("GO_TEST_MODE") == "true" || os.Getenv("CI") != "" { //nolint:osgetenvlibrary
+	// 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, 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")
 	}
 

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 that the variable was found, without logging its 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 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

pkg/envutil/envutil.go

@@ -10,6 +10,17 @@ import (
 	"github.com/github/gh-aw/pkg/logger"
 )
 
+// 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 {
+		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.
@@ -19,7 +30,7 @@ import (
 //   - 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
@@ -28,27 +39,19 @@ 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
 	}
 
 	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 +60,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
+//   - debugLog: 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
+//   - 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.
+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
+}