Repository navigation
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The QR works as described for the xAI device flow in both the CLI and the TUI (see screenshots). Tests and typecheck pass in tui, cli and core. I ran auth login xai --method device and auth login openai --method chatgpt-browser against a local serve, and drove /connect → xAI in the TUI on the base and on this branch.
Public API: unchanged. The new @opencode/tui/util/auth-qr export is internal to the workspace.
The main issue is that the QR is shown for every OAuth attempt, not only device-code flows. With auth login openai --method chatgpt-browser, the CLI prints Mobile sign-in and a QR for an authorize URL whose redirect_uri is http://localhost:1455/auth/callback. Scanning it on a phone sends the browser to the phone's own localhost, so sign-in cannot finish there. OpenAI, ChatGPT, DigitalOcean and GitLab use this loopback pattern too, and MCP OAuth goes through the same TUI dialog. The QR should only appear when the flow can be completed on another device. The cleanest way is for the provider or attempt to say so, rather than the client guessing from the URL.
Smaller points (inline):
- The CLI writes ANSI QR escapes even when stdout is not a TTY.
- The new renderer duplicates the
uqr-based pairing QR code. - The xAI URL fallback is speculative.
- The TUI's clickable prefilled link is replaced. On the base, xAI already returns
verification_uri_completeand the dialog showed it as a link; now onlyo/cremain.
The new tests are fairly large for what they check: about 250 lines, including a 90-line dialog-mcp addition and full mock servers in auth.test.ts. Please trim them to a decode/round-trip test of the renderer plus one test that the QR is hidden for non-device flows.
Related: #52548
| log.info(attempt.instructions) | ||
| log.info(attempt.url) | ||
| const columns = process.stdout.columns | ||
| const qr = renderAuthQr(attempt.url, columns ? { columns } : undefined) |
There was a problem hiding this comment.
Two things here:
- This runs for every OAuth attempt, including loopback-redirect flows (e.g.
openai --method chatgpt-browser, where I see the QR for a URL withredirect_uri=http://localhost:1455/...). On a phone that redirect cannot reach this machine, so "Mobile sign-in" is misleading. The QR should be limited to flows that can finish on another device, ideally signalled by the provider or attempt (e.g. a device flag) rather than inferred here. - The browser open below is gated on
process.stdout.isTTY, but the QR is not. When output is piped or redirected (CI, logs, the tests), the QR is written as raw 24-bit ANSI escapes. It should probably follow the same TTY check, keeping the plain URL otherwise.
There was a problem hiding this comment.
Good catch on both. Device-code attempts now set a flag, and the QR only shows for those. It also follows the same stdout TTY check as the browser open, so a pipe still gets the plain URL.
| const dialog = useDialog() | ||
| const dimensions = useTerminalDimensions() | ||
| const theme = useTheme().surface("dialog") | ||
| const qr = createMemo(() => { |
There was a problem hiding this comment.
Same gating concern in the TUI: OAuthView and OauthQr render the QR for any props.url, which includes loopback-redirect provider logins and MCP OAuth. These cannot complete on a phone.
Also, when the QR is shown and the instructions contain the bare URL, the prefilled ?user_code= link is no longer rendered. On the base, xAI showed it as a clickable link at the top of the dialog, so desktop terminal users lose that link.
There was a problem hiding this comment.
The TUI uses that same flag now, so MCP and the loopback logins don't get a QR. The prefilled link is back at the top of the dialog, and it isn't repeated next to the code.
| return qr.data | ||
| } | ||
|
|
||
| function paint(data: boolean[][], module: (top: boolean, bottom: boolean) => string) { |
There was a problem hiding this comment.
The pairing QR in packages/cli/src/commands/handlers/pair.ts and packages/tui/src/component/dialog-pair.tsx already renders with uqr's renderUnicodeCompact. This adds a second half-block painter with its own ANSI and color rules. If fixed black/white colours are needed for scannability, it would be better to have one shared helper that both pairing and auth use, rather than two diverging implementations.
There was a problem hiding this comment.
Agreed, removed the second painter. Pairing and login both go through renderUnicodeCompact via a shared helper now. Pairing looks the same; login still forces black on white so it scans on light terminals.
| }), | ||
| }) | ||
|
|
||
| export function xaiAuthorizationUrl(device: { |
There was a problem hiding this comment.
xAI already returns verification_uri_complete (on the base the dialog shows https://accounts.x.ai/oauth2/device?user_code=...). This fallback guesses a user_code query parameter, which RFC 8628 does not define, for a case that does not seem to happen. Could it be dropped, keeping value.verification_uri_complete ?? value.verification_uri as before?
There was a problem hiding this comment.
Fair, dropped the user_code guess. It's back to verification_uri_complete ?? verification_uri.
d795bec to
98cf546
Compare
There was a problem hiding this comment.
My earlier concerns are resolved:
- The QR is now gated on a provider-set
deviceflag.chatgpt-browserno longer shows one. - Non-TTY CLI output prints the plain URL with no escape codes.
- Pairing and login share one
uqr-based renderer, and pairing output is unchanged. - The xAI
user_codeguess is gone. - The TUI keeps the clickable prefilled link.
- The tests are down to a small file.
I checked it by running auth login for xAI (device) and ChatGPT browser against a local serve, in a pty and with stdout piped, and by driving /connect → xAI in the TUI (screenshots). Tests in tui, cli and core pass, typecheck is clean in tui, cli, core, schema, plugin and client, and regenerating the client leaves no diff.
Public API:
Integration.Attempt(the OAuth connect response, schema and generated client types) gains a requireddevice: boolean.- Plugin
IntegrationOAuthAuthorization(Effect and Promise) gains an optionaldevice?: boolean.
Both are additive and existing plugins keep working. One remaining point (inline): because device is required in the response schema, a newer client that decodes with it would fail against an older server that omits the field. Making it optional or defaulting it to false avoids that.
Related: #52548
| url: Schema.String, | ||
| instructions: Schema.String, | ||
| mode: Schema.Literals(["auto", "code"]), | ||
| device: Schema.Boolean, |
There was a problem hiding this comment.
device is required here, and this class is the success schema of the connect endpoint in packages/protocol. A client built from this schema (the Effect HttpApi client decodes responses) talking to an older server that doesn't send device would fail to decode the attempt, so OAuth login would break on version skew. The managed service makes that skew realistic. Since undefined already means "no QR" everywhere you read it, Schema.optional(Schema.Boolean) or a false default would keep old servers working without changing behaviour.
There was a problem hiding this comment.
Good point. Made device optional on the attempt, so a newer client still accepts a connect response that leaves it out, and a missing value just means no QR.
98cf546 to
836aeb4
Compare
|
Thanks, I checked 836aeb4. The other points from your replies (the QR only for device flows, the plain URL when output is piped, one shared renderer with pairing unchanged, the prefilled link back in the TUI) were already resolved in my previous review. This commit doesn't touch them. |
|
Merged the latest |
|
Automated PR Cleanup Thank you for contributing to opencode. Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions. This PR was closed because it matched the following cleanup criteria:
PRs created within the last month are not affected by this cleanup. If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate. Thanks again for taking the time to contribute. |
Issue for this PR
No existing issue. This is a feature.
Type of change
What does this PR do?
Device-code OAuth login prints a URL and a code, then tries to open a browser. On a headless or terminal-only machine (an SSH session, a server, a container, or a recovery console) there is no browser, so the long URL has to be typed on a phone. Omarchy's terminal-only recovery environment, which ships opencode, is one example.
This adds a Mobile sign-in QR in the CLI and the TUI for attempts that can finish on another device. Those attempts set a
deviceflag (xAI, GitHub Copilot, OpenAI device code, and the OpenCode console). Loopback redirects, including browser login, GitLab, DigitalOcean, and MCP OAuth, do not show a QR.The QR encodes the authorization URL (
verification_uri_completewhen the provider returns one). The CLI draws it only when stdout is a terminal, and prints the plain URL otherwise. The TUI keeps the clickable prefilled link at the top of the dialog and does not print that same URL again next to the QR. On a narrow terminal the QR is omitted and the URL and code stay on screen. Pairing and login share one QR renderer. The existing text and the automatic browser open are unchanged.Related: #52548
How did you verify your code works?
bun run check, plus the QR, CLI auth, xAI provider, and integration tests. The QR tests decode the shared renderer back to the exact URL, including the prefilled user code, and check that a non-device attempt and a non-TTY CLI omit the QR. Checked the CLI and TUI screens in dark and light. Tried the xAI device flow from this branch on CachyOS.Screenshots / recordings
CLI, dark
CLI, light
TUI, dark
TUI, light
Checklist
Added by the opencode bot (AI-generated).
Related: #52548
Public API: adds
device: booleanto the Integration.Attempt OAuth response and optionaldeviceto plugin IntegrationOAuthAuthorization (additive; required field in the response schema, see review)