Skip to content

Improve Kimi K2 credential errors#1917

Open
joeVenner wants to merge 8 commits into
steipete:mainfrom
joeVenner:fix/kimi-k2-credential-errors
Open

Improve Kimi K2 credential errors#1917
joeVenner wants to merge 8 commits into
steipete:mainfrom
joeVenner:fix/kimi-k2-credential-errors

Conversation

@joeVenner

@joeVenner joeVenner commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Trim surrounding whitespace from Kimi K2 API keys before sending the Authorization header.
  • Report an absent/blank key as Missing Kimi K2 API key. instead of the generic no-strategy error.
  • Classify HTTP 401 as a rejected credential with a concise invalid-or-revoked error.
  • Preserve HTTP 403 and other non-200 response bodies as API errors so insufficient credits or permissions are not mislabeled as a bad key.
  • Add focused shared-strategy/provider regressions, provider documentation, and an Unreleased changelog entry crediting @joeVenner.

Verification

Exact candidate: 60c8663fb607f9544b4093f56620970123c85dc3

  • swift test --filter 'APITokenFetchStrategyTests|KimiK2UsageFetcherTests': 17 tests passed.
  • make check: passed; SwiftFormat and SwiftLint report zero violations.
  • make test: all 50/50 local shards passed.
  • autoreview --mode local: clean; no accepted/actionable findings.
  • Signed packaged app: CodexGitCommit=60c8663f; Developer ID signature, deep/strict verification, and Gatekeeper assessment passed.
  • Bundled CLI live proof:
    • absent key exits 1 with Missing Kimi K2 API key.;
    • a deliberately rejected key exits 1 with Kimi K2 API key is invalid or revoked.;
    • neither path echoes the supplied key.
  • Exact packaged app launches, remains running, and exposes the CodexBar Debug status item.
  • The stored QA credential was injected through the scoped 1Password workflow without printing or writing it; the provider still rejects it as invalid/revoked.
  • Source-blind packaged behavior validation: 5/5 clauses passed (signature/commit, absent key, blank key, rejected key, and no key echo).

Public Model Identifier Gate: PASS (model-free diff).

Dependency freshness: PASS (no dependency changes).

Remaining live-proof blocker

A fresh key was created from the authenticated Kimrel API Keys page and stored through the scoped QA credential workflow. The correctly shaped key is currently rejected with HTTP 401 by both the documented legacy credit endpoint and the current Kimrel credit endpoint; the current chat endpoint reaches the service but returns HTTP 500. Because the required authenticated HTTP 200 credit response could not be obtained, this PR is intentionally not being merged yet.

@clawsweeper

clawsweeper Bot commented Jul 5, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed July 9, 2026, 1:54 PM ET / 17:54 UTC.

Summary
The branch adjusts Kimi K2 API-token fetching to trim keys, report missing and invalid credentials more clearly, preserve non-401 provider errors, and add focused tests plus docs/changelog context.

Reproducibility: yes. from source inspection: current main sends the raw API key after only checking the trimmed value for emptiness and lacks a dedicated 401 invalid-credentials path. The PR adds injected-transport tests for the missing-key, trimmed-key, 401, and 403 paths without requiring live credential probes.

Review metrics: 2 noteworthy metrics.

  • Changed surface: 7 files, +149/-4. The diff is small but spans shared provider strategy, Kimi K2 fetcher behavior, tests, docs, and release-note context.
  • Credential status split: 401 -> invalidCredentials; 403/default -> apiError. This is the auth-provider behavior maintainers need to accept before merge.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

  • [P1] Merging changes Kimi K2 user remediation semantics: HTTP 401 becomes the invalid-or-revoked credential path while 403 and other non-200 responses preserve provider bodies.
  • [P1] The PR body reports that a fresh live key could not produce an authenticated HTTP 200 credit response, so maintainers are accepting the success path from injected tests and the failure paths from packaged CLI proof rather than a successful paid-account live fetch.

Maintainer options:

  1. Merge after auth-semantics acceptance (recommended)
    Keep the current patch if maintainers agree that 401 should be the only invalid-credential status and 403/default should stay body-preserving for this legacy endpoint.
  2. Require stronger provider proof first
    Ask for a successful live credit fetch or a narrower classifier before merging if maintainers do not want to rely on injected success tests and invalid-key CLI proof.
  3. Pause if the endpoint is too unreliable
    Hold or close the PR if maintainers decide the unofficial Kimi K2 credit endpoint is not stable enough for further credential-specific UX work.

Next step before merge

  • Human review is the next action because the remaining blocker is auth-provider classification and normal check gating, not a mechanical code defect.

Maintainer decision needed

  • Question: Should CodexBar adopt the proposed Kimi K2 behavior where only HTTP 401 is shown as invalid credentials and HTTP 403/non-200 preserves the provider response body?
  • Rationale: The patch is narrow and tested, but auth-provider error classification changes the user-facing recovery path for an unofficial legacy endpoint, and a successful live credit fetch could not be obtained.
  • Likely owner: steipete — steipete has the strongest history signal for both the shared API-token strategy and the current Kimi K2 fetcher behavior.
  • Options:
    • Accept 401-only credential mapping (recommended): Merge the current behavior so invalid keys get a concise credential error while quota and permission responses keep the provider body.
    • Require stricter classification first: Ask for additional provider parsing, documentation, or live-provider evidence before changing the shipped Kimi K2 error classification.

Security
Cleared: The diff changes Kimi K2 request/error handling, tests, docs, and changelog context without adding dependencies, scripts, CI, secret-storage paths, or broader permissions.

Review details

Best possible solution:

Merge the focused fetcher/test/docs change after steipete accepts the Kimi K2 status-code semantics and required checks pass.

Do we have a high-confidence way to reproduce the issue?

Yes, from source inspection: current main sends the raw API key after only checking the trimmed value for emptiness and lacks a dedicated 401 invalid-credentials path. The PR adds injected-transport tests for the missing-key, trimmed-key, 401, and 403 paths without requiring live credential probes.

Is this the best way to solve the issue?

Yes, if maintainers accept the status-code mapping; the patch keeps the fix in the shared API-token seam and Kimi K2 fetcher with focused regression coverage. A stricter provider-specific classifier is the safer alternative only if maintainers want more than 401 versus body-preserving non-200 handling.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 3b039d15e125.

Label changes

Label justifications:

  • P2: This is a scoped provider credential-error improvement with limited blast radius.
  • merge-risk: 🚨 auth-provider: The PR changes how Kimi K2 credential and provider-response failures are classified for users.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes after-fix packaged CLI proof for missing and deliberately rejected Kimi K2 keys plus no-key-echo validation; the successful HTTP 200 path remains covered by injected tests because the live provider did not return 200 for a fresh key.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix packaged CLI proof for missing and deliberately rejected Kimi K2 keys plus no-key-echo validation; the successful HTTP 200 path remains covered by injected tests because the live provider did not return 200 for a fresh key.
Evidence reviewed

What I checked:

