Skip to content

refactor(providers): extract shared OAuth device-flow helper#8619

Merged
jh-block merged 3 commits into
aaif-goose:mainfrom
soilSpoon:feat/oauth-device-flow-helper
Apr 20, 2026
Merged

refactor(providers): extract shared OAuth device-flow helper#8619
jh-block merged 3 commits into
aaif-goose:mainfrom
soilSpoon:feat/oauth-device-flow-helper

Conversation

@soilSpoon
Copy link
Copy Markdown
Contributor

Summary

  • Extract shared RFC 8628 device-code flow into providers::oauth_device_flow module
  • Migrate kimicode and githubcopilot to use the shared helper
  • Fix RFC 8628 compliance bugs in githubcopilot's polling implementation

Motivation

Both kimicode and githubcopilot implemented the OAuth device-code flow independently. The two copies diverged enough to hide bugs in the githubcopilot implementation:

  • Called error_for_status() before parsing the JSON body, so RFC 8628 error payloads like authorization_pending (returned with 4xx per §3.5) could surface as plain HTTP errors
  • Ignored slow_down responses (§3.5: client MUST increase interval by 5s)
  • Used a hardcoded 36 × 5s poll window instead of the server's expires_in

The new shared helper (oauth_device_flow.rs) implements the full RFC 8628 spec once, and both providers configure it via DeviceFlowConfig.

Changes

  • New: crates/goose/src/providers/oauth_device_flow.rsrun_device_flow, poll_for_tokens, refresh_device_flow_token, with 9 wiremock tests
  • kimicode.rs: device_flow_login, poll_for_token, do_refresh_token replaced with thin wrappers over the helper
  • githubcopilot.rs: login, get_device_code, poll_for_access_token replaced; RFC 8628 bugs fixed as a side effect
  • Net: +250 in helper, −370 across both providers (~315 lines reduced)

Test plan

  • 9 new wiremock tests in oauth_device_flow: pending → success, slow_down backoff, timeout, HTTP error body, access_denied, missing refresh_token, refresh happy-path
  • 12 existing kimicode tests pass (3 duplicated poll tests removed — covered by helper)
  • Existing githubcopilot tests pass
  • cargo clippy -p goose --all-targets -- -D warnings clean

Follow-up to #8466 and #8588.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com


fn announce_user_action(device: &DeviceCodeResponse) {
if let Ok(mut clipboard) = arboard::Clipboard::new() {
let _ = clipboard.set_text(&device.user_code);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we might want to log failure here; silently not copying the code to the clipboard on failure might be a bit confusing

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done — now logs with tracing::warn! on clipboard failure.

Comment thread crates/goose/src/providers/kimicode.rs Outdated

let token_url = format!("{}/api/oauth/token", self.auth_host);
let cfg = DeviceFlowConfig {
device_auth_url: "",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Make this field an Option rather than using empty string to indicate absence

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done — DeviceFlowConfig.device_auth_url is now Option<&str>. Callers pass Some(...) for device-flow login and None for refresh-only paths.

Comment thread crates/goose/src/providers/kimicode.rs Outdated
encoding: RequestEncoding::Form,
};
let tokens = run_device_flow(&self.client, &cfg).await?;
Ok(tokens_to_kimi(tokens, ""))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Similarly here, None should be used to indicate no prior refresh, rather than empty string

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done — tokens_to_kimi now takes Option<&str> for prior_refresh. Fresh login passes None, refresh passes Some(existing_refresh).

@jh-block jh-block self-assigned this Apr 20, 2026
Both `kimicode` and `githubcopilot` implemented RFC 8628 device-code
polling independently. Their two copies diverged enough to hide bugs:

- `githubcopilot::poll_for_access_token` called `error_for_status()`
  before parsing the JSON body, so RFC 8628 error responses like
  `authorization_pending` (returned with 4xx status per §3.5) could
  surface as a plain HTTP error instead of polling continuation.
- It also ignored `slow_down` (§3.5: MUST increase interval by 5s)
  and used a hardcoded 36 × 5s poll window instead of the server's
  `expires_in`.
- `kimicode` handled both correctly — now its implementation is the
  shared one.

Add `providers::oauth_device_flow` with `run_device_flow` +
`refresh_device_flow_token` entry points. Providers configure via a
`DeviceFlowConfig` that carries endpoint URLs, client_id, scopes,
extra headers, and request encoding (`Form` for kimi, `Json` for
github).

`DeviceFlowTokens` exposes `refresh_token` and `expires_at` as
`Option`s so callers can apply their own fallback when the server
omits them (RFC 6749 §5.1 / §6).

- kimicode: `device_flow_login`, `poll_for_token`, and
  `do_refresh_token` become thin wrappers over the helper, normalizing
  the on-disk `KimiToken` shape via `tokens_to_kimi`.
- githubcopilot: `login` → `run_device_flow`; `get_device_code` and
  `poll_for_access_token` removed. The RFC 8628 compliance issues
  above are fixed as a side effect.

Net change: +250 in helper (+ tests), −370 across the two providers
(−120 for a net reduction of ~315 lines).

Testing:
- 9 new wiremock tests in `oauth_device_flow` cover pending →
  success, `slow_down` backoff, expires_in timeout, HTTP error body,
  server-side `access_denied`, missing refresh_token, and refresh
  happy-path.
- kimicode tests now focus on Kimi-specific integration (token
  cache, refresh fallback); duplicated polling tests removed — the
  helper tests cover equivalent behavior.

Follow-up to aaif-goose#8466.

Signed-off-by: DaeHee Lee <lee111dae11@proton.me>
- Remove `device_flow_config()` tuple pattern (P0): construct
  `DeviceFlowConfig` inline where URL strings are in scope, matching
  the `githubcopilot` pattern. Eliminates empty-string URL placeholder
  that could produce opaque errors if callers forgot to patch.
- Treat "no token AND no error" poll response as `Failed` (P1):
  RFC 8628 §3.5 only defines `authorization_pending` and `slow_down`
  as valid reasons to keep polling. Any other response shape now fails
  fast instead of polling until timeout.

Signed-off-by: DaeHee Lee <lee111dae11@proton.me>
- Log clipboard failure in announce_user_action instead of silently
  discarding the error.
- Make DeviceFlowConfig.device_auth_url an Option<&str> — None
  indicates the field is unused (e.g. refresh-only paths), replacing
  the misleading empty-string sentinel.
- Make tokens_to_kimi accept Option<&str> for prior_refresh — None
  for fresh login, Some for refresh fallback per RFC 6749 §6.

Signed-off-by: DaeHee Lee <lee111dae11@proton.me>
@soilSpoon soilSpoon force-pushed the feat/oauth-device-flow-helper branch from 5e06459 to fd591ed Compare April 20, 2026 14:09
@soilSpoon soilSpoon requested a review from jh-block April 20, 2026 14:13
@jh-block jh-block added this pull request to the merge queue Apr 20, 2026
@jh-block
Copy link
Copy Markdown
Collaborator

thank you @soilSpoon!

Merged via the queue into aaif-goose:main with commit e74438b Apr 20, 2026
20 checks passed
lifeizhou-ap added a commit that referenced this pull request Apr 21, 2026
* main:
  feat(hooks): add Husky git hooks for ui/goose2 (#8577)
  fix: links in chat could not be opened (#8544)
  fix: run setup before dev and dev-debug in goose2 justfile (#8718)
  Manage skills as sources over ACP (#8675)
  handle full node paths in goose2 kill recipe (#8709)
  overhaul provider inventory and agent/model selection (#8652)
  Remove unused import (#8676)
  delete the goose2 migration plan prompt (#8678)
  Add health score badge to README (#8677)
  feat(goose2): voice dictation via direct-ACP pattern (#8609)
  consistently use actions-rust-lang/setup-rust-toolchain (#8671)
  fix(developer): run shell tool under bash/sh regardless of login shell (#8659)
  refactor(providers): extract shared OAuth device-flow helper (#8619)
  Add a goose2 release workflow (#8629)
  chore(deps): bump ncipollo/release-action from 1.20.0 to 1.21.0 (#8664)
  docs: add blog post about Mesh LLM provider option (#8655)
  fix: append /chat/completions for prefixed v1 base URLs (#8521)
  Reset ChatGPT Codex auth during OAuth setup (#8569)
  chore(deps): bump EmbarkStudios/cargo-deny-action from 2.0.15 to 2.0.17 (#8665)
  Add dependabot config for pnpm workspace, cargo, and actions (#8660)
spikewang pushed a commit to spikewang/goose that referenced this pull request Apr 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants