feat(openab-agent): xAI subscription login (SuperGrok / X Premium) via device-code OAuth - #1424
Conversation
Adds SuperGrok / X Premium subscription sign-in to openab-agent: - auth.rs: XaiVendor descriptor (namespace xai-oauth) + a generic RFC 8628 device-code login driver shared by all DeviceCode-grant vendors, with pure, unit-tested parsing/classification helpers (https-only verification URIs, interval-0 fallback, slow_down with and without a replacement interval, expired/denied terminal errors) - llm.rs: XaiProvider speaking OpenAI-compatible Chat Completions at api.x.ai/v1 with the OAuth access token as Bearer; pure xai_chat_messages transcript converter (tool adjacency preserved) - main.rs: `openab-agent auth xai-device` subcommand - acp.rs: session provider wiring, model switch, and static model list (grok-4.5 / grok-4.3 / grok-build-0.1, matching Pi's trimmed list) - docs/native-agent.md: env table + xAI credentials section Client id defaults to the grok CLI public client (ecosystem convention; xAI has no public OAuth client registration) and is overridable via OPENAB_AGENT_XAI_CLIENT_ID. Refresh reuses the existing generic driver, which already keeps the prior refresh_token when the AS omits it and defaults a missing expires_in. Closes #1423
This comment has been minimized.
This comment has been minimized.
chaodu-agent
left a comment
There was a problem hiding this comment.
Important
CHANGES REQUESTED slow_down can reduce the polling interval.
Consolidated review: #1424 (comment)
GitHub event: COMMENT — self-review delivery only; this is not an approval.
| .await | ||
| .map_err(|e| anyhow!("Failed to parse xAI response: {e}"))?; | ||
| // Chat Completions shape → parse_openai_response's fallback path. | ||
| return parse_openai_response(&payload); |
There was a problem hiding this comment.
🟡 F1 — Execute tool calls even when the response also contains text
This new Chat Completions path can return both LlmEvent::Text and LlmEvent::ToolUse, but agent.rs:235 exits whenever text is present, so the tool calls are recorded but never executed.
Requested change: Continue the tool loop whenever any tool call exists, preserve the accompanying text, and add a regression test with both non-empty content and tool_calls.
| DevicePollDisposition::Pending => continue, | ||
| DevicePollDisposition::SlowDown(server_interval) => { | ||
| // RFC 8628 §3.5: bump by 5s unless the AS supplied a new interval. | ||
| poll_interval = server_interval.unwrap_or(poll_interval + 5); |
There was a problem hiding this comment.
🟡 F2 — Never reduce the polling interval after slow_down
Using the replacement verbatim allows a lower value to speed polling up (for example, 10 seconds becomes 3). RFC 8628 §3.5 requires the interval to increase by five seconds after slow_down.
Requested change: Make the next interval monotonic and at least poll_interval + 5 (or always add five seconds), and add a regression test for a lower supplied replacement.
…onotonic slow_down F1: the agent loop ended a turn whenever it carried text, even when tool_calls were also present — recorded but never executed (silently ends agentic turns on Chat Completions, where commentary before a call is common). Now the loop finishes only when a turn has no tool calls; accompanying text stays in the assistant message. Regression test uses a fail-fast unknown tool so it runs as a plain unit test. F2: a slow_down replacement interval below the current delay could speed polling up. RFC 8628 §3.5 requires increasing by 5s; the server-supplied interval is now honored only when it slows polling further (next_slow_down_interval, unit-tested).
This comment has been minimized.
This comment has been minimized.
chaodu-agent
left a comment
There was a problem hiding this comment.
Important
CHANGES REQUESTED
Consolidated review: #1424 (comment)
GitHub event: COMMENT — self-review delivery only; this is not an approval.
| base_url: std::env::var("OPENAB_AGENT_XAI_BASE_URL") | ||
| .unwrap_or_else(|_| "https://api.x.ai/v1".to_string()), | ||
| model: ModelRef::parse( | ||
| &std::env::var("OPENAB_AGENT_XAI_MODEL") |
There was a problem hiding this comment.
🟡 F1 — Preserve the model selected from config.json
resolve_provider_choice() can select xAI from AgentConfig.model, but this constructor only checks the two environment variables before defaulting to grok-4.5. A config-only value such as xai/grok-4.3 therefore selects the right provider and silently sends the wrong model.
Requested change: Fall back to AgentConfig.model before the built-in default, parse it through ModelRef, and add a config-only regression test.
| &self.model | ||
| } | ||
|
|
||
| fn is_oauth(&self) -> bool { |
There was a problem hiding this comment.
🟡 F2 — Make auth-mode preservation provider-specific
The ACP switch path treats any true value here as "the current session uses Anthropic OAuth". Returning true for xAI therefore makes an xAI → Anthropic switch call from_oauth_auto_with_model(), bypassing a configured ANTHROPIC_API_KEY and failing when no Anthropic OAuth token exists.
Requested change: Track provider/auth identity explicitly, or only preserve Anthropic OAuth when the current provider is Anthropic OAuth; cover xAI → Anthropic with API-key-only credentials.
|
|
||
| // 401: token may have expired mid-request, force refresh and retry | ||
| if status.as_u16() == 401 && attempt < max_retries { | ||
| let _ = crate::auth::force_refresh_for(crate::auth::XAI_NAMESPACE).await; |
There was a problem hiding this comment.
🟡 F3 — Surface reactive refresh failures
Discarding this result means invalid_grant, missing credentials, or storage failures are hidden. The loop retries with stale credentials and eventually reports only a generic xAI 401, losing the actionable re-authentication error.
Requested change: Refresh at most once, retry only after a successful refresh, propagate refresh failures, and add deterministic success/failure retry coverage.
| ("device_code", device.device_code.as_str()), | ||
| ]) | ||
| .send() | ||
| .await?; |
There was a problem hiding this comment.
🟡 F4 — Back off and retry after connection timeouts
This ? aborts an otherwise valid device authorization session on a transient token-poll timeout. RFC 8628 §3.5 requires clients to reduce polling frequency before retrying after a connection timeout and recommends exponential backoff.
Requested change: Handle timeout errors inside the poll loop, increase the interval, retry until the device-code deadline, and add a timeout/backoff regression test.
F1: XaiProvider now resolves its model through xai_model() — OPENAB_AGENT_XAI_MODEL → OPENAB_AGENT_MODEL → config.json model → grok-4.5 — so a config-selected xai/grok-4.3 is no longer silently replaced by the default (env-over-config, ADR §5.5). F2: auth-mode preservation on model switch is now provider-specific. LlmProvider gains provider_name(); the ACP switch path preserves Anthropic OAuth only when the current session is Anthropic OAuth, so an xAI OAuth session switching to Anthropic honors ANTHROPIC_API_KEY. F3: the xAI 401 path refreshes at most once and only continues on a successful refresh; a failed refresh propagates its actionable re-login error. Deterministic tests cover refresh success (rotated Bearer on retry) and failure (invalid_grant surfaces) via canned local HTTP servers — no live xAI. F4: device token polling treats connection timeouts/refusals as transient per RFC 8628 §3.5 — exponential backoff (clamped 5..60s) and retry until the device-code deadline; other transport failures stay fatal.
This comment has been minimized.
This comment has been minimized.
chaodu-agent
left a comment
There was a problem hiding this comment.
Important
CHANGES REQUESTED
Consolidated review: #1424 (comment)
GitHub event: COMMENT — self-review delivery only; this is not an approval.
| // per call, mirroring `OpenAiProvider`. | ||
| crate::auth::load_tokens_for(crate::auth::XAI_NAMESPACE).map_err(|e| e.to_string())?; | ||
| Ok(Self { | ||
| base_url: std::env::var("OPENAB_AGENT_XAI_BASE_URL") |
There was a problem hiding this comment.
🟡 F1 — Do not send the OAuth bearer to an arbitrary base URL
OPENAB_AGENT_XAI_BASE_URL is accepted verbatim, while the request path later attaches the stored xai-oauth bearer. A typo, plaintext URL, or non-xAI proxy can therefore exfiltrate a refreshable subscription credential.
Requested change: Require HTTPS plus an xAI-owned host allowlist for OAuth, or introduce a separate explicit trusted-proxy mode with clear warnings and tests.
| // provider (xAI, Codex) switching to Anthropic must still use | ||
| // `auto_with_model`, or it would bypass a configured API key and | ||
| // fail on deployments without an Anthropic OAuth tenant (F2). | ||
| let session_is_anthropic_oauth = { |
There was a problem hiding this comment.
🟡 F2 — Preserve auth mode per provider across round-trip switches
This check remembers only the currently active provider. An Anthropic OAuth session that switches to xAI and then back to Anthropic no longer knows its original Anthropic auth choice, so auto_with_model() can silently select ANTHROPIC_API_KEY.
Requested change: Store auth policy per provider in session state and test Anthropic OAuth → xAI → Anthropic with an API key also configured.
| tools: &'a [ToolDef], | ||
| ) -> Pin<Box<dyn std::future::Future<Output = Result<Vec<LlmEvent>>> + Send + 'a>> { | ||
| Box::pin(async move { | ||
| let mut body = json!({ |
There was a problem hiding this comment.
🟡 F3 — Honor the documented output-token limit
The xAI request body omits the documented OPENAB_AGENT_MAX_TOKENS value, so users relying on it for response size, latency, or spend receive the upstream default.
Requested change: Resolve and send the Chat Completions-compatible output-token field and assert it in a request-body test, or explicitly document that xAI is exempt.
F1: OPENAB_AGENT_XAI_BASE_URL is validated before the OAuth bearer is attached — https only, x.ai hosts only (api.x.ai or *.x.ai). A typo'd, plaintext, or non-xAI proxy value now fails loud instead of leaking a refreshable subscription credential. Documented in native-agent.md. F2: the Anthropic OAuth preference is now sticky per session: it is recorded whenever an Anthropic provider is active and retained while other providers run, so Anthropic-OAuth → xAI → Anthropic returns to OAuth instead of silently switching to ANTHROPIC_API_KEY (different account/billing). Covered at agent level (deterministic round-trip) and acp level (switch-back takes the OAuth path). F3: xAI requests now carry the documented OPENAB_AGENT_MAX_TOKENS limit (env → config.json → 8192) via an extracted, unit-tested xai_request_body builder.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Important
CHANGES REQUESTED
Consolidated review: #1424 (comment)
GitHub event: COMMENT — self-review delivery only; this is not an approval.
| crate::auth::load_tokens_for(crate::auth::XAI_NAMESPACE).map_err(|e| e.to_string())?; | ||
| let base_url = match std::env::var("OPENAB_AGENT_XAI_BASE_URL") { | ||
| Ok(raw) if !raw.is_empty() => validate_xai_base_url(&raw)?, | ||
| _ => "https://api.x.ai/v1".to_string(), |
There was a problem hiding this comment.
🔴 F1 — Route session OAuth tokens through xAI’s documented inference proxy
This default sends a refreshable device/OIDC session token to api.x.ai, but xAI’s Enterprise Deployments documentation and official Grok Build source define that host as the direct API-key path. Session tokens must use https://cli-chat-proxy.grok.com/v1; the current request also omits the required X-XAI-Token-Auth: xai-grok-cli and x-grok-model-override headers. This can make the core SuperGrok/X Premium path fail or use the wrong auth contract.
Requested change: route session credentials only through the documented proxy with its required headers/model routing, reserve api.x.ai for a distinct API-key mode, and add deterministic route/header tests.
| // failed refresh (invalid_grant, storage error) must surface its | ||
| // actionable re-login message, not decay into a generic 401 | ||
| // after re-sending the same stale token (review F3). | ||
| if status.as_u16() == 401 && !refreshed_after_401 { |
There was a problem hiding this comment.
🟡 F2 — Preserve a retry after successful reactive refresh
On attempt 3, this branch can refresh successfully and then continue past the end of 0..=max_retries. A sequence such as three retryable 429/529 responses followed by 401 therefore stores a fresh token but returns max retries exceeded without ever sending it.
Requested change: give the one-time 401 refresh its own retry allowance, or refresh only when another request can be made and return the actual 401 otherwise. Add a regression for the exhausted-budget sequence.
…etry allowance Rate-limit retries (429/529, capped at 3 with exponential backoff) and the one-time 401 refresh now have independent budgets: a 401 arriving after the rate-limit budget is exhausted still gets its post-refresh request instead of rotating the credential and then failing with a generic 'max retries exceeded'. Terminal errors now always report the actual upstream status. Regression: 429×3 → 401 → refreshed request succeeds with the rotated bearer (canned local servers).
Author Response — Round 4✅ F2 (🟡) — fixed in
|
This comment has been minimized.
This comment has been minimized.
chaodu-agent
left a comment
There was a problem hiding this comment.
Note
LGTM ✅ — All actionable findings from earlier rounds are resolved or not applicable; Pi's merged implementation corroborates the direct api.x.ai/v1 OAuth contract.
Consolidated review: #1424 (comment)
GitHub event: COMMENT — self-review delivery only; this is not an approval.
This comment has been minimized.
This comment has been minimized.
chaodu-agent
left a comment
There was a problem hiding this comment.
Important
CHANGES REQUESTED cargo fmt --check, and the PR description still advertises the removed auth xai-device command.
Consolidated review: #1424 (comment)
GitHub event: COMMENT — self-review delivery only; this is not an approval.
|
|
||
| #[test] | ||
| fn auth_subcommand_per_namespace() { | ||
| assert_eq!( |
There was a problem hiding this comment.
🟡 F1 — Restore rustfmt-compliant formatting
cargo fmt --check fails on this exact head because this assertion is split differently from rustfmt output. This blocks the native-agent CI job before its check, clippy, and test stages can run.
Requested change: Run cargo fmt --all, commit the formatted auth.rs, and verify that cargo fmt --check passes.
This comment has been minimized.
This comment has been minimized.
chaodu-agent
left a comment
There was a problem hiding this comment.
Important
CHANGES REQUESTED
Consolidated review: #1424 (comment)
GitHub event: COMMENT — self-review delivery only; this is not an approval.
This comment has been minimized.
This comment has been minimized.
chaodu-agent
left a comment
There was a problem hiding this comment.
Important
CHANGES REQUESTED â� ï¸� â�� The Pi-native xAI contract is explicit; one exact-head cargo fmt --check failure remains.
Consolidated review: #1424 (comment)
GitHub event: COMMENT � review delivery for this PR; this is not an approval.
|
Note LGTM ✅ — The requested head contains the rustfmt correction for the remaining prior-round gate issue, and no new blocking finding was introduced. What This PR DoesThis PR adds native xAI subscription authentication to How It WorksThe implementation adds an Findings
Finding Details🟢 P1: Exact-head formatting correction is completeThe only delta from the previously reviewed Previous Review Resolution
Addressing External Reviewer FeedbackNo external reviewer feedback was present for this round. Earlier keyed review artifacts were internal consolidated reviews and their findings are accounted for in Previous Review Resolution above. Baseline Check
What's Good (🟢)
5️⃣ Three Reasons We Might Not Need This PR
|
chaodu-agent
left a comment
There was a problem hiding this comment.
Note
LGTM ✅ — The exact requested head contains the rustfmt correction and no new blocking finding.
Consolidated review: #1424 (comment)
GitHub event: COMMENT — self-review delivery only; this is not an approval.
What problem does this solve?
openab-agent supports subscription sign-in for Codex (ChatGPT) and Anthropic (Claude Pro/Max), but xAI users currently need to provision and rotate an
XAI_API_KEYeven when they already pay for a SuperGrok or X Premium subscription. openab-agent also runs in headless environments such as ECS Fargate and Kubernetes, where a PKCE loopback redirect is not practical. This PR adds a device-code login that can be completed throughkubectl execorecs execute-command.Closes #1423
Discord discussion: Not applicable. This implementation follows the research and rationale captured in issue #1423.
Review Contract
The reference implementation and acceptance contract for this PR is Pi's native xAI OAuth provider (Pi #6651), not the separate official Grok Build CLI
SessionTokenproxy contract.Authorization: Bearer <token>to the OpenAI-compatiblehttps://api.x.ai/v1/chat/completionsendpoint. This matches Pi'stoAuth()behavior, which returns the OAuth access token as the API key.openid profile email offline_access grok-cli:access api:access.cli-chat-proxy.grok.com,X-XAI-Token-Auth,x-grok-model-override, andx-grok-client-versionare not part of this PR. Those belong to the official Grok BuildSessionTokenand CLI compatibility path, which is a separate integration contract.SessionTokencompatibility would require separate credential-kind routing and proxy-specific headers.Goal
A user with a SuperGrok or X Premium subscription can run
openab-agent auth xai-device, approve the login on any browser-equipped device, and then use Grok models through openab-agent (ACP sessions, model switching, and MCP sampling) with automatic token refresh and noXAI_API_KEY.Non-goals
OPENAB_AGENT_PROVIDER=xaiorgrok, or through anxai/orgrok/model prefix. Existing Anthropic-to-Codex fallback behavior remains unchanged.grok-4.5. Pi routes that model through Responses for reasoning levels, but Chat Completions serves it and is the path used by xAI's quickstart.stream: false), matching the existingOpenAiProviderbatch-parse behavior and openab-agent'sstreaming: falseACP capability.device_auth_idprotocol is not RFC 8628.Accepted Residual Risks
OPENAB_AGENT_XAI_CLIENT_IDcan override it without a code change; recovery is to log in again with the new ID.grok-4.5,grok-4.3, andgrok-build-0.1mirror Pi's trimmed built-in list and may become stale. Any model ID can still be set explicitly through environment variables or configuration; the list only feeds the ACP model picker.Acceptance Criteria
XaiVendorresolves under thexai-oauthnamespace with a device-code grant, form-encoded token requests, no loopback redirect, and the documented scope. Covered byxai_vendor_descriptor_pins_wire_contract,vendor_for_resolves_xai, andxai_vendor_is_not_a_pkce_vendor.verification_uri_complete, acceptsinterval: 0and a missingexpires_in, and fails on missingdevice_codeoruser_code. Covered byparse_device_authorization_*andvalidate_https_url_accepts_https_only.authorization_pending,slow_downwith and without a replacement interval,access_deniedandauthorization_denied,expired_token, and unknown errors as terminal. Covered byclassify_device_poll_error_dispositions.expires_indefault to 3600 seconds, and a login response withoutrefresh_tokenfails loudly. Covered bytoken_store_from_payload_defaults_expires_in. Refresh-time no-rotation continues to use the existing genericrefresh_token()fallback.toolmessages adjacent to their assistanttool_callsmessage and stringifies arguments.xai/andgrok/model references also parse. Covered bytest_xai_chat_messages_*andtest_model_ref_parses_xai_and_grok_prefixes.cargo fmtwas applied;cargo clippy --all-targetsintroduces no new warnings versus main (10 to 10);cargo testis green with 230 passed, 0 failed, and 15 new tests.Follow-ups
grok-4.5through the Responses API with reasoning-effort levels, as in Pi #6651.oauth2::BasicClientas described in the existing ADR Section 4.2. This PR does not change that work.At a glance
Prior art and industry research
extensions/xai/xai-oauth.tsuses the RFC 8628 device flow againstauth.x.aiwith the same Grok CLI public client ID and scope, and uses the access token directly as the API key.hermes_cli/auth.pyand the xAI Grok OAuth guide use the same device-code flow with background refresh and explicitly target SuperGrok and X Premium subscriptions withoutXAI_API_KEY.verification_uri_completelink, a SuperGrok login label, and a trimmed model list; this PR mirrors those details. Pi's handling of refresh without rotation, absentexpires_in,interval: 0, and non-HTTPS verification URIs is replicated here with unit tests.xai-org/grok-buildand hundreds of public repositories such as LiteLLM, Warp, and Cherry Studio. Reuse is the ecosystem convention because xAI does not offer public OAuth client registration.Proposed solution
Land xAI as the first
AuthGrant::DeviceCode-primary vendor on the existingOAuthVendordescriptor surface (ADR Section 5.1), which was explicitly earmarked for Grok:auth.rs- Add theXaiVendordescriptor and a genericlogin_device_code_flow, the standards-compliant counterpart tologin_pkce_flow. Two descriptor hooks,device_authorization_url()andextra_device_params(), have backward-compatible defaults for xAI'sreferrertag. Parsing and poll-error classification are pure functions (parse_device_authorization,validate_https_url, andclassify_device_poll_error) so edge cases are unit-testable without a live authorization server. Storage, locking, and refresh reuse the existing shared driver;refresh_token()already keeps the previous refresh token when the authorization server omits a replacement and defaults a missingexpires_in.llm.rs- AddXaiProvider, which speaks OpenAI-compatible Chat Completions atapi.x.ai/v1and sends the per-call refreshed OAuth token as a Bearer token. Response parsing reusesparse_openai_responseand the existing Chat Completions path. The provider also uses the same 429/529 backoff and 401 force-refresh retry loop asOpenAiProvider.main.rsandacp.rs- Add theauth xai-devicesubcommand and wire storedxai-oauthcredentials into session creation, model switching, and model listing.Why this approach?
OAuthVendortrait already promises that adding a vendor is a new descriptor, not a new hand-rolled flow. This PR makes the device-code axis real and removes the existingdead_codeplaceholders. Future device-code vendors such as Copilot or Kiro can reuse the driver.grok-4.5through/v1/chat/completions, so the provider stays thin and reuses the existing parser and tests.interval: 0, absentexpires_in,slow_downsemantics, non-HTTPS verification URIs, and refresh without rotation, are either unit-tested here or already covered by the shared refresh driver.Alternatives considered
localhostredirect. Device-code is also the flow used by the Grok CLI.login_codex_device_flow- Rejected because xAI uses standard RFC 8628. A generic driver supports the next device-code vendor, while Codex remains bespoke because OpenAI'sdevice_auth_idprotocol is non-standard.grok-4.5through the Responses API - Deferred. Chat Completions servesgrok-4.5; the Responses split mainly adds reasoning-effort levels at the cost of a second wire format.console.x.aiissues API keys only.Validation
cargo fmt- applied.cargo clippy --all-targets- no new warnings versus theorigin/mainbaseline (10 pre-existing warnings remain 10).cargo test- 230 passed, 0 failed, with 11 ignored integration tests and 15 new tests, including descriptor, client-ID, provider-resolution, CLI, HTTPS-validation, device-authorization, polling, token-expiry, model-reference, and Chat-Completions message-mapping coverage.auth xai-deviceis wired tologin_xai_device_flow. A live end-to-end login was not run because it requires a SuperGrok or X Premium subscription; the wire contract is pinned by unit tests and mirrors the OpenClaw, Hermes, and Pi implementations.