feat(auth): add Kimi Code OAuth login - #851
Conversation
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.
…ation, and help text
…ation, and help text
…air lock Restrict X-Msh-* device headers to canonical Kimi hosts on endpoint overrides, make device-id repair locks ownership-safe with token checking, honor cancellation in CompleteDeviceLogin before storing tokens, and strip runtime identity headers from persisted config profiles. Refs Gitlawb#708
|
Warning Review limit reached
Next review available in: 4 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
WalkthroughKimi Code is added as a device-only OAuth provider. The change adds persistent identity headers, propagates provider headers through OAuth requests, supports cancellation and stale-attempt rejection in TUI flows, adds a CLI alias, and updates profile resolution and documentation. ChangesKimi Code OAuth
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TUI
participant OAuthManager
participant KimiCode
participant ConfigStore
User->>TUI: select Kimi Code and start login
TUI->>OAuthManager: request device code
OAuthManager->>KimiCode: send device request with identity headers
KimiCode-->>TUI: return device code
TUI->>OAuthManager: poll with cancellable context
OAuthManager->>KimiCode: request token
KimiCode-->>OAuthManager: return token
OAuthManager->>ConfigStore: persist credentials
Possibly related PRs
🚥 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: 9
🧹 Nitpick comments (7)
internal/kimiidentity/kimiidentity_test.go (1)
270-274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the staleness threshold in this comment.
The comment says
> 1s. The production threshold inrepairAbandonedDeviceIDis2*time.Second. The 5-second backdate still works, but the wrong number here will mislead the next person who tunes the threshold.♻️ Proposed comment fix
- // Backdate lock file mtime to make it stale (> 1s) + // Backdate lock file mtime to make it stale (> 2s, the repair threshold)🤖 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/kimiidentity/kimiidentity_test.go` around lines 270 - 274, Update the comment above the lock-file timestamp backdating in the test to state that the mtime is made stale by exceeding the 2-second threshold used by repairAbandonedDeviceID; leave the 5-second backdate and test logic unchanged.internal/oauth/presets.go (2)
174-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTighten and clarify the host allowlist.
Two points.
First, the allowlist is partly redundant.
strings.HasSuffix(host, ".kimi.com")already coversauth.kimi.comandapi.kimi.com, so only the barekimi.comcase needs a separate test. The suffix tests are correct as written; the leading dot prevents anotkimi.commatch.Second, the domain set looks inconsistent.
.moonshot.cnis approved, but baremoonshot.cnis not, and.moonshot.aiis absent even though the existingmoonshotdescriptor usesapi.moonshot.ai. State the intent in a comment, or narrow the list to the hosts Kimi Code actually serves.♻️ Proposed simplification
host := strings.ToLower(u.Hostname()) - return host == "auth.kimi.com" || host == "api.kimi.com" || host == "kimi.com" || strings.HasSuffix(host, ".kimi.com") || strings.HasSuffix(host, ".moonshot.cn") + for _, domain := range []string{"kimi.com", "moonshot.cn"} { + if host == domain || strings.HasSuffix(host, "."+domain) { + return true + } + } + return false }🤖 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/oauth/presets.go` around lines 174 - 181, Update isCanonicalKimiHost to remove the redundant auth.kimi.com and api.kimi.com checks, retaining the bare kimi.com and dotted suffix checks. Clarify the intended allowlist with a comment or narrow it to hosts Kimi Code actually serves; ensure moonshot.cn handling is consistent with .moonshot.cn and include or exclude .moonshot.ai based on the existing api.moonshot.ai usage.
162-172: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOne non-canonical override silently strips the headers from the canonical requests too.
providerExtraHeadersreturnsnilif any single supplied endpoint is non-canonical. Consider an operator who overrides onlyZERO_OAUTH_KIMI_CODE_TOKEN_URLto a proxy and leaves the device endpoint atauth.kimi.com. The device-authorization request then goes to the canonical Kimi host without theX-Msh-*headers. Per the comment at Lines 183-190, Kimi answers 401. The user sees an authorization failure with no indication that an override caused it.The fail-closed choice is right; do not send identity headers to an unapproved host. The problem is that the decision is all-or-nothing and invisible.
Two options, in order of preference:
- Decide per endpoint. Attach the headers to a request only when that request's own host is canonical. This keeps login working when only the token URL is proxied.
- If you keep the all-or-nothing rule, surface it. Emit a warning at login time that names the offending environment variable and states that vendor-identity headers are disabled.
Also document the behavior in
docs/oauth-subscriptions.mdso the override is not a trap.🤖 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/oauth/presets.go` around lines 162 - 172, Update providerExtraHeaders to decide Kimi vendor-identity headers per endpoint rather than disabling them globally when any override is non-canonical: return headers only for requests whose own host is canonical, while preserving the fail-closed behavior for proxied hosts. Ensure the endpoint-to-header decision is available to each request, and document the mixed canonical/non-canonical override behavior in docs/oauth-subscriptions.md.internal/oauth/flow_test.go (1)
156-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test that asserts the extra headers reach the wire.
Both updated call sites pass
nilfor the newextraHeadersparameter, which is correct. No test in this cohort verifies the positive case, though.TestResolveConfigKimiCodePresetchecks only thatConfig.ExtraHeadersis populated; nothing provesapplyExtraHeadersputs those headers on the outgoing request.Add a test with an
httptestserver that recordsr.Headerand assert theX-Msh-*values arrive. CoverPostTokenat minimum, and ideallyRequestDeviceCodeandpollDeviceOncetoo, since Kimi requires the headers on all three.The repository guidelines require a regression test for behavior changes.
Also applies to: 260-260
🤖 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/oauth/flow_test.go` at line 156, Add regression coverage for extra-header propagation using an httptest server that records incoming request headers. Extend the PostToken test around its extraHeaders argument to pass representative X-Msh-* values and assert they arrive on the server request; also cover RequestDeviceCode and pollDeviceOnce if their test fixtures permit, ensuring all Kimi-required flows verify the headers reach the wire.Source: Coding guidelines
internal/oauth/providers.go (1)
80-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass the endpoint overrides as a slice for readability.
The call takes four same-typed positional arguments after
name. A reader cannot tell the order without openingproviderExtraHeaders. A single[]stringargument, or reuse of the already-computedcfgendpoint values, would read better.Note that this line resolves the endpoint values a second time. Lines 75-78 already compute the same environment lookups.
🤖 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/oauth/providers.go` at line 80, Update the ExtraHeaders construction in the provider configuration to reuse the endpoint values already computed in cfg on lines 75-78, passing them as a single ordered []string to providerExtraHeaders instead of repeating positional envValue lookups. Adjust providerExtraHeaders as needed to accept the slice while preserving endpoint order.internal/tui/onboarding.go (1)
848-857: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the device-only bypass rule into one shared helper. Both wizards independently implement the same condition to decide when to skip the browser OAuth flow for a device-only provider like Kimi Code, and each file's comment notes the other must be kept in sync manually.
internal/tui/onboarding.go#L848-L857: replace the inlinedescriptor.OAuthDeviceFlow && (descriptor.OAuthDeviceOnly || oauthPreferDeviceFlow())check with a call to a shared helper, e.g.useDeviceLogin(descriptor).internal/tui/provider_wizard_discovery.go#L46-L51: replace the identical inline check onproviderwith the same shared helper, so both wizards can never diverge on this rule.♻️ Proposed shared helper
// useDeviceLogin reports whether the OAuth flow for this provider should go // straight to device-code login instead of the browser flow: either the // provider has no browser/loopback endpoint at all (OAuthDeviceOnly), or the // environment prefers device flow (headless/SSH). func useDeviceLogin(d providercatalog.Descriptor) bool { return d.OAuthDeviceFlow && (d.OAuthDeviceOnly || oauthPreferDeviceFlow()) }🤖 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/tui/onboarding.go` around lines 848 - 857, Extract the shared device-login condition into a helper such as useDeviceLogin(d providercatalog.Descriptor), preserving the OAuthDeviceFlow, OAuthDeviceOnly, and oauthPreferDeviceFlow logic. In internal/tui/onboarding.go lines 848-857, replace the inline check with this helper; make the same replacement for provider in internal/tui/provider_wizard_discovery.go lines 46-51 so both wizards use the centralized rule.internal/providercatalog/catalog.go (1)
452-460: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
cloneDescriptordrops runtime headers for any descriptor that also has staticCustomHeaders.The
else ifmakes the two header sources mutually exclusive. A descriptor with both staticCustomHeadersandRuntimeHeaderssilently loses its runtime headers. No current descriptor has both, so this is not a live bug. The doc comment onRuntimeHeaderspresents the mechanism as provider-agnostic, so merge instead of choosing.♻️ Proposed merge of both header sources
- if descriptor.CustomHeaders != nil { - descriptor.CustomHeaders = copyStringMap(descriptor.CustomHeaders) - } else if withRuntimeHeaders && descriptor.RuntimeHeaders != nil { - descriptor.CustomHeaders = descriptor.RuntimeHeaders() - } + static := copyStringMap(descriptor.CustomHeaders) + if withRuntimeHeaders && descriptor.RuntimeHeaders != nil { + merged := descriptor.RuntimeHeaders() + for key, value := range static { + merged[key] = value + } + descriptor.CustomHeaders = merged + } else { + descriptor.CustomHeaders = static + }🤖 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 452 - 460, Update cloneDescriptor so withRuntimeHeaders merges RuntimeHeaders into any copied static CustomHeaders instead of making the sources mutually exclusive. Preserve static headers, add runtime headers when available, and retain nil behavior when neither source exists.
🤖 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/config/resolver.go`:
- Around line 1095-1110: Update the conditional guarding the custom-header
cleanup in the descriptor resolution flow to use the presence of
descriptor.RuntimeHeaders rather than len(descriptor.CustomHeaders). Preserve
removal of matching catalog headers and all X-Msh-* headers for non-canonical
endpoints, including when RuntimeHeaders produces an empty CustomHeaders map.
In `@internal/kimiidentity/kimiidentity.go`:
- Around line 71-77: Remove the exported ResetDeviceIDForTest seam from the
production file to satisfy unreachable-function checks, and replace it with a
lint-compatible test seam such as an unexported deviceIDPathOverride used by the
device-ID loading path. Update tests to drive isolation through that seam while
preserving production behavior and synchronization.
- Around line 194-216: Update createOrAdoptDeviceID’s successful os.Rename path
in repairAbandonedDeviceID to re-read path with readValidDeviceID (or the
established retry helper) and return the persisted device ID when available,
falling back to id only if necessary. Preserve the existing rename and
error-handling behavior; do not add lock-mtime refresh unless separately
required.
- Around line 53-56: Run gofmt on internal/kimiidentity/kimiidentity.go to
normalize the var-block alignment and remove the consecutive blank lines after
ResetDeviceIDForTest, ensuring the file passes make fmt-check.
In `@internal/oauth/providers_test.go`:
- Around line 90-101: Prevent device-ID test state from leaking across platforms
and test order by exporting a shared user-config redirect helper from
internal/kimiidentity, based on setUserConfigRoot, that redirects APPDATA, HOME,
and XDG_CONFIG_HOME to t.TempDir() and resets the cached ID via t.Cleanup. Use
this helper in internal/oauth/providers_test.go at lines 90-101 and
internal/oauth/presets_test.go at lines 191-197; the latter must no longer omit
HOME. Also apply the same helper to internal/providercatalog/catalog_test.go’s
TestKimiRuntimeHeadersOnlyOnGet.
- Around line 104-107: Run gofmt on the map literal passed to ResolveConfig in
the relevant test, correcting the spacing between the ZERO_OAUTH_ALLOW_PRESETS
key and its value to match the longest key. Then run gofmt -l ./... and ensure
no formatting drift remains.
In `@internal/providercatalog/catalog_test.go`:
- Around line 494-499: Real Kimi device IDs are written outside the test sandbox
because config-root isolation is incomplete. Add a shared helper modeled on
setKimiUserConfigRoot that sets XDG_CONFIG_HOME, APPDATA, and HOME to
t.TempDir() and resets the device ID before and after; use it at
internal/providercatalog/catalog_test.go lines 494-499, 458-489 before Get
calls, and at internal/tui/provider_wizard_oauth_test.go lines 624-642 before
mouseTestModel().
In `@internal/providercatalog/catalog.go`:
- Around line 263-290: Remove the unused ListByTransport, ValidTransport, and
ValidAPIFormat helpers from the change. Do not add replacement abstractions or
broaden the scope; only retain these helpers if a production caller is added and
uses them.
In `@internal/tui/provider_wizard.go`:
- Around line 2144-2155: Update the Kimi profile construction around
providercatalog.Get and the profile.CustomHeaders assignment to preserve the
re-resolved X-Msh-* identity headers expected by
TestProviderWizardProfileAppliesKimiRuntimeHeaders. Remove the provider.ID ==
"kimi-code" strip loop, leaving the cloned runtime descriptor headers in
profile.CustomHeaders.
---
Nitpick comments:
In `@internal/kimiidentity/kimiidentity_test.go`:
- Around line 270-274: Update the comment above the lock-file timestamp
backdating in the test to state that the mtime is made stale by exceeding the
2-second threshold used by repairAbandonedDeviceID; leave the 5-second backdate
and test logic unchanged.
In `@internal/oauth/flow_test.go`:
- Line 156: Add regression coverage for extra-header propagation using an
httptest server that records incoming request headers. Extend the PostToken test
around its extraHeaders argument to pass representative X-Msh-* values and
assert they arrive on the server request; also cover RequestDeviceCode and
pollDeviceOnce if their test fixtures permit, ensuring all Kimi-required flows
verify the headers reach the wire.
In `@internal/oauth/presets.go`:
- Around line 174-181: Update isCanonicalKimiHost to remove the redundant
auth.kimi.com and api.kimi.com checks, retaining the bare kimi.com and dotted
suffix checks. Clarify the intended allowlist with a comment or narrow it to
hosts Kimi Code actually serves; ensure moonshot.cn handling is consistent with
.moonshot.cn and include or exclude .moonshot.ai based on the existing
api.moonshot.ai usage.
- Around line 162-172: Update providerExtraHeaders to decide Kimi
vendor-identity headers per endpoint rather than disabling them globally when
any override is non-canonical: return headers only for requests whose own host
is canonical, while preserving the fail-closed behavior for proxied hosts.
Ensure the endpoint-to-header decision is available to each request, and
document the mixed canonical/non-canonical override behavior in
docs/oauth-subscriptions.md.
In `@internal/oauth/providers.go`:
- Line 80: Update the ExtraHeaders construction in the provider configuration to
reuse the endpoint values already computed in cfg on lines 75-78, passing them
as a single ordered []string to providerExtraHeaders instead of repeating
positional envValue lookups. Adjust providerExtraHeaders as needed to accept the
slice while preserving endpoint order.
In `@internal/providercatalog/catalog.go`:
- Around line 452-460: Update cloneDescriptor so withRuntimeHeaders merges
RuntimeHeaders into any copied static CustomHeaders instead of making the
sources mutually exclusive. Preserve static headers, add runtime headers when
available, and retain nil behavior when neither source exists.
In `@internal/tui/onboarding.go`:
- Around line 848-857: Extract the shared device-login condition into a helper
such as useDeviceLogin(d providercatalog.Descriptor), preserving the
OAuthDeviceFlow, OAuthDeviceOnly, and oauthPreferDeviceFlow logic. In
internal/tui/onboarding.go lines 848-857, replace the inline check with this
helper; make the same replacement for provider in
internal/tui/provider_wizard_discovery.go lines 46-51 so both wizards use the
centralized rule.
🪄 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 Plus
Run ID: 6fe85d3e-89da-4094-adb6-1e5d3b99e47c
📒 Files selected for processing (26)
docs/oauth-subscriptions.mdinternal/cli/auth.gointernal/config/resolver.gointernal/config/resolver_test.gointernal/kimiidentity/kimiidentity.gointernal/kimiidentity/kimiidentity_test.gointernal/oauth/device.gointernal/oauth/flow.gointernal/oauth/flow_test.gointernal/oauth/manager.gointernal/oauth/oauth.gointernal/oauth/presets.gointernal/oauth/presets_test.gointernal/oauth/providers.gointernal/oauth/providers_test.gointernal/providercatalog/catalog.gointernal/providercatalog/catalog_test.gointernal/providercatalog/export_test.gointernal/providercatalog/oauth_test.gointernal/tui/model.gointernal/tui/oauth_device.gointernal/tui/onboarding.gointernal/tui/onboarding_test.gointernal/tui/provider_wizard.gointernal/tui/provider_wizard_discovery.gointernal/tui/provider_wizard_oauth_test.go
💤 Files with no reviewable changes (1)
- internal/providercatalog/export_test.go
providerWizardProfile re-resolves through providercatalog.Get to carry Kimi's X-Msh-* RuntimeHeaders into the wizard's post-login profile, but an unconditional strip added right after undid that inside the same sameProviderBaseURL(default) branch -- so the headers it just fetched never survived. That block wasn't backed by a test or review comment, and it broke TestProviderWizardProfileAppliesKimiRuntimeHeaders, the regression test written earlier in this PR specifically to guard this path. The real canonical-host restriction already lives in internal/oauth/presets.go's isCanonicalKimiHost, which this code path doesn't need to duplicate since it only runs when the base URL is already the default.
Gate X-Msh identity stripping on RuntimeHeaders, re-read the device id after repair rename, isolate config roots in tests, move test-only reset into export_test.go, and drop unused providercatalog helpers.
|
Addressed CodeRabbit findings in
@coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 59 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/providercatalog/catalog_test.go (1)
482-503: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert that listing does not create a device ID.
The test only checks that listing descriptors have no
CustomHeaders. A regression could invokeRuntimeHeaders, discard the returned headers, and still persist the Kimi device ID.After
All()andOAuthProviders(), assert that the isolated config root has no Kimi device-ID file before callingGet("kimi-code").As per coding guidelines,
**/*_test.gorequires a regression test for behavior changes.🤖 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_test.go` around lines 482 - 503, Extend TestKimiRuntimeHeadersOnlyOnGet after the All() and OAuthProviders() listing checks to assert that the isolated config root contains no persisted Kimi device-ID file. Use the test’s existing isolation and device-ID storage symbols, and keep the assertion before Get("kimi-code") so listing cannot create or persist the device ID.Source: Coding guidelines
🧹 Nitpick comments (1)
internal/kimiidentity/kimiidentity.go (1)
65-79: 🩺 Stability & Availability | 🔵 TrivialRun identity handling under the race detector.
This package uses goroutines and shared identity-file repair paths, so run
internal/kimiidentitywith-racein an environment where Go can enable the race detector.🤖 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/kimiidentity/kimiidentity.go` around lines 65 - 79, Run the internal/kimiidentity package tests with Go’s race detector enabled, using an environment/toolchain where race detection is supported; verify that DeviceID and the shared identity-file repair paths complete without reported data races.Source: Coding guidelines
🤖 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`:
- Line 13: Add a regression test in the test flow around IsolateDeviceIDStorage
that uses two temporary config roots sequentially, calls DeviceID after
switching roots, and verifies each root receives its own device ID rather than
reusing the cached value from the first root.
---
Outside diff comments:
In `@internal/providercatalog/catalog_test.go`:
- Around line 482-503: Extend TestKimiRuntimeHeadersOnlyOnGet after the All()
and OAuthProviders() listing checks to assert that the isolated config root
contains no persisted Kimi device-ID file. Use the test’s existing isolation and
device-ID storage symbols, and keep the assertion before Get("kimi-code") so
listing cannot create or persist the device ID.
---
Nitpick comments:
In `@internal/kimiidentity/kimiidentity.go`:
- Around line 65-79: Run the internal/kimiidentity package tests with Go’s race
detector enabled, using an environment/toolchain where race detection is
supported; verify that DeviceID and the shared identity-file repair paths
complete without reported data races.
🪄 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 Plus
Run ID: 5df72e9e-020b-4def-a02f-2272ed0acc15
📒 Files selected for processing (11)
internal/config/resolver.gointernal/config/resolver_test.gointernal/kimiidentity/export_test.gointernal/kimiidentity/kimiidentity.gointernal/kimiidentity/kimiidentity_test.gointernal/oauth/presets_test.gointernal/oauth/providers_test.gointernal/providercatalog/catalog.gointernal/providercatalog/catalog_test.gointernal/tui/provider_wizard.gointernal/tui/provider_wizard_oauth_test.go
💤 Files with no reviewable changes (1)
- internal/tui/provider_wizard.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/config/resolver.go
- internal/config/resolver_test.go
- internal/tui/provider_wizard_oauth_test.go
- internal/oauth/providers_test.go
- internal/oauth/presets_test.go
Add a path-keyed DeviceID regression across two config roots, and assert All/OAuthProviders do not persist kimi-device-id before Get mints it.
|
Addressed the two new CodeRabbit findings in the follow-up commit:
Also ran @coderabbitai full review |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 52 minutes. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Thanks for the very thorough work here — the OAuth plumbing itself is in good shape and I checked the parts that usually go wrong on this path:
- Redirects are refused on all four credential-bearing POSTs (
RequestDeviceCode,pollDeviceOnce,PostToken, andRefreshviaPostToken), so the newExtraHeaderscan't ride a 307 to another origin. - I drove a throwaway
httptestTLS server through all four and confirmed theX-Msh-*values actually reach the wire — that part works. providerExtraHeadersfails closed when an endpoint is overridden off a canonical Kimi host, and resolving a retargetedkimi-codeprofile through the realconfig.Resolveentry point does drop the identity headers while keeping user-supplied ones.- The
kimialias still resolves tomoonshot,gofmt/go vetare clean, and./internal/{kimiidentity,oauth,providercatalog,config,tui}all pass (the onlyinternal/clifailure is the pre-existing symlink-privilege one).
That said, I'd like changes before this merges. The problems are all in the device-identity lifecycle, not the protocol work.
1. First-run onboarding mints the Kimi device id for every user. internal/tui/onboarding.go:117 — setupOAuthProviderOptions filters the provider list by calling providercatalog.Get(option.ID) on every option, and Get now runs RuntimeHeaders → kimiidentity.DeviceID(). That function is reached from setupMethodOptions (onboarding.go:541), which is called from the render path setupMethodLines (onboarding.go:1665) as well as advanceSetup (onboarding.go:827). So the "How do you want to connect?" screen creates <UserConfigDir>/zero/kimi-device-id on first paint, before anyone has picked a provider.
I proved it with a temp config root: after All() and OAuthProviders() the file does not exist, and after one setupOAuthProviderOptions call it does. That's the exact invariant cloneDescriptor's own comment claims to protect ("merely enumerating providers never mints ~/.config/zero/kimi-device-id for users who never touch Kimi"). The filter only needs descriptor.OAuth, which the listing descriptors already carry — use OAuthProviders()/All() here, or add a headers-free lookup. Please add a test that walks the method + provider screens and asserts the file was never created; right now nothing would catch this coming back.
2. The wizard bakes the hostname and device UUID into config.json, and the resolver ignores them anyway. internal/tui/provider_wizard.go:2145 copies the runtime headers into profile.CustomHeaders, and applyProviderWizard persists that profile through config.UpsertProvider. I upserted a wizard-built profile into a temp config and read it back:
"customHeaders": {
"X-Msh-Device-Id": "b5fd6ed8-50fb-4eec-8195-87571e6c8f60",
"X-Msh-Device-Model": "windows amd64",
"X-Msh-Device-Name": "Vasanth",
...
}
X-Msh-Device-Name is os.Hostname(). Two problems with persisting it. First, it's dead weight: applyCatalogDescriptor at internal/config/resolver.go:1074 deliberately skips stored x-msh-* values so the freshly minted ones win, so nothing ever reads the persisted copy. Second, it puts a machine hostname and a stable device UUID into a file people paste into bug reports, and the strip in resolver.go:1112 only fires while catalogId is present — resolving a profile with those headers, catalogId removed and baseURL: https://proxy.example.test/v1 forwards X-Msh-Device-Id and X-Msh-Device-Name to that host untouched. Note zero auth kimi (EnsureCatalogProvider) does not persist them, so the CLI and TUI already disagree. Simplest fix: keep the wizard's in-memory profile for the discovery call, but don't write RuntimeHeaders-derived headers to disk — the resolver re-attaches them at resolve time.
3. Both new guards in applyCatalogDescriptor are unproven. I removed the x-msh- prefix delete (resolver.go:1112-1116) and the stored-header override guard (resolver.go:1074-1076) and ran go test ./internal/config/ -count=1 — fully green. TestApplyCatalogDescriptorStripsKimiIdentityFromRetargetedProfile (resolver_test.go:1577) passes only because Require("kimi-code") returns a CustomHeaders map containing every X-Msh-* key, so the pre-existing EqualFold catalog-key loop already deletes both of the profile's headers. The only new code that test exercises is the widened else if condition. Please add: a case where the descriptor has RuntimeHeaders but empty CustomHeaders (the case the prefix branch exists for), and a case where a stale persisted X-Msh-Device-Id must lose to the freshly minted one on the canonical endpoint. Both should go red if the corresponding line is deleted.
4. Two existing tests were deleted rather than updated.
internal/providercatalog/catalog_test.go—TestListByTransportPreservesCatalogOrderis gone (it was at line 364 onmain), along with itsexport_test.gohelper. That test pinned per-transport catalog ordering and theTransportOpenAICompatible/TransportAnthropicCompatiblealias normalisation. Addingkimi-codeinto the openai-compatible run breaks itswantIDslist, so the fix was one line: add"kimi-code"after"chatgpt". Deleting the only ordering guard to make a new descriptor fit isn't a trade I want to make. (SwappingValidTransport/ValidAPIFormatfor inline maps inTestCatalogDescriptorsExposeRequiredDefaultsis fine — that keeps the per-descriptor invariant.)internal/oauth/providers_test.go—TestEnvKeywas replaced in place byTestResolveConfigKimiCodeStripsExtraHeadersOnEndpointOverride.envKeyis still production code and is exactly what mapskimi-code→ZERO_OAUTH_KIMI_CODE_*; put the old test back alongside the new one.
One thing that needs a decision, not a code change. internal/providercatalog/catalog.go:158 says the bearer is accepted directly "so no client spoofing is involved", while internal/kimiidentity/kimiidentity.go:30-37 says X-Msh-Platform must be kimi_code_cli because the coding/v1 endpoint runs a client whitelist. Those can't both be true, and the second one is the operative claim — we'd be presenting Zero as Moonshot's own CLI to get past an allowlist. That's an owner call and I'd like it made explicitly before this lands, same as we've done for other provider presets. Related: your own comments say the header names and values are reverse-engineered from kimi-cli and "should be confirmed against a real login before this ships" (and X-Msh-Version currently ships the literal string "unknown"). Has anyone run a real Kimi Code login end to end against this branch? If not, that's the last gate.
Smaller things, none blocking:
internal/oauth/presets.go:162-172: overriding one endpoint (say onlyZERO_OAUTH_KIMI_CODE_TOKEN_URL) drops the identity headers from all requests, including the ones still going toauth.kimi.com, which by your own comment means a 401 the user can't diagnose. Either decide per endpoint, or surface which env var disabled them.internal/oauth/presets.go:174-180: the allowlist takes.moonshot.cnbut not baremoonshot.cnand not.moonshot.ai, while the existingmoonshotdescriptor usesapi.moonshot.ai. Add a line saying that's deliberate or tighten the list.- No test asserts
ExtraHeadersreach the wire. I verified by hand that they do, but given the entire feature hinges on it, anhttptestserver recordingr.HeaderacrossRequestDeviceCode/pollDeviceOnce/PostToken/Refreshis worth having. internal/kimiidentity/kimiidentity.go:168: the 2s lock-staleness threshold is never refreshed while the holder works, so a stalled holder can have its lock broken mid-publish and the two racers can walk away with different ids than what's on disk. Narrow, and it needs an already-corrupt file to reach — but consider touching the lock mtime while working.docs/oauth-subscriptions.mddocuments theX-Msh-*requirement but not that one of those headers is the user's machine hostname. Worth saying out loud since it goes to a third party on every completion.
Happy to re-review as soon as 1-4 are addressed.
Everything in one pass as usual, nothing held back for a second round. The two deleted tests are the ones I'd push back on hardest — a guard removed to make a new descriptor fit is the kind of thing that's invisible in six months, and both look like one-line updates rather than deletions.
|
Addressed Vasanth review items 1-4:
Owner decision still open: catalog comment says bearer is accepted with "no client spoofing" while |
Stop first-run onboarding from minting kimi-device-id via Get on every OAuth option, strip RuntimeHeaders before wizard persist, pin applyCatalogDescriptor guards with real coverage, and restore ListByTransport/EnvKey tests.
8334dc0 to
d41e06f
Compare
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving. Re-checked on d41e06fd and all four are properly closed — I verified rather than read.
Onboarding no longer mints the device id. Filtering via OAuthProviders() listing clones instead of Get()/RuntimeHeaders is the right fix — it removes the call rather than guarding it, so there's no path left that reaches DeviceID() from a render.
The wizard no longer persists identity headers. stripRuntimeIdentityHeaders is genuinely pinned: I disabled it and TestStripRuntimeIdentityHeadersDropsXMshOnly goes red with the persist copy still carrying X-Msh-Device-Id, X-Msh-Device-Name and X-Msh-Platform — while correctly keeping X-User-Agent. That last part matters; a strip that took everything would have passed a weaker test.
Both deleted tests are back. TestListByTransportPreservesCatalogOrder returns with kimi-code added to its expected list, which was the one-line fix, and TestEnvKey sits alongside the new endpoint-override test rather than replacing it. Thanks for restoring them rather than arguing the point — the catalog-ordering guard is exactly the kind of thing nobody misses until it's needed.
internal/oauth, internal/providercatalog and internal/config all green here. The single internal/tui failure is TestAltScreenTranscriptScrollKeepsFooterFixed, which fails on origin/main for me too and passes in CI — a symlink/terminal quirk on my box, unrelated to this.
2 participants
Summary
X-Msh-*vendor-identity headersinternal/kimiidentitypackage to manage the per-device Kimi identity file, including lazy creation, abandoned/empty-file repair with an ownership-safe lock, and stripping the identity headers when a request retargets off the Kimi catalog endpointzero auth kimi), and TUI onboarding all route through the shared device-code flow, with the desktop Enter key routed to device code and polling cancelled on every quit pathTest plan
go test ./internal/kimiidentity/... ./internal/oauth/... ./internal/providercatalog/... ./internal/tui/... ./internal/cli/...Summary by CodeRabbit
New Features
zero auth kimilogin command.Bug Fixes