From 6bce09f3ce698e27c7289e56aef6cf9563bfa0a7 Mon Sep 17 00:00:00 2001 From: Russell Clarey Date: Wed, 2 Sep 2026 14:11:40 +0200 Subject: [PATCH] experimental/ssh: show a spinner per agent-shim bootstrap step 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 --- experimental/ssh/internal/client/agentshim.go | 57 ++++++++++------ experimental/ssh/internal/client/progress.go | 68 +++++++++++++++++++ .../ssh/internal/client/progress_test.go | 53 +++++++++++++++ 3 files changed, 157 insertions(+), 21 deletions(-) create mode 100644 experimental/ssh/internal/client/progress.go create mode 100644 experimental/ssh/internal/client/progress_test.go diff --git a/experimental/ssh/internal/client/agentshim.go b/experimental/ssh/internal/client/agentshim.go index ead57dc3ebe..aab384815f7 100644 --- a/experimental/ssh/internal/client/agentshim.go +++ b/experimental/ssh/internal/client/agentshim.go @@ -17,7 +17,6 @@ import ( "time" "github.com/databricks/cli/libs/auth" - "github.com/databricks/cli/libs/cmdio" "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/log" "github.com/databricks/databricks-sdk-go" @@ -349,26 +348,38 @@ func ensureToolchain(ctx context.Context, home string) error { return nil } + ui := newProgressUI(ctx) + // 1. uv (installs into ~/.local/bin). if _, err := exec.LookPath("uv"); err != nil { - 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 { + if err := runShell(ctx, out, "curl -LsSf https://astral.sh/uv/install.sh | sh"); err != nil { + return fmt.Errorf("failed to install uv: %w", err) + } + return nil + }); err != nil { + return err } } // 2. Unity Gateway CLI (pinned stock upstream release). if _, err := exec.LookPath("ucode"); err != nil { - cmdio.LogString(ctx, "Installing Unity Gateway CLI...") - if err := runCommand(ctx, "uv", "tool", "install", "git+https://github.com/"+ugRepo+"@"+ugVersion); err != nil { - return fmt.Errorf("failed to install Unity Gateway CLI: %w", err) + if err := ui.runStep(ctx, "Installing Unity Gateway CLI", func(out io.Writer) error { + if err := runCommand(ctx, out, "uv", "tool", "install", "git+https://github.com/"+ugRepo+"@"+ugVersion); err != nil { + return fmt.Errorf("failed to install Unity Gateway CLI: %w", err) + } + return nil + }); err != nil { + return err } } // 3. Node/npm (installs into depsDir/node/bin) if _, err := exec.LookPath("npm"); err != nil { - cmdio.LogString(ctx, "Installing npm...") - if _, err := ensureNode(ctx, home); err != nil { + if err := ui.runStep(ctx, "Installing Node.js", func(out io.Writer) error { + _, err := ensureNode(ctx, home, out) + return err + }); err != nil { return err } } @@ -469,9 +480,10 @@ func injectAgentContext(ctx context.Context, home string, agent agentSpec) ([]st } // download the latest Krypton LTS Node into deps/node once, returning its bin. -// Linux-only by design: the shim runs on the serverless driver, so the tarball -// name is hardcoded to linux while nodeDownloadArch guards the arch. -func ensureNode(ctx context.Context, home string) (string, error) { +// Extraction output is written to out. Linux-only by design: the shim runs on +// the serverless driver, so the tarball name is hardcoded to linux while +// nodeDownloadArch guards the arch. +func ensureNode(ctx context.Context, home string, out io.Writer) (string, error) { depsRoot := filepath.Join(home, depsDir) nodeDir := filepath.Join(depsRoot, "node") nodeBin := filepath.Join(nodeDir, "bin") @@ -506,7 +518,7 @@ func ensureNode(ctx context.Context, home string) (string, error) { } defer os.RemoveAll(tmpDir) // no-op once renamed; cleans up a failed extraction // Node's .tar.xz is the smallest download; extract it with the system tar. - if err := runCommand(ctx, "tar", "-xJf", tarball, "--strip-components=1", "-C", tmpDir); err != nil { + if err := runCommand(ctx, out, "tar", "-xJf", tarball, "--strip-components=1", "-C", tmpDir); err != nil { return "", fmt.Errorf("failed to extract Node.js: %w", err) } // Clear any partial leftover from a previously-interrupted run, then publish @@ -608,19 +620,22 @@ func npmGlobalPrefix(ctx context.Context) string { return strings.TrimSpace(string(out)) } -func runCommand(ctx context.Context, name string, args ...string) error { +// runCommand runs name with the given args, writing combined stdout+stderr to out +// (captured so it surfaces only on failure) and inheriting the process env. Stdin +// is left closed: the shim's install steps are non-interactive. +func runCommand(ctx context.Context, out io.Writer, name string, args ...string) error { cmd := exec.CommandContext(ctx, name, args...) - cmd.Stdin = os.Stdin - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr + cmd.Stdout = out + cmd.Stderr = out return cmd.Run() } -func runShell(ctx context.Context, script string) error { +// runShell runs a shell snippet — for the curl|sh / curl|tar pipelines — writing +// combined stdout+stderr to out. +func runShell(ctx context.Context, out io.Writer, script string) error { cmd := exec.CommandContext(ctx, "sh", "-c", script) - cmd.Stdin = os.Stdin - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr + cmd.Stdout = out + cmd.Stderr = out return cmd.Run() } diff --git a/experimental/ssh/internal/client/progress.go b/experimental/ssh/internal/client/progress.go new file mode 100644 index 00000000000..d850ba3714b --- /dev/null +++ b/experimental/ssh/internal/client/progress.go @@ -0,0 +1,68 @@ +package client + +import ( + "bytes" + "context" + "fmt" + "io" + "os" + "strings" + + "github.com/charmbracelet/lipgloss" + "github.com/databricks/cli/libs/cmdio" +) + +// progressUI renders the bootstrap steps: each step spins while it runs and +// leaves a checkmark line when it finishes. Subprocess output is captured per +// step and printed only when that step fails. +type progressUI struct { + w io.Writer + check string // styled "✓" prefix for a finished step + cross string // styled "✗" prefix for a failed step +} + +// newProgressUI builds a progress renderer writing its checkmark lines to stderr. +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 + } +} + +// runStep shows a cmdio spinner labelled desc while fn runs, giving fn a writer +// that captures the step's subprocess output. On success it leaves a checkmark +// line; on failure it prints the captured output (stdout+stderr, in order) before +// returning fn's error. The shared spinner shows elapsed time and degrades to no +// output in a non-interactive terminal, so the checkmark line is what the reader +// sees either way. +func (ui *progressUI) runStep(ctx context.Context, desc string, fn func(out io.Writer) error) error { + sp := cmdio.NewSpinner(ctx, cmdio.WithElapsedTime()) + // Close is idempotent; defer it so a panic in fn can't leave the spinner (and + // its tea-program slot) running, while the explicit Close below still controls + // output ordering on the normal path. + defer sp.Close() + sp.Update(desc) + + var buf bytes.Buffer + err := fn(&buf) + // Stop the spinner (clearing its line) before printing the step's outcome. + sp.Close() + + if err != nil { + fmt.Fprintln(ui.w, ui.cross+" "+desc) + if out := buf.String(); out != "" { + fmt.Fprint(ui.w, out) + if !strings.HasSuffix(out, "\n") { + fmt.Fprintln(ui.w) + } + } + return err + } + + fmt.Fprintln(ui.w, ui.check+" "+desc) + return nil +} diff --git a/experimental/ssh/internal/client/progress_test.go b/experimental/ssh/internal/client/progress_test.go new file mode 100644 index 00000000000..8da3ad3c0d3 --- /dev/null +++ b/experimental/ssh/internal/client/progress_test.go @@ -0,0 +1,53 @@ +package client + +import ( + "bytes" + "errors" + "io" + "testing" + + "github.com/databricks/cli/libs/cmdio" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newTestProgressUI returns a progressUI writing to w with sentinel check/cross +// markers so the emitted outcome line is assertable. runStep is driven with a +// non-interactive cmdio context (the mode tests run in), where the spinner +// degrades to no output, exercising the capture-and-dump logic directly. +func newTestProgressUI(w io.Writer) *progressUI { + return &progressUI{w: w, check: "OK", cross: "FAIL"} +} + +func TestRunStepHidesOutputOnSuccess(t *testing.T) { + var w bytes.Buffer + ui := newTestProgressUI(&w) + + err := ui.runStep(cmdio.MockDiscard(t.Context()), "Installing dependencies", func(out io.Writer) error { + _, _ = io.WriteString(out, "verbose installer chatter\n") + return nil + }) + require.NoError(t, err) + + // The step's subprocess output must not surface on success, but the checkmark + // line for the step is still emitted. + assert.NotContains(t, w.String(), "verbose installer chatter") + assert.Contains(t, w.String(), "OK Installing dependencies") +} + +func TestRunStepShowsOutputOnFailure(t *testing.T) { + var w bytes.Buffer + ui := newTestProgressUI(&w) + + sentinel := errors.New("install failed") + err := ui.runStep(cmdio.MockDiscard(t.Context()), "Installing ucode", func(out io.Writer) error { + _, _ = io.WriteString(out, "line to stdout\nline to stderr") + return sentinel + }) + require.ErrorIs(t, err, sentinel) + + // On failure the cross line and the full captured output are printed, with a + // trailing newline added. + assert.Contains(t, w.String(), "FAIL Installing ucode") + assert.Contains(t, w.String(), "line to stdout\nline to stderr\n") +}