Conversation
github-actions
Bot
added
commands
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c87b56d595
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved setup, cancellation, headless-flow, and progress-test issues block approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Improves OAuth device-login guidance with clearer steps, headless browser messaging, countdown progress, and cancellation handling.
Changes:
- Adds headless-environment detection and revised login transcripts.
- Adds live approval countdowns and interrupted exit handling.
- Updates authentication commands, tests, and documentation.
File summaries
| File | Summary |
|---|---|
skills/basecamp/SKILL.md |
Updates device-code documentation. |
internal/output/errors.go |
Adds interrupted error construction. |
internal/output/codes.go |
Adds interrupted status and exit code. |
internal/hostutil/hostutil.go |
Detects headless environments. |
internal/hostutil/hostutil_test.go |
Tests host detection. |
internal/commands/profile.go |
Adds cancellation and progress handling. |
internal/commands/profile_test.go |
Stabilizes profile authentication tests. |
internal/commands/auth.go |
Updates login UX and cancellation handling. |
internal/commands/auth_login_test.go |
Tests transcripts and cancellation. |
internal/cli/root.go |
Handles interrupted commands silently. |
internal/auth/progress.go |
Implements countdown progress. |
internal/auth/progress_test.go |
Tests progress rendering. |
internal/auth/device_test.go |
Tests device-flow presentation. |
internal/auth/auth.go |
Revises OAuth flows and browser behavior. |
internal/auth/auth_test.go |
Updates authentication tests. |
Review details
Suppressed comments (1)
skills/basecamp/SKILL.md:1392
- This quick-reference entry says
--device-codealways prints a one-time code, but the flag falls back to Launchpad's remote callback flow when device authorization is unavailable (the next line already documents that exception). Qualify this entry so users are not promised a code that will not be shown.
basecamp auth login --device-code # Print a link and one-time code to approve from any device
- Files reviewed: 15/15 changed files
- Comments generated: 8
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved credential-persistence cancellation risk remains, along with additional review findings.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (8)
internal/auth/auth.go:818
- The cancellation guard ends before this non-context-aware write. A signal can arrive after
verifyBeforeStorereturns and whileStore.Saveis running, so the canceled login can still persist the new credential and the caller can report success. Make the credential write cancellation-aware/transactional (including restoring any previous credential on a cancellation) rather than relying only on the pre-save check.
if err := m.store.Save(credKey, creds); err != nil {
internal/auth/auth.go:756
- The wizard's
authenticationLoggerprefixes only the beginning of each log message. Because this new message embeds leading/trailing newlines, the customized setup transcript rendersSign in to Basecampat column 0 while surrounding lines are indented. Emit the blank/header/blank lines as separate log messages (or make the logger prefix every line) so the new block stays under Step 1.
opts.log("\nSign in to Basecamp\n")
internal/auth/auth.go:628
- This new multiline log message has the same indentation problem in the customized wizard:
authenticationLoggeradds its prefix only before the first line, soSign in to Basecampis emitted at column 0 while the rest of the Launchpad block is indented. Split the blank/header/blank output or prefix every embedded line.
opts.log("\nSign in to Basecamp\n")
internal/auth/progress.go:95
remainingrounds to the nearest second, so near a boundary the countdown can report time that has already elapsed (for example, 600.6s remaining displays10:01, and 0.6s displays0:01). Since this is presented as an expiry countdown, floor the duration to whole seconds instead of rounding it.
d = d.Round(time.Second)
internal/cli/root.go:427
- The new production branch that suppresses the error envelope and exits 130 is not exercised here:
TestAuthLoginCtrlCCancelsCleanlycallsNewAuthCmd().Execute()directly, so it only observes the returned error and never enterscli.Executeat this line. A regression could therefore print a structured envelope or use the wrong process status while the current tests remain green. Add a root-level subprocess/helper test for the actual rendering and exit path.
if apiErr.Code == output.CodeInterrupted {
os.Exit(output.ExitInterrupted)
internal/commands/auth_login_test.go:1262
time.AfterFunc(300*time.Millisecond, cancel)makes this regression test depend on discovery and the first device-authorization request completing within 300 ms. A slow CI runner can cancel before the code is displayed, making the laterassert.Contains(..., "ABCD-EFGH")fail for a healthy implementation. Synchronize cancellation with the device-authorization handler/display (or a ready signal) instead of using a fixed delay.
time.AfterFunc(300*time.Millisecond, cancel)
internal/commands/wizard.go:326
- In the customized wizard,
loggerPrefixis" ", so all static login messages are indented under Step 1, but this rawwbypasses that prefix for the live approval line. The spinner therefore starts at column 0 and its clear sequence can disturb the wizard layout; pass a progress writer that inserts the same prefix after each redraw/clear, or otherwise make the live line use the wizard prefix.
result, err := app.Auth.Login(ctx, auth.LoginOptions{
Logger: authenticationLogger(w, loggerPrefix),
Progress: w,
internal/commands/wizard.go:329
- The cancellation path also writes directly to
w, bypassingloggerPrefix, so customized setup printsLogin canceled. Nothing was stored.at column 0 while the authentication step is indented. Use the same prefix-aware output path for this outcome as for the rest of the wizard transcript.
if err = loginOutcome(ctx, err, w, output.NewRenderer(w, false)); errors.Is(ctx.Err(), context.Canceled) {
- Files reviewed: 18/18 changed files
- Comments generated: 2
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66924f25cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
https://github.com/basecamp/basecamp-cli/blob/ca6adace82b08da3f0f41b7ddd4c9420ab525540/internal%20rundown/auth/auth.go#L842
If Ctrl-C arrives after this final context check but while either following Store.Save is blocked—for example, waiting on an OS keyring operation—the signal is consumed by signal.NotifyContext, but the context-free save continues and can persist the credential. Login then returns nil, so loginOutcome ignores the canceled context and reports success instead of honoring the interrupt. Make the save cancellation-aware or recheck and safely roll back/restore the previous credential after saving.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved issues remain in headless messaging and signal cancellation handling.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
internal/commands/auth.go:276
- This example still promises that
--device-codelets the user approve a printed code, but the Launchpad fallback never returns a device authorization and instead runs the pasted-callback flow (as the flag help below explains). Qualify the example by saying the code flow applies when the server offers device flow and Launchpad uses the callback URL.
basecamp auth login --device-code # Headless: approve the printed code from any device
internal/commands/auth.go:431
- The
err == nilshort-circuit skips checking ctx.Err(), while every caller keeps the signal.NotifyContext registered until after loginOutcome returns. If SIGINT/SIGTERM arrives after Login has returned successfully but before stop(), the signal is swallowed and the command proceeds to success (profile create can then persist its profile). Capture cancellation immediately after Login, unregister the handler, and pass the captured state into the outcome mapping.
if err == nil || !errors.Is(ctx.Err(), context.Canceled) {
skills/basecamp/SKILL.md:1392
- The skill entry is unconditional even though
--device-codefalls back to Launchpad's pasted callback flow and the next line explicitly says Launchpad has no device flow. An agent following this table can wait for a one-time code that will never be printed; qualify this description by provider/capability.
basecamp auth login --device-code # Print a link and one-time code to approve from any device
- Files reviewed: 18/18 changed files
- Comments generated: 1
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved findings include a critical machine-output issue and moderate device-flow timing, rendering, and documentation issues.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
skills/basecamp/SKILL.md:1392
- This skill entry promises a one-time code for every
--device-codeinvocation, but the login path falls back to Launchpad when the configured server has no device flow and then prints a callback URL/paste instructions instead (internal/auth/auth.go:591-638). Qualify the command example so agents do not relay instructions for a code that will never be shown.
internal/auth/auth.go:777
- The countdown deadline is captured only after
announceBrowserreturns. Sinceexpires_instarts when the device-authorization response is issued, a slow browser launcher delays the start of this timer and can make the live line show the code as valid longer than it really is. Capture the deadline before launching the browser and pass that value here.
if wait = startApprovalWait(opts.Progress, time.Now().Add(lifetime)); wait != nil {
internal/auth/progress.go:45
- The width budget assumes the countdown is never wider than
99:59, butlifetimecomes from the server andremainingemits unbounded minute values. For example, a 100-minute expiry renders... 100:00(42 terminal columns versus the 41-column threshold), so a 41-column terminal wraps and the next\r\033[2Kcannot remove the continuation row. Choose the short form or compute the fit from the actual remaining string.
approvalLineFullWidth = len("⠋ Waiting for approval… code expires in 99:59") - 4 // multibyte glyphs count once
approvalLineShortWidth = len("⠋ Waiting… 99:59") - 4
internal/commands/auth.go:276
- This example promises that
--device-codeprints a code, but the command forcesremote = trueand a Launchpad fallback never entersloginDevice; it prints pasted-callback instructions instead. Qualify the example the same way as the flag help so users do not expect a one-time code from Launchpad.
basecamp auth login --device-code # Headless: approve the printed code from any device
- Files reviewed: 18/18 changed files
- Comments generated: 1
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
There was a problem hiding this comment.
🔵 Needs a closer look
Two moderate findings remain unresolved, along with one setup-output indentation nit.
Review details
Suppressed comments (3)
internal/auth/progress.go:45
- The width budget assumes the countdown never exceeds
99:59, butdevAuth.ExpiresInis server-provided andlineformats any duration asm:ss. At100:00or later the rendered line is wider than the terminal, so the redraw can wrap and\r\033[2Kleaves stale text on the previous row. Base the fit decision on the actual rendered countdown (or fall back to the short/static form) instead of a fixed two-digit-minute budget.
approvalLineFullWidth = len("⠋ Waiting for approval… code expires in 99:59") - 4 // multibyte glyphs count once
approvalLineShortWidth = len("⠋ Waiting… 99:59") - 4
internal/cli/root.go:427
- The new behavior that suppresses the error envelope and exits 130 is not covered by the added test:
TestAuthLoginCtrlCCancelsCleanlyexecutesNewAuthCmddirectly and only inspects the returned error and mapping. A regression in thisExecute-level branch could still pass that test while printing an envelope or using a different process status; add a root/subprocess test that exercisescli.Executeand asserts no envelope plus status 130.
// 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)
internal/commands/wizard.go:327
- These new setup integrations bypass the wizard's indentation:
authenticationLoggerprefixes every login message, but the liveProgresswriter (andloginOutcome's cancellation line) writes directly tow. In customized setup, the spinner and canceled line therefore start at column 0 instead of underStep 1: Authentication, producing a visibly broken transcript. Route progress and cancellation output through a prefix-aware sink that preserves the redraw semantics.
result, err := app.Auth.Login(ctx, auth.LoginOptions{
Logger: authenticationLogger(w, loggerPrefix),
Progress: w,
})
err = loginOutcome(ctx, err, w, output.NewRenderer(w, false))
- Files reviewed: 18/18 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
|
Review threads: 14 resolved (12 fixed, 2 declined with the reasoning in each thread). Declined:
Rebased onto main again after #715 landed; one additive conflict in internal/auth/auth_test.go (both sides' tests kept). Fixes on this head: SIGTERM during a login exits 143 rather than 130 (c8d2e9a), the skill entry for CI is green on 3ddd221 and Copilot has reviewed it; Codex last reviewed 287913a. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ebab327e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🔵 Needs a closer look
An unresolved critical machine-output issue and additional device-flow and progress findings block approval.
Review details
Suppressed comments (6)
internal/auth/auth.go:921
- The device authorization's
ExpiresInstarts when the server issues the response, but this deadline is initialized only after the transcript has been printed and the browser-launch attempt has completed. A slow launch or output sink can therefore make the live line claim the full lifetime remains even though the server-side code is already expiring. Capture the deadline at the start ofdisplayand reuse it here.
if wait = startApprovalWait(opts.Progress, time.Now().Add(lifetime)); wait != nil {
internal/auth/progress.go:141
- For a server response with
expires_in=1, this new formatter emitsexpires in 1 seconds, producing grammatically incorrect transcript/help text. Handle the singular-second case before the plural form.
return fmt.Sprintf("%d seconds", int(d/time.Second))
internal/commands/auth.go:698
- This shared gate only checks explicit flags through
machineOutputFlagSet, butprofile createfinishes withapp.OK, whose writer honors the configuredformat=json/format=quiet. With either format set in config, the OAuth transcript (and the new progress line) is still written before the machine result, corrupting the output. Either reject configured machine formats for this command or route the interactive transcript separately; please cover that case before relying on this guard.
}
internal/commands/auth.go:277
--device-codestill falls back to Launchpad's pasted-callback flow when device discovery is unavailable: the command setsremote = trueandloginLaunchpadprints no one-time code. This example therefore promises a code for a supported path where none is printed; qualify it the same way as the flag help.
basecamp auth login --device-code # Headless: approve the printed code from any device
internal/commands/wizard.go:325
authenticationLoggerprefixes every login message withloggerPrefix, so the static wait line remains underStep 1: Authentication;approvalWaitwrites directly toProgressinstead. In step-by-step setup, the live countdown therefore starts at column 0 and its redraw/clear is outside the wizard's indented transcript. Route progress through a writer that preserves the same prefix (and clears within that line).
result, err := app.Auth.Login(ctx, auth.LoginOptions{
Logger: authenticationLogger(w, loggerPrefix),
Progress: w,
skills/basecamp/SKILL.md:1394
--device-codeis not a one-time-code flow on the Launchpad fallback: the command maps the flag to remote mode and prints a callback URL for the user to paste. This skill instruction can make an agent look for a code that will never appear; qualify provider support as the command help does.
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)
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
…own the wait 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.
…e store after a cancel 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.
…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.
…n one row 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.
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.
There was a problem hiding this comment.
🔵 Needs a closer look
The live progress renderer has an unresolved width issue for approval lifetimes of 100 minutes or more.
Review details
Suppressed comments (3)
internal/auth/auth.go:905
authenticationLoggeradds the wizard prefix once per message, so the embedded newlines here makeSign in to Basecampstart at column 0 inbasecamp setupinstead of under the authentication step (and the blank lines are not consistently prefixed). Emit the heading and surrounding blank lines as separate log messages, or make the logger prefix every embedded line.
opts.log("\nSign in to Basecamp\n")
internal/auth/auth.go:568
- These new defaults now auto-select
Remotefor CI and display-less Unix hosts as well as SSH, but theLoginOptions.Remote/Localcomments above still describe SSH as the only automatic/overridden condition. That leaves callers with an inaccurate API contract; update those comments to name all of the headless signals handled here.
// 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
internal/auth/progress.go:45
- These width budgets only allow the countdown to reach
99:59, butremainingrenders an unbounded minute count. If a server returns a lifetime of 100 minutes or more and the terminal is at this boundary, the live line exceeds the measured width, wraps, and the next\r\033[2Kclears only the continuation row, leaving stale text behind. Choose the full/short form based on the actual rendered countdown (or otherwise reserve for its maximum width) rather than hard-coding99:59.
approvalLineFullWidth = len("⠋ Waiting for approval… code expires in 99:59") - 4 // multibyte glyphs count once
approvalLineShortWidth = len("⠋ Waiting… 99:59") - 4
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e02c16c37b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
Four final comments remain, including a critical machine-output issue and moderate authentication/progress issues.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
internal/auth/auth.go:921
- The deadline is created only after
announceBrowserreturns. If launching the browser (or a custom launcher) takes time, the live countdown is extended past the server-issued expiry and polling can outlive the valid code. Capture the deadline before announcing/opening the browser and pass it here.
if wait = startApprovalWait(opts.Progress, time.Now().Add(lifetime)); wait != nil {
internal/auth/auth.go:565
- The public
LoginOptions.Remotecontract above still says auto-detection is based only on SSH variables, but these new defaults also force remote mode for CI and display-less Unix hosts. Update that comment so callers ofManager.Loginare not left with a stale description of when loopback is bypassed.
// 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()
}
internal/auth/progress.go:45
- The live-line width budget assumes the countdown never exceeds
99:59, butremainingcan render arbitrary minute values from the server-providedExpiresIn. At exactly this budget, a 100-minute lifetime produces100:00, so the full/short line can wrap and the\r\033[2Kredraw only clears the current row, leaving stale continuation text. Base the fit decision on the actual rendered duration (and fall back to the static line when neither form fits), or otherwise bound the displayed lifetime.
approvalLineFullWidth = len("⠋ Waiting for approval… code expires in 99:59") - 4 // multibyte glyphs count once
approvalLineShortWidth = len("⠋ Waiting… 99:59") - 4
- Files reviewed: 19/19 changed files
- Comments generated: 1
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 287913a682
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🔵 Needs a closer look
Two moderate issues remain in countdown width handling and machine-output gating.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
internal/output/codes.go:45
- This new comment is missing the verb that connects
CodeTerminatedto the sentence.
internal/auth/progress.go:45
- These width budgets assume the countdown never exceeds
99:59, butremainingformats arbitrary positive durations with unbounded minutes. If the authorization server returns a lifetime of 100 minutes or more, the rendered100:00makes the line wider than the budget, so it can wrap and leave stale rows behind—the exact case this redraw logic is intended to prevent. Base the width choice on the actual rendered line or use a bounded representation before enabling live redraw.
approvalLineFullWidth = len("⠋ Waiting for approval… code expires in 99:59") - 4 // multibyte glyphs count once
approvalLineShortWidth = len("⠋ Waiting… 99:59") - 4
internal/commands/profile.go:254
profile createreturnsapp.OKafter emitting this login transcript, butrefuseMachineOutputLoginonly checks explicit flags (machineOutputFlagSet), not configuredjson/quietformats or--jq. In those modes the newProgress: wand logger write prose/control sequences before the machine response, so the output is no longer a valid envelope. Use an all-machine-output gate for this command (or keep the interactive transcript off the response stream).
if err := refuseMachineOutputLogin(app, "profile create"); err != nil {
return err
}
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
There was a problem hiding this comment.
🔵 Needs a closer look
Five unresolved review comments remain, including four moderate issues.
Review details
Suppressed comments (5)
internal/auth/auth.go:905
authenticationLoggerprefixes only the beginning of each log message. Because this message embeds its own newlines, setup printsSign in to Basecampat column 0 while the subsequent steps get the wizard prefix, so the authentication block is not consistently under Step 1. Emit the blank/title/blank lines as separate log messages (or prefix every physical line).
opts.log("\nSign in to Basecamp\n")
internal/auth/auth.go:777
authenticationLoggerprefixes only the beginning of each log message. This embedded-newline block therefore leavesSign in to Basecampat column 0 in the setup wizard while the surrounding messages are indented under Step 1. Emit the blank/title/blank lines separately (or prefix every physical line) so the Launchpad transcript keeps the wizard layout.
opts.log("\nSign in to Basecamp\n")
internal/auth/progress.go:133
remainingis documented as floored, butRoundcan report a second more than is actually left (for example, 4.9s becomes0:05). That makes the expiry countdown overstate the code's valid lifetime; truncate to whole seconds instead, and update the 4.9s formatter test accordingly.
d = d.Round(time.Second)
internal/auth/progress.go:45
- The width budget assumes the countdown never exceeds
99:59, butlinerenders the server-provided lifetime without a maximum. A validexpires_inof 100 minutes produces100:00, so a terminal exactly at this threshold can wrap and the redraw/clear sequence will leave output on another row. Base the form width on the actual lifetime or use a rendering that remains within the selected budget.
approvalLineFullWidth = len("⠋ Waiting for approval… code expires in 99:59") - 4 // multibyte glyphs count once
approvalLineShortWidth = len("⠋ Waiting… 99:59") - 4
internal/commands/auth.go:277
- This example still implies that
--device-codealways yields a one-time code, but the flag forces remote mode and the supported Launchpad fallback has no device flow: it prints a callback URL to paste instead. Qualify the example by provider, as the flag description below already does, so users do not wait for a code that cannot appear.
basecamp auth login --device-code # Headless: approve the printed code from any device
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
What
Opening your browser…,Couldn't open a browser. Open the link above., orNot opening a browser here (SSH session). Open the link on any device.--no-browser,--remoteand--device-codeprint the link without commentary.hostutil.HeadlessReasonnames the hosts where a browser cannot be shown: SSH (the existing detection), a CI runner (CI=true), or a Unix host with neitherDISPLAYnorWAYLAND_DISPLAY.--localoverrides it. The Launchpad loopback flow gets the same link-first copy and browser line.⠋ Waiting for approval… code expires in 9:41, redrawn in place and cleared when the flow ends; on a pipe it is one static line naming the lifetime.Login canceled. Nothing was stored.printed, and the process exits 130 with no error envelope (newoutput.CodeInterrupted, honoured by the root).profile createruns the same login path.--helpcopy for--device-code,--remote,--local,--no-browsersays what each changes; the loginLongexplains the headless behaviour.Why
The in-house OAuth server made the device flow the default login. Its transcript put the URL and the code inside prose, then went quiet for up to ten minutes with nothing to distinguish polling from a hang. The browser was skipped over SSH silently, but on a CI runner or a display-less Linux host
xdg-openwas attempted and failed after the fact. Ctrl-C left the terminal wherever the cursor was.Before / After
Before:
After (local, terminal):
After (over SSH):
After Ctrl-C:
Borrowed from Codex
Codex's
--device-authprompt is the shape here: numbered steps, link before code, the lifetime stated on the code step, and its one-sentence "Continue only if you started this login" warning. Codex has no headless detection, countdown or interrupt handling; those are ours.Testing
internal/hostutil:HeadlessReasonmatrix (SSH, CI truthiness, display variables,--localprecedence is at the auth layer).internal/auth:TestLoginDevice_Transcriptpins the full transcript for the browser-opens, headless,--local-over-CI,--no-browserand non-terminal-progress cases;progress_test.godrives the live line with a fixed clock (countdown text, clear-on-stop, nil-safe Stop, terminal gate) and the duration formatters.internal/commands:TestAuthLoginCtrlCCancelsCleanlycancels a pending device flow and asserts the interrupted code, exit 130 mapping, the canceled line and that nothing was stored. Existing tests pinCI=""/DISPLAY=:0alongside the SSH pins so they observe the interactive answer on any runner.make fmt-check vet lint check-surface check-skill-driftpass;go test ./internal/...passes apart from three pre-existing terminal-detection tests (TestBareBasecampNeverReportsASetupError,TestDeleteConfirmableFollowsTheAudienceNotTheDevice,TestExplicitSetupStillRefuses) that fail identically onorigin/mainin this sandbox.Summary by cubic
Reworks device login output from prose and a silent wait into numbered link/code steps with a live expiry countdown, making the flow easier to follow and safer to copy. It also explains browser decisions, routes headless Launchpad logins through pasted callbacks, and makes cancellation clean.
Behavior
--localoverrides detection, while--no-browseronly suppresses launch commentary. When the host heuristic chooses this flow, the transcript says why;--remoteand--device-codestay silent.TERM=dumbterminals get one static wait line.auth loginandprofile createshare flow flags and reject--json,--agent, and--jqbefore discovery.--device-codehelp and the skill say a Launchpad login pastes the callback back.Written for commit 3ddd221. Summary will update on new commits.