Skip to content

fix(cli): bound the OAuth browser open and say when it fails - #2499

Merged
cliffhall merged 2 commits into
v2/mainfrom
v2/fix/2410-cli-open-url-fallback
Sep 26, 2026
Merged

cliffhall merged 2 commits into
v2/mainfrom
v2/fix/2410-cli-open-url-fallback

Conversation

@cliffhall

@cliffhall cliffhall commented Sep 26, 2026 •

Copy link
Copy Markdown
Member

Closes #2410

What changed

  • clients/cli/src/open-url.ts: openUrl now races open() against a timeout (OPEN_URL_TIMEOUT_MS, 5s). open 10.x resolves as soon as it has launched the opener, but before that it probes the environment (WSL default browser, PowerShell path). On a headless box or in a container any of those can stall. The OAuth flow does not wait on this call (CallbackNavigation discards the promise), but without a timeout a stalled open never settles, so the fallback line below would never print. The timer is cleared on every path.
  • clients/cli/src/cli-oauth-navigation.ts: the URL was already printed before the open, and a failure was swallowed silently. Now a failed or timed-out open writes one more stderr line: Could not open a browser automatically (<reason>). Open the URL above manually. The issue asked for a "print the URL as a fallback" path, and that already existed because the URL is always printed first. This change makes a failed open visible instead of looking like the flow is waiting on a browser.
  • clients/cli/README.md: the MCP_AUTO_OPEN_ENABLED row now describes this behavior.

Nothing changes when the open succeeds. The TUI has its own openUrl (clients/tui/src/utils/openUrl.ts); it is out of scope because the issue is filed against the CLI.

Tests

  • open-url.test.ts: a rejection propagates, a never-settling opener times out (fake timers), and the timer is cleared after a successful open.
  • cli-oauth-navigation.test.ts: the old "swallows failures" case now asserts the exact fallback line. A new case covers a non-Error rejection.

npm run local:gate passed locally. There is no UI change, so no screenshots.

🤖 Generated with Claude Code

openUrl now races `open` against a 5s timeout, so a stalled opener on a
headless box or container cannot hang the interactive OAuth flow. When
the open fails or times out, the navigation writes a stderr line telling
the user to open the already-printed URL by hand instead of swallowing
the error silently.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall cliffhall added the v2 Issues and PRs for v2 label Sep 26, 2026
@cliffhall cliffhall linked an issue Sep 26, 2026 that may be closed by this pull request
2 tasks done
@cliffhall
cliffhall requested a balanced review from Copilot September 26, 2026 03:19

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The behavior is correctly implemented and tested; only a non-blocking documentation correction remains.

Review effort: Balanced
Findings: 1 Low severity

Open (1)
What changed in this PR

Adds bounded CLI browser opening and a visible manual fallback for OAuth.

Changes:

  • Adds a five-second timeout to openUrl.
  • Reports browser-open failures to stderr.
  • Documents and tests timeout/fallback behavior.
File Description
clients/​cli/​src/​open-url.ts Adds the browser-open timeout.
clients/​cli/​src/​cli-oauth-navigation.ts Prints manual-open guidance on failure.
clients/​cli/​README.md Documents fallback behavior.
clients/​cli/​__tests__/​open-url.test.ts Tests rejection, timeout, and cleanup.
clients/​cli/​__tests__/​cli-oauth-navigation.test.ts Tests failure messages.

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread clients/cli/src/open-url.ts Resolved
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: cliffhall <cliff@futurescale.com>
@cliffhall

Copy link
Copy Markdown
Member Author

Copilot round 1 — 1 finding (low), no suppressed comments.

  • open-url.ts timeout rationale was wrong: the OAuth flow does not await openUrl (CallbackNavigation discards the promise). Fixed in f3e7c66. The comment now says the timeout exists so a stalled open settles and the manual-open line prints. The PR body is corrected too. The change is comment-only, so behavior and tests are unchanged.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The focused implementation satisfies the linked issue and is adequately tested and documented.

Review effort: Balanced
Findings: None

Resolved since last review (1)

@cliffhall

Copy link
Copy Markdown
Member Author

Copilot round 2 — clean: no findings, no inline comments, no suppressed block. The round-1 finding is marked resolved. The review loop is closed (exit: first clean round).

@cliffhall
cliffhall merged commit cba3f72 into v2/main Sep 26, 2026
5 checks passed
@cliffhall
cliffhall deleted the v2/fix/2410-cli-open-url-fallback branch September 26, 2026 03:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CLI's open() call has no timeout, no catch, and no fallback message

2 participants