Conversation
The agent-shim command streamed the raw output of its bootstrap subprocesses (uv, ucode, node/npm) straight to the terminal, which was noisy and hard to follow. Render each step with the shared cmdio spinner and a short description instead: the subprocess output is captured and hidden, the spinner is replaced with a checkmark when the step succeeds, and the full captured output (stdout+stderr, in order) is printed only when a step fails. In a non-interactive terminal it degrades to a plain checkmark line per step. Co-authored-by: Isaac <no-reply@databricks.com>
Integration test reportCommit: 6bce09f
Top 5 slowest tests (at least 2 minutes):
|
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
Review (automated, 2 agent(s))
Verdict: Approved
0 Critical | 0 Major | 0 Gap | 2 Nit | 1 Suggestion
See inline comments for details.
| func newProgressUI(ctx context.Context) *progressUI { | ||
| // Mint the checkmark styles from a renderer targeting stderr so color handling | ||
| // stays centralized in cmdio.NewRenderer, matching cmdio's own spinner. | ||
| r, _ := cmdio.NewRenderer(ctx, os.Stderr) | ||
| return &progressUI{ | ||
| w: os.Stderr, | ||
| check: r.NewStyle().Foreground(lipgloss.Color("10")).Render("✓"), // green | ||
| cross: r.NewStyle().Foreground(lipgloss.Color("9")).Render("✗"), // red | ||
| } |
There was a problem hiding this comment.
[Suggestion] Outcome lines write to os.Stderr while the spinner writes to cmdio's stderr
runStep starts cmdio.NewSpinner(ctx), which paints on c.err from context (cmd.ErrOrStderr()). Check/cross lines and the failure dump go to a hardcoded os.Stderr minted here. In the default CLI process those are the same fd, so this works today. They diverge as soon as cobra's error writer is not os.Stderr (tests using SetErr, a wrapped stderr, NewTestContextWithStderr). Color is also decided on os.Stderr (cmdio.NewRenderer(ctx, os.Stderr)) while spinner interactivity is decided on c.err. The sandbox spinner wrapper prints the post-spinner mark with cmdio.LogString so both phases share one stream. The progressUI.w seam exists so tests can avoid MockDiscard's discarded stderr; cmdio.NewTestContextWithStderr already gives a capturable, non-TTY stderr and would make the split unnecessary.
Suggestion: Print the ✓/✗ line (and the failure dump) through cmdio, e.g. cmdio.LogString for the status line and LogString of the trimmed capture for the dump, and drive tests with NewTestContextWithStderr instead of injecting os.Stderr. If a raw writer is kept, initialize w from the same stream the spinner uses rather than os.Stderr.
| cmdio.LogString(ctx, "Installing uv...") | ||
| if err := runShell(ctx, "curl -LsSf https://astral.sh/uv/install.sh | sh"); err != nil { | ||
| return fmt.Errorf("failed to install uv: %w", err) | ||
| if err := ui.runStep(ctx, "Installing dependencies", func(out io.Writer) error { |
There was a problem hiding this comment.
[Nit] First step label is vaguer than the siblings and than the error it wraps
The uv step is shown as "Installing dependencies" while the next two steps are "Installing Unity Gateway CLI" and "Installing Node.js". The old message was "Installing uv...", and the error returned from the same closure is still failed to install uv. On failure the user sees ✗ Installing dependencies followed by Error: failed to install uv: ..., which names two different things. The other labels name the tool; this one does not.
Suggestion: Use "Installing uv" (or "Installing uv..." if you want to keep the old wording).
| // runShell runs a shell snippet — for the curl|sh / curl|tar pipelines — writing | ||
| // combined stdout+stderr to out. |
There was a problem hiding this comment.
[Nit] runShell comment mentions a curl|tar pipeline that no longer exists
The new comment says runShell is for the curl|sh / curl|tar pipelines. The only caller is the uv curl … | sh line. Node extraction goes through runCommand(..., "tar", ...) in ensureNode, not runShell.
Suggestion: Drop the curl|tar mention; say it runs the uv curl|sh install (or "a shell snippet").
This branch has not been deployed
3 participants
Changes
Show spinners per install stage for
ssh agent-shimWhy
Looks nicer
Tests
test new progress renderer