Adapt spinner output for invoking agents - #14191
Conversation
Detect the invoking agent once during startup and reuse the result for IOStreams, telemetry, and command factory initialization. Prefer text progress indicators for agents while preserving GH_SPINNER_DISABLED as the explicit override. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: edd6fa49-4788-4882-93f6-ca7caca4b7ed
Isolate spinner precedence tests from inherited environment overrides and verify startup propagation of agent detection through telemetry in the acceptance suite. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: edd6fa49-4788-4882-93f6-ca7caca4b7ed
There was a problem hiding this comment.
Pull request overview
Detects the invoking agent once and adapts spinner output for agent-driven sessions.
Changes:
- Reuses agent detection across IOStreams, telemetry, and command setup.
- Applies environment, agent, then configuration spinner precedence.
- Adds unit and acceptance coverage.
Show a summary per file
| File | Description |
|---|---|
internal/ghcmd/cmd.go |
Integrates agent-aware spinner initialization. |
internal/ghcmd/cmd_test.go |
Tests spinner precedence and environment isolation. |
acceptance/testdata/telemetry/agent-dimensions.txtar |
Verifies startup telemetry dimensions. |
Review details
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
babakks
left a comment
babakks
left a comment
There was a problem hiding this comment.
Thanks for tackling this, @niik! ✨
Really nice, focused change. Collapsing agent detection down to a single Detect() call at startup and threading that one value through IOStreams, telemetry, and the factory is a clean improvement over the previous repeated detection. The spinner precedence (env override, then detected agent, then config) reads well, and I appreciate the acceptance test covering the full startup path.
I only have a couple of minor comments inline, nothing blocking.
| t.Setenv("GH_SPINNER_DISABLED", "") | ||
| require.NoError(t, os.Unsetenv("GH_SPINNER_DISABLED")) |
There was a problem hiding this comment.
question: why do we need both of these? The os.Unsetenv call should be enough on its own to clear the variable, right? If the goal is just to guarantee a clean slate before each case, the t.Setenv("GH_SPINNER_DISABLED", "") line looks redundant. Or is t.Setenv there to register the var with the testing framework so it gets restored after the test, while Unsetenv actually clears it for the run? If so, a short comment explaining the intent would help.
There was a problem hiding this comment.
is t.Setenv there to register the var with the testing framework so it gets restored after the test, while Unsetenv actually clears it for the run? If so, a short comment explaining the intent would help.
Yeah, that's it. I've pushed a comment, please take a look and see if you think it's explanatory enough
Closes #12839
Related to #12522
Description
ghalready detects when it is invoked by an AI coding agent, but detection happened after IOStreams initialization and was repeated for telemetry and HTTP setup.This change detects the invoking agent once during startup and shares that result with IOStreams, telemetry, and the command factory. When an agent is detected, animated braille spinners use the existing text-only progress indicator instead.
GH_SPINNER_DISABLEDremains the highest-precedence override, including falsey values that explicitly restore animated spinners.How did you test this change?
Manually exercised a spinner-producing command under a real PTY:
AI_AGENTorCLAUDECODE,Working...appeared without braille animation.GH_SPINNER_DISABLED=false, animated braille was restored.Key points
Notes for reviewers
Start with
internal/ghcmd/cmd.go, where detection moves ahead of IOStreams initialization and spinner precedence is applied.internal/ghcmd/cmd_test.gocovers precedence and environment isolation.acceptance/testdata/telemetry/agent-dimensions.txtarverifies the complete startup path.Authorship and follow-up
Who wrote this:
Who answers review comments: