Suppress ANSI installer colors in CI and explicit no-color environments#39875
Suppress ANSI installer colors in CI and explicit no-color environments#39875Copilot wants to merge 3 commits into
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>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. The PR 'Suppress ANSI installer colors in CI and explicit no-color environments' only modifies a workflow lock YAML file and two shell installer scripts (install-gh-aw.sh and actions/setup-cli/install.sh). Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #39875 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold 100). |
There was a problem hiding this comment.
Pull request overview
This PR aims to improve installer log readability in CI and other non-interactive/no-color environments by conditionally disabling ANSI color escape sequences, and to keep the composite action installer behavior consistent with the main installer script.
Changes:
- Added a color “no-color gate” to disable ANSI escape sequences when
CI/NO_COLOR/NO_COLORSis set, stdout is not a TTY, orTERM=dumb. - Synced the same behavior into
actions/setup-cli/install.shto match the primary installer. - Included a large update to
.github/workflows/daily-formal-spec-verifier.lock.yml(appears to be a broader recompile/regeneration beyond the installer changes).
Show a summary per file
| File | Description |
|---|---|
| install-gh-aw.sh | Disables ANSI color codes in CI/no-color/non-TTY contexts for cleaner logs. |
| actions/setup-cli/install.sh | Mirrors the same color gating logic for action-based installs. |
| .github/workflows/daily-formal-spec-verifier.lock.yml | Large generated workflow lockfile update that goes beyond the installer-focused PR scope. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
| name: "Daily Formal Spec Verifier" | ||
| on: | ||
| schedule: | ||
| - cron: "10 10 * * *" | ||
| - cron: "23 15 * * *" | ||
| # Friendly format: daily (scattered) |
![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 /tdd, /diagnose, and /zoom-out — no blocking issues, three observations worth addressing.
📋 Key Themes & Highlights
Key Themes
- Missing regression tests: The five-condition no-color gate has no test coverage. Both existing test files (
scripts/test-install-script.sh,actions/setup-cli/install_test.sh) use grep-based assertions that would make adding this trivial. - Dual-file sync with no automated guard:
install-gh-aw.shandactions/setup-cli/install.share identical and this PR correctly syncs them, but future changes could silently drift without a CI diff check or symlink. NO_COLORSnon-standard variable: Including it is a fine compatibility measure; a one-line comment clarifying it covers tools that deviate from the [no-color.org]((nocolor.org/redacted) standard would make the intent clear.
Positive Highlights
- ✅ The five-condition gate is comprehensive:
CI,NO_COLOR,NO_COLORS, non-TTY stdout (! -t 1), andTERM=dumb— covers all practical log-capture contexts. - ✅ Using empty-string overrides (rather than flag variables) is the idiomatic shell approach and works correctly with the existing
echo -eprint functions. - ✅ PR description is clear and well-structured with the exact code change shown inline.
- ✅ Lock-file diff is machine-generated and correctly excluded from semantic review.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer
|
|
||
| # Disable colors when output is captured or color is explicitly disabled. | ||
| # This keeps logs readable in CI systems like GitHub Actions. | ||
| if [ -n "${CI:-}" ] || [ -n "${NO_COLOR:-}" ] || [ -n "${NO_COLORS:-}" ] || [ ! -t 1 ] || [ "${TERM:-}" = "dumb" ]; then |
There was a problem hiding this comment.
[/tdd] No regression test covers the five-condition no-color gate — if this block is accidentally removed or broken, nothing will catch it.
The existing suite in scripts/test-install-script.sh already uses grep-based assertions; adding one more in the same style is straightforward.
💡 Suggested test addition for scripts/test-install-script.sh
# Test N: Verify no-color gate for CI environments
echo ""
echo "Test N: Verify no-color gate exists"
if grep -q 'NO_COLOR' "$PROJECT_ROOT/install-gh-aw.sh" && \
grep -q 'RED=""' "$PROJECT_ROOT/install-gh-aw.sh"; then
echo " ✓ PASS: No-color gate present"
else
echo " ✗ FAIL: No-color gate missing"
exit 1
fiA matching test should also be added to actions/setup-cli/install_test.sh.
| BLUE='\033[0;34m' | ||
| NC='\033[0m' # No Color | ||
|
|
||
| # Disable colors when output is captured or color is explicitly disabled. |
There was a problem hiding this comment.
[/zoom-out] install-gh-aw.sh and actions/setup-cli/install.sh are byte-for-byte identical. This PR correctly syncs them, but there is no automated gate to detect future drift between the two copies.
💡 Options to prevent future drift
- CI diff check — add a
maketarget or CI step:diff install-gh-aw.sh actions/setup-cli/install.sh
- Symlink — replace
actions/setup-cli/install.shwith a symlink toinstall-gh-aw.shso they can never drift. - Copier/include comment — at minimum, add a comment at the top of the composite-action copy:
# Kept in sync with ../../install-gh-aw.sh — edit that file, then copy here.
Without one of these, the next change to either file risks silently re-introducing inconsistent behaviour between composite-action and direct-script installs.
|
|
||
| # Disable colors when output is captured or color is explicitly disabled. | ||
| # This keeps logs readable in CI systems like GitHub Actions. | ||
| if [ -n "${CI:-}" ] || [ -n "${NO_COLOR:-}" ] || [ -n "${NO_COLORS:-}" ] || [ ! -t 1 ] || [ "${TERM:-}" = "dumb" ]; then |
There was a problem hiding this comment.
[/diagnose] NO_COLORS (with trailing S) is not part of the official [no-color.org]((nocolor.org/redacted) standard — only NO_COLOR is. Including it is a pragmatic compatibility measure, but without an inline comment explaining why, future readers may assume it is a typo and remove it.
💡 Suggestion
Extend the existing comment to make the intent explicit:
# Disable colors when output is captured or color is explicitly disabled.
# NO_COLOR is the no-color.org standard; NO_COLORS covers tools that use the non-standard variant.
if [ -n "${CI:-}" ] || [ -n "${NO_COLOR:-}" ] || [ -n "${NO_COLORS:-}" ] || [ ! -t 1 ] || [ "${TERM:-}" = "dumb" ]; then
There was a problem hiding this comment.
No blocking issues. Two non-blocking observations on the shell changes.
### Findings summary
1. NO_COLOR spec compliance (install-gh-aw.sh + actions/setup-cli/install.sh line 60)
[ -n "${NO_COLOR:-}" ] silently ignores export NO_COLOR="" (empty string), but the [no-color.org]((nocolor.org/redacted) spec says colors must be disabled whenever the variable is set, regardless of value. The [ ! -t 1 ] TTY check already catches the dominant CI case, so the practical impact is low — but it's a spec violation worth fixing: use [ "${NO_COLOR+set}" = "set" ] instead.
2. Duplicate files with no sync enforcement (install-gh-aw.sh and actions/setup-cli/install.sh)
Both files now share the same git blob hash. The sync is intentional per the PR description, but there is no symlink, CI diff check, or comment enforcing the invariant. The next PR that touches only one file will introduce silent behavioral divergence between the two install paths.
Lock file changes are auto-generated and expected (version bumps: firewall 0.27.2→0.27.4, github-mcp-server v1.1.2→v1.3.0, cron scatter, local ./actions/setup instead of pinned external action). No action needed there.
🔎 Code quality review by PR Code Quality Reviewer
| # Disable colors when output is captured or color is explicitly disabled. | ||
| # This keeps logs readable in CI systems like GitHub Actions. | ||
| if [ -n "${CI:-}" ] || [ -n "${NO_COLOR:-}" ] || [ -n "${NO_COLORS:-}" ] || [ ! -t 1 ] || [ "${TERM:-}" = "dumb" ]; then | ||
| RED="" |
There was a problem hiding this comment.
NO_COLOR empty-string edge case violates the no-color.org spec: [ -n "${NO_COLOR:-}" ] only disables colors when NO_COLOR is non-empty, but the spec says colors must be suppressed whenever the variable is set, even if set to an empty string.
💡 Suggested fix
Replace the NO_COLOR and NO_COLORS checks with parameter-set tests:
if [ -n "${CI:-}" ] || [ "${NO_COLOR+set}" = "set" ] || [ "${NO_COLORS+set}" = "set" ] || [ ! -t 1 ] || [ "${TERM:-}" = "dumb" ]; then${NO_COLOR+set} expands to set if the variable is defined at all (even as empty), and to empty string if unset — exactly what the spec requires. [ -n "${NO_COLOR:-}" ] by contrast silently treats export NO_COLOR="" the same as unset, leaving colors enabled.
The same fix applies to actions/setup-cli/install.sh line 60 (identical change).
| # Disable colors when output is captured or color is explicitly disabled. | ||
| # This keeps logs readable in CI systems like GitHub Actions. | ||
| if [ -n "${CI:-}" ] || [ -n "${NO_COLOR:-}" ] || [ -n "${NO_COLORS:-}" ] || [ ! -t 1 ] || [ "${TERM:-}" = "dumb" ]; then | ||
| RED="" |
There was a problem hiding this comment.
Identical files with no automated sync check creates a maintenance trap: install-gh-aw.sh and actions/setup-cli/install.sh now share the same git blob hash — they are byte-for-byte identical. There is nothing to prevent them from silently diverging on the next change to either file.
💡 Options to enforce the invariant
Pick one:
-
Symlink — make
actions/setup-cli/install.sha symlink to../../install-gh-aw.sh. Consumers that fetch viacurlor composite action checkout would need to dereference it, whichactions/checkoutdoes automatically. -
CI diff check — add a
maketarget or GitHub Actions step that assertsdiff install-gh-aw.sh actions/setup-cli/install.shexits 0, blocking PRs that update one without the other. -
Document the contract — if full parity is intentional, at minimum add a comment at the top of each file pointing to the other and instructing authors to keep them in sync.
Without one of these, the next PR that touches only one file will introduce invisible behavioral differences between the two install paths.
Installer logs currently emit ANSI escape sequences in GitHub Actions and other non-interactive environments, reducing readability in captured logs and summaries. This change makes color output conditional so CI/no-color contexts receive plain text logs.
Behavior change: color gating
install-gh-aw.shthat disables color codes when any of the following are true:CIis setNO_COLORis setNO_COLORSis setTERM=dumbDistribution consistency
actions/setup-cli/install.shfrom the source installer so composite action installs and direct script installs behave identically.Resulting output characteristics
[INFO]/[SUCCESS]/[WARNING]/[ERROR]prefixes.