feat(auth): add Kimi Code OAuth login#708
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Kimi Code as a device-code-only OAuth provider across CLI authentication, OAuth presets and headers, provider catalog resolution, setup wizard flows, onboarding, documentation, and tests. ChangesKimi OAuth provider
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SetupWizard
participant OAuthRegistry
participant KimiAuth
User->>SetupWizard: select Kimi Code and press Enter
SetupWizard->>OAuthRegistry: resolve kimi-code device preset
OAuthRegistry->>KimiAuth: request device code and poll token
KimiAuth-->>OAuthRegistry: return bearer token
OAuthRegistry-->>SetupWizard: authentication result
SetupWizard-->>User: complete device-code login
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/auth.go`:
- Around line 81-87: Update the "kimi" case in the auth command dispatch to pass
the subcommand name together with args[1:] into runAuthLogin, preserving
user-provided flags such as --scope and --help for generic parsing.
In `@internal/tui/provider_wizard.go`:
- Line 329: Update the Kimi entry near the provider-wizard subtitle to remove it
from the browser-login description or explicitly identify it as
device-code-only; also update docs/oauth-subscriptions.md lines 40 and 54-61 to
exclude Kimi from the one-click browser-login list and describe its separate
device-code authentication flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c4ed4a70-9661-4fca-8f55-60ea4bc0fa13
📒 Files selected for processing (8)
docs/oauth-subscriptions.mdinternal/cli/auth.gointernal/oauth/presets.gointernal/oauth/presets_test.gointernal/providercatalog/catalog.gointernal/providercatalog/catalog_test.gointernal/providercatalog/oauth_test.gointernal/tui/provider_wizard.go
94cafb7 to
56a63fe
Compare
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Blocking: inserting "kimi" into the TransportOpenAICompat slice in catalog_test.go dropped the trailing comma after the closing brace, so the providercatalog test package won't compile — go vet and go test both fail with missing ',' before newline in composite literal at catalog_test.go:326. Re-adding that comma (after custom-openai-compatible"}) fixes it. Two minor follow-ups: the openrouter line in the zero auth help text picked up an extra leading space (3 vs. 2), and the baked-in Kimi client_id and managed endpoint are a contributor-chosen trust anchor I'd want kevin to sign off on explicitly — same opt-in preset pattern as xAI, but worth a conscious yes.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Restore the Moonshot
kimialias before adding this canonical ID
internal/providercatalog/catalog.go:147
moonshotalready exposeskimias an alias, butGetreturns an exact descriptor ID before it reaches later aliases. Existing profiles withcatalogID: "kimi"therefore resolve to this new OAuth-only descriptor instead of Moonshot, changing their endpoint, default model, andMOONSHOT_API_KEYauthentication behavior. Use a non-conflicting ID or add an explicit migration/compatibility path for the old alias. -
[P1] Implement Kimi's device-identity protocol before exposing its OAuth preset
internal/oauth/presets.go:110
The generic device flow only sends form fields plusContent-Type/Accept, while Kimi Code's own OAuth client sends its persistentX-Msh-*device identity headers on device authorization, polling, and refresh; the managed API client also applies them to model calls. Zero has no place to provide those headers for either OAuth or the new catalog descriptor, so this does not implement the provider's OAuth/runtime contract and the advertised login can be rejected or become unrefreshable. Add the supported provider-specific identity/header handling end-to-end, or do not offer this OAuth integration. -
[P1] Route device-only Kimi through the UI's device-code flow on desktops
internal/providercatalog/catalog.go:149
Kimi is marked device-flow-only, but a normal desktop Enter in/provideror first-run onboarding takes the generic token-login branch.Manager.Logincorrectly chooses device flow from the preset, then writes the verification URL and code to a nilOutwriter (which becomesio.Discard); the UI only shows a spinner until it times out. Always use the existing two-phase device-code UI for device-only providers, and cover the desktop path. -
[P1] Fix the invalid Kimi Code model default
internal/providercatalog/catalog.go:147
The managed Kimi Code endpoint documentskimi-for-codingandkimi-for-coding-highspeed, notkimi-k2.7-code-highspeed. A profile created byzero auth kimitherefore never selects the advertised HighSpeed tier (Kimi silently falls back to standard for mistyped IDs). Use a supported model ID and assert the descriptor default in tests. -
[P2] Restore the missing composite-literal comma so this package parses
internal/providercatalog/catalog_test.go:326
The updatedTransportOpenAICompatslice is the final value in a multiline map literal but has no trailing comma. Go rejects the package withmissing ',' before newline in composite literal, which explains the three failed Smoke jobs and prevents the claimed Go test suite from running. -
[P2] Forward flags from the
zero auth kimishortcut
internal/cli/auth.go:87
This calls the generic login with only[]string{"kimi"}and discards every user-supplied argument. Consequentlyzero auth kimi --helpbegins a real device authorization,--scopeis silently omitted, and typos are accepted instead of going through the generic parser. Passargs[1:]through (or reject extras explicitly) and add coverage for help, scopes, and invalid flags. -
[P3] Stop advertising the device-only path as browser login
internal/tui/provider_wizard.go:379
Kimi has no loopback/authorization endpoint, yet the newly updated chooser and the documentation include it in “One-click browser login.” This leaves users expecting a browser flow that cannot occur; describe it as device-code-only consistently.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround — the comma fix landed and the openrouter line is realigned, so my compile blocker is cleared (Smoke is green on all three platforms now). Keeping this at changes requested for two reasons though. First, jatmn's alias-collision point is real and I verified it: moonshot already carries kimi as an alias, and since Get() walks the catalog in order, the new kimi entry now wins — anyone with catalogID "kimi" today gets silently repointed from api.moonshot.ai/v1 + MOONSHOT_API_KEY to the OAuth-only managed endpoint. That needs a non-conflicting ID or an explicit migration story. Second, I still want kevin's explicit yes on shipping the kimi-code client_id and auth.kimi.com endpoints as a baked-in trust anchor — I don't see that on the thread yet. While you're in there, zero auth kimi should forward args[1:] (right now zero auth kimi --help kicks off a real device flow), and the wizard/docs shouldn't list Kimi under one-click browser login when it's device-code only — jatmn's review covers the rest.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Propagate Kimi's required identity headers to model requests
internal/providercatalog/catalog.go:166
The new descriptor has noCustomHeaders, while the newX-Msh-*values live only inoauth.Config.ExtraHeaders, whose consumers are the device and token request builders. Afterzero auth kimisaves its bearer, the normal OpenAI-compatible runtime therefore sends/coding/v1/chat/completionswith only the bearer and no Kimi identity headers. Kimi's own client exposes these headers for its managed-service client as well, so a successful login still leaves the advertised coding endpoint rejecting every completion. Carry the identity contract into the catalog/runtime request path (including its stable device identity) and cover the outbound request. -
[P1] Route device-only providers through the device-code onboarding UI
internal/tui/onboarding.go:781
First-run onboarding only usesstartSetupDeviceLoginwhen the environment prefers device flow. On a desktop, selecting Kimi and pressing Enter takessetupOAuthCmdinstead; its generic manager correctly chooses Kimi's device flow, but its nil output discards the verification URL and user code while onboarding displays a browser-login spinner. The user cannot authorize and the login times out. Mirror theOAuthDeviceOnlyEnter handling already added to/providerand add the desktop onboarding regression test. -
[P2] Correct the documented Kimi OAuth override names
docs/oauth-subscriptions.md:115
The documentedZERO_OAUTH_KIMI_{ISSUER,DEVICE,TOKEN}_URLvariables are never read: the shortcut resolves providerkimi-code, andenvKeyderivesZERO_OAUTH_KIMI_CODE_*. Users following the documentation cannot override the OAuth endpoints. Document theKIMI_CODEnames (or deliberately support the aliases).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/providercatalog/catalog.go (1)
449-454: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid double-allocation when lazily populating
CustomHeaders.When
descriptor.CustomHeadersisnilforkimi-code,kimiidentity.Headers()allocates and returns a new map. The immediately followingif descriptor.CustomHeaders != nilblock then makes a redundant copy of this freshly allocated map.♻️ Proposed refactor
- if descriptor.ID == "kimi-code" && descriptor.CustomHeaders == nil { - descriptor.CustomHeaders = kimiidentity.Headers() - } - if descriptor.CustomHeaders != nil { - descriptor.CustomHeaders = copyStringMap(descriptor.CustomHeaders) - } + if descriptor.CustomHeaders != nil { + descriptor.CustomHeaders = copyStringMap(descriptor.CustomHeaders) + } else if descriptor.ID == "kimi-code" { + descriptor.CustomHeaders = kimiidentity.Headers() + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/providercatalog/catalog.go` around lines 449 - 454, Update the CustomHeaders handling near the kimi-code descriptor initialization to avoid copying the map freshly returned by kimiidentity.Headers(). Preserve copying for pre-existing non-nil CustomHeaders while ensuring lazily populated headers are assigned without the redundant copy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/kimiidentity/kimiidentity.go`:
- Around line 61-65: Update the persistent ID write path in the path-handling
block to create the file with os.OpenFile using exclusive creation (os.O_EXCL),
avoiding blind os.WriteFile overwrites. If creation fails because another
process created the file, read and adopt the existing stored ID so concurrent
processes use the same identity; preserve the current directory and permission
behavior.
---
Nitpick comments:
In `@internal/providercatalog/catalog.go`:
- Around line 449-454: Update the CustomHeaders handling near the kimi-code
descriptor initialization to avoid copying the map freshly returned by
kimiidentity.Headers(). Preserve copying for pre-existing non-nil CustomHeaders
while ensuring lazily populated headers are assigned without the redundant copy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5ec83a3f-9e5a-47f0-ac2c-b0f0ac1c8ecf
📒 Files selected for processing (6)
docs/oauth-subscriptions.mdinternal/kimiidentity/kimiidentity.gointernal/oauth/presets.gointernal/providercatalog/catalog.gointernal/tui/onboarding.gointernal/tui/onboarding_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/oauth-subscriptions.md
|
Pushed 9bdec37 for the latest round: (1) the X-Msh-* identity now reaches model requests — the header set moved to a shared internal/kimiidentity package used by both the OAuth flows and the kimi-code descriptor's CustomHeaders (populated lazily at catalog access so importing the catalog does no filesystem IO), which the resolver already merges into the profile for canonical-endpoint requests. The device ID is persisted under the user config dir (mirroring kimi-cli's ~/.kimi/device_id), so login, refresh, and completions present one stable device identity. (2) First-run onboarding now routes a desktop Enter on an OAuthDeviceOnly provider through the device-code UI, mirroring the /provider fix, with a desktop-environment regression test. (3) The docs now name the ZERO_OAUTH_KIMI_CODE_* override variables that envKey actually derives. The baked-in client_id/endpoint trust anchor still needs kevin's explicit sign-off, per Vasanth. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Route mouse activation through the device-code UI
internal/tui/provider_wizard_discovery.go:47
The newOAuthDeviceOnlyhandling exists only in the keyboard handler. A desktop user who double-clicks Kimi Code reachesadvanceProviderWizard, which checks onlyoauthPreferDeviceFlow()and starts the generic OAuth command. Kimi's preset then selects its device flow, but that path discards the verification URL and user code, leaving the browser-login spinner to time out. IncludeOAuthDeviceOnlyin this branch (or centralize the choice) and cover mouse activation. -
[P2] Make Kimi device-ID initialization safe across processes
internal/kimiidentity/kimiidentity.go:51
Two first-run Zero processes can both miss the ID file, mint different IDs, and blindly overwrite it at line 63. A login completed under the losing process's in-memory ID is then refreshed or used for completions with the persisted winning ID, despite this integration requiring one identity across those calls. Create the file exclusively (or lock it), and on an already-created result read and adopt the winner.
Maintainer decision
- Vasanthdev2004's request for Kevin's explicit approval of the baked-in Kimi client identity and
auth.kimi.comtrust anchor remains unanswered on the current head. Please record that decision before shipping this opt-in preset.
|
Pushed 4305942. Fixed:
Still open, not something a code fix resolves:
|
This comment was marked as low quality.
This comment was marked as low quality.
|
Came back to this after digging into the reverse-engineered values, since that was the one thing I was holding on. On the baked-in client_id and the auth.kimi.com endpoints: I'm good with those as a maintainer, no separate sign-off needed. And they check out. I cross-referenced every login-critical value against the upstream kimi-cli (MoonshotAI/kimi-cli, src/kimi_cli/auth/oauth.py):
Implementation side is clean too: the device flow is RFC 8628 correct, the vendor headers are wired into device-auth, poll, exchange, refresh, and the completions calls, DeviceID is persistent and race-safe, and the alias logic is safe (resolving "kimi" still lands on moonshot, no repoint). One thing worth a conscious call before merge: Minor: The code is solid and accurately sourced. Once a real Kimi login is confirmed to actually complete a model call with the |
|
Pushed 2b5c45a on top of 4305942. Code blockers from the earlier reviews are closed on this head:
This commit also addresses the latest nits:
Still not a pure code fix (needs a live account):
Leaving this as draft until that live check (or a deliberate platform decision) is recorded. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/providercatalog/catalog.go (1)
440-456: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVendor-specific ID check baked into generic catalog cloning.
cloneDescriptorhardcodesdescriptor.ID == "kimi-code"to decide when to lazily mint runtime headers. This works today, but couples the shared, generic clone helper to one specific provider. If another provider ever needs the same lazy-header pattern, this function grows another special case.Consider generalizing via a field on
Descriptor(e.g.RuntimeHeaders func() map[string]string, set only for the kimi-code entry) socloneDescriptorstays provider-agnostic.♻️ Sketch of a provider-agnostic alternative
type Descriptor struct { ... OAuthDeviceOnly bool + // RuntimeHeaders, if set, lazily produces headers to attach to + // CustomHeaders only when withRuntimeHeaders is true (e.g. Kimi's + // vendor-identity headers). + RuntimeHeaders func() map[string]string } func cloneDescriptor(descriptor Descriptor, withRuntimeHeaders bool) Descriptor { ... if descriptor.CustomHeaders != nil { descriptor.CustomHeaders = copyStringMap(descriptor.CustomHeaders) - } else if withRuntimeHeaders && descriptor.ID == "kimi-code" { - descriptor.CustomHeaders = kimiidentity.Headers() + } else if withRuntimeHeaders && descriptor.RuntimeHeaders != nil { + descriptor.CustomHeaders = descriptor.RuntimeHeaders() } return descriptor }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/providercatalog/catalog.go` around lines 440 - 456, Make cloneDescriptor provider-agnostic by adding a Descriptor-level runtime-header provider, such as RuntimeHeaders, and invoke it when withRuntimeHeaders is true and CustomHeaders is nil. Configure only the kimi-code descriptor to supply kimiidentity.Headers, preserving lazy header creation and existing explicit CustomHeaders behavior without checking descriptor.ID inside cloneDescriptor.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/kimiidentity/kimiidentity_test.go`:
- Around line 32-97: Factor the production load-or-create logic from DeviceID()
into a path-parameterized internal helper such as loadOrCreateDeviceIDAt(path
string), preserving exclusive creation and winner read-back behavior. Update
DeviceID() to resolve the user config path and delegate to this helper, then
replace the inline OpenFile/read-back algorithm in
TestLoadOrCreateDeviceIDExclusiveCreate with direct concurrent calls to the
helper.
---
Nitpick comments:
In `@internal/providercatalog/catalog.go`:
- Around line 440-456: Make cloneDescriptor provider-agnostic by adding a
Descriptor-level runtime-header provider, such as RuntimeHeaders, and invoke it
when withRuntimeHeaders is true and CustomHeaders is nil. Configure only the
kimi-code descriptor to supply kimiidentity.Headers, preserving lazy header
creation and existing explicit CustomHeaders behavior without checking
descriptor.ID inside cloneDescriptor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 407b3e6d-2d9c-4065-8406-968a98ecfee6
📒 Files selected for processing (7)
internal/kimiidentity/kimiidentity.gointernal/kimiidentity/kimiidentity_test.gointernal/oauth/presets_test.gointernal/providercatalog/catalog.gointernal/providercatalog/catalog_test.gointernal/tui/provider_wizard_discovery.gointernal/tui/provider_wizard_oauth_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/tui/provider_wizard_oauth_test.go
- internal/oauth/presets_test.go
- internal/kimiidentity/kimiidentity.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/providercatalog/catalog.go (1)
463-467: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMerge
RuntimeHeadersinstead of making it mutually exclusive withCustomHeaders.The current
else iflogic mutually excludesCustomHeadersandRuntimeHeaders. If a future provider defines both staticCustomHeadersand dynamicRuntimeHeaders, the dynamic ones will be silently ignored. Merging them avoids this trap and future-proofs the resolution logic.🛠️ Proposed refactor
- if descriptor.CustomHeaders != nil { - descriptor.CustomHeaders = copyStringMap(descriptor.CustomHeaders) - } else if withRuntimeHeaders && descriptor.RuntimeHeaders != nil { - descriptor.CustomHeaders = descriptor.RuntimeHeaders() + if descriptor.CustomHeaders != nil { + descriptor.CustomHeaders = copyStringMap(descriptor.CustomHeaders) + } + if withRuntimeHeaders && descriptor.RuntimeHeaders != nil { + rh := descriptor.RuntimeHeaders() + if descriptor.CustomHeaders == nil { + descriptor.CustomHeaders = rh + } else { + for k, v := range rh { + descriptor.CustomHeaders[k] = v + } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/providercatalog/catalog.go` around lines 463 - 467, Update the descriptor header resolution logic to process RuntimeHeaders independently of CustomHeaders rather than using an else-if. Preserve and copy existing CustomHeaders, then merge the headers returned by descriptor.RuntimeHeaders() when withRuntimeHeaders is enabled, ensuring dynamic values are included when both sources are defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/providercatalog/catalog.go`:
- Around line 463-467: Update the descriptor header resolution logic to process
RuntimeHeaders independently of CustomHeaders rather than using an else-if.
Preserve and copy existing CustomHeaders, then merge the headers returned by
descriptor.RuntimeHeaders() when withRuntimeHeaders is enabled, ensuring dynamic
values are included when both sources are defined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c7fa937-fc56-4701-897e-83ddde945906
📒 Files selected for processing (3)
internal/kimiidentity/kimiidentity.gointernal/kimiidentity/kimiidentity_test.gointernal/providercatalog/catalog.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/kimiidentity/kimiidentity.go
15eac90 to
ddb1625
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/oauth-subscriptions.md`:
- Around line 108-111: Update the Kimi header scope description near
ZERO_OAUTH_KIMI_CODE_* to cover code exchange and managed runtime requests in
addition to device authorization, polling, and refresh. State that the
vendor-identity X-Msh-* headers are required across all applicable OAuth and API
calls, matching the behavior documented by the OAuth and provider integration
symbols.
- Line 40: Update the “Sign in with OAuth” chooser summary in the documentation
to mention device-code login support for xAI and Hugging Face alongside Kimi
Code, keeping it consistent with the detailed guidance later in the document.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ad503a0f-f642-41f4-80d7-5d1639ed8e9b
📒 Files selected for processing (19)
docs/oauth-subscriptions.mdinternal/cli/auth.gointernal/kimiidentity/kimiidentity.gointernal/kimiidentity/kimiidentity_test.gointernal/oauth/device.gointernal/oauth/flow.gointernal/oauth/flow_test.gointernal/oauth/oauth.gointernal/oauth/presets.gointernal/oauth/presets_test.gointernal/oauth/providers.gointernal/providercatalog/catalog.gointernal/providercatalog/catalog_test.gointernal/providercatalog/oauth_test.gointernal/tui/onboarding.gointernal/tui/onboarding_test.gointernal/tui/provider_wizard.gointernal/tui/provider_wizard_discovery.gointernal/tui/provider_wizard_oauth_test.go
🚧 Files skipped from review as they are similar to previous changes (17)
- internal/oauth/device.go
- internal/oauth/flow_test.go
- internal/oauth/oauth.go
- internal/providercatalog/oauth_test.go
- internal/cli/auth.go
- internal/kimiidentity/kimiidentity_test.go
- internal/oauth/presets.go
- internal/tui/onboarding_test.go
- internal/oauth/presets_test.go
- internal/oauth/flow.go
- internal/tui/provider_wizard_oauth_test.go
- internal/tui/onboarding.go
- internal/tui/provider_wizard_discovery.go
- internal/kimiidentity/kimiidentity.go
- internal/providercatalog/catalog.go
- internal/providercatalog/catalog_test.go
- internal/tui/provider_wizard.go
|
Rebased onto main and fixed a CI failure. No textual conflict, but a semantic one: this PR adds an extraHeaders param to PostToken, while main separately added TestPostTokenRefusesRedirect using PostToken's old signature. The merge compiled the wrong test call, which go vet caught. Updated that call site to pass the new arg. Verified after rebase: go build ./..., go vet ./..., and go test ./... all pass. Smoke is now green on macOS, Ubuntu, and Windows. |
ddb1625 to
16d1059
Compare
|
Pushed 16d1059 ( Fixes in this push:
Already on this branch from earlier review rounds (re-verified on this head):
Still draft. Maintainer approval of the baked-in Kimi client identity (client_id / auth.kimi.com trust anchor) remains outstanding for an explicit Kevin sign-off before this should be marked ready for review. Leaving draft. Verification: gofmt check, go vet on touched packages, go test ./internal/kimiidentity ./internal/oauth ./internal/providercatalog ./internal/cli, focused tui wizard device-flow tests. |
|
Addressed in a4c5329 (draft status unchanged; still pending live-login verification). [P1] Send the Kimi platform identity its service accepts
[P1] Preserve Kimi runtime headers in the
Device-poll cancel on Esc
Test hygiene
Verified: |
If an exclusive create leaves an empty or invalid file (winner dies before writing the UUID), remove it once after the adopt retry window and exclusive-create again so callers converge on a persisted identity instead of permanently diverging. Refs Gitlawb#708
|
Additional device-id hardening on top of the prior round ( [P2] Abandoned empty/invalid device-id file
Still draft pending live-login verification. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Restore atomic cron job-ID reservation
internal/cron/store.go:102
allocIDnow checks a timestamp directory withos.Statand returns before anything reserves it. Two stores adding a job in the same second can both choose that ID, then race throughwriteJobon the samemetadata.json.tmpand final metadata path; both callers can report success while one scheduled job is overwritten (and a concurrent remove can again be resurrected). This rolls back the base's exclusiveMkdirplus held per-ID lock, and deletes its regression tests. Restore an atomic reservation held through initial persistence and retain the concurrent-add/remove coverage. -
[P1] Strip Kimi identity headers after retargeting a saved profile
internal/config/resolver.go:1092
The wizard persists Kimi'sX-Msh-*values, including the stable device ID and host/device metadata, when the profile usesapi.kimi.com. If the user later changes that profile'sbaseURLto a proxy, staging server, or another host, this non-canonical branch only strips AIMLAPI headers, so the Kimi identity headers are sent to the arbitrary override endpoint. Apply the same descriptor-owned-header cleanup for Kimi (while preserving user headers), and cover a canonical Kimi profile retargeted to a custom URL. -
[P2] Make abandoned-device-ID repair mutually exclusive
internal/kimiidentity/kimiidentity.go:102
When an invalid/empty ID file is present, every racing process waits, then unconditionally removes the path. A process can create and publish its replacement between another process's retry read andos.Remove; the latter then unlinks the valid winner and creates a second ID. The first process may return its now-unpersisted ID, so OAuth and later runtime requests use different device identities. Coordinate repair with an exclusive lock/recheck (or an atomic replacement protocol) and add a concurrent abandoned-file test. -
[P2] Cancel device polling on every TUI quit path
internal/tui/onboarding.go:161
The new cancel function is called for Esc, but Ctrl-C returnstea.Quitwithout calling it. The normal provider-wizard quit path has the same omission inmodel.quit, which only cancels the AIMLAPI flow. Because the TUI is launched withcontext.Background(), approving the device code after quitting can still let the background poll persist a credential the user deliberately abandoned. Cancel any active setup/provider-wizard device login before every quit and add coverage for Ctrl-C. -
[P2] Reject stale onboarding device-code attempts for the same provider
internal/tui/onboarding.go:568
Setup device-code messages carry onlyproviderID, unlike the provider wizard's attempt ID. Start Kimi login, press Esc before phase one returns, and start Kimi again: the late result from the first request matches the same provider while the second request is pending, replaces its displayed code/cancel function, and starts polling the canceled authorization. Its eventual success is accepted and persisted. Add an attempt generation to phase-one and poll-result messages, increment it when abandoning/restarting, and require it when applying results. -
[P3] Correct the Kimi preset instruction in the OAuth guide
docs/oauth-subscriptions.md:107
The guide says Kimi is opt-in viaZERO_OAUTH_ALLOW_PRESETS=1, but bothzero auth kimiandzero auth login kimi-codeusenewAuthManager, which setsAllowPresets: trueunconditionally. Users are told to set an unnecessary variable for the documented commands. Align this text with the actual CLI behavior.
Add a first-class Kimi Code OAuth provider modeled on the generic device-code preset path (like xAI), not the bespoke ChatGPT/Codex path. Kimi's returned access token works directly as a Bearer on its managed coding endpoint (api.kimi.com/coding/v1), so no claim extraction is needed. - internal/providercatalog/catalog.go: new `kimi` catalog entry, OpenAI-compatible at https://api.kimi.com/coding/v1, flagged OAuth + OAuthDeviceFlow, RequiresAuth with no API-key env var. Listed directly below the ChatGPT entry so it appears below ChatGPT in the OAuth options. - internal/oauth/presets.go: kimi device-code preset (auth.kimi.com endpoints, public kimi-code client_id); overridable via ZERO_OAUTH_KIMI_*. - internal/cli/auth.go: add `zero auth kimi` sugar routing to the generic device-code login; list kimi below chatgpt in help text. - internal/tui/provider_wizard.go: mention Kimi in the OAuth method subtitle and include it in the OAuth provider list. - docs/oauth-subscriptions.md: document the Kimi Code OAuth path. - Tests: presets, catalog, and oauth coverage updated.
One extra leading space made it the only misaligned entry in the provider list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…s model default Get() matches an exact descriptor ID before it ever reaches another descriptor's aliases. moonshot already aliases "kimi" to itself (its API-key path at api.moonshot.ai), so giving the new Kimi Code OAuth descriptor the same canonical ID silently stole that alias: any existing profile with catalogID "kimi" would resolve to the new OAuth-only descriptor instead of moonshot, changing its endpoint, default model, and auth method without the user asking for it. Renamed the descriptor (and preset key, CLI routing, TUI references, docs) to "kimi-code" — "zero auth kimi" still works as a shortcut, it just forwards to the non-colliding ID. Also fixed the default model: "kimi-k2.7-code-highspeed" does not exist on the managed endpoint. The real models are "kimi-for-coding" (standard tier, all members) and "kimi-for-coding-highspeed" (requires a higher subscription tier). Defaulted to the standard tier so a fresh login doesn't select a model the user's plan may not include. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Kimi Code's OAuth/API backend rejects device-authorization, poll, and refresh requests with 401 unless a handful of vendor-identity headers are present; the generic RFC 8628/OAuth2 form bodies this package builds had no way to carry them, so every real login attempt would have failed. Added Config.ExtraHeaders (mirroring the existing ExtraAuthParams pattern), applied at every request-building site (RequestDeviceCode, pollDeviceOnce, PostToken — shared by code exchange and refresh), and a new providerExtraHeaders hook in ResolveConfig that supplies Kimi's headers regardless of ZERO_OAUTH_ALLOW_PRESETS (this is a protocol requirement of Kimi's own backend, not tied to which client_id is in use). Header names, general shape, and the client_id were reverse-engineered from the open-source kimi-cli client (github.com/MoonshotAI/kimi-cli, src/kimi_cli/auth/oauth.py) since Kimi has no public API documentation for this; values are a best-effort match, not a verified spec — confirm against a real login before this ships. Device-Id is generated fresh per process rather than persisted to disk (kimi-cli persists it across runs); the header just needs to be present and ASCII-safe, not stable long-term, so this is a disclosed simplification, not a functional gap. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…device code zero auth kimi discarded every argument after "kimi", so `zero auth kimi --help` started a real device authorization instead of showing help, --scope was silently dropped, and typos went unvalidated. Now forwards args[1:] through the same parser `zero auth login` uses. Kimi Code has no browser/loopback OAuth endpoint at all, but the provider wizard's plain Enter on an OAuth-capable provider defaults to the loopback flow unless the environment already prefers device code (headless/SSH) — on a normal desktop session, pressing Enter on Kimi in /provider or first-run onboarding would attempt a flow that has no endpoint to hit. Added Descriptor.OAuthDeviceOnly and gated Enter to also start device login when it's set, alongside the existing "d" shortcut. Adjusted the footer hint and the "Sign in with OAuth" subtitle, which advertised Kimi under "one-click browser login" alongside providers that actually have one. Updated docs/oauth-subscriptions.md, which had the same "Kimi has browser login" claim in two places, directly contradicting its own "Kimi is device-code only" section further down. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Three review findings: - The X-Msh-* vendor-identity headers existed only in the OAuth request path, so after a successful login the OpenAI-compatible runtime sent /coding/v1 completions with just the bearer and Kimi's backend could reject every call. The header set now lives in a shared kimiidentity package used by both the OAuth flows and the kimi-code descriptor's CustomHeaders (populated lazily at catalog access so package import does no filesystem IO), and the device ID is persisted under the user config dir — mirroring kimi-cli's ~/.kimi/device_id — so login, refresh, and completions present one stable device identity. - First-run onboarding routed a desktop Enter on a device-only provider through the generic browser-login command, which discarded the device flow's verification URL and code and left the spinner to time out. The OAuthDeviceOnly check the /provider wizard already gained now applies to onboarding too, with a desktop regression test. - docs/oauth-subscriptions.md documented ZERO_OAUTH_KIMI_* override names that nothing reads; the provider resolves as kimi-code, so the documented names now carry the KIMI_CODE prefix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Avoid a race where concurrent first-run processes could both miss the device-identity file and overwrite each other's ID, by using an exclusive create and adopting the winner's ID on conflict. Remove a redundant header map copy in provider catalog cloning. Route the mouse double-click activation path through the same OAuthDeviceOnly check the keyboard path already used, so it no longer attempts a browser OAuth flow that doesn't exist for Kimi.
Only attach kimi-code X-Msh-* headers (and mint the on-disk device id) on Get/Require, not All/OAuthProviders listing. Add tests for moonshot "kimi" alias resolution, runtime-header laziness, exclusive device-id creation, and mouse advance on device-only providers.
… + testable device-id loader - Add Descriptor.RuntimeHeaders so cloneDescriptor no longer special-cases descriptor.ID == "kimi-code"; the kimi-code entry supplies kimiidentity.Headers, keeping the generic clone helper provider-agnostic and avoiding the redundant map copy on lazy header population. - Parameterize DeviceID loading behind loadOrCreateDeviceIDAt(path) so tests exercise the production exclusive-create + winner-adopt logic directly instead of re-implementing it, and add a read-existing test.
main added this test with PostToken's old 6-arg signature after this branch forked; PostToken here already takes extraHeaders, so the merge with main compiled fine textually but failed go vet.
Retry-read after exclusive create so concurrent first-run processes adopt the winner UUID even if they observe an empty file mid-write. Sync the written id before closing. Document device-code for xAI and Hugging Face in the OAuth chooser summary, and state that Kimi X-Msh-* headers apply across all OAuth and managed API calls.
Use kimi_code_cli for X-Msh-Platform, re-resolve RuntimeHeaders when the provider wizard builds a profile, and cancel in-flight device-code polls on Esc so abandoned logins do not silently persist credentials. Refs Gitlawb#708
If an exclusive create leaves an empty or invalid file (winner dies before writing the UUID), remove it once after the adopt retry window and exclusive-create again so callers converge on a persisted identity instead of permanently diverging. Refs Gitlawb#708
…alog endpoint A profile that started on api.kimi.com and persisted the descriptor's X-Msh-* headers only had those headers stripped on retargeting if the descriptor was aimlapi. Generalize the existing non-canonical-endpoint cleanup to any descriptor with catalog-owned headers, so a Kimi profile whose baseURL is later pointed at a proxy or staging host no longer sends its device identity there. User-supplied headers are still preserved.
When the persisted device-id file was invalid or empty (a previous process died mid-publish), every racing process would retry-read, then unconditionally remove and recreate it. A process could remove another process's just-published winner between that process's failed retry read and its own remove call, so the original process kept an id that no longer matched what was on disk. Repair is now serialized through an exclusive lock file: only the lock holder removes and recreates the id file, and everyone else waits to adopt whatever it publishes instead of attempting their own repair. Added a concurrent test that repairs the same abandoned file from many goroutines and checks they all converge on one persisted id.
…ttempts Ctrl+C during a first-run device-code poll, and the provider wizard's own quit path (model.quit, which only reset the aimlapi sub-flow), both returned tea.Quit without canceling the in-flight poll. Since the TUI runs on context.Background(), a login the user backed out of by quitting could still complete in the background and get persisted. Both quit paths now cancel any active setup/provider-wizard device login first. Separately, first-run setup's device-code messages only carried a providerID, so abandoning a Kimi login with Esc and immediately restarting it let a late phase-one result from the first attempt overwrite the second attempt's displayed code and start polling an authorization already backed out of. Added an attempt generation (mirroring the provider wizard's existing oauthAttemptID) that bumps on every restart and is required for a phase-one or poll result to apply. Added regression tests for Ctrl+C canceling the poll in both setup and the provider wizard, and for a stale device-code attempt being rejected.
The guide told users to set ZERO_OAUTH_ALLOW_PRESETS=1 for Kimi, but zero auth kimi and zero auth login kimi-code both go through the auth login engine, which enables presets unconditionally. The documented commands already work without that variable.
e609e40 to
86c1eac
Compare
|
Addressed all six findings from the latest review. [P1] Cron job-ID reservation - this was a rebase issue, not new code from this PR: [P1] Strip Kimi identity headers after retargeting a saved profile - [P2] Make abandoned-device-ID repair mutually exclusive - [P2] Cancel device polling on every TUI quit path - Ctrl+C during a first-run device-code poll, and [P2] Reject stale onboarding device-code attempts for the same provider - first-run setup's device-code messages only carried [P3] Correct the Kimi preset instruction in the OAuth guide - Verification: Pushed as a rebase onto current |
|
Independently re-verified the current head ( jatmn's cron/store.go P1 — this is a stale-branch artifact, not new code in this PR: Vasanthdev2004's three code items — all confirmed fixed on this head:
Verification: Still open, not mine to decide: Vasanthdev2004's original ask for kevin's explicit sign-off on shipping the baked-in |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Recover an abandoned device-ID repair lock
internal/kimiidentity/kimiidentity.go:128
If a process dies after creatingkimi-device-id.lockbut before its deferred cleanup, the lock persists while the ID file remains invalid. Every later process waits only 200 ms at the existing-lock branch and then returns a fresh, unpersisted UUID, so OAuth and runtime calls can no longer converge on the stableX-Msh-Device-Idthis integration requires. Use a crash-safe locking/publish protocol (or recover a stale lock) and cover an interrupted repair owner. -
[P2] Isolate Kimi device identity creation in every test package
internal/oauth/presets_test.go:191
Resolvingkimi-codeinvokeskimiidentity.Headers()and the process-globalDeviceID, which writes the real user config path. The catalog tests do the same throughGet("kimi-code")atinternal/providercatalog/catalog_test.go:480. Consequently normal focused or full tests create and retain~/.config/zero/kimi-device-idfor a developer or CI account despite never using Kimi. Redirect the config root before the first header-producing call in each package (or make the header/device-ID dependency injectable) so tests are side-effect free. -
[P3] Make preset setup guidance match the command behavior
internal/cli/auth.go:609
The newly updated help sayszero auth kimi/zero auth login kimi-coderequireZERO_OAUTH_ALLOW_PRESETS=1, butnewAuthManageralways setsAllowPresets: true, and the later Kimi guide correctly says the flag is unnecessary. The wizard section ofdocs/oauth-subscriptions.mdstill gives the same contradictory instruction. Correct the help and wizard documentation (and add output coverage) so users are not told to configure a prerequisite the documented flows do not need.
…ation, and help text
|
Addressed all review findings from @jatmn's latest review:
All unit tests pass (go test ./internal/kimiidentity/... ./internal/oauth/... ./internal/providercatalog/...). Ready for re-review! |
3 participants
Summary
Adds a first-class Kimi Code OAuth login to Zero, listed directly below ChatGPT in the OAuth options.
Kimi is modeled on the generic device-code preset path (like xAI), not the bespoke ChatGPT/Codex path — because Kimi's returned access token works directly as a
Beareron its managed coding endpoint, so no claim extraction is needed.Changes
internal/providercatalog/catalog.go— newkimicatalog entry, OpenAI-compatible athttps://api.kimi.com/coding/v1, flaggedOAuth+OAuthDeviceFlow,RequiresAuthwith no API-key env var. Placed immediately after the ChatGPT entry so it appears below ChatGPT in the OAuth provider list.internal/oauth/presets.go—kimidevice-code preset (auth.kimi.com endpoints, publickimi-codeclient_id); overridable viaZERO_OAUTH_KIMI_*.internal/cli/auth.go— addszero auth kimisugar routing to the generic device-code login; listskimibelowchatgptin the help text.internal/tui/provider_wizard.go— mentions Kimi in the OAuth method subtitle and includes it in the OAuth provider list.docs/oauth-subscriptions.md— documents the Kimi Code OAuth path.presets_test.go,catalog_test.go,oauth_test.goupdated.Safety / ToS note
The preset ships the public
kimi-codeclient_id and Moonshot'sauth.kimi.comendpoints copied from the open-sourcekimi-codeCLI. It does nothing a user couldn't already do manually (standard RFC 8628 device-code flow, no client spoofing). Off by default; enable withZERO_OAUTH_ALLOW_PRESETS=1or explicit env overrides.Verification
go fmt,go vet, golangci-lint (unused,ineffassign,staticcheck), andgovulncheckpass (no issues in modified files).go test ./internal/oauth/... ./internal/providercatalog/... ./internal/cli/...pass.🤖 Generated with Zero
Summary by CodeRabbit
zero auth kimisupport.ZERO_OAUTH_ALLOW_PRESETS=1andZERO_OAUTH_KIMI_CODE_*overrides.