Skip to content

fix(mac): accept Kimi Code Plan API keys - #1211

Open
MiguelMachado-dev wants to merge 2 commits into
getagentseal:mainfrom
MiguelMachado-dev:fix/kimi-code-plan-api-keys
Open

MiguelMachado-dev wants to merge 2 commits into
getagentseal:mainfrom
MiguelMachado-dev:fix/kimi-code-plan-api-keys

Conversation

@MiguelMachado-dev

Copy link
Copy Markdown
Contributor

Problem

The mac menubar rejects Kimi Code Plan API keys, so users on Kimi's flat-rate coding plan can't connect their quota to the Capacity Dock.

Fix

  • Accept Code Plan API keys in KimiSubscriptionService (key format validation + the quota endpoint that plan uses)
  • Show the Kimi short window first when both windows are present
  • Settings UI copy for the Code Plan auth path

Tests

  • KimiUsageParsingTests extended (30 new assertions covering the Code Plan key path)
  • Verified locally against a live Code Plan key: quota windows load and render in the dock

Kimi's special settings path bypassed provider-scoped API credentials and required a short-lived CLI token. Save the Code Plan key in CodeBurn's Keychain item and prefer it for the existing usage endpoint, with the CLI token retained as fallback.
@MiguelMachado-dev

Copy link
Copy Markdown
Contributor Author

The test failures are unrelated to this PR — pre-existing date-dependent bug in tests/cli-budget.test.ts (seeds at today 12:00 UTC, which is in the future before noon UTC, so the overview shows no usage). All four of these PRs fail on the identical test. Reproduced and fixed in #1216; a re-run after that merges (or after 12:00 UTC) goes green.

@ozymandiashh ozymandiashh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks. swift build passes on the branch and on a merge with current main (clean). Three blockers, one of which main already solved for another provider.

  1. Disconnect no longer sticks. A saved Keychain key is now sufficient for background activation (AppStore.swift:151-155) and the refresh guard accepts CapacityDockProviderCredentialPresence.contains(...) (line 1277). But disconnectKimi() (1301-1309) neither deletes the key nor records an opt-out, and disconnectCapacityDockProvider(.kimiCode) returns through legacyFilter before the credential remover runs. So the user hits Disconnect and the next kimiDue tick reconnects, with the secret still in the Keychain. Main fixed exactly this for Copilot in 9794e94e / 94218bf1 with CopilotExplicitDisconnect; Kimi needs the same persisted opt-out, plus the remover actually reachable from the dock's Disconnect.

  2. Keychain read has no fallback. fetchKimiUsage() resolves the key through loadAsync inside performAsync(timeout: 2.5). On timeout it throws CapacityDockProviderCredentialStoreError.timedOut, which is not a FetchError, so Kimi lands in .failed with "Keychain did not respond" even when a working CLI credential is on disk. After the merge this is more reachable, not less: main's da20fd0f routes Presence.set through DispatchQueue.main.sync, and load(for:) calls it on the same background read racing the deadline. On any credential-read failure fall back to freshToken(deps:), or map the loader error into a FetchError.

  3. The description doesn't match the diff. There is no key-format validation anywhere; the endpoint is unchanged (api.kimi.com/coding/v1/usages, as your own commit message says); and "30 new assertions" is one test with one assertion covering trim + bearer. The one protocol change that is in the diff, dropping X-Msh-Platform / X-Msh-Device-Id for key auth, replaces a comment with its opposite and has no fixture. Please rewrite the body to describe what changed, and add tests for apiKeyRejected, the header suppression, the CLI fallback and the timeout path. You built a Dependencies DI seam but never wired it through AppStore (deps: .live); main's CopilotQuotaRuntime is the pattern to follow so the store-level path is testable.

Separately: commit 2's "short window first" is a details.append(details.removeFirst()) rotation applied to every Kimi user including CLI-only ones, correct only for the two-window shape, and untested. Drop it from this PR.

The security side is fine: Keychain-only storage, SecureField, no logging, sanitizeForUI redacts the bearer.

@iamtoruk iamtoruk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rebased this on main to check status. All three earlier blockers are still open, no commits since Aug 31. Also, main added a localization coverage test since your last push (LocalizationCoverageTests), and this PR fails it: 8 new strings in SettingsView.swift are not wrapped in L(...), so a zh-Hans build shows English. Wrap them and add the keys to both en.lproj and zh-Hans.lproj.

Needed before this moves forward:

  • Disconnect actually removing the Keychain key and sticking (see the CopilotExplicitDisconnect pattern)
  • Keychain read falling back to the CLI token on timeout
  • deps: .live wired through AppStore, not just defined
  • Drop the window rotation change, or add a test and scope it to the two-window case
  • Screenshots of the new Settings section, light and dark
  • Confirm on your machine that the plain CLI-token path still works after this change

The description also needs to match the diff: there is no key-format validation, and it is one test, not thirty assertions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants