Extend envutil for bool/string access and remove direct CI bypasses#44097
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 Triage Assessment
Summary: Adds GetBoolFromEnv and GetStringFromEnv to envutil with tests. Draft (+342/−27, 11 files). No CI triggered. Next: Convert to ready-for-review, trigger CI.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
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
Extends pkg/envutil to provide centralized boolean and string environment variable accessors (alongside the existing int helper), then migrates remaining callsites away from direct os.Getenv usage—especially around CLI CI/Codespaces detection and GitHub token lookup.
Changes:
- Add
GetBoolFromEnvandGetStringFromEnvtopkg/envutil, sharing warning/logging behavior with existing helpers. - Replace remaining direct env checks in parser + CLI codepaths with
envutilhelpers andcli.IsRunningInCI(). - Update
pkg/envutilpackage spec/docs and add unit/spec coverage for the new helpers.
Show a summary per file
| File | Description |
|---|---|
| pkg/envutil/envutil.go | Adds GetBoolFromEnv/GetStringFromEnv and shared warning helper. |
| pkg/envutil/README.md | Documents new helpers and their logging/defaulting contracts. |
| pkg/envutil/spec_test.go | Adds spec-style tests for bool/string documented behavior. |
| pkg/envutil/envutil_test.go | Adds unit tests for bool/string helpers. |
| pkg/parser/github.go | Routes token env lookup through envutil.GetStringFromEnv. |
| pkg/parser/github_wasm.go | Uses envutil.GetStringFromEnv for wasm token lookup (nil logger). |
| pkg/cli/ci-related callsites (interactive.go, add_wizard_command.go, add_interactive_orchestrator.go) | Removes direct CI getenv checks in favor of IsRunningInCI() and uses GetBoolFromEnv for GO_TEST_MODE. |
| pkg/cli/codespace.go | Centralizes Codespaces env access via GetStringFromEnv. |
| pkg/cli/add_interactive_workflow.go | Uses centralized Codespaces detection helper. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 11/11 changed files
- Comments generated: 4
- Review effort level: Low
| 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 |
| originalValue := os.Getenv(testEnvVar) | ||
| defer func() { | ||
| if originalValue != "" { | ||
| os.Setenv(testEnvVar, originalValue) | ||
| } else { | ||
| os.Unsetenv(testEnvVar) | ||
| } | ||
| }() |
| originalValue := os.Getenv(testEnvVar) | ||
| defer func() { | ||
| if originalValue != "" { | ||
| os.Setenv(testEnvVar, originalValue) | ||
| } else { | ||
| os.Unsetenv(testEnvVar) | ||
| } | ||
| }() |
| // GitHub Codespaces sets CODESPACES=true environment variable | ||
| isCodespace := strings.EqualFold(os.Getenv("CODESPACES"), "true") //nolint:osgetenvlibrary | ||
| isCodespace := strings.EqualFold(envutil.GetStringFromEnv("CODESPACES", "", codespaceLog), "true") | ||
| codespaceLog.Printf("Codespace detection: is_codespace=%v", isCodespace) |
|
✅ 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. ✅ |
|
@copilot please run the
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 96/100 — Excellent
📊 Metrics (26 tests)
Test Classification
✅ VerdictPASS — 7.7% implementation tests (threshold: 30%). Exceptional quality indicators:
This PR demonstrates expert test design: the split between exploratory table-driven tests (envutil_test.go) and formal specification tests (spec_test.go) creates a defense-in-depth testing strategy that will catch regressions and document the public API contract. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
…elpers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (356 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. ❓ 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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on two design consistency issues and one test coverage gap.
📋 Key Themes & Highlights
Key Themes
- Boolean env var handled as string (
codespace.go):CODESPACESis a boolean env var but is read viaGetStringFromEnv+strings.EqualFold, inconsistent with the newGetBoolFromEnvhelper used forGO_TEST_MODE. This is the most important fix — it breaks the PR's own consistency goal. - Double-logging (
parser/github.go):GetStringFromEnvalready logs via the provided logger; the caller also logs immediately after, producing two log lines per token lookup. - Missing warning-path test for logger:
TestGetBoolFromEnvonly exercises thenil-logger path; thedebugLog != nil+ invalid-value warning branch is untested. IsRunningInCIitself still uses rawos.Getenv: minor follow-up opportunity to complete the centralization story inci.go.- README MUST-bullet for
GetStringFromEnvomits the empty-string case in the contract description.
Positive Highlights
- ✅ Clean extraction of the shared
warn()helper — removes duplication betweenGetIntFromEnvandGetBoolFromEnv - ✅ Consistent
strconv.ParseBoolsemantics (handles1,t,TRUE, etc.) align with Go idiom - ✅ Secret-safe logging design for
GetStringFromEnv(logs variable name only, never value) — well thought-through - ✅ Good test/spec coverage for the happy paths and default cases
- ✅ README updates are thorough and match the behavioral contracts
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 105 AIC · ⌖ 5.55 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/cli/codespace.go:17
[/codebase-design] CODESPACES is a boolean env var — using GetStringFromEnv + strings.EqualFold bypasses GetBoolFromEnv, breaking consistency with GO_TEST_MODE and silently rejecting valid spellings like "1" or "TRUE" that strconv.ParseBool handles.
<details>
<summary>💡 Suggested fix</summary>
Replace the string comparison with the centralised bool helper:
func isRunningInCodespace() bool {
isCodespace := envutil.GetBoolFromEnv("CODESPACES", false, codespaceLog)
…
</details>
<details><summary>pkg/parser/github.go:67</summary>
**[/codebase-design]** Double-logging on the success path: `GetStringFromEnv` already logs `Using GITHUB_TOKEN from environment` via `githubLog`, then line 67 logs `Found GITHUB_TOKEN environment variable`. The caller gets two log lines for the same event.
<details>
<summary>💡 Suggested fix</summary>
Remove the now-redundant explicit log lines (67 and 71) and let `GetStringFromEnv` provide the single log signal:
```go
if token := envutil.GetStringFromEnv("GITHUB_TOKEN", "", githubLog); toke…
</details>
<details><summary>pkg/cli/add_interactive_orchestrator.go:23</summary>
**[/codebase-design]** `IsRunningInCI` still calls raw `os.Getenv` for `CI`, `CONTINUOUS_INTEGRATION`, and `GITHUB_ACTIONS` (see `pkg/cli/ci.go`), meaning this PR centralizes some call sites but leaves the underlying `ci.go` function un-centralized. The refactor is incomplete — a future change could migrate `ci.go` to use `GetBoolFromEnv` for all three vars to close the gap.
<details>
<summary>💡 Context</summary>
Not a blocking issue for this PR since `ci.go` was pre-existing, but worth noti…
</details>
<details><summary>pkg/envutil/envutil_test.go:437</summary>
**[/tdd]** `TestGetBoolFromEnv` only passes `nil` as the logger — the warning-emission code path (when `debugLog != nil` and the value is invalid) is never exercised. A logger-with-invalid-value test would confirm that warnings route through `debugLog.Printf` rather than to stderr.
<details>
<summary>💡 Suggested test case</summary>
```go
{
name: "invalid value with logger",
envValue: "invalid",
defaultValue: true,
expected: true,
// pass a real logger and …
</details>
<details><summary>pkg/envutil/README.md:178</summary>
**[/codebase-design]** The documented contract says `GetStringFromEnv` returns `defaultValue` when the env var is *absent or empty*, but the comment only mentions "not set". The README text at line 168 (`Returns defaultValue when the variable is absent or empty`) is accurate, but the MUST bullet on this line (`when the environment variable is not set`) omits the empty-string case — a reader relying on the spec could miss that `CODESPACES=""` triggers the default path.
<details>
<summary>💡 Sug…
</details>
There was a problem hiding this comment.
Review Summary
The envutil API additions (GetBoolFromEnv, GetStringFromEnv) and the CI-detection centralisation are solid. The refactor is well-tested and the documentation is clear. Two correctness issues and one minor inaccuracy need addressing before merge.
Blocking:
- Double logging in
parser/github.go—GetStringFromEnvalready logs via the passeddebugLog, so the caller-sidegithubLog.Print("Found ... environment variable")produces a duplicate line for every token lookup. Remove the redundant log statements (or passniltoGetStringFromEnv). isRunningInCodespaceshould useGetBoolFromEnv— the migration fromos.Getenv(...)toGetStringFromEnvstill wraps the result instrings.EqualFold(..., "true"), which missesCODESPACES=1,CODESPACES=T, etc. UseGetBoolFromEnv("CODESPACES", false, codespaceLog)to be consistent with the boolean pattern this PR establishes.
Non-blocking:
3. warn docstring — the comment lists only GetIntFromEnv and GetBoolFromEnv as users, which is accurate; just make it explicit that GetStringFromEnv intentionally does not call warn to avoid future confusion.
Overall assessment
- API design: ✅ Clean, consistent with existing
GetIntFromEnv - Test coverage: ✅ Good unit + spec coverage including logger path and invalid values
- Documentation: ✅ README updated with correct behavioral contracts
- CI bypass removal: ✅ All three direct
os.Getenv("CI")checks replaced withIsRunningInCI() GO_TEST_MODEmigration: ✅ Correct use ofGetBoolFromEnv- Wasm path: ✅
nillogger is intentional and documented
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 100.8 AIC · ⌖ 6.24 AIC · ⊞ 4.8K
Comments that could not be inline-anchored
pkg/parser/github.go:67
Redundant log after GetStringFromEnv
GetStringFromEnv with a non-nil debugLog already emits "Using GITHUB_TOKEN from environment" (see envutil.go line 326). The call on this line then emits a second log: "Found GITHUB_TOKEN environment variable". The same duplication occurs for GH_TOKEN at line 71. Either remove both caller-side githubLog.Print("Found ... environment variable") statements, or pass nil instead of githubLog to GetStringFromEnv here and log only once at …
pkg/cli/codespace.go:17
isRunningInCodespace should use GetBoolFromEnv for consistency
This PR introduces GetBoolFromEnv specifically to replace ad-hoc boolean env var checks. isRunningInCodespace was migrated from strings.EqualFold(os.Getenv(...), "true") to strings.EqualFold(GetStringFromEnv(...), "true"), but that still does not recognise the other values accepted by strconv.ParseBool (1, T, TRUE, etc.). The equivalent with the new helper would be:
func isRunningInCodespace() bool {
…
</details>
<details><summary>pkg/envutil/envutil.go:221</summary>
**Warning comment in `warn` docstring is inaccurate — `GetStringFromEnv` does not call `warn`**
The function comment says the helper is "shared by `GetIntFromEnv` and `GetBoolFromEnv`". That is correct, but it is worth noting explicitly (or updating the comment) that `GetStringFromEnv` does **not** use `warn` (string values cannot be "invalid"). This comment is a minor accuracy gap, but if `GetStringFromEnv` is later extended to emit a warning for some case, a reader might be misled into think…
</details>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the unresolved review feedback in 8d278fe: removed the duplicate token/Codespaces env logs and fixed the new envutil tests to restore unset vs empty env vars correctly. Local validation passed ( |
|
🎉 This pull request is included in a new release. Release: |
This change completes more of the
os.Getenvcentralization work by adding bool and string helpers topkg/envutil, then using them in the remaining related callsites. It also removes same-packageCIbypasses inpkg/cliso CI detection flows throughIsRunningInCI()consistently.envutil API
GetBoolFromEnv(envVar string, defaultValue bool, debugLog *logger.Logger) boolGetStringFromEnv(envVar, defaultValue string, debugLog *logger.Logger) stringCLI CI detection
os.Getenv("CI")checks inpkg/cliwithIsRunningInCI()GO_TEST_MODEandCODESPACESaccess on envutil-backed paths while preserving existing behavior where it mattersRelated env access cleanup
GetStringFromEnvos.GetenvSpec and package docs
pkg/envutilREADME to document the new helpers alongsideGetIntFromEnvExample: