From 0aa8e4144b2982b1d56d2917a0b6d9e75f7718d7 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Thu, 11 Jun 2026 22:41:09 -0500 Subject: [PATCH 01/18] ui: buffer spinner + worker rows into single synchronized writes The spinner output flickered in terminals that don't handle rapid ANSI cursor movement well (Copilot CLI terminal, VS Code terminal, some iTerm2 configs). Root cause: renderWorkersLocked made N+1 separate fmt.Fprintf calls per frame (one per worker line + cursor-up), and Write passed the spinner frame through before appending workers, so the terminal briefly rendered partial frames between syscalls. Three changes: 1. Extract buildWorkerFrameLocked that writes to a strings.Builder instead of directly to the terminal. renderWorkersLocked and Write both use it to compose a complete frame in memory first. 2. Write now combines the spinner frame bytes and worker row escapes into a single buffer before writing, eliminating the gap between the spinner frame and its worker rows. 3. Wrap all multi-line output in DEC synchronized output sequences (\033[?2026h / \033[?2026l). Terminals that support mode 2026 (kitty, iTerm2, VS Code, foot, WezTerm) defer rendering until the end marker, eliminating flicker entirely. Terminals that don't recognize the sequence ignore it harmlessly. clearSpinnerLines gets the same single-write treatment for consistency, though it fires less frequently. --- internal/ui/ui.go | 55 +++++++++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 19 deletions(-) diff --git a/internal/ui/ui.go b/internal/ui/ui.go index 9a6dc8be..c31357eb 100644 --- a/internal/ui/ui.go +++ b/internal/ui/ui.go @@ -380,20 +380,27 @@ func (sw *spinnerWriter) Write(p []byte) (n int, err error) { sw.mu.Lock() defer sw.mu.Unlock() - n, err = sw.w.Write(p) - if err != nil || len(p) == 0 || p[0] != '\r' { + if len(p) == 0 || p[0] != '\r' { + n, err = sw.w.Write(p) return } - sw.renderWorkersLocked() + + // Combine the spinner frame and worker rows into a single + // synchronized write so the terminal never shows a partial frame. + var buf strings.Builder + buf.WriteString("\033[?2026h") // begin synchronized output + buf.Write(p) + sw.buildWorkerFrameLocked(&buf) + buf.WriteString("\033[?2026l") // end synchronized output + _, err = sw.w.Write([]byte(buf.String())) + n = len(p) return } -// renderWorkersLocked redraws the worker rows below the spinner line, leaving -// the cursor back on the spinner line. Caller must hold sw.mu and the cursor -// must currently be on the spinner line. Glyph frames are picked from the wall -// clock so animation continues even when triggered by the independent ticker -// instead of by a spinner Write. -func (sw *spinnerWriter) renderWorkersLocked() { +// buildWorkerFrameLocked appends the escape sequences for worker rows into +// buf, leaving the cursor back on the spinner line. Caller must hold sw.mu +// and the cursor must currently be on the spinner line. +func (sw *spinnerWriter) buildWorkerFrameLocked(buf *strings.Builder) { step := int(time.Now().UnixNano() / int64(workerFrameInterval)) // Per-slot phase offset so rows visibly cascade instead of all hitting // the same frame in lockstep — that lockstep was what made them look @@ -452,24 +459,34 @@ func (sw *spinnerWriter) renderWorkersLocked() { // is a no-op at the last row and the subsequent ESC[NA cursor-up // would then overshoot, landing on (and clobbering) lines above // the spinner — including the user's typed command line. - fmt.Fprintf(sw.w, "\n\r\033[2K%s", line) + fmt.Fprintf(buf, "\n\r\033[2K%s", line) } // Erase stale lines left over from a previous render that had more // active workers. Without this, ghost "→ dep" rows from finished // workers persist below the current set. for i := nLines; i < sw.nRendered; i++ { - fmt.Fprintf(sw.w, "\n\r\033[2K") + buf.WriteString("\n\r\033[2K") } totalDown := nLines if sw.nRendered > nLines { totalDown = sw.nRendered } if totalDown > 0 { - fmt.Fprintf(sw.w, "\033[%dA\r", totalDown) + fmt.Fprintf(buf, "\033[%dA\r", totalDown) } sw.nRendered = nLines } +// renderWorkersLocked redraws the worker rows below the spinner line as a +// single synchronized write. Caller must hold sw.mu. +func (sw *spinnerWriter) renderWorkersLocked() { + var buf strings.Builder + buf.WriteString("\033[?2026h") // begin synchronized output + sw.buildWorkerFrameLocked(&buf) + buf.WriteString("\033[?2026l") // end synchronized output + sw.w.Write([]byte(buf.String())) +} + // startAnimator launches a goroutine that periodically redraws the worker // rows so their glyphs keep pulsing even when the spinner library's own // writes stall or coalesce. Caller MUST NOT hold sw.mu. @@ -589,17 +606,17 @@ func (u *UI) clearSpinnerLines() { u.spinWriter.nRendered = 0 u.spinWriter.mu.Unlock() } - // Erase the spinner's own line plus exactly the known worker lines - // below it, then move the cursor back up. Using targeted \033[2K - // per line instead of \033[J (erase-to-end-of-screen) avoids - // clobbering the shell prompt if it starts drawing before we exit. - fmt.Fprint(u.w, "\r\033[2K") + // Buffer the entire clear into a single write so the terminal + // never shows a partially erased frame. + var buf strings.Builder + buf.WriteString("\r\033[2K") for i := 0; i < lines; i++ { - fmt.Fprint(u.w, "\n\033[2K") + buf.WriteString("\n\033[2K") } if lines > 0 { - fmt.Fprintf(u.w, "\033[%dA\r", lines) + fmt.Fprintf(&buf, "\033[%dA\r", lines) } + fmt.Fprint(u.w, buf.String()) u.progHasDetail = false } From f0d8aa8f0d8bef120a670d04d2c59933bfd1f1f6 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Thu, 11 Jun 2026 22:51:01 -0500 Subject: [PATCH 02/18] ui: erase spinner line before redraw, hide cursor during animation Two more sources of primary-bar flicker: 1. The spinner library writes \r+content without erasing the line first. When the label shrinks (e.g. [10/10] -> [1/1]), leftover characters from the previous longer frame remain visible for one tick. Fix: replace the leading \r in Write with \r\033[2K so the line is always cleared before the new frame is painted. 2. The cursor is visible during animation and its per-tick repositioning creates visual noise, especially in terminals with slower cursor rendering. Fix: hide the cursor in startAnimator (\033[?25l) and restore it unconditionally in stopAnimator (\033[?25h), so it stays hidden for exactly the duration of the animation and can't be left permanently invisible if the process exits early. --- internal/ui/ui.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/internal/ui/ui.go b/internal/ui/ui.go index c31357eb..0b9ae071 100644 --- a/internal/ui/ui.go +++ b/internal/ui/ui.go @@ -389,7 +389,11 @@ func (sw *spinnerWriter) Write(p []byte) (n int, err error) { // synchronized write so the terminal never shows a partial frame. var buf strings.Builder buf.WriteString("\033[?2026h") // begin synchronized output - buf.Write(p) + // Replace the leading \r with \r\033[2K (go-to-col-0 + erase line) + // so that when the label shrinks between ticks the old longer text + // is fully cleared instead of leaving leftover characters visible. + buf.WriteString("\r\033[2K") + buf.Write(p[1:]) // p[0] is the \r we already emitted above sw.buildWorkerFrameLocked(&buf) buf.WriteString("\033[?2026l") // end synchronized output _, err = sw.w.Write([]byte(buf.String())) @@ -502,6 +506,12 @@ func (sw *spinnerWriter) startAnimator() { done := sw.done sw.mu.Unlock() + // Hide the cursor for the duration of the animation so per-tick + // cursor repositioning doesn't create visual noise. Restored in + // stopAnimator unconditionally so a crash or early-stop can't leave + // the cursor permanently invisible. + fmt.Fprint(sw.w, "\033[?25l") + go func() { defer close(done) t := time.NewTicker(workerFrameInterval) @@ -542,6 +552,8 @@ func (sw *spinnerWriter) stopAnimator() { } close(stop) <-done + // Restore the cursor that startAnimator hid. + fmt.Fprint(sw.w, "\033[?25h") } // setDetail is a backward-compat shim that sets a single worker slot (slot 0). From e2e9fe5d1694b36bed96a2334efcba56a9413295 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Thu, 11 Jun 2026 23:08:35 -0500 Subject: [PATCH 03/18] ui: fix data race on spinner Suffix between label updates and render goroutine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The spinner library (briandowns/spinner) reads s.Prefix and s.Suffix inside s.mu on every tick. renderProgress was writing u.spinner.Suffix directly from the calling goroutine without holding s.mu — a genuine data race. On amd64 a string is pointer+length (two 64-bit words), so a concurrent write can produce a torn read where pointer and length are from different updates, yielding garbage output or the wrong label text for one frame. go test -race confirms the race is now gone. Fix: add pendingSuffix to spinnerWriter. renderProgress writes it under sw.mu. A PreUpdate callback on the spinner (called while s.mu is held, before s.Suffix is read for the frame) applies pendingSuffix to s.Suffix atomically. Lock order is s.mu → sw.mu, consistent with how the spinner goroutine already calls sw.Write under s.mu. This eliminates the 'Resolving actions' / 'Planning pins' label text flickering at phase transitions: the label now only changes at tick boundaries, never mid-frame, and never races with the render goroutine. --- internal/ui/ui.go | 42 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/internal/ui/ui.go b/internal/ui/ui.go index 0b9ae071..4fe637ce 100644 --- a/internal/ui/ui.go +++ b/internal/ui/ui.go @@ -364,6 +364,15 @@ type spinnerWriter struct { // deferredHints mirrors deferredWrites for hint state so concurrent // stall-watcher updates aren't clobbered by printLine's restore. deferredHints map[int]string + + // pendingSuffix holds the label text to apply to the spinner on the + // next PreUpdate callback. PreUpdate fires inside s.mu (the spinner's + // internal lock) so the s.Suffix assignment is race-free with the + // spinner goroutine's concurrent read. renderProgress writes here + // under sw.mu instead of writing s.Suffix directly; the lock order + // s.mu → sw.mu is consistent with how Write is called from the + // spinner goroutine. + pendingSuffix string } // workerSpinFrames is the rotating glyph shown next to each ACTIVE worker row @@ -1054,6 +1063,16 @@ func (u *UI) StartProgress(label string) { opts = append(opts, spinner.WithColor("fgCyan")) } sp := spinner.New(spinner.CharSets[11], 120*time.Millisecond, opts...) + // PreUpdate fires inside the spinner's internal lock (s.mu) immediately + // before Suffix is read for the frame. Applying pendingSuffix here + // makes label updates race-free: renderProgress writes pendingSuffix + // under sw.mu; PreUpdate reads it under s.mu→sw.mu, consistent with + // how Write is called from the same goroutine. + sp.PreUpdate = func(s *spinner.Spinner) { + sw.mu.Lock() + s.Suffix = sw.pendingSuffix + sw.mu.Unlock() + } u.spinner = sp u.progLabel = label u.progDetail = "" @@ -1308,11 +1327,26 @@ func (u *UI) renderProgress() { suffix = label } - u.spinner.Prefix = "" - if suffix != "" { - u.spinner.Suffix = " " + suffix + // Store the suffix in pendingSuffix; PreUpdate applies it to s.Suffix + // under the spinner's internal lock, eliminating the data race between + // this goroutine's write and the spinner goroutine's concurrent read. + if u.spinWriter != nil { + u.spinWriter.mu.Lock() + if suffix != "" { + u.spinWriter.pendingSuffix = " " + suffix + } else { + u.spinWriter.pendingSuffix = "" + } + u.spinWriter.mu.Unlock() } else { - u.spinner.Suffix = "" + // spinWriter not yet set (shouldn't happen after StartProgress, + // but be defensive). + u.spinner.Prefix = "" + if suffix != "" { + u.spinner.Suffix = " " + suffix + } else { + u.spinner.Suffix = "" + } } } From a8ddcc9fb811a02d6099214c7df048cb9ac5718e Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Thu, 11 Jun 2026 23:11:11 -0500 Subject: [PATCH 04/18] ui: erase worker rows immediately on ClearWorkerStatuses instead of lazily MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The old code cleared worker slot data then waited up to 120ms for the next spinner tick to erase the stale rows from the terminal. During that window the rows sat on screen, then all vanished at once — visible as a jump at the Resolving→Planning phase transition. Fix: call renderWorkersLocked (already under sw.mu) before releasing the lock. It sees all-empty workers, erases the previously-rendered rows, and resets nRendered to 0, all in one synchronized write. --- internal/ui/ui.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/ui/ui.go b/internal/ui/ui.go index 4fe637ce..b5471ec9 100644 --- a/internal/ui/ui.go +++ b/internal/ui/ui.go @@ -1230,6 +1230,11 @@ func (u *UI) ClearWorkerStatuses() { for i := range u.spinWriter.hints { u.spinWriter.hints[i] = "" } + // Immediately erase the old rows from the terminal rather than + // waiting for the next spinner tick (~120ms). Without this, the + // stale rows sit on screen for one tick and then all vanish at once, + // which looks like a jump at phase transitions. + u.spinWriter.renderWorkersLocked() u.spinWriter.mu.Unlock() } From 58daf3b764497e7f0fcd070467edda9e4fa3651a Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Thu, 11 Jun 2026 23:15:19 -0500 Subject: [PATCH 05/18] ui: static glyph-only top line, all detail in worker rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Experiment: remove the dynamic label text from the spinner's top line entirely. The top line now shows only the braille glyph — static, never redraws. All per-phase and per-action information lives in the worker rows below, which already handle it well. This eliminates the last remaining source of top-line flicker: label text changing between phases. Similar to how npm/yarn show a bare spinner during installs — the glyph signals 'working' and the rows below tell you what. UpdateLabel is now headless-only (logs phase transitions for --json / CI mode) and a no-op on TTY. --- internal/ui/ui.go | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/internal/ui/ui.go b/internal/ui/ui.go index b5471ec9..66da3291 100644 --- a/internal/ui/ui.go +++ b/internal/ui/ui.go @@ -1074,11 +1074,12 @@ func (u *UI) StartProgress(label string) { sw.mu.Unlock() } u.spinner = sp - u.progLabel = label + u.progLabel = "" u.progDetail = "" u.progLast = "" u.progPaused = false - u.renderProgress() + // No suffix — top line is just the spinning glyph. All detail lives + // in the worker rows below. // Defer the visible start so fast runs never flicker. done := make(chan struct{}) @@ -1238,8 +1239,9 @@ func (u *UI) ClearWorkerStatuses() { u.spinWriter.mu.Unlock() } -// UpdateLabel changes the spinner prefix label (e.g. to show per-workflow -// "[i/N] path" progress). No-op when no spinner is active. +// UpdateLabel records the phase label for headless output. On a TTY the +// spinner glyph is the only top-line indicator (static, no text suffix), so +// label changes don't cause any redraws — all detail is in the worker rows. func (u *UI) UpdateLabel(label string) { u.traceProgress("label", label) if u.headless { @@ -1248,13 +1250,7 @@ func (u *UI) UpdateLabel(label string) { u.headlessEmit(stem) u.headlessLabelStem = stem } - return - } - if u.spinner == nil { - return } - u.progLabel = label - u.renderProgress() } // labelStem returns the label trimmed of whitespace, used as a phase From 1bab6822ce85bed918b8cc0adb520b15fc4fa27c Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Thu, 11 Jun 2026 23:23:30 -0500 Subject: [PATCH 06/18] =?UTF-8?q?ui:=20align=20spinner=20with=20cli/cli=20?= =?UTF-8?q?=E2=80=94=20label=20left,=20glyph=20right,=20race-free?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Switch from Suffix-based labels to Prefix-based (cli/cli style). Label sits left of the glyph: 'Resolving actions ⠋'. Label changes come from UpdateLabel, which writes pendingPrefix under sw.mu; PreUpdate reads it under s.mu→sw.mu eliminating the data race. renderProgress is stripped down to only the detail/slot-0 path. Dead fields (progLabel, progLast) removed. --- internal/ui/ui.go | 170 +++++++++++++++------------------------------- 1 file changed, 54 insertions(+), 116 deletions(-) diff --git a/internal/ui/ui.go b/internal/ui/ui.go index 66da3291..cf45be74 100644 --- a/internal/ui/ui.go +++ b/internal/ui/ui.go @@ -52,35 +52,21 @@ type UI struct { // mode so the same phase label isn't printed more than once. headlessLabelStem string - // progLabel and progDetail hold the two halves of the active spinner line - // (the per-workflow label and the resolver's current-action detail). They - // are recombined and truncated to one terminal row on every update so the - // spinner never wraps — a wrapped spinner breaks the library's - // backspace-based erase and causes line jumping/leftover fragments. - progLabel string + // progDetail holds the current resolver detail shown in worker slot 0. progDetail string - // progLast holds the most recent non-empty rendered line. If an update - // transiently leaves both label and detail empty (e.g. detail cleared - // between phases before the next label is set), we keep showing progLast - // so the spinner never flashes a bare, label-less glyph. - progLast string - // progPaused is set while the spinner is temporarily halted (e.g. to let an // interactive prompt own the terminal). The spinner object is retained so // ResumeProgress can restart it with the same label/detail. progPaused bool - // progHasDetail tracks whether the last renderProgress call rendered a - // second detail line. clearSpinnerLines uses this to know whether to also - // erase line 2 after stopping the spinner. + // progHasDetail tracks whether the last renderProgress call put something + // in worker slot 0. Used by clearSpinnerLines to know the row count. progHasDetail bool // spinWriter is a thin io.Writer wrapper set while a spinner is active. - // It intercepts each spinner tick write (which starts with '\r') and - // appends the detail line below the spinner WITHOUT putting the detail text - // in the spinner Suffix — keeping the suffix short so the library's - // byte-count wrap detection never triggers on the second line's content. + // It intercepts each spinner tick write and appends worker status rows + // below the spinner line in a single synchronized write. spinWriter *spinnerWriter // progGrace delays spinner visibility so fast runs never flicker. When @@ -365,14 +351,12 @@ type spinnerWriter struct { // stall-watcher updates aren't clobbered by printLine's restore. deferredHints map[int]string - // pendingSuffix holds the label text to apply to the spinner on the - // next PreUpdate callback. PreUpdate fires inside s.mu (the spinner's - // internal lock) so the s.Suffix assignment is race-free with the - // spinner goroutine's concurrent read. renderProgress writes here - // under sw.mu instead of writing s.Suffix directly; the lock order - // s.mu → sw.mu is consistent with how Write is called from the - // spinner goroutine. - pendingSuffix string + // pendingPrefix holds the spinner Prefix (label + space, glyph + // appended by the library) to apply on the next PreUpdate callback. + // PreUpdate fires inside s.mu so the assignment is race-free with the + // spinner goroutine's read. Lock order: s.mu → sw.mu, consistent with + // how Write is called from the spinner goroutine. + pendingPrefix string } // workerSpinFrames is the rotating glyph shown next to each ACTIVE worker row @@ -1063,23 +1047,25 @@ func (u *UI) StartProgress(label string) { opts = append(opts, spinner.WithColor("fgCyan")) } sp := spinner.New(spinner.CharSets[11], 120*time.Millisecond, opts...) - // PreUpdate fires inside the spinner's internal lock (s.mu) immediately - // before Suffix is read for the frame. Applying pendingSuffix here - // makes label updates race-free: renderProgress writes pendingSuffix - // under sw.mu; PreUpdate reads it under s.mu→sw.mu, consistent with - // how Write is called from the same goroutine. + // PreUpdate fires inside s.mu before Prefix/Suffix are read for the + // frame. Applying pendingPrefix here keeps label updates race-free: + // callers write pendingPrefix under sw.mu; PreUpdate reads it under + // s.mu → sw.mu, consistent with how Write is already called. sp.PreUpdate = func(s *spinner.Spinner) { sw.mu.Lock() - s.Suffix = sw.pendingSuffix + s.Prefix = sw.pendingPrefix + s.Suffix = "" sw.mu.Unlock() } u.spinner = sp - u.progLabel = "" u.progDetail = "" - u.progLast = "" u.progPaused = false - // No suffix — top line is just the spinning glyph. All detail lives - // in the worker rows below. + // Set the initial prefix. The goroutine hasn't started yet so + // writing the field directly is safe here. + if label != "" { + sw.pendingPrefix = label + " " + sp.Prefix = label + " " + } // Defer the visible start so fast runs never flicker. done := make(chan struct{}) @@ -1170,9 +1156,7 @@ func (u *UI) StopProgress() { u.spinner.Stop() u.spinner = nil u.spinWriter = nil - u.progLabel = "" u.progDetail = "" - u.progLast = "" u.progPaused = false } } @@ -1239,9 +1223,10 @@ func (u *UI) ClearWorkerStatuses() { u.spinWriter.mu.Unlock() } -// UpdateLabel records the phase label for headless output. On a TTY the -// spinner glyph is the only top-line indicator (static, no text suffix), so -// label changes don't cause any redraws — all detail is in the worker rows. +// UpdateLabel changes the spinner prefix label. On a TTY the label sits to +// the LEFT of the glyph (cli/cli style: "Resolving actions ⠋") and is +// updated race-free via pendingPrefix / PreUpdate. In headless mode it logs +// a plain-text phase boundary instead. func (u *UI) UpdateLabel(label string) { u.traceProgress("label", label) if u.headless { @@ -1250,7 +1235,18 @@ func (u *UI) UpdateLabel(label string) { u.headlessEmit(stem) u.headlessLabelStem = stem } + return + } + if u.spinWriter == nil { + return } + u.spinWriter.mu.Lock() + if label != "" { + u.spinWriter.pendingPrefix = label + " " + } else { + u.spinWriter.pendingPrefix = "" + } + u.spinWriter.mu.Unlock() } // labelStem returns the label trimmed of whitespace, used as a phase @@ -1260,95 +1256,37 @@ func labelStem(label string) string { return strings.TrimSpace(label) } -// renderProgress recombines the label and detail into a single line that is -// truncated to fit the terminal width (leaving room for the spinner glyph and -// a space). The spinner glyph is anchored at the left edge (column 0): the -// combined line is assigned to the spinner Suffix with an empty Prefix, so the -// library always renders "\r{glyph} {label} — {detail}". Keeping the glyph -// fixed on the left stops it from drifting as the detail text changes width. -// The whole string is truncated to one terminal row — wrapping would defeat -// the library's backspace-based erase and cause the line jumping the user -// sees. +// renderProgress updates worker slot 0 with the current detail string. +// The label (top-line prefix) is managed separately by UpdateLabel. func (u *UI) renderProgress() { if u.spinner == nil { return } - label := u.progLabel detail := u.progDetail - - if label == "" { - if u.progLast == "" { - return - } - label = u.progLast - } else { - u.progLast = label + if detail == "" { + return } + // Worker rows animate only when text starts with "→ "; UpdateProgress + // callers (resolver hooks) pass plain strings. Prepend the arrow so + // the slot pulses instead of looking frozen. + if !strings.HasPrefix(detail, "→ ") { + detail = "→ " + detail + } width := u.termWidth() - if width > 8 { - budget := width - 6 + if width > 4 { + budget := width - 4 if !u.noColor { - budget -= 7 // bold escape open+close + budget -= 7 } - label = truncateBytes(label, budget) + detail = truncateBytes(detail, budget) } - if detail != "" { - // Worker rows render a pulsing glyph only when the text starts - // with "→ "; UpdateProgress callers (resolver progress hooks) - // pass plain strings like "resolving foo@bar". Prepend the - // arrow so the slot animates instead of looking frozen. - if !strings.HasPrefix(detail, "→ ") { - detail = "→ " + detail - } - if width > 4 { - budget := width - 4 // " " indent + faint open+close - if !u.noColor { - budget -= 7 - } - detail = truncateBytes(detail, budget) - } - } - - // Pass detail (slot 0) to the writer; it appends worker lines on every - // spinner tick without inflating the Suffix byte count. - // Only write when detail is non-empty: calling setDetail("") would - // overwrite slot 0 that the pool's worker status may have set. - if u.spinWriter != nil && detail != "" { - u.spinWriter.setDetail(detail) - } - u.progHasDetail = detail != "" - - var suffix string - if !u.noColor { - suffix = u.output.String(label).Bold().String() - } else { - suffix = label - } - - // Store the suffix in pendingSuffix; PreUpdate applies it to s.Suffix - // under the spinner's internal lock, eliminating the data race between - // this goroutine's write and the spinner goroutine's concurrent read. if u.spinWriter != nil { - u.spinWriter.mu.Lock() - if suffix != "" { - u.spinWriter.pendingSuffix = " " + suffix - } else { - u.spinWriter.pendingSuffix = "" - } - u.spinWriter.mu.Unlock() - } else { - // spinWriter not yet set (shouldn't happen after StartProgress, - // but be defensive). - u.spinner.Prefix = "" - if suffix != "" { - u.spinner.Suffix = " " + suffix - } else { - u.spinner.Suffix = "" - } + u.spinWriter.setDetail(detail) } + u.progHasDetail = true } // termWidth returns the terminal column count for the spinner writer, or 0 if From 5f904febc2de51db51e4f536957b4d2e24d0bf74 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Thu, 11 Jun 2026 23:27:25 -0500 Subject: [PATCH 07/18] =?UTF-8?q?ui:=20static=20spinner=20label=20?= =?UTF-8?q?=E2=80=94=20set=20once=20in=20StartProgress,=20never=20changed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit UpdateLabel is now a TTY no-op. The label set in StartProgress stays for the spinner's lifetime, eliminating any mid-run flicker from label swaps. Headless mode still logs phase boundaries. PreUpdate hook and pendingPrefix field removed — no longer needed. --- internal/ui/ui.go | 53 +++++++++++------------------------------------ 1 file changed, 12 insertions(+), 41 deletions(-) diff --git a/internal/ui/ui.go b/internal/ui/ui.go index cf45be74..a803f7d4 100644 --- a/internal/ui/ui.go +++ b/internal/ui/ui.go @@ -350,13 +350,6 @@ type spinnerWriter struct { // deferredHints mirrors deferredWrites for hint state so concurrent // stall-watcher updates aren't clobbered by printLine's restore. deferredHints map[int]string - - // pendingPrefix holds the spinner Prefix (label + space, glyph - // appended by the library) to apply on the next PreUpdate callback. - // PreUpdate fires inside s.mu so the assignment is race-free with the - // spinner goroutine's read. Lock order: s.mu → sw.mu, consistent with - // how Write is called from the spinner goroutine. - pendingPrefix string } // workerSpinFrames is the rotating glyph shown next to each ACTIVE worker row @@ -1047,25 +1040,14 @@ func (u *UI) StartProgress(label string) { opts = append(opts, spinner.WithColor("fgCyan")) } sp := spinner.New(spinner.CharSets[11], 120*time.Millisecond, opts...) - // PreUpdate fires inside s.mu before Prefix/Suffix are read for the - // frame. Applying pendingPrefix here keeps label updates race-free: - // callers write pendingPrefix under sw.mu; PreUpdate reads it under - // s.mu → sw.mu, consistent with how Write is already called. - sp.PreUpdate = func(s *spinner.Spinner) { - sw.mu.Lock() - s.Prefix = sw.pendingPrefix - s.Suffix = "" - sw.mu.Unlock() + // The label is static for the lifetime of this spinner. Set Prefix + // before sp.Start() — the goroutine isn't running yet so no lock needed. + if label != "" { + sp.Prefix = label + " " } u.spinner = sp u.progDetail = "" u.progPaused = false - // Set the initial prefix. The goroutine hasn't started yet so - // writing the field directly is safe here. - if label != "" { - sw.pendingPrefix = label + " " - sp.Prefix = label + " " - } // Defer the visible start so fast runs never flicker. done := make(chan struct{}) @@ -1223,30 +1205,19 @@ func (u *UI) ClearWorkerStatuses() { u.spinWriter.mu.Unlock() } -// UpdateLabel changes the spinner prefix label. On a TTY the label sits to -// the LEFT of the glyph (cli/cli style: "Resolving actions ⠋") and is -// updated race-free via pendingPrefix / PreUpdate. In headless mode it logs -// a plain-text phase boundary instead. +// UpdateLabel is a no-op on TTY — the spinner label is static for the +// lifetime of the spinner. In headless mode it logs a plain-text phase +// boundary when the label changes. func (u *UI) UpdateLabel(label string) { u.traceProgress("label", label) - if u.headless { - stem := labelStem(label) - if stem != "" && stem != u.headlessLabelStem { - u.headlessEmit(stem) - u.headlessLabelStem = stem - } + if !u.headless { return } - if u.spinWriter == nil { - return + stem := labelStem(label) + if stem != "" && stem != u.headlessLabelStem { + u.headlessEmit(stem) + u.headlessLabelStem = stem } - u.spinWriter.mu.Lock() - if label != "" { - u.spinWriter.pendingPrefix = label + " " - } else { - u.spinWriter.pendingPrefix = "" - } - u.spinWriter.mu.Unlock() } // labelStem returns the label trimmed of whitespace, used as a phase From fda2ea3244f29b6025ad93f361a8943d4e613a3d Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Fri, 12 Jun 2026 08:30:26 -0500 Subject: [PATCH 08/18] ui: skip worker row render on spinner erase writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit briandowns calls Write twice per tick: once with \r\033[K (erase the previous frame) and once with the actual frame glyph+prefix. Previously both triggered buildWorkerFrameLocked, giving two full cursor-down/up sequences per 120ms tick — the root cause of residual flicker. Now the erase write passes straight through; worker rows are refreshed only on the frame write, halving cursor movement per tick. --- internal/ui/ui.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/internal/ui/ui.go b/internal/ui/ui.go index a803f7d4..ededd36f 100644 --- a/internal/ui/ui.go +++ b/internal/ui/ui.go @@ -371,6 +371,22 @@ func (sw *spinnerWriter) Write(p []byte) (n int, err error) { return } + // briandowns calls Write twice per tick: + // 1. erase: \r\033[K — wipe the previous frame + // 2. frame: \r{glyph}{prefix} — paint the new frame + // + // Only render worker rows on the frame write. Rendering them on the + // erase write too means two cursor-down/up sequences per 120ms tick, + // which is the primary cause of residual flicker on embedded terminals. + isErase := len(p) >= 3 && p[1] == '\033' && p[2] == '[' + if isErase { + // Just pass the erase through; worker rows are still on screen + // from the previous frame and will be refreshed momentarily. + n, err = sw.w.Write(p) + n = len(p) + return + } + // Combine the spinner frame and worker rows into a single // synchronized write so the terminal never shows a partial frame. var buf strings.Builder From c2182f1815be4add5c63c19455ff2fea2b374e88 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Fri, 12 Jun 2026 09:42:07 -0500 Subject: [PATCH 09/18] ui: move worker row glyph to right, matching main spinner style MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both lines now render as 'text ⠋' — label left, glyph right — consistent with the cli/cli pattern used for the main spinner. Also strips the GH_ACTIONS_PIN_DEBUG_SPINNER trace instrumentation added for diagnosis. --- internal/ui/ui.go | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/internal/ui/ui.go b/internal/ui/ui.go index ededd36f..89306d55 100644 --- a/internal/ui/ui.go +++ b/internal/ui/ui.go @@ -330,6 +330,11 @@ type spinnerWriter struct { nRendered int // number of worker lines written in the last tick noColor bool output *termenv.Output + // prefix is the static label text written before the spinner glyph + // (e.g. "Resolving actions "). Stored here so startAnimator can write + // an immediate frame on resume, avoiding the one-tick blank gap that + // occurs because briandowns never fires a tick immediately on Start(). + prefix string // stop closes to signal the independent worker-redraw ticker to exit. // done is closed once the ticker goroutine has returned. The ticker // keeps worker glyphs animating even when the spinner library coalesces, @@ -420,7 +425,7 @@ func (sw *spinnerWriter) buildWorkerFrameLocked(buf *strings.Builder) { body := w if len(w) >= len("→ ") && w[:len("→ ")] == "→ " { frame := workerSpinFrames[(step+slot)%len(workerSpinFrames)] - body = frame + " " + w[len("→ "):] + body = w[len("→ "):] + " " + frame } hint := "" if slot < len(sw.hints) { @@ -508,12 +513,25 @@ func (sw *spinnerWriter) startAnimator() { done := sw.done sw.mu.Unlock() - // Hide the cursor for the duration of the animation so per-tick - // cursor repositioning doesn't create visual noise. Restored in - // stopAnimator unconditionally so a crash or early-stop can't leave - // the cursor permanently invisible. fmt.Fprint(sw.w, "\033[?25l") + // Write a synthetic first frame immediately so the spinner line is never + // blank during the ~120ms gap before the library's first ticker tick. + // briandowns never writes a frame on Start() — it always waits for the + // first tick — so without this, every Resume causes a visible blank line. + sw.mu.Lock() + if sw.prefix != "" { + var buf strings.Builder + buf.WriteString("\033[?2026h") + buf.WriteString("\r\033[2K") + buf.WriteString(sw.prefix) + buf.WriteString(workerSpinFrames[0]) // placeholder glyph; real tick replaces it + sw.buildWorkerFrameLocked(&buf) + buf.WriteString("\033[?2026l") + sw.w.Write([]byte(buf.String())) + } + sw.mu.Unlock() + go func() { defer close(done) t := time.NewTicker(workerFrameInterval) @@ -1060,6 +1078,7 @@ func (u *UI) StartProgress(label string) { // before sp.Start() — the goroutine isn't running yet so no lock needed. if label != "" { sp.Prefix = label + " " + sw.prefix = label + " " } u.spinner = sp u.progDetail = "" From c6a7637e3b7d90445c0db5acde4df604e5640d42 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Fri, 12 Jun 2026 10:20:49 -0500 Subject: [PATCH 10/18] address Copilot review: tighten erase detection, lock cursor escapes, drop []byte allocs --- internal/ui/ui.go | 47 +++++++++++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 18 deletions(-) diff --git a/internal/ui/ui.go b/internal/ui/ui.go index 89306d55..aae36e65 100644 --- a/internal/ui/ui.go +++ b/internal/ui/ui.go @@ -378,17 +378,23 @@ func (sw *spinnerWriter) Write(p []byte) (n int, err error) { // briandowns calls Write twice per tick: // 1. erase: \r\033[K — wipe the previous frame - // 2. frame: \r{glyph}{prefix} — paint the new frame + // 2. frame: \r{Prefix}{glyph}{Suffix} — paint the new frame // // Only render worker rows on the frame write. Rendering them on the // erase write too means two cursor-down/up sequences per 120ms tick, // which is the primary cause of residual flicker on embedded terminals. - isErase := len(p) >= 3 && p[1] == '\033' && p[2] == '[' + // + // Match the two specific erase sequences the library emits rather than + // any generic CSI prefix — frame writes that start with a color SGR + // (e.g. \r\033[36m…) must not be misclassified as erases. + isErase := (len(p) == 4 && string(p) == "\r\033[K\n") || + (len(p) == 3 && string(p) == "\r\033[K") || + (len(p) == 5 && string(p) == "\r\033[2K\n") || + (len(p) == 4 && string(p) == "\r\033[2K") if isErase { // Just pass the erase through; worker rows are still on screen // from the previous frame and will be refreshed momentarily. n, err = sw.w.Write(p) - n = len(p) return } @@ -403,7 +409,7 @@ func (sw *spinnerWriter) Write(p []byte) (n int, err error) { buf.Write(p[1:]) // p[0] is the \r we already emitted above sw.buildWorkerFrameLocked(&buf) buf.WriteString("\033[?2026l") // end synchronized output - _, err = sw.w.Write([]byte(buf.String())) + _, err = io.WriteString(sw.w, buf.String()) n = len(p) return } @@ -495,7 +501,7 @@ func (sw *spinnerWriter) renderWorkersLocked() { buf.WriteString("\033[?2026h") // begin synchronized output sw.buildWorkerFrameLocked(&buf) buf.WriteString("\033[?2026l") // end synchronized output - sw.w.Write([]byte(buf.String())) + io.WriteString(sw.w, buf.String()) } // startAnimator launches a goroutine that periodically redraws the worker @@ -513,23 +519,25 @@ func (sw *spinnerWriter) startAnimator() { done := sw.done sw.mu.Unlock() - fmt.Fprint(sw.w, "\033[?25l") + sw.mu.Lock() + // Wrap cursor-hide in a synchronized block so it can't interleave + // with a concurrent spinner frame write mid-output. + io.WriteString(sw.w, "\033[?2026h\033[?25l\033[?2026l") + sw.mu.Unlock() // Write a synthetic first frame immediately so the spinner line is never // blank during the ~120ms gap before the library's first ticker tick. // briandowns never writes a frame on Start() — it always waits for the // first tick — so without this, every Resume causes a visible blank line. sw.mu.Lock() - if sw.prefix != "" { - var buf strings.Builder - buf.WriteString("\033[?2026h") - buf.WriteString("\r\033[2K") - buf.WriteString(sw.prefix) - buf.WriteString(workerSpinFrames[0]) // placeholder glyph; real tick replaces it - sw.buildWorkerFrameLocked(&buf) - buf.WriteString("\033[?2026l") - sw.w.Write([]byte(buf.String())) - } + var buf strings.Builder + buf.WriteString("\033[?2026h") + buf.WriteString("\r\033[2K") + buf.WriteString(sw.prefix) + buf.WriteString(workerSpinFrames[0]) // placeholder glyph; real tick replaces it + sw.buildWorkerFrameLocked(&buf) + buf.WriteString("\033[?2026l") + io.WriteString(sw.w, buf.String()) sw.mu.Unlock() go func() { @@ -572,8 +580,11 @@ func (sw *spinnerWriter) stopAnimator() { } close(stop) <-done - // Restore the cursor that startAnimator hid. - fmt.Fprint(sw.w, "\033[?25h") + // Restore the cursor that startAnimator hid, synchronized so it can't + // interleave with any write still draining from the ticker goroutine. + sw.mu.Lock() + io.WriteString(sw.w, "\033[?2026h\033[?25h\033[?2026l") + sw.mu.Unlock() } // setDetail is a backward-compat shim that sets a single worker slot (slot 0). From df0255da1efc7c9545315d61be2809a826eb165b Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Thu, 11 Jun 2026 23:07:00 -0500 Subject: [PATCH 11/18] support GH_ACTIONS_PIN_WORKFLOWS_DIR for non-standard workflow locations Undocumented env var for lab/non-standard environments where workflows live outside .github/workflows/. When set, workflow discovery and lockfile I/O (both read and write) use the override directory. Implementation: - workflowfile: extract DiscoverWorkflowsIn(dir) from DiscoverWorkflows - lockfile: replace State.repoRoot with State.lockPath; add LoadStateAt for explicit lockfile paths - root: read env var in newRun, route to DiscoverWorkflowsIn and LoadStateAt when set --- cmd/gh-actions-pin/root.go | 23 +++++++++++++++++++--- internal/lockfile/state.go | 16 ++++++++++----- internal/workflowfile/workflowfile.go | 7 ++++++- internal/workflowfile/workflowfile_test.go | 19 ++++++++++++++++++ 4 files changed, 56 insertions(+), 9 deletions(-) diff --git a/cmd/gh-actions-pin/root.go b/cmd/gh-actions-pin/root.go index 58dfba97..0ba89b7b 100644 --- a/cmd/gh-actions-pin/root.go +++ b/cmd/gh-actions-pin/root.go @@ -124,7 +124,8 @@ $ gh actions-pin --no-fix --json=valid,findings // from the existing lockfile so repeat scans short-circuit the per-branch // Compare walk. newResolver is the DI seam; pass nil for production wiring. func newRun(workflowPaths []string, hostname string, pool *pinpool.Pool, newResolver resolverFunc) ([]string, *resolve.Resolver, *lockfile.State, error) { - paths, err := discoverWorkflowPaths(workflowPaths) + workflowsDir := os.Getenv("GH_ACTIONS_PIN_WORKFLOWS_DIR") + paths, err := discoverWorkflowPaths(workflowPaths, workflowsDir) if err != nil { return nil, nil, nil, err } @@ -139,7 +140,12 @@ func newRun(workflowPaths []string, hostname string, pool *pinpool.Pool, newReso return nil, nil, nil, err } - store, err := lockfile.LoadState(".", r) + var store *lockfile.State + if workflowsDir != "" { + store, err = lockfile.LoadStateAt(filepath.Join(workflowsDir, "actions.lock"), r) + } else { + store, err = lockfile.LoadState(".", r) + } if err != nil { return nil, nil, nil, fmt.Errorf("opening lockfile: %w", err) } @@ -148,11 +154,22 @@ func newRun(workflowPaths []string, hostname string, pool *pinpool.Pool, newReso return paths, r, store, nil } -func discoverWorkflowPaths(existing []string) ([]string, error) { +func discoverWorkflowPaths(existing []string, workflowsDir string) ([]string, error) { if len(existing) > 0 { return expandWorkflowPaths(existing) } + if workflowsDir != "" { + paths, err := workflowfile.DiscoverWorkflowsIn(workflowsDir) + if err != nil { + return nil, err + } + if len(paths) == 0 { + return nil, fmt.Errorf("no workflow files found in %s", workflowsDir) + } + return paths, nil + } + paths, err := workflowfile.DiscoverWorkflows() if err != nil { return nil, err diff --git a/internal/lockfile/state.go b/internal/lockfile/state.go index 57b1c28e..f7f00d54 100644 --- a/internal/lockfile/state.go +++ b/internal/lockfile/state.go @@ -38,7 +38,7 @@ type MetadataResolver interface { // a single store instance without external synchronization. type State struct { mu sync.Mutex - repoRoot string + lockPath string // full path to actions.lock on disk file parserlock.File meta MetadataResolver idCache map[string][2]int64 @@ -48,8 +48,14 @@ type State struct { // LoadState reads the lockfile at repoRoot, returning an empty in-memory file // when none exists on disk. func LoadState(repoRoot string, meta MetadataResolver) (*State, error) { - full := filepath.Join(repoRoot, parserlock.Path) - contents, err := os.ReadFile(full) + return LoadStateAt(filepath.Join(repoRoot, parserlock.Path), meta) +} + +// LoadStateAt reads the lockfile at the given path, returning an empty +// in-memory file when none exists on disk. Use this when the lockfile +// lives outside the standard .github/workflows/ location. +func LoadStateAt(lockfilePath string, meta MetadataResolver) (*State, error) { + contents, err := os.ReadFile(lockfilePath) var file parserlock.File switch { @@ -88,7 +94,7 @@ func LoadState(repoRoot string, meta MetadataResolver) (*State, error) { } s := &State{ - repoRoot: repoRoot, + lockPath: lockfilePath, file: file, meta: meta, idCache: map[string][2]int64{}, @@ -360,7 +366,7 @@ func (s *State) Save() error { } } - full := filepath.Join(s.repoRoot, parserlock.Path) + full := s.lockPath if len(s.file.Dependencies) == 0 && len(s.file.Workflows) == 0 { if err := os.Remove(full); err != nil && !errors.Is(err, os.ErrNotExist) { diff --git a/internal/workflowfile/workflowfile.go b/internal/workflowfile/workflowfile.go index c32dd7b4..6ac049ab 100644 --- a/internal/workflowfile/workflowfile.go +++ b/internal/workflowfile/workflowfile.go @@ -90,7 +90,12 @@ func (f *File) ExtractActionRefs() ([]parserlock.ActionRef, []string, []string) // DiscoverWorkflows finds all workflow files in .github/workflows/ relative to // the current directory. Returns nil if the directory doesn't exist. func DiscoverWorkflows() ([]string, error) { - dir := filepath.Join(".github", "workflows") + return DiscoverWorkflowsIn(filepath.Join(".github", "workflows")) +} + +// DiscoverWorkflowsIn finds all workflow files (*.yml, *.yaml) in dir. +// Returns nil if the directory doesn't exist. +func DiscoverWorkflowsIn(dir string) ([]string, error) { entries, err := os.ReadDir(dir) if os.IsNotExist(err) { return nil, nil diff --git a/internal/workflowfile/workflowfile_test.go b/internal/workflowfile/workflowfile_test.go index 95be9e79..0677efbd 100644 --- a/internal/workflowfile/workflowfile_test.go +++ b/internal/workflowfile/workflowfile_test.go @@ -43,6 +43,25 @@ func TestExtractActionRefsMixed(t *testing.T) { assert.Contains(t, warnings[0], "expression-based") } +func TestDiscoverWorkflowsIn(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "ci.yml"), []byte("name: ci\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "deploy.yaml"), []byte("name: deploy\n"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "README.md"), []byte("# ignore\n"), 0o644)) + + paths, err := DiscoverWorkflowsIn(dir) + require.NoError(t, err) + assert.Len(t, paths, 2) + assert.Equal(t, filepath.Join(dir, "ci.yml"), paths[0]) + assert.Equal(t, filepath.Join(dir, "deploy.yaml"), paths[1]) +} + +func TestDiscoverWorkflowsIn_MissingDir(t *testing.T) { + paths, err := DiscoverWorkflowsIn(filepath.Join(t.TempDir(), "nope")) + require.NoError(t, err) + assert.Nil(t, paths) +} + func TestExtractLocalCompositeRefs_RejectsPathTraversal(t *testing.T) { repoRoot := t.TempDir() require.NoError(t, os.Mkdir(filepath.Join(repoRoot, ".git"), 0o755)) From 21e7a050c63f7d179674cbacb2a9c569e5376653 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Thu, 11 Jun 2026 22:33:28 -0500 Subject: [PATCH 12/18] remove duplicate 'All valid' message from terminal report MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PresentResults and renderPinSummary both printed 'All N workflows valid' on the happy path. Remove the one in PresentResults — renderPinSummary is the canonical post-remediation summary renderer. --- cmd/gh-actions-pin/format/terminal.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cmd/gh-actions-pin/format/terminal.go b/cmd/gh-actions-pin/format/terminal.go index 1aa8900c..6fedb693 100644 --- a/cmd/gh-actions-pin/format/terminal.go +++ b/cmd/gh-actions-pin/format/terminal.go @@ -37,9 +37,7 @@ func PresentResults(out *ui.UI, report *checks.Report, valid bool, willRemediate } checked := validCount + failedCount - if valid && checked > 0 { - out.Success("All %d %s valid", checked, ui.Pluralize(checked, "workflow", "workflows")) - } else if checked > 0 { + if !valid && checked > 0 { renderErrorFindings(out, report, failedCount, checked) } From 4d00102d9b077ef9fc844cb6fec35a5a4a6d75f8 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Thu, 11 Jun 2026 22:34:15 -0500 Subject: [PATCH 13/18] remove duplicate SSO URL from --no-fix path The --no-fix block printed the SSO authorization URL separately from the canonical location that fires after remediation. Remove the --no-fix copy so there is one authoritative SSO hint site. --- cmd/gh-actions-pin/check.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/cmd/gh-actions-pin/check.go b/cmd/gh-actions-pin/check.go index db0c529e..e9867b6e 100644 --- a/cmd/gh-actions-pin/check.go +++ b/cmd/gh-actions-pin/check.go @@ -269,12 +269,6 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) // Strict gate — any blocking finding is a non-zero exit. if opts.noFix { console.StopProgress() - if gc := r.GHClient(); gc != nil { - if ssoURL := gc.SSOURL(); ssoURL != "" { - console.TermBlank() - console.TermDetail("Authorize in your web browser: %s", ssoURL) - } - } if opts.jsonFields != "" { if err := format.WriteJSON(out, report, valid, opts.jsonFields, cliVersion(), store.File().Version); err != nil { return err From 833e5934232a3d179d154133a44da9805780befe Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Thu, 11 Jun 2026 22:34:31 -0500 Subject: [PATCH 14/18] pin summary: handle zero-workflow case gracefully 'All 0 workflows valid' is confusing when a repo has no workflow files. Print 'No workflows to check' and return early instead. --- cmd/gh-actions-pin/pin_summary.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cmd/gh-actions-pin/pin_summary.go b/cmd/gh-actions-pin/pin_summary.go index e5d8a99a..7abf8a4e 100644 --- a/cmd/gh-actions-pin/pin_summary.go +++ b/cmd/gh-actions-pin/pin_summary.go @@ -35,6 +35,10 @@ func renderPinSummary(console *ui.UI, record *pin.Record, report *checks.Report, } total := len(report.Workflows) + if total == 0 { + console.TermNeutral("No workflows to check") + return nil + } if len(pinned) == 0 && len(investigated) == 0 && len(unresolvedEntries) == 0 && !hasInconclusive { console.TermSuccess("All %d %s valid", total, ui.Pluralize(total, "workflow", "workflows")) if skippedRescan > 0 { From be3ca41a16a028c04efd757264e03b5b7098ee56 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Thu, 11 Jun 2026 22:34:46 -0500 Subject: [PATCH 15/18] TermCaution: use yellow instead of red TermCaution is for non-fatal but important warnings. Red reads as an error; yellow matches the caution semantics and keeps the '!' icon. --- internal/ui/ui.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/ui/ui.go b/internal/ui/ui.go index aae36e65..d4265bb8 100644 --- a/internal/ui/ui.go +++ b/internal/ui/ui.go @@ -892,15 +892,15 @@ func (u *UI) TermWarn(msg string, args ...any) { fmt.Fprintf(u.w, "%s %s\n", u.paint("3", IconWarning), fmt.Sprintf(msg, args...)) } -// TermCaution prints a red "!" summary line directly to the terminal. Use for +// TermCaution prints a yellow "!" summary line directly to the terminal. Use for // non-fatal but attention-worthy signals (e.g. a commit pinned only after a -// full-branch-scan fallback) that warrant red without the "✗ failure" framing. +// full-branch-scan fallback) that warrant emphasis without the "✗ failure" framing. func (u *UI) TermCaution(msg string, args ...any) { if u.headless { u.headlessEmit(fmt.Sprintf(msg, args...)) return } - fmt.Fprintf(u.w, "%s %s\n", u.paint("1", IconWarning), fmt.Sprintf(msg, args...)) + fmt.Fprintf(u.w, "%s %s\n", u.paint("3", IconWarning), fmt.Sprintf(msg, args...)) } // TermDetail prints an indented summary detail line directly to the terminal. From 58050c643eb74668e64d76d71a4de0d170caef99 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Thu, 11 Jun 2026 22:34:59 -0500 Subject: [PATCH 16/18] resolution record path: use TermDetail for better visibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TermNeutral renders dim gray, making the path hard to read. TermDetail is visible but not prominent — right for the one thing users copy. --- cmd/gh-actions-pin/check.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/gh-actions-pin/check.go b/cmd/gh-actions-pin/check.go index e9867b6e..99658132 100644 --- a/cmd/gh-actions-pin/check.go +++ b/cmd/gh-actions-pin/check.go @@ -326,7 +326,7 @@ func runCheck(cmd *cobra.Command, opts *checkOptions, newResolver resolverFunc) if path, werr := record.WriteJSON(); werr == nil { defer func() { console.TermBlank() - console.TermNeutral("Resolution record: %s", path) + console.TermDetail("Resolution record: %s", path) }() } From 090b0473977bdc1d232551edf4f8cb6d61c6072d Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Thu, 11 Jun 2026 22:51:58 -0500 Subject: [PATCH 17/18] add scenario and unit test coverage for CLI UX fixes - Unit test: PresentResults no longer emits 'All valid' success line - Scenario: no_workflows_summary asserts no 'All 0' in output - Scenario: sso_no_fix_single_url asserts SSO URL shown once in --no-fix --- cmd/gh-actions-pin/format/terminal_test.go | 18 ++++++++++++++++ test/scenarios/catalog.yml | 25 ++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/cmd/gh-actions-pin/format/terminal_test.go b/cmd/gh-actions-pin/format/terminal_test.go index ca7a1b5c..63f451f1 100644 --- a/cmd/gh-actions-pin/format/terminal_test.go +++ b/cmd/gh-actions-pin/format/terminal_test.go @@ -135,6 +135,24 @@ func TestPresentResults_WarningsReachTerminal(t *testing.T) { } } +// TestPresentResults_NoSuccessLine verifies PresentResults no longer emits +// the "All N workflows valid" success line — renderPinSummary owns that. +func TestPresentResults_NoSuccessLine(t *testing.T) { + u, buf := newTestUI() + report := &checks.Report{ + Workflows: []checks.WorkflowReport{ + {Path: ".github/workflows/a.yml"}, + {Path: ".github/workflows/b.yml"}, + }, + } + PresentResults(u, report, true, false) + + got := buf.String() + if strings.Contains(got, "All") && strings.Contains(got, "valid") { + t.Errorf("PresentResults should not print success line, got:\n%s", got) + } +} + // TestPresentResults_RemediateHints locks the willRemediate-aware "↳" // follow-up lines that PresentResults emits under each warning headline. // Categories the remediator auto-fixes (NotPinned, SHAAsRef) flip between diff --git a/test/scenarios/catalog.yml b/test/scenarios/catalog.yml index 1ac3631e..b49f023a 100644 --- a/test/scenarios/catalog.yml +++ b/test/scenarios/catalog.yml @@ -164,6 +164,21 @@ scenarios: - "actions/github-script" custom: sso_dedup_count + - name: sso_no_fix_single_url + category: sso_auth + description: "SSO URL shown only once in --no-fix path" + needs_stub: true + tags: [stub] + flags: ["--no-fix"] + fixtures: + workflows: + ci.yml: + name: CI + actions: ["actions/checkout@v4"] + expect: + exit: 1 + custom: sso_url_shown_once + - name: mixed_failures category: sso_auth description: "SSO + repo-not-found — different errors not deduped" @@ -302,6 +317,16 @@ scenarios: expect: exit: 2 + - name: no_workflows_summary + category: workflow_parsing + description: "No workflows to check — summary says so, not 'All 0 valid'" + tags: [stub] + fixtures: + workflows: {} + expect: + exit: 2 + output_excludes: ["All 0"] + - name: run_only_workflow category: workflow_parsing description: "Workflow with only run: steps (no actions) — RunOnly finding" From 04d160aadce398223e78c6a2d63d6f09d8f91e6c Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Fri, 12 Jun 2026 10:43:21 -0500 Subject: [PATCH 18/18] fix isErase detection (wrong byte lengths), clear slot 0 on empty detail --- internal/ui/ui.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/internal/ui/ui.go b/internal/ui/ui.go index d4265bb8..0a6621ca 100644 --- a/internal/ui/ui.go +++ b/internal/ui/ui.go @@ -384,13 +384,12 @@ func (sw *spinnerWriter) Write(p []byte) (n int, err error) { // erase write too means two cursor-down/up sequences per 120ms tick, // which is the primary cause of residual flicker on embedded terminals. // - // Match the two specific erase sequences the library emits rather than - // any generic CSI prefix — frame writes that start with a color SGR + // Match the specific erase sequence the library emits rather than any + // generic CSI prefix — frame writes that start with a color SGR // (e.g. \r\033[36m…) must not be misclassified as erases. - isErase := (len(p) == 4 && string(p) == "\r\033[K\n") || - (len(p) == 3 && string(p) == "\r\033[K") || - (len(p) == 5 && string(p) == "\r\033[2K\n") || - (len(p) == 4 && string(p) == "\r\033[2K") + // briandowns.erase() always writes "\r\033[K" (4 bytes) for single-line + // spinners, optionally followed by "\033[F\033[K" per additional line. + isErase := len(p) >= 4 && p[0] == '\r' && p[1] == '\033' && p[2] == '[' && p[3] == 'K' if isErase { // Just pass the erase through; worker rows are still on screen // from the previous frame and will be refreshed momentarily. @@ -534,7 +533,7 @@ func (sw *spinnerWriter) startAnimator() { buf.WriteString("\033[?2026h") buf.WriteString("\r\033[2K") buf.WriteString(sw.prefix) - buf.WriteString(workerSpinFrames[0]) // placeholder glyph; real tick replaces it + buf.WriteString(workerSpinFrames[0]) // placeholder glyph from shared braille charset; real tick replaces it sw.buildWorkerFrameLocked(&buf) buf.WriteString("\033[?2026l") io.WriteString(sw.w, buf.String()) @@ -1282,6 +1281,12 @@ func (u *UI) renderProgress() { detail := u.progDetail if detail == "" { + if u.progHasDetail { + if u.spinWriter != nil { + u.spinWriter.setDetail("") + } + u.progHasDetail = false + } return }