From cbe82bc39ccfc3b16fd81fd86b822c2ca6133a28 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 18:16:33 -0700 Subject: [PATCH 01/10] Present the device login as steps, say why no browser opened, count down the wait MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The device flow printed the link and the code inside prose, so a double-click grabbed the wrong span, and then went silent for up to ten minutes: a person on a slow approval page could not tell polling from a hang. The browser was skipped over SSH without a word, but not in CI or on a Linux host with no display, where xdg-open failed after the fact. Ctrl-C killed the process mid-line. Now the transcript is two numbered steps — link on its own line, code on its own line with its lifetime beside it — followed by the RFC 8628 §5.4 warning about codes handed over by someone else, one line saying what happened with the browser (opened, could not open, or not attempted and why), and on a terminal a live spinner with "code expires in m:ss" that clears itself when the flow ends. hostutil.HeadlessReason names the environments where no browser can be shown (SSH, CI, no DISPLAY or WAYLAND_DISPLAY on Unix); --local overrides it, --no-browser and the remote flags print the link without commentary. An interrupt cancels the flow through the context so the wait line is cleared and the listener closed, prints "Login canceled. Nothing was stored." and exits 130 without an error envelope. --- internal/auth/auth.go | 106 ++++++++++++++++++------ internal/auth/auth_test.go | 2 + internal/auth/device_test.go | 96 +++++++++++++++++++++- internal/auth/progress.go | 116 +++++++++++++++++++++++++++ internal/auth/progress_test.go | 64 +++++++++++++++ internal/cli/root.go | 6 ++ internal/commands/auth.go | 48 +++++++++-- internal/commands/auth_login_test.go | 65 ++++++++++++++- internal/commands/profile.go | 10 ++- internal/commands/profile_test.go | 4 + internal/hostutil/hostutil.go | 49 +++++++++++ internal/hostutil/hostutil_test.go | 51 ++++++++++++ internal/output/codes.go | 12 +++ internal/output/errors.go | 6 ++ skills/basecamp/SKILL.md | 2 +- 15 files changed, 598 insertions(+), 39 deletions(-) create mode 100644 internal/auth/progress.go create mode 100644 internal/auth/progress_test.go diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 96964ce82..38c26dd16 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -533,6 +533,18 @@ type LoginOptions struct { // If nil, messages are suppressed for headless/SDK use. Logger func(msg string) + // Progress, when it is a terminal, carries the live wait line drawn + // while the device flow polls for approval (spinner and expiry + // countdown, redrawn in place). Any other writer, or nil, gets a static + // "Waiting for approval" line through Logger instead. + Progress io.Writer + + // headlessReason is why defaults() turned the browser launch off on its + // own ("SSH session", "no display"), so the flow can say so instead of + // silently printing a link. Empty when the caller asked (NoBrowser) or + // a launch is going to be tried. + headlessReason string + // deviceOptions are appended last to the SDK device-flow options. // Test seam: lets tests inject WithDeviceSleep/WithDeviceClock. deviceOptions []oauth.DeviceOption @@ -540,17 +552,48 @@ type LoginOptions struct { // defaults fills in default values for LoginOptions. func (o *LoginOptions) defaults() { - if !o.Remote && !o.Local && hostutil.IsRemoteSession() { + autoRemote := !o.Remote && !o.Local && hostutil.IsRemoteSession() + if autoRemote { o.Remote = true } - if o.Remote || config.NonInteractiveEnv() { + // The launch is turned off when nobody could see the browser, and the + // reason is kept for the transcript when the CLI decided that on its + // own: the environment says no one is at this terminal, or the host has + // nowhere to open one. A caller who asked (--no-browser, --remote, + // --device-code) gets the link without commentary. --local is the + // person's word that the browser is right here and wins over the host + // heuristics. + switch { + case o.NoBrowser, o.Remote && !autoRemote: o.NoBrowser = true + case config.NonInteractiveEnv(): + o.NoBrowser, o.headlessReason = true, "BASECAMP_NONINTERACTIVE is set" + case !o.Local: + if reason := hostutil.HeadlessReason(); reason != "" { + o.NoBrowser, o.headlessReason = true, reason + } } if o.BrowserLauncher == nil && !o.NoBrowser { o.BrowserLauncher = openBrowser } } +// announceBrowser tries the launch and says what happened in one line. A +// failed launch is not a failed login — the link is already on screen — so +// the line points back at it. Explicit --no-browser prints nothing: the +// person asked for the link alone. +func (o *LoginOptions) announceBrowser(target string) { + switch { + case o.headlessReason != "": + o.log(fmt.Sprintf("Not opening a browser here (%s). Open the link on any device.", o.headlessReason)) + case o.NoBrowser || o.BrowserLauncher == nil: + case o.BrowserLauncher(target) != nil: + o.log("Couldn't open a browser. Open the link above.") + default: + o.log("Opening your browser… If nothing appears, open the link above.") + } +} + // log outputs a message if a logger is configured. func (o *LoginOptions) log(msg string) { if o.Logger != nil { @@ -721,17 +764,12 @@ func (m *Manager) loginLaunchpad(ctx context.Context, credKey string, oauthCfg * } defer func() { _ = listener.Close() }() - // Open browser for authentication - if opts.BrowserLauncher != nil { - if launchErr := opts.BrowserLauncher(authURL); launchErr != nil { - opts.log("\nCouldn't open browser automatically.\nOpen this URL in your browser:\n" + authURL + "\n\nWaiting for authentication...") - } else { - opts.log("\nOpening browser for authentication...") - opts.log("If the browser doesn't open, visit: " + authURL + "\n\nWaiting for authentication...") - } - } else { - opts.log("\nOpen this URL in your browser:\n" + authURL + "\n\nWaiting for authentication...") - } + opts.log("\nSign in to Basecamp\n") + opts.log(" Open this link in your browser") + opts.log(" " + authURL) + opts.log("") + opts.announceBrowser(authURL) + opts.log("Waiting for you to finish signing in… (times out in 5 minutes)") // Wait for OAuth callback with a hard timeout to avoid hanging indefinitely waitCtx, cancel := context.WithTimeout(ctx, 5*time.Minute) @@ -821,6 +859,7 @@ func (m *Manager) loginDevice(ctx context.Context, credKey string, oauthCfg *oau defer cancelDev() var displayErr error + var wait *approvalWait display := func(devAuth oauth.DeviceAuthorization) { // Validate the raw server-supplied URIs before printing or launching // anything: browser target is the code-embedding URI when valid, @@ -846,28 +885,43 @@ func (m *Manager) loginDevice(ctx context.Context, credKey string, oauthCfg *oau return } - opts.log("\nTo authenticate, open this URL in a browser on any device:") - opts.log(" " + shownURI) - opts.log("") - opts.log("and enter the code: " + userCode) - if devAuth.ExpiresIn > 0 { - opts.log(fmt.Sprintf("The code expires in %v.", time.Duration(devAuth.ExpiresIn)*time.Second)) + // Link first, code second, each on its own line so a double-click + // or a triple-click copies exactly one of them; the lifetime is + // stated where the code is. The warning is RFC 8628 §5.4's remote + // phishing defense in one sentence: a code someone else handed over + // approves their device, not this one. + lifetime := time.Duration(devAuth.ExpiresIn) * time.Second + codeStep := " 2. Enter this one-time code when asked" + if lifetime > 0 { + codeStep += " (expires in " + expiresIn(lifetime) + ")" } + opts.log("\nSign in to Basecamp\n") + opts.log(" 1. Open this link on any device") + opts.log(" " + shownURI) + opts.log(codeStep) + opts.log(" " + userCode) + opts.log("") + opts.log("Only continue if you started this login yourself. If a website or another") + opts.log("person gave you this code, press Ctrl-C now.") + opts.log("") // Flag matrix: default/--local launch the browser; --remote, // --device-code, and --no-browser (Remote implies NoBrowser) print // only. defaults() leaves BrowserLauncher nil in headless modes, but - // honor NoBrowser too so an injected launcher can't override it. - if !opts.NoBrowser && opts.BrowserLauncher != nil { - if launchErr := opts.BrowserLauncher(target); launchErr != nil { - opts.log("\nCouldn't open browser automatically — use the URL above.") - } else { - opts.log("\nOpening browser for authentication...") + // announceBrowser honors NoBrowser too so an injected launcher can't + // override it. + opts.announceBrowser(target) + if lifetime > 0 { + if wait = startApprovalWait(opts.Progress, time.Now().Add(lifetime)); wait != nil { + return } + opts.log("Waiting for approval… (the code expires in " + expiresIn(lifetime) + ")") + return } - opts.log("\nWaiting for approval...") + opts.log("Waiting for approval…") } token, err := oauth.PerformDeviceLogin(devCtx, oauthCfg, bc5ClientID, display, devOpts...) + wait.Stop() if displayErr != nil { // The malformed display data — not the cancellation it triggered — // is the real cause. diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 805451f12..26f9e0071 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -883,6 +883,8 @@ func TestLoginDefaultsNeverOpenABrowserUnderNonInteractiveEnv(t *testing.T) { t.Setenv("SSH_CONNECTION", "") t.Setenv("SSH_CLIENT", "") t.Setenv("SSH_TTY", "") + t.Setenv("CI", "") + t.Setenv("DISPLAY", ":0") opts := LoginOptions{Local: true} opts.defaults() assert.True(t, opts.NoBrowser) diff --git a/internal/auth/device_test.go b/internal/auth/device_test.go index b5b16c767..4e8ca4399 100644 --- a/internal/auth/device_test.go +++ b/internal/auth/device_test.go @@ -174,6 +174,9 @@ func newDeviceTestManager(t *testing.T, baseURL string) *Manager { t.Setenv("SSH_CONNECTION", "") t.Setenv("SSH_CLIENT", "") t.Setenv("SSH_TTY", "") + t.Setenv("CI", "") + t.Setenv("DISPLAY", ":0") + t.Setenv("BASECAMP_NONINTERACTIVE", "") // A pinned issuer in the developer's environment would bypass the // discovery every test here exercises. t.Setenv("BASECAMP_OAUTH_ISSUER", "") @@ -410,7 +413,98 @@ func TestLoginDevice_BrowserLaunchFailureContinues(t *testing.T) { }) require.NoError(t, err, "launch failure must not abort the flow") assert.Equal(t, "bc5", result.OAuthType) - assert.Contains(t, cl.joined(), "Couldn't open browser") + assert.Contains(t, cl.joined(), "Couldn't open a browser. Open the link above.") +} + +// TestLoginDevice_Transcript pins the device-flow copy: link first, code +// second, each on its own line, the lifetime beside the code, the phishing +// warning, and one browser line that says what happened. +func TestLoginDevice_Transcript(t *testing.T) { + as := startDeviceAS(t) + resource := startResourceServer(t, as.srv.URL) + + t.Run("browser opens", func(t *testing.T) { + m := newDeviceTestManager(t, resource.URL) + cl := &collectLogger{} + _, err := m.Login(context.Background(), LoginOptions{ + Logger: cl.log, + BrowserLauncher: func(string) error { return nil }, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + }) + require.NoError(t, err) + assert.Contains(t, cl.joined(), strings.Join([]string{ + "Sign in to Basecamp", + "", + " 1. Open this link on any device", + " " + as.srv.URL + "/verify?user_code=ABCD-EFGH", + " 2. Enter this one-time code when asked (expires in 10 minutes)", + " ABCD-EFGH", + "", + "Only continue if you started this login yourself. If a website or another", + "person gave you this code, press Ctrl-C now.", + "", + "Opening your browser… If nothing appears, open the link above.", + "Waiting for approval… (the code expires in 10 minutes)", + }, "\n")) + }) + + t.Run("headless host says why", func(t *testing.T) { + m := newDeviceTestManager(t, resource.URL) + t.Setenv("SSH_TTY", "/dev/pts/3") + cl := &collectLogger{} + launched := 0 + _, err := m.Login(context.Background(), LoginOptions{ + Logger: cl.log, + BrowserLauncher: func(string) error { launched++; return nil }, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + }) + require.NoError(t, err) + assert.Zero(t, launched) + assert.Contains(t, cl.joined(), "Not opening a browser here (SSH session). Open the link on any device.") + }) + + t.Run("--local overrides the host heuristics", func(t *testing.T) { + m := newDeviceTestManager(t, resource.URL) + t.Setenv("CI", "true") + launched := 0 + _, err := m.Login(context.Background(), LoginOptions{ + Local: true, + BrowserLauncher: func(string) error { launched++; return nil }, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + }) + require.NoError(t, err) + assert.Equal(t, 1, launched) + }) + + t.Run("--no-browser prints the link alone", func(t *testing.T) { + m := newDeviceTestManager(t, resource.URL) + cl := &collectLogger{} + _, err := m.Login(context.Background(), LoginOptions{ + NoBrowser: true, + Logger: cl.log, + BrowserLauncher: func(string) error { t.Fatal("must not launch"); return nil }, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + }) + require.NoError(t, err) + logs := cl.joined() + assert.NotContains(t, logs, "browser") + assert.Contains(t, logs, "Waiting for approval… (the code expires in 10 minutes)") + }) + + t.Run("a non-terminal progress writer gets the static line", func(t *testing.T) { + m := newDeviceTestManager(t, resource.URL) + cl := &collectLogger{} + var progress strings.Builder + _, err := m.Login(context.Background(), LoginOptions{ + NoBrowser: true, + Logger: cl.log, + Progress: &progress, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + }) + require.NoError(t, err) + assert.Empty(t, progress.String(), "nothing is drawn on a non-terminal") + assert.Contains(t, cl.joined(), "Waiting for approval… (the code expires in 10 minutes)") + }) } func TestLoginDevice_ScopeWiring(t *testing.T) { diff --git a/internal/auth/progress.go b/internal/auth/progress.go new file mode 100644 index 000000000..79e7de754 --- /dev/null +++ b/internal/auth/progress.go @@ -0,0 +1,116 @@ +package auth + +import ( + "fmt" + "io" + "os" + "sync" + "time" + + "github.com/charmbracelet/x/term" +) + +// approvalWait is the live line a terminal shows while the device flow polls +// for approval: a spinner, the prompt, and the time left before the code +// expires, redrawn in place and cleared when the flow ends. It exists so a +// person staring at the terminal can tell the CLI is still working and how +// long the code is good for — a silent poll and a "waiting" line printed +// once are indistinguishable from a hang. Non-terminal writers get nothing +// drawn; the caller logs a static line for those. +type approvalWait struct { + w io.Writer + deadline time.Time + now func() time.Time + interval time.Duration + + stopOnce sync.Once + stop chan struct{} + done chan struct{} +} + +var approvalFrames = [...]string{"⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"} + +const approvalInterval = 100 * time.Millisecond + +// startApprovalWait begins drawing on w when it is a terminal and returns +// nil otherwise, so callers can treat "no live line" uniformly: Stop on a +// nil *approvalWait is a no-op. +func startApprovalWait(w io.Writer, deadline time.Time) *approvalWait { + if !writerIsTerminal(w) { + return nil + } + return runApprovalWait(w, deadline, time.Now, approvalInterval) +} + +// runApprovalWait is the injectable core: tests drive it with a buffer, a +// fixed clock, and a short interval. +func runApprovalWait(w io.Writer, deadline time.Time, now func() time.Time, interval time.Duration) *approvalWait { + a := &approvalWait{ + w: w, + deadline: deadline, + now: now, + interval: interval, + stop: make(chan struct{}), + done: make(chan struct{}), + } + go a.run() + return a +} + +func (a *approvalWait) run() { + defer close(a.done) + ticker := time.NewTicker(a.interval) + defer ticker.Stop() + frame := 0 + for { + fmt.Fprintf(a.w, "\r\033[2K%s Waiting for approval… code expires in %s", approvalFrames[frame], remaining(a.deadline.Sub(a.now()))) + select { + case <-a.stop: + fmt.Fprint(a.w, "\r\033[2K") + return + case <-ticker.C: + frame = (frame + 1) % len(approvalFrames) + } + } +} + +// Stop clears the line and waits for the drawing goroutine to finish, so +// nothing the caller prints next can interleave with a redraw. +func (a *approvalWait) Stop() { + if a == nil { + return + } + a.stopOnce.Do(func() { + close(a.stop) + <-a.done + }) +} + +// remaining renders a duration as m:ss, floored at 0:00 — the shape a person +// reads as a countdown, unlike time.Duration's "9m41.3s". +func remaining(d time.Duration) string { + if d < 0 { + d = 0 + } + d = d.Round(time.Second) + return fmt.Sprintf("%d:%02d", int(d/time.Minute), int(d%time.Minute/time.Second)) +} + +// expiresIn renders a code lifetime for a static line: whole minutes when it +// is one, seconds otherwise ("10 minutes", "90 seconds"). +func expiresIn(d time.Duration) string { + switch { + case d >= time.Minute && d%time.Minute == 0: + if d == time.Minute { + return "1 minute" + } + return fmt.Sprintf("%d minutes", int(d/time.Minute)) + default: + return fmt.Sprintf("%d seconds", int(d/time.Second)) + } +} + +func writerIsTerminal(w io.Writer) bool { + f, ok := w.(*os.File) + return ok && term.IsTerminal(f.Fd()) +} diff --git a/internal/auth/progress_test.go b/internal/auth/progress_test.go new file mode 100644 index 000000000..e3a31bea0 --- /dev/null +++ b/internal/auth/progress_test.go @@ -0,0 +1,64 @@ +package auth + +import ( + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// syncBuffer is a strings.Builder the drawing goroutine and the test can +// share. +type syncBuffer struct { + cl collectLogger +} + +func (b *syncBuffer) Write(p []byte) (int, error) { + b.cl.log(string(p)) + return len(p), nil +} + +func (b *syncBuffer) String() string { + b.cl.mu.Lock() + defer b.cl.mu.Unlock() + return strings.Join(b.cl.logs, "") +} + +func TestApprovalWaitDrawsCountdownAndClears(t *testing.T) { + base := time.Date(2026, 9, 12, 12, 0, 0, 0, time.UTC) + clock := base + buf := &syncBuffer{} + wait := runApprovalWait(buf, base.Add(10*time.Minute), func() time.Time { return clock }, time.Millisecond) + + require.Eventually(t, func() bool { return strings.Contains(buf.String(), "code expires in 10:00") }, time.Second, time.Millisecond) + clock = base.Add(19 * time.Second) + require.Eventually(t, func() bool { return strings.Contains(buf.String(), "code expires in 9:41") }, time.Second, time.Millisecond) + + wait.Stop() + wait.Stop() // idempotent + + out := buf.String() + assert.True(t, strings.HasSuffix(out, "\r\033[2K"), "the line is cleared when the wait ends") + assert.Contains(t, out, "Waiting for approval…") + assert.NotContains(t, out, "\n", "the live line never scrolls") + + var none *approvalWait + none.Stop() +} + +func TestStartApprovalWaitNeedsATerminal(t *testing.T) { + assert.Nil(t, startApprovalWait(nil, time.Now().Add(time.Minute))) + assert.Nil(t, startApprovalWait(&strings.Builder{}, time.Now().Add(time.Minute))) +} + +func TestRemainingAndExpiresIn(t *testing.T) { + assert.Equal(t, "9:41", remaining(9*time.Minute+41*time.Second)) + assert.Equal(t, "0:05", remaining(4900*time.Millisecond)) + assert.Equal(t, "0:00", remaining(-time.Second)) + assert.Equal(t, "10 minutes", expiresIn(10*time.Minute)) + assert.Equal(t, "1 minute", expiresIn(time.Minute)) + assert.Equal(t, "90 seconds", expiresIn(90*time.Second)) + assert.Equal(t, "45 seconds", expiresIn(45*time.Second)) +} diff --git a/internal/cli/root.go b/internal/cli/root.go index 16301d5f9..c2eb34ddd 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -421,6 +421,12 @@ func Execute() { // Convert error to structured output apiErr := output.AsError(err) + // An interrupted command has already told the person what stopped; + // an error envelope on top would dress a Ctrl-C up as a failure. + if apiErr.Code == output.CodeInterrupted { + os.Exit(output.ExitInterrupted) + } + // Commands whose stdout speaks a wire protocol (basecamp mcp: // JSON-RPC) keep errors off stdout entirely — an error envelope // there is a malformed protocol message that hides the real failure diff --git a/internal/commands/auth.go b/internal/commands/auth.go index 28fca10e8..13edc519c 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -7,9 +7,11 @@ import ( "fmt" "io" "os" + "os/signal" "sort" "strconv" "strings" + "syscall" "time" "unicode" @@ -263,9 +265,15 @@ func buildLoginCmd(use string) *cobra.Command { Short: "Authenticate with Basecamp", Long: `Start the OAuth flow to authenticate with Basecamp, or import a personal access token. +The default flow prints a link and a one-time code, opens the link in your +browser, and waits for you to approve it. Over SSH, in CI, or on a host with no +display the browser is skipped and the link is yours to open on any device — +your phone included; --local forces a launch anyway, --no-browser skips it. +Ctrl-C cancels the wait. + Examples: basecamp auth login # Browser (or device) flow - basecamp auth login --device-code # Headless: approve the printed code elsewhere + basecamp auth login --device-code # Headless: approve the printed code from any device basecamp auth login --expect-identity 12345 # Refuse the login unless it is this identity Import a personal access token from stdin (never pass it as an argument): @@ -336,21 +344,25 @@ named profile, creating the profile when --account is given. fmt.Fprintln(w, r.Summary.Render("Starting Basecamp authentication...")) } + ctx, stop := loginContext(cmd) + defer stop() + // With an expectation the login is assertive: the token is // checked before it is stored, and a mismatch stores nothing. // Without one the identity line stays informational. verifier := &loginVerifier{app: app, expectIdentity: expect, account: app.Config.AccountID, strict: expect != 0} - result, err := app.Auth.Login(cmd.Context(), auth.LoginOptions{ + result, err := app.Auth.Login(ctx, auth.LoginOptions{ Scope: scope, NoBrowser: noBrowser, Remote: remote, Local: local, LoginHint: loginHint, Logger: func(msg string) { fmt.Fprintln(w, msg) }, + Progress: w, Verify: verifier.verify, }) if err != nil { - return err + return loginOutcome(ctx, err, w, r) } fmt.Fprintln(w) @@ -374,10 +386,10 @@ named profile, creating the profile when --account is given. } cmd.Flags().StringVar(&scope, "scope", "", "OAuth scope: 'read' or 'full' (default full; ignored by Launchpad)") - cmd.Flags().BoolVar(&noBrowser, "no-browser", false, "Don't open browser automatically") - cmd.Flags().BoolVar(&remote, "remote", false, "Force remote/headless mode (paste callback URL instead of local listener)") - cmd.Flags().BoolVar(&local, "local", false, "Force local mode (override SSH auto-detection)") - cmd.Flags().BoolVar(&deviceCode, "device-code", false, "Headless authentication with manual browser instructions") + cmd.Flags().BoolVar(&noBrowser, "no-browser", false, "Print the link instead of opening a browser") + cmd.Flags().BoolVar(&remote, "remote", false, "Treat this as a remote session: print the link, and paste the callback URL back when the server has no device flow (auto-detected over SSH)") + cmd.Flags().BoolVar(&local, "local", false, "Treat this as a local session: open the browser here even over SSH, in CI, or without a display") + cmd.Flags().BoolVar(&deviceCode, "device-code", false, "Print a link and one-time code to approve from any device; never opens a browser here") cmd.Flags().BoolVar(&withToken, "with-token", false, "Read a personal access token from stdin instead of running OAuth (requires --profile)") cmd.Flags().StringVar(&expectIdentity, "expect-identity", "", "Identity ID the login must authenticate as; otherwise store nothing") cmd.Flags().StringVar(&loginHint, "login-hint", "", "Email address to sign in as on the device-flow approval page (ignored by Launchpad)") @@ -390,6 +402,28 @@ named profile, creating the profile when --account is given. return cmd } +// loginContext derives the context an interactive login waits under: Ctrl-C +// (and a SIGTERM) cancels it instead of killing the process mid-line, so +// the flow can put the terminal back — clear the live wait line, close the +// loopback listener, stop polling — and say it was canceled. Callers must +// stop it before printing their outcome. +func loginContext(cmd *cobra.Command) (context.Context, context.CancelFunc) { + return signal.NotifyContext(cmd.Context(), os.Interrupt, syscall.SIGTERM) +} + +// loginOutcome turns a failed Login into the error the root will render. A +// login the person canceled is not a failure: the human line goes to the +// terminal here and the error carries the interrupted code, which the root +// exits with silently. Everything else is returned as it came. +func loginOutcome(ctx context.Context, err error, w io.Writer, r *output.Renderer) error { + if !errors.Is(ctx.Err(), context.Canceled) { + return err + } + fmt.Fprintln(w) + fmt.Fprintln(w, r.Muted.Render("Login canceled. Nothing was stored.")) + return output.ErrInterrupted("login canceled") +} + // runLoginWithToken imports a personal access token from stdin as the // active profile's credential. Everything that can be checked without the // token is checked before stdin is read; the token is then verified through diff --git a/internal/commands/auth_login_test.go b/internal/commands/auth_login_test.go index 6dcb6bf2c..b9baa7ba2 100644 --- a/internal/commands/auth_login_test.go +++ b/internal/commands/auth_login_test.go @@ -40,6 +40,8 @@ func TestAuthLoginDeviceCodeForcesRemoteMode(t *testing.T) { t.Setenv("SSH_CONNECTION", "") t.Setenv("SSH_CLIENT", "") t.Setenv("SSH_TTY", "") + t.Setenv("CI", "") + t.Setenv("DISPLAY", ":0") // No protected-resource metadata (404) → Launchpad fallback, pointed at // this server. The token endpoint is never reached. @@ -780,7 +782,8 @@ func TestAuthLoginDeviceCodeRunsUnderNonInteractiveEnv(t *testing.T) { out, err := runLogin(t, app, strings.NewReader(""), "--device-code") require.NoError(t, err, out) - assert.Contains(t, out, "and enter the code: ABCD-EFGH") + assert.Contains(t, out, " 2. Enter this one-time code when asked (expires in 10 minutes)\n ABCD-EFGH") + assert.NotContains(t, out, "browser", "--device-code asked for the link alone; no launch, no commentary") assert.Contains(t, out, "Authentication successful") } @@ -820,6 +823,8 @@ func TestAuthLoginDeviceFlowExpectIdentityStoresNothingOnMismatch(t *testing.T) t.Setenv("SSH_CONNECTION", "") t.Setenv("SSH_CLIENT", "") t.Setenv("SSH_TTY", "") + t.Setenv("CI", "") + t.Setenv("DISPLAY", ":0") out, err := runLogin(t, app, strings.NewReader(""), "--device-code", "--expect-identity", "1") require.Error(t, err) @@ -1001,6 +1006,8 @@ func TestAuthLoginDeviceFlowWithoutExpectationKeepsBestEffortIdentity(t *testing t.Setenv("SSH_CONNECTION", "") t.Setenv("SSH_CLIENT", "") t.Setenv("SSH_TTY", "") + t.Setenv("CI", "") + t.Setenv("DISPLAY", ":0") out, err := runLogin(t, app, strings.NewReader(""), "--device-code") require.NoError(t, err, out) @@ -1190,6 +1197,8 @@ func TestAuthLoginDeviceFlowExpectIdentityKeepsAShortLivedAccessToken(t *testing t.Setenv("SSH_CONNECTION", "") t.Setenv("SSH_CLIENT", "") t.Setenv("SSH_TTY", "") + t.Setenv("CI", "") + t.Setenv("DISPLAY", ":0") out, err := runLogin(t, app, strings.NewReader(""), "--device-code", "--expect-identity", "28142355") require.NoError(t, err, out) @@ -1219,3 +1228,57 @@ func TestAuthLoginWithTokenRefusesToRewriteANonObjectProfilesValue(t *testing.T) _, loadErr := app.Auth.GetStore().Load("profile:bot") assert.Error(t, loadErr) } + +// TestAuthLoginCtrlCCancelsCleanly: an interrupt while the device flow is +// waiting for approval ends the login with the interrupted code (exit 130, +// rendered by nothing but the command's own line) and stores nothing. +func TestAuthLoginCtrlCCancelsCleanly(t *testing.T) { + srv := startLoginIdentityServer(t, "dev-tok") + pending := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/oauth/device_authorizations": + fmt.Fprintf(w, `{"device_code":"dc","user_code":"ABCD-EFGH","verification_uri":%q,"expires_in":600,"interval":1}`, "http://"+r.Host+"/verify") + case "/oauth/tokens": + w.WriteHeader(http.StatusBadRequest) + fmt.Fprint(w, `{"error":"authorization_pending"}`) + default: + srv.srv.Config.Handler.ServeHTTP(w, r) + } + }) + srv.srv.Config.Handler = pending + app, _ := loginTestApp(t, srv, &config.Config{}) + t.Setenv("BASECAMP_OAUTH_ISSUER", srv.srv.URL) + t.Setenv("SSH_CONNECTION", "") + t.Setenv("SSH_CLIENT", "") + t.Setenv("SSH_TTY", "") + t.Setenv("CI", "") + t.Setenv("DISPLAY", ":0") + + // The parent context stands in for the signal: loginContext derives + // from it, so canceling it is what Ctrl-C does to the flow. + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + time.AfterFunc(300*time.Millisecond, cancel) + + cmd := NewAuthCmd() + cmd.SetArgs([]string{"login", "--no-browser"}) + cmd.SetContext(appctx.WithApp(ctx, app)) + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&out) + cmd.SilenceErrors = true + cmd.SilenceUsage = true + + err := cmd.Execute() + require.Error(t, err) + var outErr *output.Error + require.ErrorAs(t, err, &outErr) + assert.Equal(t, output.CodeInterrupted, outErr.Code) + assert.Equal(t, output.ExitInterrupted, output.ExitCodeFor(outErr.Code)) + + assert.Contains(t, out.String(), "ABCD-EFGH", "the code was shown before the cancel") + assert.Contains(t, out.String(), "Login canceled. Nothing was stored.") + assert.NotContains(t, out.String(), "Authentication successful") + assert.False(t, app.Auth.IsAuthenticated(), "a canceled login stores nothing") +} diff --git a/internal/commands/profile.go b/internal/commands/profile.go index 177abdecc..c6fe2c3c3 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -280,13 +280,17 @@ Examples: // With an expectation the credential is checked before it is // stored and a mismatch stores nothing; without one the // identity lookup stays informational. + ctx, stop := loginContext(cmd) + defer stop() + w := cmd.OutOrStdout() verifier := &loginVerifier{app: app, expectIdentity: expect, account: accountID, strict: expect != 0} - loginResult, err := app.Auth.Login(cmd.Context(), auth.LoginOptions{ + loginResult, err := app.Auth.Login(ctx, auth.LoginOptions{ Scope: scope, NoBrowser: noBrowser, Remote: remote, Local: local, - Logger: func(msg string) { fmt.Println(msg) }, + Logger: func(msg string) { fmt.Fprintln(w, msg) }, + Progress: w, Verify: verifier.verify, }) if err != nil { @@ -294,7 +298,7 @@ Examples: delete(app.Config.Profiles, name) app.Config.ActiveProfile = prevActiveProfile app.Config.BaseURL = prevBaseURL - return err + return loginOutcome(ctx, err, w, output.NewRenderer(w, false)) } // Login succeeded — persist profile to config diff --git a/internal/commands/profile_test.go b/internal/commands/profile_test.go index 386aa1102..2cd0ca677 100644 --- a/internal/commands/profile_test.go +++ b/internal/commands/profile_test.go @@ -246,6 +246,8 @@ func TestProfileCreateDeviceCodeForcesRemoteMode(t *testing.T) { t.Setenv("SSH_CONNECTION", "") t.Setenv("SSH_CLIENT", "") t.Setenv("SSH_TTY", "") + t.Setenv("CI", "") + t.Setenv("DISPLAY", ":0") // No protected-resource metadata (404) → Launchpad fallback, pointed at // this server. The token endpoint is never reached. @@ -1331,6 +1333,8 @@ func TestProfileCreateExpectIdentityCreatesNothingOnMismatch(t *testing.T) { t.Setenv("SSH_CONNECTION", "") t.Setenv("SSH_CLIENT", "") t.Setenv("SSH_TTY", "") + t.Setenv("CI", "") + t.Setenv("DISPLAY", ":0") create := func(expect string) error { return executeProfileCommand(NewProfileCmd(), app, "create", "bot", "--base-url", srv.srv.URL, "--account", "999", "--device-code", "--expect-identity", expect) } diff --git a/internal/hostutil/hostutil.go b/internal/hostutil/hostutil.go index d93b85c1a..8837c3eb7 100644 --- a/internal/hostutil/hostutil.go +++ b/internal/hostutil/hostutil.go @@ -125,6 +125,55 @@ func IsRemoteSession() bool { os.Getenv("SSH_TTY") != "" } +// HeadlessReason reports why this host cannot show a browser to the person +// at the terminal, or "" when launching one is worth trying. It is the +// browser-launch half of session detection: IsRemoteSession decides whether a +// loopback callback can be reached at all, this decides whether `open` / +// `xdg-open` would land anywhere the user can see. The reason is written for +// a terminal line ("Not opening a browser here (SSH session)"). +// +// Only environment is consulted, so the answer is cheap and deterministic: +// an SSH session (any of the three variables sshd sets), a CI runner (the +// CI variable every major provider exports), or a Unix host with neither an +// X11 nor a Wayland display. macOS and Windows have no display variable to +// check; a GUI-less session there is rare enough to leave to the launch +// error path. +func HeadlessReason() string { + switch { + case IsRemoteSession(): + return "SSH session" + case envTruthy(os.Getenv("CI")): + return "CI environment" + case unixWithoutDisplay(): + return "no display" + default: + return "" + } +} + +// unixWithoutDisplay is true on the Unix platforms whose browsers need a +// display server when neither DISPLAY nor WAYLAND_DISPLAY is set. +func unixWithoutDisplay() bool { + switch runtime.GOOS { + case "linux", "freebsd", "openbsd", "netbsd", "dragonfly", "solaris", "illumos": + return os.Getenv("DISPLAY") == "" && os.Getenv("WAYLAND_DISPLAY") == "" + default: + return false + } +} + +// envTruthy reads a boolean-ish environment value the way CI providers set +// it: "true", "1", "yes" and "on" (any case) count; everything else, +// including the empty string, does not. +func envTruthy(v string) bool { + switch strings.ToLower(strings.TrimSpace(v)) { + case "1", "true", "yes", "on": + return true + default: + return false + } +} + // OpenBrowser opens the specified URL in the default browser. func OpenBrowser(url string) error { var cmd string diff --git a/internal/hostutil/hostutil_test.go b/internal/hostutil/hostutil_test.go index ab65fcf74..87427c721 100644 --- a/internal/hostutil/hostutil_test.go +++ b/internal/hostutil/hostutil_test.go @@ -203,3 +203,54 @@ func TestIsTrustedBasecampHost(t *testing.T) { }) } } + +// pinInteractiveHost clears every signal HeadlessReason reads so a test +// observes the interactive answer regardless of the machine it runs on (CI +// exports CI=true and Linux runners have no display). +func pinInteractiveHost(t *testing.T) { + t.Helper() + for _, key := range []string{"SSH_CONNECTION", "SSH_CLIENT", "SSH_TTY", "CI"} { + t.Setenv(key, "") + } + t.Setenv("DISPLAY", ":0") + t.Setenv("WAYLAND_DISPLAY", "") +} + +func TestHeadlessReason(t *testing.T) { + pinInteractiveHost(t) + assert.Empty(t, HeadlessReason(), "a display and no SSH or CI is interactive") + + t.Run("ssh", func(t *testing.T) { + pinInteractiveHost(t) + t.Setenv("SSH_TTY", "/dev/pts/0") + assert.Equal(t, "SSH session", HeadlessReason()) + }) + t.Run("ci", func(t *testing.T) { + for _, v := range []string{"true", "1", "YES"} { + pinInteractiveHost(t) + t.Setenv("CI", v) + assert.Equal(t, "CI environment", HeadlessReason(), "CI=%s", v) + } + pinInteractiveHost(t) + t.Setenv("CI", "false") + assert.Empty(t, HeadlessReason(), "CI=false is not a CI runner") + }) + t.Run("ssh outranks ci", func(t *testing.T) { + pinInteractiveHost(t) + t.Setenv("CI", "true") + t.Setenv("SSH_CLIENT", "10.0.0.1 12345 22") + assert.Equal(t, "SSH session", HeadlessReason()) + }) + t.Run("no display", func(t *testing.T) { + pinInteractiveHost(t) + t.Setenv("DISPLAY", "") + want := "" + if unixWithoutDisplay() { + want = "no display" + } + assert.Equal(t, want, HeadlessReason()) + + t.Setenv("WAYLAND_DISPLAY", "wayland-0") + assert.Empty(t, HeadlessReason(), "a Wayland display is a display") + }) +} diff --git a/internal/output/codes.go b/internal/output/codes.go index e6f70766d..262c43740 100644 --- a/internal/output/codes.go +++ b/internal/output/codes.go @@ -40,6 +40,16 @@ const ( ExitLimit = 10 // Account limit reached (507) ) +// CodeInterrupted marks a command the person stopped with Ctrl-C after it +// had started waiting on them (a login waiting for approval). The command +// has already said what happened on its own terms, so the root renders +// nothing more and exits with the shell's conventional status for a +// SIGINT-terminated process. +const ( + CodeInterrupted = "interrupted" + ExitInterrupted = 130 +) + // ExitCodeFor returns the exit code for a given error code. func ExitCodeFor(code string) int { switch code { @@ -47,6 +57,8 @@ func ExitCodeFor(code string) int { return ExitValidation case CodeLimitExceeded: return ExitLimit + case CodeInterrupted: + return ExitInterrupted } return clioutput.ExitCodeFor(code) } diff --git a/internal/output/errors.go b/internal/output/errors.go index d48b0a0f6..f645d0068 100644 --- a/internal/output/errors.go +++ b/internal/output/errors.go @@ -122,6 +122,12 @@ func ErrAuth(msg string) *Error { } } +// ErrInterrupted reports a command stopped by the person at the terminal. +// msg is for machine consumers; the command prints its own human line. +func ErrInterrupted(msg string) *Error { + return &Error{Code: CodeInterrupted, Message: msg} +} + func ErrForbiddenScope() *Error { return &Error{ Code: CodeForbidden, diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index bb708a396..c1e6065e3 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -1391,7 +1391,7 @@ basecamp auth status # Check auth basecamp auth login # Re-authenticate basecamp auth login --scope full # Full access (the default; ignored by Launchpad) basecamp auth login --scope read # Read-only access (ignored by Launchpad) -basecamp auth login --device-code # Headless authentication with manual browser instructions +basecamp auth login --device-code # Print a link and one-time code to approve from any device BASECAMP_NONINTERACTIVE=1 basecamp auth login --device-code # The only OAuth login that runs under BASECAMP_NONINTERACTIVE, and only where the server offers the device flow (Launchpad does not); browser and pasted-callback flows refuse — prefer --with-token basecamp auth login --with-token -P bot --account # Import a personal access token from stdin (pipe it in) basecamp auth login --expect-identity # Discard the login unless it authenticated as this identity From 8535966719828c4e05b30e8d3fa38b0e314b0970 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 18:32:34 -0700 Subject: [PATCH 02/10] Route headless Launchpad logins through the pasted callback, guard the store after a cancel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A CI runner or display-less Unix host now skips the browser, but a Launchpad login there still bound the loopback callback while the transcript told the person to open the link on any device — a device that could never reach this host's 127.0.0.1. A headless host is a remote one: defaults() selects the pasted-callback flow for it, and --local still keeps the listener. A cancel that lands between the token being issued and stored — Ctrl-C in the instant the approval completes — reached the store, because the non-strict verifier answers a canceled request with nil. verifyBeforeStore checks the context on both sides of Verify. The setup wizard shares the login path, so it gets the signal-aware context, the live wait line and the canceled outcome too, and its logger drops the Launchpad discovery line instead of rewriting it into a second "opening browser" line. The four flow flags are registered once for auth login and profile create so their help cannot drift, and --device-code's help says what happens on a Launchpad server. The progress test keeps its injected clock in an atomic so the race detector is satisfied. --- e2e/auth.bats | 6 +++-- internal/auth/auth.go | 45 ++++++++++++++++++++++++-------- internal/auth/auth_test.go | 24 +++++++++++++++++ internal/auth/device_test.go | 24 +++++++++++++++++ internal/auth/progress_test.go | 10 ++++--- internal/commands/auth.go | 16 +++++++++--- internal/commands/profile.go | 5 +--- internal/commands/wizard.go | 19 +++++++++----- internal/commands/wizard_test.go | 10 +++---- 9 files changed, 124 insertions(+), 35 deletions(-) diff --git a/e2e/auth.bats b/e2e/auth.bats index 986508c65..bdc400dea 100644 --- a/e2e/auth.bats +++ b/e2e/auth.bats @@ -77,7 +77,8 @@ load test_helper @test "basecamp auth login --help describes flags provider-neutrally" { run basecamp auth login --help assert_success - assert_output_contains "Headless authentication with manual browser instructions" + assert_output_contains "one-time code to approve from any device" + assert_output_contains "Launchpad has no device flow" assert_output_contains "ignored by Launchpad" assert_output_contains "default full" } @@ -85,7 +86,8 @@ load test_helper @test "basecamp profile create --help describes flags provider-neutrally" { run basecamp profile create --help assert_success - assert_output_contains "Headless authentication with manual browser instructions" + assert_output_contains "one-time code to approve from any device" + assert_output_contains "Launchpad has no device flow" assert_output_contains "ignored by Launchpad" assert_output_contains "default full" } diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 38c26dd16..9b970947d 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -563,6 +563,11 @@ func (o *LoginOptions) defaults() { // --device-code) gets the link without commentary. --local is the // person's word that the browser is right here and wins over the host // heuristics. + // + // A headless host is also a remote one: the link is going to be opened + // on some other device, so a Launchpad login must take the pasted + // callback rather than listen on this host's loopback, which that + // device could never reach. switch { case o.NoBrowser, o.Remote && !autoRemote: o.NoBrowser = true @@ -570,7 +575,7 @@ func (o *LoginOptions) defaults() { o.NoBrowser, o.headlessReason = true, "BASECAMP_NONINTERACTIVE is set" case !o.Local: if reason := hostutil.HeadlessReason(); reason != "" { - o.NoBrowser, o.headlessReason = true, reason + o.NoBrowser, o.Remote, o.headlessReason = true, true, reason } } if o.BrowserLauncher == nil && !o.NoBrowser { @@ -792,11 +797,8 @@ func (m *Manager) loginLaunchpad(ctx context.Context, credKey string, oauthCfg * creds.TokenEndpoint = oauthCfg.TokenEndpoint creds.Scope = "" - if opts.Verify != nil { - if err := opts.Verify(ctx, creds.AccessToken, oauthTypeLaunchpad); err != nil { - m.discardGrant(ctx, creds, opts.log) - return nil, err - } + if err := m.verifyBeforeStore(ctx, opts, creds, oauthTypeLaunchpad); err != nil { + return nil, err } if err := m.store.Save(credKey, creds); err != nil { return nil, err @@ -955,11 +957,8 @@ func (m *Manager) loginDevice(ctx context.Context, credKey string, oauthCfg *oau creds.ExpiresAt = token.ExpiresAt.Unix() } - if opts.Verify != nil { - if err := opts.Verify(ctx, creds.AccessToken, oauthTypeBC5); err != nil { - m.discardGrant(ctx, creds, opts.log) - return nil, err - } + if err := m.verifyBeforeStore(ctx, opts, creds, oauthTypeBC5); err != nil { + return nil, err } if err := m.store.Save(credKey, creds); err != nil { return nil, err @@ -968,6 +967,30 @@ func (m *Manager) loginDevice(ctx context.Context, credKey string, oauthCfg *oau return &LoginResult{OAuthType: oauthTypeBC5, Scope: effectiveScope}, nil } +// verifyBeforeStore runs the caller's Verify hook and refuses to let a +// canceled login reach the store. The token arrives from the flow after +// the person may already have pressed Ctrl-C — the poll or exchange can +// complete in the same instant — and a non-strict verifier answers a +// canceled request with nil, so without this check a login the person +// stopped would still be saved and announced as a success. +func (m *Manager) verifyBeforeStore(ctx context.Context, opts *LoginOptions, creds *Credentials, oauthType string) error { + if err := ctx.Err(); err != nil { + m.discardGrant(ctx, creds, opts.log) + return err + } + if opts.Verify != nil { + if err := opts.Verify(ctx, creds.AccessToken, oauthType); err != nil { + m.discardGrant(ctx, creds, opts.log) + return err + } + } + if err := ctx.Err(); err != nil { + m.discardGrant(ctx, creds, opts.log) + return err + } + return nil +} + // validVerificationURL validates a server-supplied verification URI with the // same policy as other OAuth browser URLs (https, or http on loopback, no // userinfo). Returns the raw URL when valid, "" otherwise. diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 26f9e0071..3e1a3a6aa 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -2212,3 +2212,27 @@ func TestSetUserIdentity_WritesUnderEnvToken(t *testing.T) { assert.Equal(t, "2", creds.UserID) assert.Equal(t, "who@example.com", creds.UserEmail) } + +// TestLoginLaunchpad_HeadlessHostTakesThePastedCallback: a host that cannot +// show a browser (here a CI runner) is also one whose loopback the browser +// on another device could never reach, so the Launchpad flow must ask for +// the pasted callback URL rather than listen. +func TestLoginLaunchpad_HeadlessHostTakesThePastedCallback(t *testing.T) { + t.Setenv("BASECAMP_NONINTERACTIVE", "") + t.Setenv("SSH_CONNECTION", "") + t.Setenv("SSH_CLIENT", "") + t.Setenv("SSH_TTY", "") + t.Setenv("DISPLAY", ":0") + t.Setenv("CI", "true") + + opts := LoginOptions{} + opts.defaults() + assert.True(t, opts.Remote, "a headless host pastes the callback") + assert.True(t, opts.NoBrowser) + assert.Equal(t, "CI environment", opts.headlessReason) + + local := LoginOptions{Local: true} + local.defaults() + assert.False(t, local.Remote, "--local keeps the loopback listener") + assert.False(t, local.NoBrowser) +} diff --git a/internal/auth/device_test.go b/internal/auth/device_test.go index 4e8ca4399..bb0f6dfc9 100644 --- a/internal/auth/device_test.go +++ b/internal/auth/device_test.go @@ -1319,3 +1319,27 @@ func TestDiscoverOAuth_PinnedIssuerIsSanitizedForTheTerminal(t *testing.T) { assert.NotContains(t, err.Error(), "\u0085") assert.NotContains(t, cl.joined(), "\u0085") } + +// TestLoginDevice_CancelDuringVerifyStoresNothing: a cancel that lands after +// the token was issued but before it is stored — Ctrl-C in the same instant +// the approval completes — must not save the credential, even though a +// non-strict verifier answers a canceled request with nil. +func TestLoginDevice_CancelDuringVerifyStoresNothing(t *testing.T) { + as := startDeviceAS(t) + resource := startResourceServer(t, as.srv.URL) + m := newDeviceTestManager(t, resource.URL) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + _, err := m.Login(ctx, LoginOptions{ + NoBrowser: true, + deviceOptions: []oauth.DeviceOption{instantSleep()}, + Verify: func(context.Context, string, string) error { + cancel() + return nil + }, + }) + require.ErrorIs(t, err, context.Canceled) + _, loadErr := m.store.Load(config.NormalizeBaseURL(resource.URL)) + require.Error(t, loadErr, "a canceled login stores nothing") +} diff --git a/internal/auth/progress_test.go b/internal/auth/progress_test.go index e3a31bea0..2087ef7df 100644 --- a/internal/auth/progress_test.go +++ b/internal/auth/progress_test.go @@ -2,6 +2,7 @@ package auth import ( "strings" + "sync/atomic" "testing" "time" @@ -28,12 +29,15 @@ func (b *syncBuffer) String() string { func TestApprovalWaitDrawsCountdownAndClears(t *testing.T) { base := time.Date(2026, 9, 12, 12, 0, 0, 0, time.UTC) - clock := base + // The drawing goroutine reads the clock while the test advances it, so + // the elapsed time lives in an atomic. + var elapsed atomic.Int64 + now := func() time.Time { return base.Add(time.Duration(elapsed.Load())) } buf := &syncBuffer{} - wait := runApprovalWait(buf, base.Add(10*time.Minute), func() time.Time { return clock }, time.Millisecond) + wait := runApprovalWait(buf, base.Add(10*time.Minute), now, time.Millisecond) require.Eventually(t, func() bool { return strings.Contains(buf.String(), "code expires in 10:00") }, time.Second, time.Millisecond) - clock = base.Add(19 * time.Second) + elapsed.Store(int64(19 * time.Second)) require.Eventually(t, func() bool { return strings.Contains(buf.String(), "code expires in 9:41") }, time.Second, time.Millisecond) wait.Stop() diff --git a/internal/commands/auth.go b/internal/commands/auth.go index 13edc519c..13b9b1ef3 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -386,10 +386,7 @@ named profile, creating the profile when --account is given. } cmd.Flags().StringVar(&scope, "scope", "", "OAuth scope: 'read' or 'full' (default full; ignored by Launchpad)") - cmd.Flags().BoolVar(&noBrowser, "no-browser", false, "Print the link instead of opening a browser") - cmd.Flags().BoolVar(&remote, "remote", false, "Treat this as a remote session: print the link, and paste the callback URL back when the server has no device flow (auto-detected over SSH)") - cmd.Flags().BoolVar(&local, "local", false, "Treat this as a local session: open the browser here even over SSH, in CI, or without a display") - cmd.Flags().BoolVar(&deviceCode, "device-code", false, "Print a link and one-time code to approve from any device; never opens a browser here") + registerLoginFlowFlags(cmd, &noBrowser, &remote, &local, &deviceCode) cmd.Flags().BoolVar(&withToken, "with-token", false, "Read a personal access token from stdin instead of running OAuth (requires --profile)") cmd.Flags().StringVar(&expectIdentity, "expect-identity", "", "Identity ID the login must authenticate as; otherwise store nothing") cmd.Flags().StringVar(&loginHint, "login-hint", "", "Email address to sign in as on the device-flow approval page (ignored by Launchpad)") @@ -402,6 +399,17 @@ named profile, creating the profile when --account is given. return cmd } +// registerLoginFlowFlags declares the flags that choose how the OAuth flow +// reaches a browser, once, for every command that runs a login (auth login, +// profile create): their help text is the one place the headless behavior +// is described, so the two commands must not drift. +func registerLoginFlowFlags(cmd *cobra.Command, noBrowser, remote, local, deviceCode *bool) { + cmd.Flags().BoolVar(noBrowser, "no-browser", false, "Print the link instead of opening a browser") + cmd.Flags().BoolVar(remote, "remote", false, "Treat this as a remote session: print the link without opening a browser, and paste the callback URL back when the server has no device flow (auto-detected over SSH, in CI, and without a display)") + cmd.Flags().BoolVar(local, "local", false, "Treat this as a local session: open the browser here even over SSH, in CI, or without a display") + cmd.Flags().BoolVar(deviceCode, "device-code", false, "Print a link and one-time code to approve from any device, never opening a browser here (Launchpad has no device flow: paste the callback URL back instead)") +} + // loginContext derives the context an interactive login waits under: Ctrl-C // (and a SIGTERM) cancels it instead of killing the process mid-line, so // the flow can put the terminal back — clear the live wait line, close the diff --git a/internal/commands/profile.go b/internal/commands/profile.go index c6fe2c3c3..bb930358b 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -337,10 +337,7 @@ Examples: cmd.Flags().StringVar(&baseURL, "base-url", "", "Basecamp API base URL (default: https://3.basecampapi.com)") cmd.Flags().StringVar(&scope, "scope", "", "OAuth scope: 'read' or 'full' (default full; ignored by Launchpad)") cmd.Flags().StringVar(&accountID, "account", "", "Account ID") - cmd.Flags().BoolVar(&noBrowser, "no-browser", false, "Don't open browser automatically") - cmd.Flags().BoolVar(&remote, "remote", false, "Force remote/headless mode (paste callback URL instead of local listener)") - cmd.Flags().BoolVar(&local, "local", false, "Force local mode (override SSH auto-detection)") - cmd.Flags().BoolVar(&deviceCode, "device-code", false, "Headless authentication with manual browser instructions") + registerLoginFlowFlags(cmd, &noBrowser, &remote, &local, &deviceCode) cmd.Flags().StringVar(&expectIdentity, "expect-identity", "", "Identity ID the login must authenticate as; otherwise create nothing") cmd.MarkFlagsMutuallyExclusive("remote", "local") cmd.MarkFlagsMutuallyExclusive("device-code", "local") diff --git a/internal/commands/wizard.go b/internal/commands/wizard.go index d7bdba1a8..9ba4434e6 100644 --- a/internal/commands/wizard.go +++ b/internal/commands/wizard.go @@ -1,6 +1,7 @@ package commands import ( + "context" "encoding/json" "errors" "fmt" @@ -274,13 +275,13 @@ func showAuthenticationStart(w io.Writer, styles *tui.Styles, stepByStep bool) s return " " } +// authenticationLogger indents the login transcript under the wizard's +// step and drops the Launchpad discovery line: it names an authorization +// URL nobody needs to see, and the flow's own "Sign in to Basecamp" block +// that follows says everything the person does. func authenticationLogger(w io.Writer, prefix string) func(string) { - launchpadOpeningShown := false return func(message string) { if strings.HasPrefix(message, "Authenticating via launchpad (") { - message = "Opening browser for Basecamp login..." - launchpadOpeningShown = true - } else if launchpadOpeningShown && strings.TrimSpace(message) == "Opening browser for authentication..." { return } fmt.Fprintln(w, prefix+message) @@ -318,10 +319,16 @@ func wizardAuth(cmd *cobra.Command, app *appctx.App, styles *tui.Styles, showRes } loggerPrefix := showAuthenticationStart(w, styles, showResult) - result, err := app.Auth.Login(cmd.Context(), auth.LoginOptions{ - Logger: authenticationLogger(w, loggerPrefix), + ctx, stop := loginContext(cmd) + defer stop() + result, err := app.Auth.Login(ctx, auth.LoginOptions{ + Logger: authenticationLogger(w, loggerPrefix), + Progress: w, }) if err != nil { + if err = loginOutcome(ctx, err, w, output.NewRenderer(w, false)); errors.Is(ctx.Err(), context.Canceled) { + return "", err + } return "", fmt.Errorf("authentication failed: %w", err) } diff --git a/internal/commands/wizard_test.go b/internal/commands/wizard_test.go index c2ad8ca83..76e3fa19d 100644 --- a/internal/commands/wizard_test.go +++ b/internal/commands/wizard_test.go @@ -380,20 +380,20 @@ func TestShowFastAuthenticationStart(t *testing.T) { prefix := showAuthenticationStart(&buf, styles, false) log := authenticationLogger(&buf, prefix) log("Authenticating via launchpad (https://launchpad.37signals.com/authorization/new)") - log("\nOpening browser for authentication...") + log("Opening your browser… If nothing appears, open the link above.") assert.Empty(t, prefix) - assert.Equal(t, "Opening browser for Basecamp login...\n", buf.String()) + assert.Equal(t, "Opening your browser… If nothing appears, open the link above.\n", buf.String(), + "the discovery line is dropped; the flow's own browser line is the only one") assert.NotContains(t, buf.String(), "Step 1") assert.NotContains(t, buf.String(), "launchpad") - assert.NotContains(t, buf.String(), "Opening browser for authentication") var deviceFlow bytes.Buffer deviceLog := authenticationLogger(&deviceFlow, "") deviceLog("Authenticating via https://3.basecampapi.com (device flow)") - deviceLog("\nOpening browser for authentication...") + deviceLog("Opening your browser… If nothing appears, open the link above.") assert.Contains(t, deviceFlow.String(), "Authenticating via https://3.basecampapi.com (device flow)") - assert.Contains(t, deviceFlow.String(), "Opening browser for authentication") + assert.Contains(t, deviceFlow.String(), "Opening your browser") } func TestShowFastSuccess(t *testing.T) { From 923de9c87609a673f384115a509131e0f373158a Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 18:51:36 -0700 Subject: [PATCH 03/10] Keep headless routing under --no-browser and stop the signal handler when Login returns --no-browser on a CI or display-less host skipped the launch but also skipped the classification that makes such a host a remote one, so a Launchpad login still listened on a loopback the other device cannot reach. The host check now runs before the browser decision; --no-browser only silences the launch and its commentary. The deferred stop left the interrupt handler registered until the command finished, so a Ctrl-C after a successful login was swallowed while the wizard fetched the profile or profile create wrote its config. Each call site now reads the outcome and stops the handler as soon as Login returns. The login Long says which server gets the code flow and which the browser callback. --- internal/auth/auth.go | 28 ++++++++++++++------------- internal/auth/auth_test.go | 6 ++++++ internal/commands/auth.go | 37 ++++++++++++++++++++---------------- internal/commands/profile.go | 7 ++++--- internal/commands/wizard.go | 6 ++++-- 5 files changed, 50 insertions(+), 34 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 9b970947d..bef15e9bc 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -552,7 +552,18 @@ type LoginOptions struct { // defaults fills in default values for LoginOptions. func (o *LoginOptions) defaults() { - autoRemote := !o.Remote && !o.Local && hostutil.IsRemoteSession() + // A host that cannot show a browser (SSH, CI, no display) is a remote + // one whatever else was asked: the link is going to be opened on some + // other device, so a Launchpad login must take the pasted callback + // rather than listen on this host's loopback, which that device could + // never reach. --local is the person's word that the browser is right + // here and wins over the host heuristics; --no-browser only silences + // the launch and must not silence this. + hostReason := "" + if !o.Local { + hostReason = hostutil.HeadlessReason() + } + autoRemote := !o.Remote && hostReason != "" if autoRemote { o.Remote = true } @@ -560,23 +571,14 @@ func (o *LoginOptions) defaults() { // reason is kept for the transcript when the CLI decided that on its // own: the environment says no one is at this terminal, or the host has // nowhere to open one. A caller who asked (--no-browser, --remote, - // --device-code) gets the link without commentary. --local is the - // person's word that the browser is right here and wins over the host - // heuristics. - // - // A headless host is also a remote one: the link is going to be opened - // on some other device, so a Launchpad login must take the pasted - // callback rather than listen on this host's loopback, which that - // device could never reach. + // --device-code) gets the link without commentary. switch { case o.NoBrowser, o.Remote && !autoRemote: o.NoBrowser = true case config.NonInteractiveEnv(): o.NoBrowser, o.headlessReason = true, "BASECAMP_NONINTERACTIVE is set" - case !o.Local: - if reason := hostutil.HeadlessReason(); reason != "" { - o.NoBrowser, o.Remote, o.headlessReason = true, true, reason - } + case autoRemote: + o.NoBrowser, o.headlessReason = true, hostReason } if o.BrowserLauncher == nil && !o.NoBrowser { o.BrowserLauncher = openBrowser diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 3e1a3a6aa..44d960d76 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -2235,4 +2235,10 @@ func TestLoginLaunchpad_HeadlessHostTakesThePastedCallback(t *testing.T) { local.defaults() assert.False(t, local.Remote, "--local keeps the loopback listener") assert.False(t, local.NoBrowser) + + quiet := LoginOptions{NoBrowser: true} + quiet.defaults() + assert.True(t, quiet.Remote, "--no-browser silences the launch, not the headless routing") + assert.True(t, quiet.NoBrowser) + assert.Empty(t, quiet.headlessReason, "the person asked for the link alone; no commentary") } diff --git a/internal/commands/auth.go b/internal/commands/auth.go index 13b9b1ef3..b073cb641 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -265,11 +265,12 @@ func buildLoginCmd(use string) *cobra.Command { Short: "Authenticate with Basecamp", Long: `Start the OAuth flow to authenticate with Basecamp, or import a personal access token. -The default flow prints a link and a one-time code, opens the link in your -browser, and waits for you to approve it. Over SSH, in CI, or on a host with no -display the browser is skipped and the link is yours to open on any device — -your phone included; --local forces a launch anyway, --no-browser skips it. -Ctrl-C cancels the wait. +Against Basecamp's own authorization server the flow prints a link and a +one-time code, opens the link in your browser, and waits for you to approve it +(a Launchpad server signs you in through a browser callback instead). Over SSH, +in CI, or on a host with no display the browser is skipped and the link is yours +to open on any device — your phone included; --local forces a launch anyway, +--no-browser skips it. Ctrl-C cancels the wait. Examples: basecamp auth login # Browser (or device) flow @@ -344,13 +345,11 @@ named profile, creating the profile when --account is given. fmt.Fprintln(w, r.Summary.Render("Starting Basecamp authentication...")) } - ctx, stop := loginContext(cmd) - defer stop() - // With an expectation the login is assertive: the token is // checked before it is stored, and a mismatch stores nothing. // Without one the identity line stays informational. verifier := &loginVerifier{app: app, expectIdentity: expect, account: app.Config.AccountID, strict: expect != 0} + ctx, stop := loginContext(cmd) result, err := app.Auth.Login(ctx, auth.LoginOptions{ Scope: scope, NoBrowser: noBrowser, @@ -361,8 +360,10 @@ named profile, creating the profile when --account is given. Progress: w, Verify: verifier.verify, }) + err = loginOutcome(ctx, err, w, r) + stop() if err != nil { - return loginOutcome(ctx, err, w, r) + return err } fmt.Fprintln(w) @@ -413,18 +414,22 @@ func registerLoginFlowFlags(cmd *cobra.Command, noBrowser, remote, local, device // loginContext derives the context an interactive login waits under: Ctrl-C // (and a SIGTERM) cancels it instead of killing the process mid-line, so // the flow can put the terminal back — clear the live wait line, close the -// loopback listener, stop polling — and say it was canceled. Callers must -// stop it before printing their outcome. +// loopback listener, stop polling — and say it was canceled. The stop +// function must run as soon as Login returns, after loginOutcome has read +// the context: stopping cancels the context too, and while the handler is +// registered a signal is swallowed instead of ending whatever the command +// does next. func loginContext(cmd *cobra.Command) (context.Context, context.CancelFunc) { return signal.NotifyContext(cmd.Context(), os.Interrupt, syscall.SIGTERM) } -// loginOutcome turns a failed Login into the error the root will render. A -// login the person canceled is not a failure: the human line goes to the -// terminal here and the error carries the interrupted code, which the root -// exits with silently. Everything else is returned as it came. +// loginOutcome turns the result of Login into the error the root will +// render. A login the person canceled is not a failure: the human line goes +// to the terminal here and the error carries the interrupted code, which +// the root exits with silently. Everything else, nil included, is returned +// as it came. func loginOutcome(ctx context.Context, err error, w io.Writer, r *output.Renderer) error { - if !errors.Is(ctx.Err(), context.Canceled) { + if err == nil || !errors.Is(ctx.Err(), context.Canceled) { return err } fmt.Fprintln(w) diff --git a/internal/commands/profile.go b/internal/commands/profile.go index bb930358b..e6cff4743 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -280,10 +280,9 @@ Examples: // With an expectation the credential is checked before it is // stored and a mismatch stores nothing; without one the // identity lookup stays informational. - ctx, stop := loginContext(cmd) - defer stop() w := cmd.OutOrStdout() verifier := &loginVerifier{app: app, expectIdentity: expect, account: accountID, strict: expect != 0} + ctx, stop := loginContext(cmd) loginResult, err := app.Auth.Login(ctx, auth.LoginOptions{ Scope: scope, NoBrowser: noBrowser, @@ -293,12 +292,14 @@ Examples: Progress: w, Verify: verifier.verify, }) + err = loginOutcome(ctx, err, w, output.NewRenderer(w, false)) + stop() if err != nil { // Restore in-memory state delete(app.Config.Profiles, name) app.Config.ActiveProfile = prevActiveProfile app.Config.BaseURL = prevBaseURL - return loginOutcome(ctx, err, w, output.NewRenderer(w, false)) + return err } // Login succeeded — persist profile to config diff --git a/internal/commands/wizard.go b/internal/commands/wizard.go index 9ba4434e6..4b0c8d072 100644 --- a/internal/commands/wizard.go +++ b/internal/commands/wizard.go @@ -320,13 +320,15 @@ func wizardAuth(cmd *cobra.Command, app *appctx.App, styles *tui.Styles, showRes loggerPrefix := showAuthenticationStart(w, styles, showResult) ctx, stop := loginContext(cmd) - defer stop() result, err := app.Auth.Login(ctx, auth.LoginOptions{ Logger: authenticationLogger(w, loggerPrefix), Progress: w, }) + err = loginOutcome(ctx, err, w, output.NewRenderer(w, false)) + canceled := errors.Is(ctx.Err(), context.Canceled) + stop() if err != nil { - if err = loginOutcome(ctx, err, w, output.NewRenderer(w, false)); errors.Is(ctx.Err(), context.Canceled) { + if canceled { return "", err } return "", fmt.Errorf("authentication failed: %w", err) From f3f25943824167533d1fbcb667869fc630058a39 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 19:07:57 -0700 Subject: [PATCH 04/10] Say why the pasted-callback flow was chosen, and keep the wait line on one row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Launchpad remote branch never reached announceBrowser, so a host the CLI classified as headless printed the paste instructions without the "Not opening a browser here (…)" line the device flow gives; --remote and --device-code stay silent because the person asked. The live wait line was one fixed sentence, so in a pane narrower than it the redraw wrapped onto a second row and cleared only the last one, leaving spinner fragments behind on every tick. The line now picks its form from the terminal width — full, short, or none (the static line) — so it always fits on the row it clears. --- internal/auth/auth.go | 3 +++ internal/auth/auth_test.go | 43 +++++++++++++++++++++++++++++++ internal/auth/progress.go | 47 ++++++++++++++++++++++++++-------- internal/auth/progress_test.go | 20 ++++++++++++++- 4 files changed, 102 insertions(+), 11 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index bef15e9bc..b3ec7acfb 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -748,6 +748,9 @@ func (m *Manager) loginLaunchpad(ctx context.Context, credKey string, oauthCfg * opts.log(" 4. Copy the full URL from your browser's address bar and") opts.log(" paste it below.") opts.log("") + // Remote implies NoBrowser, so this never launches: it says why the + // CLI chose this flow when the host, not a flag, chose it. + opts.announceBrowser(authURL) reader := opts.InputReader if reader == nil { diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 44d960d76..99077d3fb 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -2242,3 +2242,46 @@ func TestLoginLaunchpad_HeadlessHostTakesThePastedCallback(t *testing.T) { assert.True(t, quiet.NoBrowser) assert.Empty(t, quiet.headlessReason, "the person asked for the link alone; no commentary") } + +// TestLoginLaunchpad_RemoteTranscriptSaysWhyWhenTheHostChose: the pasted +// callback flow announces the headless reason when the CLI selected it, +// and stays silent when --remote asked for it. +func TestLoginLaunchpad_RemoteTranscriptSaysWhyWhenTheHostChose(t *testing.T) { + t.Setenv("BASECAMP_NONINTERACTIVE", "") + t.Setenv("SSH_CONNECTION", "") + t.Setenv("SSH_CLIENT", "") + t.Setenv("SSH_TTY", "") + t.Setenv("DISPLAY", ":0") + t.Setenv("CI", "") + + // newDeviceTestManager pins the interactive host, so the CI variable + // is set after it; both cases select remote mode, so a loopback + // listener (and its five-minute wait) never starts. + run := func(t *testing.T, ci string, opts LoginOptions) string { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { http.NotFound(w, r) })) + t.Cleanup(srv.Close) + t.Setenv("BASECAMP_LAUNCHPAD_URL", srv.URL) + m := newDeviceTestManager(t, srv.URL) + t.Setenv("CI", ci) + cl := &collectLogger{} + opts.Logger = cl.log + opts.InputReader = strings.NewReader("") + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + _, err := m.Login(ctx, opts) + require.Error(t, err, "EOF on the paste prompt ends the login") + return cl.joined() + } + + t.Run("host chose", func(t *testing.T) { + out := run(t, "true", LoginOptions{}) + assert.Contains(t, out, "Paste the callback URL") + assert.Contains(t, out, "Not opening a browser here (CI environment). Open the link on any device.") + }) + t.Run("--remote asked", func(t *testing.T) { + out := run(t, "", LoginOptions{Remote: true}) + assert.Contains(t, out, "Paste the callback URL") + assert.NotContains(t, out, "Not opening a browser") + }) +} diff --git a/internal/auth/progress.go b/internal/auth/progress.go index 79e7de754..dc236e182 100644 --- a/internal/auth/progress.go +++ b/internal/auth/progress.go @@ -22,6 +22,11 @@ type approvalWait struct { deadline time.Time now func() time.Time interval time.Duration + // width is the terminal's column count, which picks the line's form: + // the full sentence, a short one, or nothing at all when even that + // would wrap — a wrapped line is redrawn from its continuation row and + // leaves the previous row behind on every tick. + width int stopOnce sync.Once stop chan struct{} @@ -32,24 +37,42 @@ var approvalFrames = [...]string{"⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦" const approvalInterval = 100 * time.Millisecond +// Column budgets for the two forms of the live line, counted on the +// widest countdown they render ("99:59"); a terminal narrower than the +// short form draws nothing and the caller logs the static line instead. +const ( + approvalLineFullWidth = len("⠋ Waiting for approval… code expires in 99:59") - 4 // multibyte glyphs count once + approvalLineShortWidth = len("⠋ Waiting… 99:59") - 4 +) + // startApprovalWait begins drawing on w when it is a terminal and returns // nil otherwise, so callers can treat "no live line" uniformly: Stop on a // nil *approvalWait is a no-op. func startApprovalWait(w io.Writer, deadline time.Time) *approvalWait { - if !writerIsTerminal(w) { + f, ok := w.(*os.File) + if !ok || !term.IsTerminal(f.Fd()) { return nil } - return runApprovalWait(w, deadline, time.Now, approvalInterval) + width, _, err := term.GetSize(f.Fd()) + if err != nil { + width = 80 + } + return runApprovalWait(w, deadline, time.Now, approvalInterval, width) } // runApprovalWait is the injectable core: tests drive it with a buffer, a -// fixed clock, and a short interval. -func runApprovalWait(w io.Writer, deadline time.Time, now func() time.Time, interval time.Duration) *approvalWait { +// fixed clock, a short interval, and a chosen width. A width too narrow +// for even the short form yields nil, like a non-terminal. +func runApprovalWait(w io.Writer, deadline time.Time, now func() time.Time, interval time.Duration, width int) *approvalWait { + if width < approvalLineShortWidth { + return nil + } a := &approvalWait{ w: w, deadline: deadline, now: now, interval: interval, + width: width, stop: make(chan struct{}), done: make(chan struct{}), } @@ -57,13 +80,22 @@ func runApprovalWait(w io.Writer, deadline time.Time, now func() time.Time, inte return a } +// line renders one tick of the live line in the form the width allows. +func (a *approvalWait) line(frame int) string { + left := remaining(a.deadline.Sub(a.now())) + if a.width < approvalLineFullWidth { + return fmt.Sprintf("%s Waiting… %s", approvalFrames[frame], left) + } + return fmt.Sprintf("%s Waiting for approval… code expires in %s", approvalFrames[frame], left) +} + func (a *approvalWait) run() { defer close(a.done) ticker := time.NewTicker(a.interval) defer ticker.Stop() frame := 0 for { - fmt.Fprintf(a.w, "\r\033[2K%s Waiting for approval… code expires in %s", approvalFrames[frame], remaining(a.deadline.Sub(a.now()))) + fmt.Fprint(a.w, "\r\033[2K"+a.line(frame)) select { case <-a.stop: fmt.Fprint(a.w, "\r\033[2K") @@ -109,8 +141,3 @@ func expiresIn(d time.Duration) string { return fmt.Sprintf("%d seconds", int(d/time.Second)) } } - -func writerIsTerminal(w io.Writer) bool { - f, ok := w.(*os.File) - return ok && term.IsTerminal(f.Fd()) -} diff --git a/internal/auth/progress_test.go b/internal/auth/progress_test.go index 2087ef7df..0ccc58997 100644 --- a/internal/auth/progress_test.go +++ b/internal/auth/progress_test.go @@ -34,7 +34,7 @@ func TestApprovalWaitDrawsCountdownAndClears(t *testing.T) { var elapsed atomic.Int64 now := func() time.Time { return base.Add(time.Duration(elapsed.Load())) } buf := &syncBuffer{} - wait := runApprovalWait(buf, base.Add(10*time.Minute), now, time.Millisecond) + wait := runApprovalWait(buf, base.Add(10*time.Minute), now, time.Millisecond, 80) require.Eventually(t, func() bool { return strings.Contains(buf.String(), "code expires in 10:00") }, time.Second, time.Millisecond) elapsed.Store(int64(19 * time.Second)) @@ -57,6 +57,24 @@ func TestStartApprovalWaitNeedsATerminal(t *testing.T) { assert.Nil(t, startApprovalWait(&strings.Builder{}, time.Now().Add(time.Minute))) } +// TestApprovalWaitFitsTheTerminalWidth: a narrow pane gets the short form +// and a pane too narrow for that gets no live line, so the redraw never +// wraps onto a second row it cannot clear. +func TestApprovalWaitFitsTheTerminalWidth(t *testing.T) { + base := time.Date(2026, 9, 12, 12, 0, 0, 0, time.UTC) + now := func() time.Time { return base } + deadline := base.Add(9*time.Minute + 41*time.Second) + + buf := &syncBuffer{} + wait := runApprovalWait(buf, deadline, now, time.Millisecond, 30) + require.NotNil(t, wait) + require.Eventually(t, func() bool { return strings.Contains(buf.String(), "Waiting… 9:41") }, time.Second, time.Millisecond) + wait.Stop() + assert.NotContains(t, buf.String(), "Waiting for approval", "the full form does not fit in 30 columns") + + assert.Nil(t, runApprovalWait(&syncBuffer{}, deadline, now, time.Millisecond, 10), "narrower than the short form draws nothing") +} + func TestRemainingAndExpiresIn(t *testing.T) { assert.Equal(t, "9:41", remaining(9*time.Minute+41*time.Second)) assert.Equal(t, "0:05", remaining(4900*time.Millisecond)) From 4b1485dfc500ef25fd4b842eb63667eab6857de4 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sat, 12 Sep 2026 19:17:49 -0700 Subject: [PATCH 05/10] Refuse profile create under a machine output mode, as auth login does The login transcript and the live wait line go to stdout, which a --json or --agent envelope also owns, so profile create under one of those flags wrote prose and control sequences ahead of its envelope. The gate auth login already has is shared by both commands and runs before discovery. --- internal/commands/auth.go | 22 +++++++++++++++----- internal/commands/profile.go | 3 +++ internal/commands/profile_test.go | 34 +++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 5 deletions(-) diff --git a/internal/commands/auth.go b/internal/commands/auth.go index b073cb641..28d30de98 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -309,11 +309,8 @@ named profile, creating the profile when --account is given. if app.Flags.JQFilter != "" { return output.ErrJQNotSupported("the login command") } - if machineOutputFlagSet(app) { - return output.ErrUsageHint("Interactive login cannot run under a machine output mode", - "Browser and device logins print instructions and wait for approval, which no envelope can carry. "+ - "Check credentials with `basecamp auth status`, or import a token headlessly: "+ - "`... | basecamp auth login --with-token -P --account --json`.") + if err := refuseMachineOutputLogin(app); err != nil { + return err } if err := refuseNonInteractiveLogin(deviceCode); err != nil { return err @@ -691,6 +688,21 @@ func refuseNonInteractiveLogin(deviceCode bool) error { "or check credentials with `basecamp auth status`.") } +// refuseMachineOutputLogin is the output half of the login gate, shared by +// every command that runs an OAuth flow: the transcript and the live wait +// line go to stdout, which a machine-output envelope also owns, so a +// login under --json would write prose and control sequences ahead of the +// envelope. +func refuseMachineOutputLogin(app *appctx.App) error { + if !machineOutputFlagSet(app) { + return nil + } + return output.ErrUsageHint("Interactive login cannot run under a machine output mode", + "Browser and device logins print instructions and wait for approval, which no envelope can carry. "+ + "Check credentials with `basecamp auth status`, or import a token headlessly: "+ + "`... | basecamp auth login --with-token -P --account --json`.") +} + // machineOutputFlagSet reports whether an explicit output flag asked for a // machine format. The config-driven formats are deliberately excluded: a // configured format=json must not lock a person out of an interactive login. diff --git a/internal/commands/profile.go b/internal/commands/profile.go index e6cff4743..6c083463f 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -249,6 +249,9 @@ Examples: profileCfg.AccountID = accountID } + if err := refuseMachineOutputLogin(app); err != nil { + return err + } if err := refuseNonInteractiveLogin(deviceCode); err != nil { return err } diff --git a/internal/commands/profile_test.go b/internal/commands/profile_test.go index 2cd0ca677..89e3947f0 100644 --- a/internal/commands/profile_test.go +++ b/internal/commands/profile_test.go @@ -289,6 +289,40 @@ func TestProfileCreateDeviceCodeForcesRemoteMode(t *testing.T) { "--device-code must select the remote paste-callback flow") } +// TestProfileCreateRefusesMachineOutput: the login transcript and the live +// wait line share stdout with the envelope, so an explicit machine-output +// flag refuses the flow before discovery, as auth login does. +func TestProfileCreateRefusesMachineOutput(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Errorf("no request may be made under a machine output mode: %s", r.URL.Path) + http.NotFound(w, r) + })) + defer srv.Close() + + cfg := &config.Config{BaseURL: srv.URL, CacheDir: t.TempDir(), Sources: make(map[string]string)} + authMgr := auth.NewManager(cfg, srv.Client()) + app := &appctx.App{Config: cfg, Auth: authMgr} + app.Flags.JSON = true + + root := &cobra.Command{Use: "basecamp"} + root.AddCommand(NewProfileCmd()) + root.SetArgs([]string{"profile", "create", "test-profile", "--base-url", srv.URL}) + root.SetContext(appctx.WithApp(context.Background(), app)) + root.SetOut(&bytes.Buffer{}) + root.SetErr(&bytes.Buffer{}) + root.SilenceErrors = true + root.SilenceUsage = true + + err := root.Execute() + require.Error(t, err) + var outErr *output.Error + require.ErrorAs(t, err, &outErr) + assert.Equal(t, output.CodeUsage, outErr.Code) + assert.Contains(t, outErr.Message, "machine output mode") +} + // TestProfileCreateRefusesNonInteractiveEnv: profile create runs the same // OAuth flows as login, so it carries the same gate. Without --device-code // it refuses before discovery; with it, a Launchpad-backed server — whose From c8d2e9a94f91a3f1715bfa3f783418491e781c1d Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 13 Sep 2026 21:21:28 -0700 Subject: [PATCH 06/10] Exit 143, not 130, when a SIGTERM stops a login --- internal/cli/root.go | 7 +- internal/commands/auth.go | 43 +++++++++--- .../commands/auth_login_signal_unix_test.go | 67 +++++++++++++++++++ internal/output/codes.go | 13 ++-- internal/output/errors.go | 6 ++ 5 files changed, 120 insertions(+), 16 deletions(-) create mode 100644 internal/commands/auth_login_signal_unix_test.go diff --git a/internal/cli/root.go b/internal/cli/root.go index c2eb34ddd..d1b03d187 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -422,9 +422,10 @@ func Execute() { apiErr := output.AsError(err) // An interrupted command has already told the person what stopped; - // an error envelope on top would dress a Ctrl-C up as a failure. - if apiErr.Code == output.CodeInterrupted { - os.Exit(output.ExitInterrupted) + // an error envelope on top would dress a Ctrl-C or a SIGTERM up as + // a failure. + if apiErr.Code == output.CodeInterrupted || apiErr.Code == output.CodeTerminated { + os.Exit(output.ExitCodeFor(apiErr.Code)) } // Commands whose stdout speaks a wire protocol (basecamp mcp: diff --git a/internal/commands/auth.go b/internal/commands/auth.go index 28d30de98..3e620cde8 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -411,26 +411,51 @@ func registerLoginFlowFlags(cmd *cobra.Command, noBrowser, remote, local, device // loginContext derives the context an interactive login waits under: Ctrl-C // (and a SIGTERM) cancels it instead of killing the process mid-line, so // the flow can put the terminal back — clear the live wait line, close the -// loopback listener, stop polling — and say it was canceled. The stop -// function must run as soon as Login returns, after loginOutcome has read -// the context: stopping cancels the context too, and while the handler is -// registered a signal is swallowed instead of ending whatever the command -// does next. +// loopback listener, stop polling, discard a grant the server already +// issued — and say it was canceled. The signal that fired is the context's +// cause, so the exit status still tells an interrupt from a termination. +// The stop function must run as soon as Login returns, after loginOutcome +// has read the context: stopping cancels the context too, and while the +// handler is registered a signal is swallowed instead of ending whatever +// the command does next. func loginContext(cmd *cobra.Command) (context.Context, context.CancelFunc) { - return signal.NotifyContext(cmd.Context(), os.Interrupt, syscall.SIGTERM) + ctx, cancel := context.WithCancelCause(cmd.Context()) + signals := make(chan os.Signal, 1) + signal.Notify(signals, os.Interrupt, syscall.SIGTERM) + go func() { + select { + case sig := <-signals: + cancel(loginSignalError{sig}) + case <-ctx.Done(): + } + }() + return ctx, func() { + signal.Stop(signals) + cancel(nil) + } } +// loginSignalError is the cause a login context is canceled with when a signal, +// rather than the parent context, ended the wait. +type loginSignalError struct{ os.Signal } + +func (s loginSignalError) Error() string { return "login stopped by " + s.String() } + // loginOutcome turns the result of Login into the error the root will // render. A login the person canceled is not a failure: the human line goes -// to the terminal here and the error carries the interrupted code, which -// the root exits with silently. Everything else, nil included, is returned -// as it came. +// to the terminal here and the error carries the interrupted code — or the +// terminated code when a SIGTERM ended the wait — which the root exits with +// silently. Everything else, nil included, is returned as it came. func loginOutcome(ctx context.Context, err error, w io.Writer, r *output.Renderer) error { if err == nil || !errors.Is(ctx.Err(), context.Canceled) { return err } fmt.Fprintln(w) fmt.Fprintln(w, r.Muted.Render("Login canceled. Nothing was stored.")) + var sig loginSignalError + if errors.As(context.Cause(ctx), &sig) && sig.Signal == syscall.SIGTERM { + return output.ErrTerminated("login terminated") + } return output.ErrInterrupted("login canceled") } diff --git a/internal/commands/auth_login_signal_unix_test.go b/internal/commands/auth_login_signal_unix_test.go new file mode 100644 index 000000000..6e0aad59f --- /dev/null +++ b/internal/commands/auth_login_signal_unix_test.go @@ -0,0 +1,67 @@ +//go:build unix + +package commands + +import ( + "bytes" + "context" + "os" + "syscall" + "testing" + "time" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/output" +) + +// The signal is sent to this process while loginContext's handler is +// registered, so it cancels the login instead of ending the test binary. +func TestLoginContextExitsWithTheStatusOfTheSignalThatStoppedIt(t *testing.T) { + for _, tc := range []struct { + signal syscall.Signal + code string + exit int + }{ + {syscall.SIGINT, output.CodeInterrupted, 130}, + {syscall.SIGTERM, output.CodeTerminated, 143}, + } { + t.Run(tc.signal.String(), func(t *testing.T) { + cmd := &cobra.Command{} + cmd.SetContext(context.Background()) + ctx, stop := loginContext(cmd) + defer stop() + + require.NoError(t, syscall.Kill(os.Getpid(), tc.signal)) + select { + case <-ctx.Done(): + case <-time.After(5 * time.Second): + t.Fatal("the signal did not cancel the login context") + } + + var out bytes.Buffer + err := loginOutcome(ctx, context.Canceled, &out, output.NewRenderer(&out, false)) + var outErr *output.Error + require.ErrorAs(t, err, &outErr) + assert.Equal(t, tc.code, outErr.Code) + assert.Equal(t, tc.exit, output.ExitCodeFor(outErr.Code)) + assert.Contains(t, out.String(), "Login canceled. Nothing was stored.") + }) + } +} + +func TestLoginContextStopReleasesTheSignalHandler(t *testing.T) { + cmd := &cobra.Command{} + cmd.SetContext(context.Background()) + ctx, stop := loginContext(cmd) + stop() + + select { + case <-ctx.Done(): + case <-time.After(5 * time.Second): + t.Fatal("stop did not cancel the login context") + } + assert.NotErrorAs(t, context.Cause(ctx), new(loginSignalError), "a stop is not a signal") +} diff --git a/internal/output/codes.go b/internal/output/codes.go index 262c43740..a0b620e3d 100644 --- a/internal/output/codes.go +++ b/internal/output/codes.go @@ -41,13 +41,16 @@ const ( ) // CodeInterrupted marks a command the person stopped with Ctrl-C after it -// had started waiting on them (a login waiting for approval). The command -// has already said what happened on its own terms, so the root renders -// nothing more and exits with the shell's conventional status for a -// SIGINT-terminated process. +// had started waiting on them (a login waiting for approval); +// CodeTerminated the same stop by a SIGTERM from whatever supervises the +// process. The command has already said what happened on its own terms, so +// the root renders nothing more and exits with the shell's conventional +// status for a process the signal ended. const ( CodeInterrupted = "interrupted" ExitInterrupted = 130 + CodeTerminated = "terminated" + ExitTerminated = 143 ) // ExitCodeFor returns the exit code for a given error code. @@ -59,6 +62,8 @@ func ExitCodeFor(code string) int { return ExitLimit case CodeInterrupted: return ExitInterrupted + case CodeTerminated: + return ExitTerminated } return clioutput.ExitCodeFor(code) } diff --git a/internal/output/errors.go b/internal/output/errors.go index f645d0068..2d365b01d 100644 --- a/internal/output/errors.go +++ b/internal/output/errors.go @@ -128,6 +128,12 @@ func ErrInterrupted(msg string) *Error { return &Error{Code: CodeInterrupted, Message: msg} } +// ErrTerminated reports a command stopped by a SIGTERM while it waited. +// msg is for machine consumers; the command prints its own human line. +func ErrTerminated(msg string) *Error { + return &Error{Code: CodeTerminated, Message: msg} +} + func ErrForbiddenScope() *Error { return &Error{ Code: CodeForbidden, From e02c16c37ba24b1e0373019b53a8fd3cd6c29efa Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 13 Sep 2026 21:21:28 -0700 Subject: [PATCH 07/10] Say in the skill that --device-code pastes the callback back on Launchpad --- skills/basecamp/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index c1e6065e3..1a3485ae2 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -1391,7 +1391,7 @@ basecamp auth status # Check auth basecamp auth login # Re-authenticate basecamp auth login --scope full # Full access (the default; ignored by Launchpad) basecamp auth login --scope read # Read-only access (ignored by Launchpad) -basecamp auth login --device-code # Print a link and one-time code to approve from any device +basecamp auth login --device-code # Print a link and one-time code to approve from any device, never opening a browser here (Launchpad has no device flow: paste the callback URL back instead) BASECAMP_NONINTERACTIVE=1 basecamp auth login --device-code # The only OAuth login that runs under BASECAMP_NONINTERACTIVE, and only where the server offers the device flow (Launchpad does not); browser and pasted-callback flows refuse — prefer --with-token basecamp auth login --with-token -P bot --account # Import a personal access token from stdin (pipe it in) basecamp auth login --expect-identity # Discard the login unless it authenticated as this identity From 287913a682c8594f3367cbd8e692c73db0e529bf Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 13 Sep 2026 21:34:51 -0700 Subject: [PATCH 08/10] Keep the live wait line off a dumb terminal --- internal/auth/progress.go | 14 ++++++++++---- internal/auth/progress_test.go | 22 ++++++++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/internal/auth/progress.go b/internal/auth/progress.go index dc236e182..4be635da0 100644 --- a/internal/auth/progress.go +++ b/internal/auth/progress.go @@ -45,12 +45,18 @@ const ( approvalLineShortWidth = len("⠋ Waiting… 99:59") - 4 ) -// startApprovalWait begins drawing on w when it is a terminal and returns -// nil otherwise, so callers can treat "no live line" uniformly: Stop on a -// nil *approvalWait is a no-op. +// isTerminal is term.IsTerminal, swappable so a test can stand a pipe in +// for a terminal. +var isTerminal = term.IsTerminal + +// startApprovalWait begins drawing on w when it is a terminal that can take +// the redraw and returns nil otherwise, so callers can treat "no live line" +// uniformly: Stop on a nil *approvalWait is a no-op. A dumb terminal (an +// editor's shell buffer, a screen reader's session) is a real TTY that +// cannot erase a line, so it gets the static line like a pipe would. func startApprovalWait(w io.Writer, deadline time.Time) *approvalWait { f, ok := w.(*os.File) - if !ok || !term.IsTerminal(f.Fd()) { + if !ok || !isTerminal(f.Fd()) || os.Getenv("TERM") == "dumb" { return nil } width, _, err := term.GetSize(f.Fd()) diff --git a/internal/auth/progress_test.go b/internal/auth/progress_test.go index 0ccc58997..59d278a27 100644 --- a/internal/auth/progress_test.go +++ b/internal/auth/progress_test.go @@ -1,11 +1,13 @@ package auth import ( + "os" "strings" "sync/atomic" "testing" "time" + "github.com/charmbracelet/x/term" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -57,6 +59,26 @@ func TestStartApprovalWaitNeedsATerminal(t *testing.T) { assert.Nil(t, startApprovalWait(&strings.Builder{}, time.Now().Add(time.Minute))) } +// TestStartApprovalWaitSkipsADumbTerminal: a TTY that reports TERM=dumb +// cannot erase the line, so the redraw stays off there and the static wait +// line is what the caller logs. +func TestStartApprovalWaitSkipsADumbTerminal(t *testing.T) { + r, w, err := os.Pipe() + require.NoError(t, err) + defer r.Close() + defer w.Close() + isTerminal = func(uintptr) bool { return true } + defer func() { isTerminal = term.IsTerminal }() + + t.Setenv("TERM", "dumb") + assert.Nil(t, startApprovalWait(w, time.Now().Add(time.Minute))) + + t.Setenv("TERM", "xterm-256color") + wait := startApprovalWait(w, time.Now().Add(time.Minute)) + require.NotNil(t, wait, "a capable terminal gets the live line") + wait.Stop() +} + // TestApprovalWaitFitsTheTerminalWidth: a narrow pane gets the short form // and a pane too narrow for that gets no live line, so the redraw never // wraps onto a second row it cannot clear. From a059f8b2b9d9674fc31b16723764ddea62ad8e09 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 13 Sep 2026 21:45:11 -0700 Subject: [PATCH 09/10] Refuse --jq in the shared login gate, so profile create refuses it too --- internal/commands/auth.go | 10 ++--- internal/commands/auth_login_test.go | 9 ++++ internal/commands/profile.go | 2 +- internal/commands/profile_test.go | 63 ++++++++++++++++------------ 4 files changed, 52 insertions(+), 32 deletions(-) diff --git a/internal/commands/auth.go b/internal/commands/auth.go index 3e620cde8..d6b458c3b 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -306,10 +306,7 @@ named profile, creating the profile when --account is given. return runLoginWithToken(cmd, app, scope, expect) } - if app.Flags.JQFilter != "" { - return output.ErrJQNotSupported("the login command") - } - if err := refuseMachineOutputLogin(app); err != nil { + if err := refuseMachineOutputLogin(app, "the login command"); err != nil { return err } if err := refuseNonInteractiveLogin(deviceCode); err != nil { @@ -718,7 +715,10 @@ func refuseNonInteractiveLogin(deviceCode bool) error { // line go to stdout, which a machine-output envelope also owns, so a // login under --json would write prose and control sequences ahead of the // envelope. -func refuseMachineOutputLogin(app *appctx.App) error { +func refuseMachineOutputLogin(app *appctx.App, command string) error { + if app.Flags.JQFilter != "" { + return output.ErrJQNotSupported(command) + } if !machineOutputFlagSet(app) { return nil } diff --git a/internal/commands/auth_login_test.go b/internal/commands/auth_login_test.go index b9baa7ba2..247632e7c 100644 --- a/internal/commands/auth_login_test.go +++ b/internal/commands/auth_login_test.go @@ -743,6 +743,15 @@ func TestAuthLoginRefusesMachineOutputForInteractiveFlows(t *testing.T) { assert.Empty(t, srv.seenBearers()) }) } + t.Run("jq", func(t *testing.T) { + srv := startLoginIdentityServer(t, "bc_at_secret") + app, _ := loginTestApp(t, srv, &config.Config{}) + app.Flags.JQFilter = ".name" + _, err := runLogin(t, app, strings.NewReader(""), "--device-code") + require.Error(t, err) + assert.Contains(t, err.Error(), "--jq is not supported by the login command") + assert.Empty(t, srv.seenBearers()) + }) } // TestAuthLoginRefusesNonInteractiveEnvWithoutDeviceCode covers the env half diff --git a/internal/commands/profile.go b/internal/commands/profile.go index 6c083463f..bbf74ef45 100644 --- a/internal/commands/profile.go +++ b/internal/commands/profile.go @@ -249,7 +249,7 @@ Examples: profileCfg.AccountID = accountID } - if err := refuseMachineOutputLogin(app); err != nil { + if err := refuseMachineOutputLogin(app, "profile create"); err != nil { return err } if err := refuseNonInteractiveLogin(deviceCode); err != nil { diff --git a/internal/commands/profile_test.go b/internal/commands/profile_test.go index 89e3947f0..26804a695 100644 --- a/internal/commands/profile_test.go +++ b/internal/commands/profile_test.go @@ -291,36 +291,47 @@ func TestProfileCreateDeviceCodeForcesRemoteMode(t *testing.T) { // TestProfileCreateRefusesMachineOutput: the login transcript and the live // wait line share stdout with the envelope, so an explicit machine-output -// flag refuses the flow before discovery, as auth login does. +// flag — --jq included — refuses the flow before discovery, as auth login +// does. func TestProfileCreateRefusesMachineOutput(t *testing.T) { - t.Setenv("BASECAMP_NO_KEYRING", "1") - t.Setenv("XDG_CONFIG_HOME", t.TempDir()) - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - t.Errorf("no request may be made under a machine output mode: %s", r.URL.Path) - http.NotFound(w, r) - })) - defer srv.Close() + for name, tc := range map[string]struct { + set func(*appctx.App) + message string + }{ + "json": {func(a *appctx.App) { a.Flags.JSON = true }, "machine output mode"}, + "jq": {func(a *appctx.App) { a.Flags.JQFilter = ".name" }, "--jq is not supported by profile create"}, + } { + t.Run(name, func(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Errorf("no request may be made under a machine output mode: %s", r.URL.Path) + http.NotFound(w, r) + })) + defer srv.Close() - cfg := &config.Config{BaseURL: srv.URL, CacheDir: t.TempDir(), Sources: make(map[string]string)} - authMgr := auth.NewManager(cfg, srv.Client()) - app := &appctx.App{Config: cfg, Auth: authMgr} - app.Flags.JSON = true + cfg := &config.Config{BaseURL: srv.URL, CacheDir: t.TempDir(), Sources: make(map[string]string)} + authMgr := auth.NewManager(cfg, srv.Client()) + app := &appctx.App{Config: cfg, Auth: authMgr} + tc.set(app) - root := &cobra.Command{Use: "basecamp"} - root.AddCommand(NewProfileCmd()) - root.SetArgs([]string{"profile", "create", "test-profile", "--base-url", srv.URL}) - root.SetContext(appctx.WithApp(context.Background(), app)) - root.SetOut(&bytes.Buffer{}) - root.SetErr(&bytes.Buffer{}) - root.SilenceErrors = true - root.SilenceUsage = true + root := &cobra.Command{Use: "basecamp"} + root.AddCommand(NewProfileCmd()) + root.SetArgs([]string{"profile", "create", "test-profile", "--base-url", srv.URL}) + root.SetContext(appctx.WithApp(context.Background(), app)) + root.SetOut(&bytes.Buffer{}) + root.SetErr(&bytes.Buffer{}) + root.SilenceErrors = true + root.SilenceUsage = true - err := root.Execute() - require.Error(t, err) - var outErr *output.Error - require.ErrorAs(t, err, &outErr) - assert.Equal(t, output.CodeUsage, outErr.Code) - assert.Contains(t, outErr.Message, "machine output mode") + err := root.Execute() + require.Error(t, err) + var outErr *output.Error + require.ErrorAs(t, err, &outErr) + assert.Equal(t, output.CodeUsage, outErr.Code) + assert.Contains(t, outErr.Message, tc.message) + }) + } } // TestProfileCreateRefusesNonInteractiveEnv: profile create runs the same From 3ddd221f0a67a747c8c8376421989e51bffd8254 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Sun, 13 Sep 2026 21:45:12 -0700 Subject: [PATCH 10/10] Release the login signal handler on the first signal --- internal/commands/auth.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/internal/commands/auth.go b/internal/commands/auth.go index d6b458c3b..65a0955e9 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -411,10 +411,12 @@ func registerLoginFlowFlags(cmd *cobra.Command, noBrowser, remote, local, device // loopback listener, stop polling, discard a grant the server already // issued — and say it was canceled. The signal that fired is the context's // cause, so the exit status still tells an interrupt from a termination. -// The stop function must run as soon as Login returns, after loginOutcome -// has read the context: stopping cancels the context too, and while the -// handler is registered a signal is swallowed instead of ending whatever -// the command does next. +// The handler is released by the first signal, so a second one while the +// cleanup is still under way ends the process the default way instead of +// being swallowed. The stop function must run as soon as Login returns, +// after loginOutcome has read the context: stopping cancels the context +// too, and while the handler is registered a signal is swallowed instead +// of ending whatever the command does next. func loginContext(cmd *cobra.Command) (context.Context, context.CancelFunc) { ctx, cancel := context.WithCancelCause(cmd.Context()) signals := make(chan os.Signal, 1) @@ -422,6 +424,7 @@ func loginContext(cmd *cobra.Command) (context.Context, context.CancelFunc) { go func() { select { case sig := <-signals: + signal.Stop(signals) cancel(loginSignalError{sig}) case <-ctx.Done(): }