Likely related people:

  • steipete: steipete introduced the shared API-token strategy, has substantial current Kimi K2 fetcher history, and authored several commits on this branch touching the exact behavior. (role: recent area contributor and likely decision owner; confidence: high; commits: 6335f2aa6569, a5fb02b7d0f2, 762f60b382b3; files: Sources/CodexBarCore/Providers/APITokenFetchStrategy.swift, Sources/CodexBarCore/Providers/KimiK2/KimiK2UsageFetcher.swift, Sources/CodexBarCore/Providers/KimiK2/KimiK2ProviderDescriptor.swift)
  • joeVenner: joeVenner authored recent merged Kimi K2 parser hardening commits and is co-author on the current branch, so this is more than proposer-only involvement. (role: recent area contributor; confidence: high; commits: 4fe943fd2a69, b00797537dbf; files: Sources/CodexBarCore/Providers/KimiK2/KimiK2UsageFetcher.swift, Tests/CodexBarTests/KimiK2UsageFetcherTests.swift)
  • 0-CYBERDYNE-SYSTEMS-0: The original Kimi K2 provider support added the descriptor, usage fetcher, token store, and provider docs that this PR adjusts. (role: introduced provider behavior; confidence: medium; commits: eade57bfde35; files: Sources/CodexBarCore/Providers/KimiK2/KimiK2UsageFetcher.swift, Sources/CodexBarCore/Providers/KimiK2/KimiK2ProviderDescriptor.swift, docs/kimi-k2.md)
  • mturac: A later Kimi K2 clarification commit touched the provider descriptor and docs that define the unofficial provider boundary. (role: adjacent provider docs contributor; confidence: medium; commits: bf4424bb7ff8; files: Sources/CodexBarCore/Providers/KimiK2/KimiK2ProviderDescriptor.swift, docs/kimi-k2.md)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (13 earlier review cycles; latest 8 shown)
  • reviewed 2026-07-06T23:45:19.905Z sha 8cc104d :: needs changes before merge. :: [P2] Keep 403 out of invalid credentials
  • reviewed 2026-07-07T00:27:44.276Z sha f682a0a :: needs maintainer review before merge. :: none
  • reviewed 2026-07-07T00:34:06.387Z sha f682a0a :: needs maintainer review before merge. :: none
  • reviewed 2026-07-07T04:30:58.072Z sha 8428895 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-09T02:20:17.280Z sha 39ca87c :: needs maintainer review before merge. :: none
  • reviewed 2026-07-09T16:15:00.826Z sha 5b58bdc :: needs maintainer review before merge. :: none
  • reviewed 2026-07-09T16:21:58.860Z sha 5b58bdc :: needs maintainer review before merge. :: none
  • reviewed 2026-07-09T17:48:04.993Z sha 60c8663 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal priority bug or improvement with limited blast radius. labels Jul 5, 2026
@steipete steipete force-pushed the fix/kimi-k2-credential-errors branch 2 times, most recently from 5954ec4 to 8cc104d Compare July 6, 2026 23:35

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8cc104d8f0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +138 to +139
case 401, 403:
throw KimiK2UsageError.invalidCredentials

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep 403 credit responses out of credential failures

For a valid key with no credits or insufficient permissions, the Kimrel/Kimi K2 docs list HTTP 403 as an “insufficient credits or permissions” response and also document GET /api/user/credits as the usage endpoint (https://kimrel.com/api-docs). This branch now turns that quota/permission state into “API key is invalid or expired” and discards the response body that previously reached apiError, so affected users will be told to replace credentials instead of topping up or fixing permissions; keep 403 in the API-error path or classify it from the error body.

Useful? React with 👍 / 👎.

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jul 6, 2026
@joeVenner joeVenner force-pushed the fix/kimi-k2-credential-errors branch from 8cc104d to f682a0a Compare July 7, 2026 00:22
@joeVenner

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 7, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jul 7, 2026
@steipete steipete force-pushed the fix/kimi-k2-credential-errors branch from f682a0a to 8428895 Compare July 7, 2026 04:26
steipete and others added 7 commits July 9, 2026 08:48
Co-authored-by: JoeVenner <ylafrimi@gmail.com>
…keys

Map only HTTP 401 to invalidCredentials. Kimrel returns 403 for
insufficient credits or missing permissions, so keep 403 (and other
non-200 responses) on the body-preserving apiError path so users with a
valid key see the real actionable error instead of a misleading
'invalid or expired' message. Split the status-mapping test into 401
invalidCredentials and 403 body-preserving cases.
Co-authored-by: JoeVenner <ylafrimi@gmail.com>
Co-authored-by: JoeVenner <ylafrimi@gmail.com>
Co-authored-by: JoeVenner <ylafrimi@gmail.com>
@steipete steipete force-pushed the fix/kimi-k2-credential-errors branch from 39ca87c to 5b58bdc Compare July 9, 2026 16:06
Co-authored-by: joeVenner <ylafrimi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants