fix: resolve osgetenvlibrary lint findings in boundary-layer packages#42163
Conversation
Resolves 13 environment-coupling findings from the osgetenvlibrary
custom linter. For call sites that ARE the outermost boundary layer
(init functions, dedicated env-reading helpers, config-loading
functions), add //nolint:osgetenvlibrary. For spinner.go, replace the
duplicate os.Getenv("ACCESSIBLE") call with the existing IsAccessibleMode()
helper instead.
Files changed:
- pkg/logger/logger.go: nolint initDebugEnv + debugColors (3 findings)
- pkg/console/accessibility.go: nolint IsAccessibleMode (3 findings)
- pkg/console/spinner.go: use IsAccessibleMode() instead of os.Getenv (1 finding)
- pkg/constants/constants.go: nolint GetWorkflowDir (1 finding)
- pkg/envutil/envutil.go: nolint GetIntFromEnv (1 finding)
- pkg/github/label_objective_mapping.go: nolint LoadObjectiveMappingFromConfig (1 finding)
- pkg/parser/github.go: nolint GetGitHubHost/GetGitHubToken (3 findings)
Closes #42160
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. PR #42163 only contains production code changes (os.Getenv → envutil.Getenv lint fixes in 7 boundary-layer packages with no accompanying test additions or modifications). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR resolves osgetenvlibrary linter findings by explicitly suppressing os.Getenv usage in library packages where environment reads are the intended boundary (env-to-config adapters), and by consolidating spinner accessibility detection onto the existing IsAccessibleMode() helper.
Changes:
- Added targeted
//nolint:osgetenvlibrarysuppressions for env-reading boundary functions/initializers across several packages. - Updated
pkg/console/spinner.goto useIsAccessibleMode()instead of directly readingACCESSIBLE. - Centralized accessibility-related env checks via
IsAccessibleMode()to avoid duplicated logic.
Show a summary per file
| File | Description |
|---|---|
| pkg/parser/github.go | Suppresses linter on env reads in GitHub host/token boundary helpers. |
| pkg/logger/logger.go | Suppresses linter on init-time env reads used to configure logging/debug behavior. |
| pkg/github/label_objective_mapping.go | Suppresses linter on env override for objective mapping config. |
| pkg/envutil/envutil.go | Suppresses linter in a dedicated env-reading utility helper. |
| pkg/constants/constants.go | Suppresses linter on workflow directory env override boundary. |
| pkg/console/spinner.go | Uses IsAccessibleMode() to determine whether spinners should be enabled. |
| pkg/console/accessibility.go | Suppresses linter for env reads inside the accessibility boundary helper. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Low
| // Automatically disabled when not running in a TTY or when ACCESSIBLE env var is set. | ||
| func NewSpinner(message string) *SpinnerWrapper { | ||
| isTTY := tty.IsStderrTerminal() | ||
| isAccessible := os.Getenv("ACCESSIBLE") != "" | ||
| isAccessible := IsAccessibleMode() |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #42163 does not have the 'implementation' label and has only 13 new lines of code in business logic directories (threshold: 100). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: osgetenvlibrary lint fix
Overall: ✅ Approving. All nolint suppressions are correctly placed at genuine environment-reading boundary functions. The spinner.go refactor to IsAccessibleMode() is a net improvement — it removes duplicated env-var logic and aligns spinner behavior with the canonical accessibility helper.
What's correct:
- All 13 nolint suppressions are at boundary-layer callsites (env abstraction utilities, purpose-built config readers, init-time readers) — exactly where the linter expects suppression rather than refactoring.
- The
spinner.gobehavioral change (now also disabling whenTERM=dumborNO_COLORis set) is correct: a spinner in a dumb terminal or no-color terminal is undesirable. - Token values are never logged in
parser/github.go— only the presence of the env var is recorded.
Minor (non-blocking):
- One inline comment on
spinner.go: the function-level and package-level docstrings were not updated to reflect the expanded accessibility conditions now checked viaIsAccessibleMode().
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 30.3 AIC · ⌖ 9.03 AIC · ⊞ 4.9K
| func NewSpinner(message string) *SpinnerWrapper { | ||
| isTTY := tty.IsStderrTerminal() | ||
| isAccessible := os.Getenv("ACCESSIBLE") != "" | ||
| isAccessible := IsAccessibleMode() |
There was a problem hiding this comment.
The refactor to IsAccessibleMode() is correct and improves behavior (spinner now also disabled for TERM=dumb and NO_COLOR). However, two nearby docstrings are now stale and should be updated along with this change:
- Package doc (spinner.go line 11):
Accessibility: Respects ACCESSIBLE environment variable to disable animations— should mention all three env vars thatIsAccessibleMode()checks. - Function doc (line 101):
when ACCESSIBLE env var is set— should say something likewhen accessibility mode is active (seeIsAccessibleMode)to avoid misleading future readers.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out and /improve-codebase-architecture — clean lint cleanup with one minor doc follow-up.
📋 Key Themes & Highlights
Key Themes
- Stale documentation: The
pkg/console/spinner.gopackage doc, Accessibility section, andNewSpinnerfunc doc still reference only theACCESSIBLEenv var, but the code now delegates toIsAccessibleMode()which also coversTERM=dumbandNO_COLOR. All three locations need updating (see inline comment). - Spinner tests are ACCESSIBLE-only:
spinner_test.go's accessibility tests only setACCESSIBLE=1; worth addingTERM=dumbandNO_COLORscenarios to match the new delegate semantics, even thoughIsAccessibleMode()is unit-tested separately.
Positive Highlights
- ✅ Good boundary identification: All 12 nolint suppressions sit at genuine package boundaries (logger init, envutil abstraction, github token/host resolution). None feel like lazy workarounds.
- ✅ Spinner consolidation is a net win: Replacing the duplicated
os.Getenv("ACCESSIBLE")withIsAccessibleMode()removes a silent inconsistency — before this change, the spinner was not disabled byTERM=dumborNO_COLOReven though every other console component respected them. - ✅ Minimal blast radius: The per-line nolint approach keeps the linter suppression as local as possible.
- ✅
IsAccessibleMode()already has comprehensive test coverage inaccessibility_test.go.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 55.3 AIC · ⌖ 7.35 AIC · ⊞ 6.6K
Comment /matt to run again
| func NewSpinner(message string) *SpinnerWrapper { | ||
| isTTY := tty.IsStderrTerminal() | ||
| isAccessible := os.Getenv("ACCESSIBLE") != "" | ||
| isAccessible := IsAccessibleMode() |
There was a problem hiding this comment.
[/zoom-out] Three doc comments in this file now understate the accessibility conditions after switching from a direct os.Getenv("ACCESSIBLE") check to IsAccessibleMode() — the broader semantics aren't reflected in the docs.
💡 Stale comment locations
- Line 11 (package doc): "Respects ACCESSIBLE environment variable to disable animations" — should mention all three conditions
- Lines 34–37 (Accessibility section): describes only the
ACCESSIBLEenv var with an example;TERM=dumbandNO_COLORare omitted - Line 101 (func doc): "when ACCESSIBLE env var is set" — should reflect the full IsAccessibleMode() semantics
Suggested update for line 101:
// Automatically disabled when not running in a TTY or when IsAccessibleMode() returns true
// (ACCESSIBLE, TERM=dumb, or NO_COLOR env vars).@copilot please address this.
There was a problem hiding this comment.
Non-blocking documentation gap in spinner.go
The nolint suppressions are all correctly placed at genuine env-boundary functions. The one real cleanup is in spinner.go, where the spinner behavior has been expanded but the docs haven't caught up — and the gap is in two places, not one.
Details
The existing review thread already flagged the NewSpinner function comment at line 101 (// Automatically disabled when not running in a TTY or when ACCESSIBLE env var is set.).
There are also two more stale doc locations in the package-level block:
- Line 11:
Accessibility: Respects ACCESSIBLE environment variable to disable animations - Lines 33–38: The full
# Accessibilitysection, which explicitly documentsexport ACCESSIBLE=1as the only way to disable spinner animations.
All three locations need updating to reflect that TERM=dumb and NO_COLOR now also disable spinners via IsAccessibleMode(). The underlying behavior change is intentional and correctly implemented — only the docs lag behind.
No other issues. All other //nolint:osgetenvlibrary additions are correct and appropriately scoped.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 75.1 AIC · ⌖ 7 AIC · ⊞ 5.2K
Comment /review to run again
Comments that could not be inline-anchored
pkg/console/spinner.go:11
Stale package-level doc — two locations missed: The bullet at line 11 and the # Accessibility block (lines 33–38) still promise only ACCESSIBLE disables spinners. After this change, TERM=dumb and NO_COLOR also disable spinners via IsAccessibleMode().
<details>
<summary>💡 Suggested fixes</summary>
Line 11 (accessibility bullet in package overview):
// - Accessibility: Disabled when ACCESSIBLE, TERM=dumb, or NO_COLOR is set (see IsAccessibleMode)Lines 33–38 …
🤖 PR Triage — §28357644191
Adds
|
|
🎉 This pull request is included in a new release. Release: |
The
osgetenvlibrarycustom linter flagged 13os.Getenvcalls across 7 library packages. All affected call sites are already at the outermost practical boundary — functions whose explicit purpose is to read environment configuration — so the fix is either suppression with//nolint:osgetenvlibraryor consolidation onto an existing helper.Changes
pkg/logger/logger.go— nolint oninitDebugEnv()and thedebugColorspackage-var init; these are the init-time env-read boundary for the logger packagepkg/console/accessibility.go— nolint on eachos.GetenvinIsAccessibleMode(); this function is the accessibility env boundarypkg/console/spinner.go— replace directos.Getenv("ACCESSIBLE")withIsAccessibleMode(), which already consolidatesACCESSIBLE,TERM=dumb, andNO_COLOR; removes the coupling and aligns behavior with the canonical helperpkg/constants/constants.go— nolint inGetWorkflowDir()(GH_AW_WORKFLOWS_DIRoverride)pkg/envutil/envutil.go— nolint inGetIntFromEnv(); this utility is the env-reading abstractionpkg/github/label_objective_mapping.go— nolint inLoadObjectiveMappingFromConfig()(OBJECTIVE_MAPPING_JSONoverride)pkg/parser/github.go— nolint inGetGitHubHost()andGetGitHubToken(); both are purpose-built env-to-config boundary functions