feat: kimi (Moonshot AI) built-in provider (ADR-0026) #20

2026-07-18T07:00:05Z

steve commented

2026-07-18 07:00:05 +00:00

What

Adds a first-class kimi built-in provider and kimi:// DSN scheme for Moonshot AI's Kimi models, served over an OpenAI-compatible Chat Completions API.

kimi/kimi-k2-0711-preview (and any Moonshot model id) now parses, chains, aliases, and participates in health/failover out of the box, keyed by KIMI_API_KEY.

How

No new package. Both the built-in and the kimi:// scheme construct provider/openai pointed at Moonshot's endpoint — the chat path inherits every openai feature/fix automatically (same pattern ADR-0015 used for llama-swap).
Default base URL is the international host https://api.moonshot.ai/v1; the China endpoint (or any host) is reachable via a DSN, e.g. LLM_KCN=kimi://[email protected]/v1.
KIMI_API_KEY is read through the registry's injected envLookup (hermetically testable via WithEnvLookup).
WithAPIKey is passed unconditionally, even when empty — openai.New defaults its key to OPENAI_API_KEY, so without this an unset KIMI_API_KEY would silently authenticate Kimi with the OpenAI key. This severs that fallthrough.
New openai.WithAPIKeyName option customizes only the synthetic-401 missing-key hint (default OPENAI_API_KEY); the kimi built-in/scheme name KIMI_API_KEY so a keyless call points at the right variable.

Decisions (per project owner)

KIMI_API_KEY only (no MOONSHOT_API_KEY alias) · provider name kimi only (no moonshot) · kimi:// scheme only · include the WithAPIKeyName error-hint refinement.

Tests (hermetic, no network/creds)

builtin_kimi_test.go via a capturing RoundTripper:

built-in resolves in Parse, targets https://api.moonshot.ai/v1/chat/completions, sends Bearer <KIMI_API_KEY>;
missing key → synthetic 401 whose hint names KIMI_API_KEY and not OPENAI_API_KEY, with no network hit;
kimi:// DSN round-trips against the China host.

Docs kept in sync (same commit)

README built-in env-var table, DSN scheme list, support matrix (+ footnote ³), .env.example, env.go DSN doc comment, ADR-0026 (+ index — also backfilled the missing 0024/0025 rows), progress.md.

Gates

go build, go vet, go test -race -count=1 ./..., go mod tidy — all clean.

🤖 Generated with Claude Code

## What Adds a first-class **`kimi`** built-in provider and **`kimi://`** DSN scheme for Moonshot AI's Kimi models, served over an OpenAI-compatible Chat Completions API. `kimi/kimi-k2-0711-preview` (and any Moonshot model id) now parses, chains, aliases, and participates in health/failover out of the box, keyed by **`KIMI_API_KEY`**. ## How - **No new package.** Both the built-in and the `kimi://` scheme construct `provider/openai` pointed at Moonshot's endpoint — the chat path inherits every openai feature/fix automatically (same pattern ADR-0015 used for llama-swap). - Default base URL is the international host `https://api.moonshot.ai/v1`; the China endpoint (or any host) is reachable via a DSN, e.g. `LLM_KCN=kimi://[email protected]/v1`. - `KIMI_API_KEY` is read through the registry's injected `envLookup` (hermetically testable via `WithEnvLookup`). - **`WithAPIKey` is passed unconditionally, even when empty** — `openai.New` defaults its key to `OPENAI_API_KEY`, so without this an unset `KIMI_API_KEY` would silently authenticate Kimi with the OpenAI key. This severs that fallthrough. - New **`openai.WithAPIKeyName`** option customizes only the synthetic-401 missing-key hint (default `OPENAI_API_KEY`); the kimi built-in/scheme name `KIMI_API_KEY` so a keyless call points at the right variable. ## Decisions (per project owner) `KIMI_API_KEY` only (no `MOONSHOT_API_KEY` alias) · provider name `kimi` only (no `moonshot`) · `kimi://` scheme only · include the `WithAPIKeyName` error-hint refinement. ## Tests (hermetic, no network/creds) `builtin_kimi_test.go` via a capturing `RoundTripper`: - built-in resolves in Parse, targets `https://api.moonshot.ai/v1/chat/completions`, sends `Bearer <KIMI_API_KEY>`; - missing key → synthetic 401 whose hint names `KIMI_API_KEY` and **not** `OPENAI_API_KEY`, with no network hit; - `kimi://` DSN round-trips against the China host. ## Docs kept in sync (same commit) README built-in env-var table, DSN scheme list, support matrix (+ footnote ³), `.env.example`, `env.go` DSN doc comment, **ADR-0026** (+ index — also backfilled the missing 0024/0025 rows), `progress.md`. ## Gates `go build`, `go vet`, `go test -race -count=1 ./...`, `go mod tidy` — all clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

steve added 1 commit 2026-07-18 07:00:05 +00:00

feat: kimi (Moonshot AI) built-in provider (ADR-0026)

CI / Tidy (pull_request) Successful in 9m29s

Details

CI / Build & Test (pull_request) Successful in 10m23s

Details

Gadfly review (reusable) / review (pull_request) Successful in 18m38s

Details

Adversarial Review (Gadfly) / review (pull_request) Successful in 18m38s

Details

2bfffff47a

Add a first-class `kimi` provider and `kimi://` DSN scheme for Moonshot AI's
OpenAI-compatible Chat Completions API. Both reuse provider/openai (no new
client, mirroring llama-swap's chat path). Default endpoint is the
international host; the China endpoint is reachable via a kimi:// LLM_* DSN.

- Credential is KIMI_API_KEY, read through the registry's injected envLookup
  so it stays hermetically testable. WithAPIKey is passed unconditionally so
  an unset KIMI_API_KEY can never fall through to the openai client's
  OPENAI_API_KEY default.
- New openai.WithAPIKeyName option customizes the missing-key error hint
  (default OPENAI_API_KEY); kimi names KIMI_API_KEY.
- Hermetic tests: built-in base URL + bearer, missing-key hint names
  KIMI_API_KEY with no OPENAI fallthrough and no network hit, kimi:// scheme
  round-trips against the China host.
- Docs in sync: README built-in table + DSN scheme list + support matrix,
  .env.example, env.go DSN doc, ADR-0026 (+ index, backfilling 0024/0025),
  progress.md.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

gitea-actions bot commented

2026-07-18 07:00:07 +00:00

🪰 Gadfly — live review status

5/5 reviewers finished · updated 2026-07-18 07:18:43Z

`claude-code/sonnet` · claude-code — ✅ done

✅ security — No material issues found
✅ correctness — No material issues found
✅ maintainability — Minor issues
✅ performance — No material issues found
✅ error-handling — Minor issues

`glm-5.2:cloud` · ollama-cloud — ✅ done

✅ security — No material issues found
✅ correctness — No material issues found
⚠️ maintainability — could not complete
✅ performance — No material issues found
✅ error-handling — No material issues found

`kimi-k2.6:cloud` · ollama-cloud — ✅ done

✅ security — No material issues found
✅ correctness — No material issues found
✅ maintainability — No material issues found
✅ performance — No material issues found
✅ error-handling — No material issues found

`opencode/glm-5.2:cloud` · opencode — ✅ done

✅ security — No material issues found
✅ correctness — Minor issues
✅ maintainability — No material issues found
✅ performance — No material issues found
✅ error-handling — No material issues found

`opencode/kimi-k2.6:cloud` · opencode — ✅ done

✅ security — No material issues found
✅ correctness — No material issues found
✅ maintainability — Minor issues
✅ performance — No material issues found
✅ error-handling — No material issues found

_{Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.}

## 🪰 Gadfly — live review status 5/5 reviewers finished · updated 2026-07-18 07:18:43Z #### `claude-code/sonnet` · claude-code — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `glm-5.2:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ⚠️ **maintainability** — could not complete - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `kimi-k2.6:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `opencode/glm-5.2:cloud` · opencode — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — Minor issues - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `opencode/kimi-k2.6:cloud` · opencode — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.

gitea-actions bot reviewed 2026-07-18 07:18:43 +00:00

gitea-actions bot left a comment

🪰 Gadfly consensus review — 3 inline findings on changed lines. See the consensus comment for the full ranked summary.

_{Advisory only — does not block merge.}

🪰 **Gadfly consensus review** — 3 inline findings on changed lines. See the consensus comment for the full ranked summary. Advisory only — does not block merge.

builtin.go Outdated

						
				@@ -77,0 +96,4 @@

					)...)

					// kimi:// DSN scheme: an OpenAI-compatible target labeled kimi, base URL

					// from the DSN host (e.g. kimi://[email protected]/v1 for China).

					r.schemes[ProviderKimi] = func(name string, dsn DSN) (llm.Provider, error) {

gitea-actions bot commented

2026-07-18 07:18:43 +00:00

⚪ kimi:// scheme's missing-key error always hints KIMI_API_KEY even when the fix is adding a token to the DSN, not setting that env var

error-handling · flagged by 1 model

builtin.go:99-105 (the kimi:// scheme factory) hard-codes openai.WithAPIKeyName("KIMI_API_KEY") regardless of the DSN's registered name. For a scheme-constructed provider like LLM_KCN=kimi://api.moonshot.cn/v1 (no token before @), dsn.Token is "" → the missing-key error names KIMI_API_KEY, but the actual fix is adding a token to the LLM_KCN DSN — KIMI_API_KEY isn't consulted at all for scheme-constructed providers. This misdirects the operator's remediation. Low severity: c…

_{🪰 Gadfly · advisory}

⚪ **kimi:// scheme's missing-key error always hints KIMI_API_KEY even when the fix is adding a token to the DSN, not setting that env var** _error-handling · flagged by 1 model_ - `builtin.go:99-105` (the `kimi://` scheme factory) hard-codes `openai.WithAPIKeyName("KIMI_API_KEY")` regardless of the DSN's registered name. For a scheme-constructed provider like `LLM_KCN=kimi://api.moonshot.cn/v1` (no token before `@`), `dsn.Token` is `""` → the missing-key error names `KIMI_API_KEY`, but the actual fix is adding a token to the `LLM_KCN` DSN — `KIMI_API_KEY` isn't consulted at all for scheme-constructed providers. This misdirects the operator's remediation. Low severity: c… 🪰 Gadfly · advisory

builtin.go

						
				@@ -77,0 +101,4 @@

							openai.WithName(name),

							openai.WithBaseURL(dsn.BaseURL()),

							openai.WithAPIKey(dsn.Token),

							openai.WithAPIKeyName("KIMI_API_KEY"),

gitea-actions bot commented

2026-07-18 07:18:43 +00:00

🟡 kimi:// DSN scheme hardcodes KIMI_API_KEY in the missing-key hint even for custom-named DSN providers whose credential comes from the DSN token

correctness · flagged by 1 model

builtin.go:104 — The kimi:// DSN scheme factory hardcodes WithAPIKeyName("KIMI_API_KEY") for every provider built from a kimi:// DSN, regardless of the registry name. For a DSN-defined provider (e.g. LLM_KCN=kimi://api.moonshot.cn/v1 with no token), the credential comes from the DSN token, not from the KIMI_API_KEY env var. If the token is empty, the synthetic 401 at provider/openai/model.go:85 reads "no API key configured: set KIMI_API_KEY or use WithAPIKey" — naming a var…

_{🪰 Gadfly · advisory}

🟡 **kimi:// DSN scheme hardcodes KIMI_API_KEY in the missing-key hint even for custom-named DSN providers whose credential comes from the DSN token** _correctness · flagged by 1 model_ - `builtin.go:104` — The `kimi://` DSN scheme factory hardcodes `WithAPIKeyName("KIMI_API_KEY")` for every provider built from a `kimi://` DSN, regardless of the registry name. For a DSN-defined provider (e.g. `LLM_KCN=kimi://api.moonshot.cn/v1` with **no token**), the credential comes from the DSN token, not from the `KIMI_API_KEY` env var. If the token is empty, the synthetic 401 at `provider/openai/model.go:85` reads `"no API key configured: set KIMI_API_KEY or use WithAPIKey"` — naming a var… 🪰 Gadfly · advisory

builtin_kimi_test.go

						
				@@ -0,0 +19,4 @@

				// captureRT records the last request and returns a canned response without

				// touching the network, so these tests stay hermetic while still exercising

				// the real openai client the kimi built-in reuses (base URL + auth header).

				type captureRT struct {

gitea-actions bot commented

2026-07-18 07:18:43 +00:00

🟡 hand-rolled RoundTripper duplicates the codebase's established httptest pattern for DSN round-trip tests

maintainability · flagged by 1 model

builtin_kimi_test.go:22-35 — The new captureRT hand-rolled http.RoundTripper duplicates functionality the codebase already has an established pattern for. provider/openai/openai_test.go, provider/openai/stream_test.go, and this same package's env_test.go (TestEnvForemanChatRoundTrip) all use httptest.NewServer/NewTLSServer for the identical "verify a built-in/DSN scheme sends the right URL + auth header" scenario, matching the CLAUDE.md convention "provider clients test again…

_{🪰 Gadfly · advisory}

🟡 **hand-rolled RoundTripper duplicates the codebase's established httptest pattern for DSN round-trip tests** _maintainability · flagged by 1 model_ - `builtin_kimi_test.go:22-35` — The new `captureRT` hand-rolled `http.RoundTripper` duplicates functionality the codebase already has an established pattern for. `provider/openai/openai_test.go`, `provider/openai/stream_test.go`, and this same package's `env_test.go` (`TestEnvForemanChatRoundTrip`) all use `httptest.NewServer`/`NewTLSServer` for the identical "verify a built-in/DSN scheme sends the right URL + auth header" scenario, matching the CLAUDE.md convention "provider clients test again… 🪰 Gadfly · advisory

gitea-actions bot commented

2026-07-18 07:18:44 +00:00

🪰 Gadfly review — consensus across 5 models

Verdict: Minor issues · 4 findings (0 with multi-model agreement)

4 single-model findings (lower confidence)

	Finding	Where	Model	Lens
🟡	kimi:// DSN scheme hardcodes KIMI_API_KEY in the missing-key hint even for custom-named DSN providers whose credential comes from the DSN token	`builtin.go:104`	opencode/glm-5.2:cloud	correctness
🟡	hand-rolled RoundTripper duplicates the codebase's established httptest pattern for DSN round-trip tests	`builtin_kimi_test.go:22`	claude-code/sonnet	maintainability
🟡	TestBuiltinsResolve omits new kimi built-in (and pre-existing llama-swap)	`parse_test.go:216`	opencode/kimi-k2.6:cloud	maintainability
⚪	kimi:// scheme's missing-key error always hints KIMI_API_KEY even when the fix is adding a token to the DSN, not setting that env var	`builtin.go:99`	claude-code/sonnet	error-handling

Per-model detail

claude-code/sonnet (claude-code) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

No secret leakage. This is a well-contained, low-risk change — nothing new from a security standpoint beyond the existing openai-compat scheme pattern (which it faithfully mirrors). No injection surface, no auth bypass, no logging of secrets, and DSN/base-URL handling is identical to the existing openai and llama-swap schemes.

VERDICT: No material issues found

Credential handling mirrors the existing openai/llama-swap scheme pattern exactly (dsn.Token → WithAPIKey, base URL from dsn.BaseURL()); no new SSRF/auth surface introduced.
Verified WithAPIKey is passed unconditionally (builtin.go:96, builtin.go:106) so an unset KIMI_API_KEY cannot fall through to OPENAI_API_KEY — confirmed by reading provider/openai/openai.go:117 (os.Getenv("OPENAI_API_KEY") default) and the new missing-key test.
The new WithAPIKeyName only changes the variable name shown in the synthetic-401 hint (provider/openai/model.go:85), never the key value itself — no secret leakage.
DSN.BaseURL() (env.go:40) always forces https://, so a malicious/loose DSN host can't downgrade the kimi scheme to plaintext HTTP; consistent with existing openai/anthropic schemes.

🎯 Correctness — No material issues found

I've verified the key correctness claims against the actual code: envLookup capture ordering in registry.go (New() applies WithEnvLookup before registerBuiltins runs, so the kimi built-in correctly picks up the injected/test lookup), the unconditional WithAPIKey override severing the OPENAI_API_KEY fallthrough in provider/openai/openai.go, the apiKeyName field threading into the synthetic-401 message in model.go, DSN→base-URL→request-URL construction for both the built-in and kimi:// scheme, and that errors.AsType is an established pattern already used throughout the test suite (not a new/broken API).

VERDICT: No material issues found

Findings (correctness lens only):

No logic bugs identified. Specifically checked and confirmed correct:
- builtin.go:94 — r.envLookup("KIMI_API_KEY") is evaluated after r.envLookup is assigned in registry.go:187 (which happens before registerBuiltins is called at registry.go:190), so the test's WithEnvLookup override is honored, not the real os.Getenv.
- WithAPIKey(r.envLookup(...)) in builtin.go:94/103 correctly overrides openai.New's default os.Getenv("OPENAI_API_KEY") seed (openai.go:117) even when the looked-up value is "", since WithAPIKey unconditionally assigns p.apiKey = key — verified this actually prevents the OpenAI-key fallthrough the PR claims to fix.
- model.go:85's new m.p.apiKeyName substitution only fires on the pre-existing missing-key branch (m.p.apiKey == ""), and apiKeyName defaults to "OPENAI_API_KEY" in openai.go:118, so all non-Kimi callers are unaffected.
- URL construction traced end-to-end for both the built-in (kimiBaseURL → .../v1/chat/completions) and the kimi:// DSN scheme (DSN.BaseURL() = https:// + parsed host/path → same suffix), matching both test assertions.
- No openai.Provider{...} struct literals exist elsewhere that the new apiKeyName field could break; provider/llamaswap and the pre-existing openai DSN scheme construct via New()/options only.

Nothing else in this diff (README/ADR/progress.md prose, doc footnotes) constitutes executable logic subject to this lens.

🧹 Code cleanliness & maintainability — Minor issues

Verdict

Minor issues

builtin_kimi_test.go:22-35 — The new captureRT hand-rolled http.RoundTripper duplicates functionality the codebase already has an established pattern for. provider/openai/openai_test.go, provider/openai/stream_test.go, and this same package's env_test.go (TestEnvForemanChatRoundTrip) all use httptest.NewServer/NewTLSServer for the identical "verify a built-in/DSN scheme sends the right URL + auth header" scenario, matching the CLAUDE.md convention "provider clients test against httptest." This PR introduces a second, one-off test-double mechanism instead of reusing the existing pattern already present one file away. Not broken, but it's a maintainability inconsistency a future reader has to reconcile — consider using httptest.NewServer for consistency with TestEnvForemanChatRoundTrip, which is really the same test shape (DSN scheme → real chat round trip).

(The builtin.go gofmt-alignment finding did not survive verification: a byte-level check of the file shows the const block is split into separate tabwriter alignment groups by the interposed doc comments — ProviderOpenAI sits alone in its own group, immediately followed by a comment, so its single-space padding is gofmt-correct, not a violation.)

⚡ Performance — No material issues found

registerBuiltins is invoked once per Registry construction (registry.go:190), not per request — so the added kimi provider/scheme registration is a one-time, fixed-cost operation, not a hot-path concern. The error-message string concatenation in model.go only runs on the missing-key error path. There's no loop, no repeated allocation, no N+1 pattern, and no request-path change introduced by this diff — it purely reuses the existing provider/openai client machinery.

VERDICT: No material issues found

No performance-relevant regressions in this diff. The new kimi built-in/scheme construction happens once at registry setup (verified via registry.go:190 → registerBuiltins), and the provider/openai changes (apiKeyName field, WithAPIKeyName option, error-message concatenation in model.go:85) touch only construction and the already-cold missing-key error path — no additional work on the request/response hot path (Generate/Stream/do).

🧯 Error handling & edge cases — Minor issues

All findings in the draft check out against the actual code. Everything cited — builtin.go lines, model.go's fail-fast check, registration ordering in registry.go, and the test file contents — matches exactly what's in the repo.

VERDICT: Minor issues

builtin.go:99-105 (the kimi:// scheme factory) hard-codes openai.WithAPIKeyName("KIMI_API_KEY") regardless of the DSN's registered name. For a scheme-constructed provider like LLM_KCN=kimi://api.moonshot.cn/v1 (no token before @), dsn.Token is "" → the missing-key error names KIMI_API_KEY, but the actual fix is adding a token to the LLM_KCN DSN — KIMI_API_KEY isn't consulted at all for scheme-constructed providers. This misdirects the operator's remediation. Low severity: classification/fail-fast behavior is correct, only the human-readable hint is imprecise, and the same imprecision pre-existed for openai:// (default OPENAI_API_KEY hint) — not a regression, just extended to a new scheme.

glm-5.2:cloud (ollama-cloud) — No material issues found

Verdict: No material issues found · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

Verdict: No material issues found

Through the security lens, this change is actually security-positive: the headline design decision (passing WithAPIKey unconditionally, even when empty) correctly severs a credential fallthrough that would otherwise silently authenticate Kimi requests with OPENAI_API_KEY. I verified the relevant code paths:

Credential fallthrough severed (verified). builtin.go:91-96 passes WithAPIKey(r.envLookup("KIMI_API_KEY")) unconditionally. openai.New defaults apiKey to os.Getenv("OPENAI_API_KEY") (openai.go:117), and Options overwrite that, so an empty KIMI_API_KEY yields apiKey == "", which triggers the synthetic 401 at model.go:76-86 before any network call. No silent cross-provider authentication.
No secret leakage in errors (verified). The missing-key message at model.go:85 interpolates only m.p.apiKeyName — a fixed string set in code ("KIMI_API_KEY" / "OPENAI_API_KEY"), never the key value itself. The test TestKimiBuiltinMissingKey confirms the error names the env var, not the secret.
DSN token handling (verified). The kimi:// scheme (builtin.go:99-106) passes dsn.Token straight to WithAPIKey, matching the existing openai:// scheme (builtin.go:77-83). DSN.BaseURL() (env.go:41) is https:// + host, so the token never leaks into the URL. An empty token → empty key → synthetic 401, no silent unauthenticated call.
SSRF / host control (verified, not a new issue). The kimi:// host is operator-controlled config (LLM_* env vars), the same trust boundary as every other scheme. No new untrusted-input surface is introduced.
WithAPIKeyName default (verified). openai.go:118 defaults apiKeyName to "OPENAI_API_KEY", so existing openai-compat providers keep their behavior; only the kimi built-in/scheme override it.

No authn/authz gaps, injection, secret leakage, or insecure defaults introduced by this change.

🎯 Correctness — No material issues found

VERDICT: No material issues found

Through the correctness lens I verified the core behaviors against the checked-out code:

Fallthrough severance (builtin.go:91-96, openai.go:114-128, model.go:76-87): New defaults apiKey to os.Getenv("OPENAI_API_KEY"), then applies options. WithAPIKey(r.envLookup("KIMI_API_KEY")) unconditionally overwrites it — even with "" — so an unset KIMI_API_KEY yields apiKey=="", and do() returns the synthetic 401 before any network call. The OPENAI default cannot leak into Kimi. Correct.
Error-hint name (openai.go:68-74, model.go:85): apiKeyName defaults to "OPENAI_API_KEY"; kimi overrides to "KIMI_API_KEY", and do() interpolates m.p.apiKeyName into the message. Existing openai-compat providers keep the default. Correct.
Base URL resolution (builtin.go:37, openai.go:126, model.go:92): kimiBaseURL="https://api.moonshot.ai/v1" → TrimRight(...,"/") (no-op) → + "/chat/completions" = https://api.moonshot.ai/v1/chat/completions. Matches the test expectation. Correct.
DSN scheme (env.go:41, env.go:46-64, builtin.go:99-106): kimi://[email protected]/v1 parses to scheme=kimi, token=tok, host=api.moonshot.cn/v1; BaseURL()=https://api.moonshot.cn/v1; final URL https://api.moonshot.cn/v1/chat/completions. Matches test. Correct.
401 classification (llm/errors.go:129-137): synthetic 401 (Status>=400) → ClassPermanent, consistent with the "permanent auth failure" intent. Correct.
Test harness (parse_test.go:14-21): newTestRegistry prepends WithoutEnvProviders() + an envLookup returning "", but still calls New → registerBuiltins, so the kimi built-in is registered and KIMI_API_KEY reads "". The missing-key test therefore hits the synthetic 401 path with no network hit. Correct.

No semantic, conversion, formula, or boundary issues found in this change.

🧹 Code cleanliness & maintainability — ⚠️ could not complete

⚠️ This reviewer failed to complete: agent: tool-error guard tripped: "read_file" called identically more than 4 times

⚡ Performance — No material issues found

VERDICT: No material issues found

Through the performance lens, this change is additive and lightweight:

The kimi built-in provider is constructed once at registry registration time (registerBuiltins), exactly like the existing openai/ollama built-ins — no per-request or per-parse overhead added.
r.envLookup("KIMI_API_KEY") is a single lookup at registration, not on any hot path.
The kimi:// scheme factory constructs a new openai.Provider per DSN load (not per request), consistent with the existing openai:// scheme; New just allocates a small struct and applies options. No unbounded growth, no N+1, no work inside loops.
WithAPIKeyName adds one string field and a string concat in the (error-only) missing-key path — negligible and off the hot path.

I verified registerBuiltins (builtin.go:42-106) and openai.New (provider/openai/openai.go:114-128) to confirm construction is one-time and the request path is unchanged.

🧯 Error handling & edge cases — No material issues found

Looking at the error-handling lens, I verified the key edge cases by reading the actual code.

Verdict: No material issues found

I checked the unhappy paths this PR introduces or touches:

Missing key → synthetic 401 (provider/openai/model.go:76-87): do() checks m.p.apiKey == "" before building any request, returning *llm.APIError with Status: 401, Code: "missing_api_key", and a message using m.p.apiKeyName. For kimi, WithAPIKey(r.envLookup("KIMI_API_KEY")) is passed unconditionally, so an unset key yields "" (verified envLookup defaults to a no-op "" return in newTestRegistry at parse_test.go:18, and to os.Getenv in New at registry.go:173). The empty key short-circuits before any network call — no nil deref, no swallowed error. The TestKimiBuiltinMissingKey test asserts rt.req == nil, confirming no network hit. ✓
apiKeyName never empty (provider/openai/openai.go:113-122): New defaults apiKeyName to "OPENAI_API_KEY", and kimi overrides it via WithAPIKeyName("KIMI_API_KEY"). No path leaves it "", so the error message at model.go:85 can never produce "set or use WithAPIKey". ✓
kimi:// DSN with no token: ParseDSN (env.go:46-64) leaves Token empty when there's no @; the scheme factory passes WithAPIKey(dsn.Token) = "", reusing the same synthetic-401 path. No panic, clear error. ✓
kimi:// DSN with empty host: ParseDSN returns ErrInvalidDSN for host == "" (env.go:60-62), so the scheme factory never sees an empty dsn.BaseURL(). ✓
Scheme factory error path: r.schemes[ProviderKimi] always returns a non-nil *Provider and nil error (consistent with the existing openai scheme at builtin.go:77-83); openai.New never fails. No ignored error. ✓
Eager vs lazy key capture: the built-in reads KIMI_API_KEY at registry construction (builtin.go:94), matching the existing OpenAI built-in's eager os.Getenv default in openai.New. Not a regression in error handling.

The synthetic-401-before-network design is exactly the right edge-case handling, and the WithAPIKeyName refinement ensures the error names the correct variable. Nothing in the error-handling lane is materially wrong.

kimi-k2.6:cloud (ollama-cloud) — No material issues found

Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

No material issues found

The PR correctly addresses the primary security concern: credential isolation. By unconditionally passing WithAPIKey(r.envLookup("KIMI_API_KEY")) — even when empty — the built-in kimi provider overwrites the openai.New default of os.Getenv("OPENAI_API_KEY"), preventing a silent cross-provider fallthrough where an unset KIMI_API_KEY would authenticate against Moonshot's endpoint using the user's OpenAI key (builtin.go:91-96). The same protection applies to the kimi:// DSN scheme (builtin.go:99-106).

Other security-relevant aspects verified:

HTTPS enforced: The built-in hardcodes https://api.moonshot.ai/v1, and the DSN BaseURL() always prefixes https:// (env.go:41), so no downgrade to HTTP is possible.
No secret leakage: The WithAPIKeyName option only affects the synthetic 401 error hint (provider/openai/model.go:85); the actual key is never logged or exposed.
No new injection surface: The kimi:// scheme follows the identical DSN parsing and construction path as existing openai://, anthropic://, etc. schemes — arbitrary host/path values are constrained by the same https:// + /chat/completions pattern.

🎯 Correctness — No material issues found

No material issues found

After verifying the code through the Correctness lens:

builtin.go:91-96 — The built-in kimi provider correctly passes WithAPIKey(r.envLookup("KIMI_API_KEY")) unconditionally, even when empty. This severs the fallback to OPENAI_API_KEY that openai.New would otherwise default to. Verified by reading provider/openai/openai.go:114-128 where apiKey defaults to os.Getenv("OPENAI_API_KEY") and WithAPIKey unconditionally overwrites it.
builtin.go:99-105 — The kimi:// DSN scheme factory correctly wires dsn.BaseURL(), dsn.Token, and WithAPIKeyName("KIMI_API_KEY"). The openaiOpts helper properly injects httpClient when non-nil (verified at builtin.go:70-75), consistent with every other provider in registerBuiltins.
provider/openai/openai.go:68-74 + model.go:85 — The new WithAPIKeyName option initializes apiKeyName to "OPENAI_API_KEY" by default, preserving existing behavior for all other consumers, while the synthetic 401 now dynamically names the correct env var. Verified by reading both files.
builtin_kimi_test.go — Tests verify the base URL (https://api.moonshot.ai/v1), bearer token injection (Bearer kimi-secret), missing-key fast-fail with hint KIMI_API_KEY (and explicitly not OPENAI_API_KEY), no network hit on missing key, and DSN round-trip to the China endpoint. The errors.AsType usage is pre-existing throughout the codebase (llm/errors.go, openai_test.go, anthropic_test.go).

The change is semantically correct: the credential isolation is properly implemented, the default endpoint and DSN parsing behave as documented, and the error-hint customization preserves backward compatibility.

🧹 Code cleanliness & maintainability — No material issues found

No material issues found

The PR adds a new built-in provider by following the exact same pattern already established by llama-swap and the existing openai/anthropic/google registrations. No duplication, no new abstraction layers, no leaked internals. The WithAPIKeyName option added to provider/openai is a small, backward-compatible refinement that serves a real purpose (correct error hint) without clutter. Test structure is consistent with the rest of the package.

⚡ Performance — No material issues found

No material issues found.

The changes are structurally clean from a performance standpoint:

No hot-path modifications: All request-building, JSON marshaling, base64 encoding, streaming, and image-handling code in provider/openai/wire.go and model.go is untouched. The buildRequest, contentValue, and checkRequest paths remain identical.
Construction-time only: The new ProviderKimi registration and apiKeyName field affect registry setup and error-message formatting, not per-request throughput.
Cold-path string concat: The "no API key configured: set " + m.p.apiKeyName + " or use WithAPIKey" concatenation in model.go:85 only executes when the API key is missing, which is a fast-fail that avoids a network round-trip. The allocation is negligible.
No allocation regressions: The openaiOpts variadic-helper slice creation happens once per provider at registry construction — same pattern as all existing built-ins. No new allocs on the request path.
Client reuse is a win: Delegating to the existing provider/openai package avoids duplicating the request/response pipeline, keeping the binary smaller and cache friendlier.

🧯 Error handling & edge cases — No material issues found

No material issues found

I verified the KIMI_API_KEY empty-string handling: builtin.go:94 passes WithAPIKey(r.envLookup("KIMI_API_KEY")) unconditionally, which correctly overrides the openai client's OPENAI_API_KEY default because options are applied after defaults in openai.New (line 123-125 of provider/openai/openai.go). An unset key therefore surfaces as a synthetic 401 at request time, not as a silent fallback.
The WithAPIKeyName("KIMI_API_KEY") option ensures the synthetic 401 error message names the correct variable (provider/openai/model.go:85).
The kimi:// DSN scheme factory (builtin.go:99-106) propagates the DSN token and the same key-name hint, with no new nil/empty edge cases introduced.
Tests cover the missing-key path with a capturing RoundTripper, confirming no network hit occurs when the key is absent.

opencode/glm-5.2:cloud (opencode) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

Verdict: No material issues found

Reviewed through the security lens (authn/authz, credential handling, injection, secret leakage). The change is actually a security improvement, and I found no material issues.

Verified:

builtin.go:91-96 — the kimi built-in passes WithAPIKey(r.envLookup("KIMI_API_KEY")) unconditionally. In provider/openai/openai.go:114-128, New sets apiKey: os.Getenv("OPENAI_API_KEY") first, then applies options, so WithAPIKey("") (unset key) overrides the OpenAI default to empty. This correctly severs the credential fallthrough the PR describes — a keyless kimi call will not silently authenticate as OpenAI. Verified by reading openai.go:114-128.
provider/openai/model.go:75-87 — when apiKey == "", the synthetic 401 is returned before any HTTP request is built or sent, so no network call and no credential header is constructed with an empty/leaked value. The kimi test asserts rt.req == nil to lock this in.
builtin.go:99-106 — the kimi:// DSN scheme passes WithAPIKey(dsn.Token); an empty DSN token sets apiKey="", triggering the same synthetic 401 path rather than falling back to OPENAI_API_KEY. No fallthrough.
env.go:39-63 — BaseURL() always prepends https:// to the parsed host; the scheme is split off at :// so a DSN cannot inject an arbitrary scheme or path-only base URL. The host is operator-configured via LLM_* env (trusted config), not untrusted input.
provider/openai/model.go:85 — the error message interpolates m.p.apiKeyName, which for kimi is the static constant "KIMI_API_KEY" (set via WithAPIKeyName), never user-controlled or key-material. No injection / no secret leakage in the error.
The error message names the env var, never the key value itself — no credential leakage in error reporting.

🎯 Correctness — Minor issues

Verdict: Minor issues

builtin.go:104 — The kimi:// DSN scheme factory hardcodes WithAPIKeyName("KIMI_API_KEY") for every provider built from a kimi:// DSN, regardless of the registry name. For a DSN-defined provider (e.g. LLM_KCN=kimi://api.moonshot.cn/v1 with no token), the credential comes from the DSN token, not from the KIMI_API_KEY env var. If the token is empty, the synthetic 401 at provider/openai/model.go:85 reads "no API key configured: set KIMI_API_KEY or use WithAPIKey" — naming a variable the operator never wired up for this provider, instead of pointing at the DSN token. Verified by reading model.do (the empty-key check at model.go:76 uses m.p.apiKeyName in the message at line 85) and the scheme factory at builtin.go:99-106. TestKimiScheme only exercises a non-empty token (kimi://tok@...), so this path is untested. Suggested fix: either omit WithAPIKeyName for the DSN scheme (fall back to OPENAI_API_KEY, no worse than the existing openai:// scheme) or use the DSN-derived name in the hint — but the cleanest is to not hardcode KIMI_API_KEY for arbitrary DSN names. Severity: small (misleading error message, no functional break).

🧹 Code cleanliness & maintainability — No material issues found

I'll verify the code against the actual repository state, focusing on cleanliness/maintainability concerns.
Let me check the openai scheme for comparison — does it pass WithAPIKeyName? And check the existing openai scheme's pattern for consistency.
Now let me check the const block alignment issue — the gofmt alignment of the const block after the change.
The alignment looks odd — let me verify with the actual column positions. ProviderOpenAI = "openai" (no padding) but ProviderKimi = "kimi" (padded). This is actually gofmt-correct because the doc comment lines between break the alignment groups — ProviderOpenAI is in its own group (followed by a comment), and the rest align in a separate group. Let me verify by checking the column widths.

ProviderOpenAI = "openai" — ProviderOpenAI is 14 chars, then =, then value.
ProviderKimi = "kimi" — ProviderKimi is 12 chars + 8 spaces = 20, then =.

Wait — ProviderLlamaSwapTLS = "llama-swaps" (20 chars name) is the longest in the second group. ProviderKimi (12) + 8 spaces = 20. ✓ Aligned to ProviderLlamaSwapTLS. And ProviderAnthropic (16) + 4 = 20. ✓

So gofmt aligns the second group correctly. ProviderOpenAI stands alone (comment-separated). This is fine and gofmt-clean.

Let me verify the test helper targetsOf and newTestRegistry exist.
All test helpers and the errors.AsType generic helper exist and are used consistently elsewhere. Let me check the kimi:// DSN scheme — note that the openai scheme (line 77-83) does NOT pass WithAPIKeyName, but kimi scheme does. Is this an inconsistency worth noting? Actually no — openai uses the default OPENAI_API_KEY name which is correct for the openai scheme. Kimi correctly overrides. That's the intended behavior, not a problem.

One thing to check: the kimiBaseURL constant is defined at package scope in builtin.go, breaking the local pattern where other base URLs (like defaultBaseURL in openai.go) are in their provider packages. But since kimi has no package, this is the right place. Let me check if there are other top-level constants in builtin.go for comparison — no, kimiBaseURL is the only one. It's fine.

Let me verify the test file's captureRT helper isn't duplicating something already available.
The rest of the suite uses httptest.NewServer heavily, while this new test uses a custom captureRT RoundTripper. Both are valid hermetic patterns. The captureRT is appropriate here because the tests need to assert against the exact URL (https://api.moonshot.ai/v1/chat/completions) which httptest.NewServer would replace with its own host — so capturing the request to verify the real base URL is the right approach. Not a duplication concern.

I've verified enough. Let me assess findings through the maintainability lens:

Observations:

The const block alignment is gofmt-correct (comment-separated groups).
kimiBaseURL at package scope is reasonable since kimi has no own package.
The kimi built-in and scheme blocks are near-identical to the openai pattern, with the addition of WithAPIKey + WithAPIKeyName. Consistent with surrounding code.
The new WithAPIKeyName option is well-documented and follows the existing Option pattern.
Test helpers (newTestRegistry, targetsOf, errors.AsType) are reused, not reinvented.
The captureRT test helper is a single-use helper in the new file — acceptable, and justified by needing to assert the exact URL.
Comments are thorough and follow the // Why: convention where rationale isn't obvious.

No material cleanliness/maintainability issues. The change is clean, consistent with existing patterns, and well-documented.

VERDICT: No material issues found

Verified builtin.go:14-33 const block alignment is gofmt-correct: ProviderOpenAI sits in its own comment-separated group (unpadded), while the rest align to the longest name (ProviderLlamaSwapTLS, 20 chars). Not a misalignment.
Verified builtin.go:91-106: the kimi built-in and kimi:// scheme blocks mirror the existing openai/anthropic patterns (lines 76-83, 141-147), with the addition of WithAPIKey + WithAPIKeyName. Consistent with surrounding code, no copy-paste divergence.
Verified provider/openai/openai.go:68-74: the new WithAPIKeyName option follows the established Option-func pattern and carries a // Why: doc comment per house conventions.
Verified builtin_kimi_test.go reuses existing test helpers newTestRegistry/targetsOf (from parse_test.go) and the generic errors.AsType (used across 19 sites), rather than reinventing them. The file-local captureRT is justified: the tests assert the exact Moonshot URL, which httptest.NewServer (the suite's dominant pattern) cannot preserve.
kimiBaseURL at package scope in builtin.go:37 is the right home given kimi has no own provider package; no leaky abstraction.

⚡ Performance — No material issues found

VERDICT: No material issues found

Reviewed through the performance lens. The change adds a built-in provider registration and a DSN scheme factory that both construct an openai.Provider once at registry setup time (builtin.go:91-106), mirroring the existing openai and llama-swap patterns. No hot-path work, no per-request allocations, no new loops, no unbounded growth, no blocking calls introduced.

r.envLookup("KIMI_API_KEY") runs once during registerBuiltins (registry construction), not per request — verified at builtin.go:94.
The kimi:// scheme factory allocates one openai.Provider per DSN registration (same as the existing openai scheme at builtin.go:77-83) — no per-parse/per-request construction.
WithAPIKeyName just stores a string field on Provider (openai.go:73-75), read only on the missing-key error path (model.go:85) — no runtime overhead on the success path.

🧯 Error handling & edge cases — No material issues found

I've verified the key code paths. Let me check one more edge case around the synthetic-401 message for the DSN scheme.
I've verified the relevant code paths. Let me check one more thing about the DSN token handling.
I have enough context. Let me finalize my review.

VERDICT: No material issues found

I reviewed the diff through the error handling & edge cases lens, verifying the actual checked-out code (not just the diff).

What I checked and confirmed is sound:

Missing-key path (builtin.go:91-96, model.go:76-87): the built-in passes WithAPIKey(r.envLookup("KIMI_API_KEY")) unconditionally, so an unset key becomes the empty string rather than falling through to os.Getenv("OPENAI_API_KEY"). In model.do, an empty apiKey short-circuits to the synthetic *llm.APIError{Status: 401, Code: "missing_api_key"} before any HTTP call — so no network hit and a permanent (per Classify) error. WithAPIKeyName("KIMI_API_KEY") correctly threads into that message. The TestKimiBuiltinMissingKey test asserts rt.req == nil (no network hit), confirming the early-return. Verified by reading model.go:75-87.
kimi:// DSN scheme with empty token (builtin.go:99-106): if a user writes LLM_X=kimi://host/v1 (no token@), ParseDSN leaves Token="" (env.go:52-58), WithAPIKey("") is passed, and the same apiKey == "" guard in model.do produces the synthetic 401. The error hint still names KIMI_API_KEY (via WithAPIKeyName), which is arguably the wrong variable in that case (the operator set LLM_X without a token, not KIMI_API_KEY), but this is a cosmetic hint mismatch, not a swallowed error or panic — within the "no material issue" bar for this lens. The error is still surfaced as a hard 401.
apiKeyName default: New initializes apiKeyName: "OPENAI_API_KEY" (openai.go:118), so every existing openai/compat consumer that doesn't pass WithAPIKeyName gets byte-identical behavior to before. The diff only changes the constructed message string when the option is explicitly set. No nil/zero-value hazard.
Error wrapping / deferred cleanup: the new code adds no I/O or resource acquisition of its own — it only calls openai.New(...), which returns *Provider (never errors), and registers it. Nothing to defer, nothing to roll back. The kimi scheme factory returns (nil, nil)-shaped (provider, nil) only; it cannot fail. No error swallowed.
No panics on bad input: WithAPIKey(""), WithAPIKeyName("") (if ever), WithBaseURL("") are all plain field assignments; the subsequent strings.TrimRight(p.baseURL, "/") in New is safe on empty. No nil deref introduced.
Tests' captureRT (builtin_kimi_test.go): returns a fixed 200 with io.NopCloser — no body close leak on the test's own response, and the real client's Generate path defer httpResp.Body.Close() (model.go:36) handles cleanup. The missing-key test never reaches the transport, so there's no unclosed-body edge case there.

One cosmetic note (not a finding): in the kimi:// scheme factory, the missing-key hint is hardcoded to KIMI_API_KEY even when the key actually comes from the DSN token rather than that env var. For a keyless DSN the message points the operator at a variable that has no bearing on that provider. This is a hint-quality nit, not an error-handling defect — the failure is still surfaced as a real 401 with no swallowed error — so I'm not raising it as a finding.

opencode/kimi-k2.6:cloud (opencode) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

Verdict: No material issues found

Security analysis complete. This PR adds a kimi built-in provider by reusing the provider/openai client with a dedicated KIMI_API_KEY credential and https://api.moonshot.ai/v1 endpoint.

Credential isolation (verified):

builtin.go:94 passes openai.WithAPIKey(r.envLookup("KIMI_API_KEY")) unconditionally, including when empty. This correctly severs the fallback to OPENAI_API_KEY that openai.New would otherwise use.
builtin.go:95 sets openai.WithAPIKeyName("KIMI_API_KEY"), so the synthetic 401 at provider/openai/model.go:85 names the correct env var in its error message.
builtin_kimi_test.go:83-107 confirms both the synthetic 401 and the correct hint, and verifies no network request is sent when the key is missing.

DSN / transport (verified):

env.go:41 forces https:// for all DSN-derived base URLs; the built-in constant kimiBaseURL is also explicitly https://.
builtin_kimi_test.go:113-138 verifies the DSN scheme round-trips with the token placed in the Authorization: Bearer ... header.
Go's net/http header validation prevents injection through the token value.

Secret handling (verified):

Tests use fake placeholders ("kimi-secret", "tok"); no live credentials are present.
Error messages surface only the env-var name (apiKeyName), never the key value itself (provider/openai/model.go:85).

No authn/authz gaps, injection vectors, secret leakage, or insecure defaults were found in this change.

🎯 Correctness — No material issues found

Verdict: No material issues found

The PR adds a kimi built-in provider and kimi:// DSN scheme that reuse provider/openai pointed at Moonshot's endpoint. Through the correctness lens:

Credential isolation is sound. builtin.go:94 passes openai.WithAPIKey(r.envLookup("KIMI_API_KEY")) unconditionally, including when empty. This correctly overrides openai.New's default of reading OPENAI_API_KEY from the environment, severing the fallthrough. Verified by reading provider/openai/openai.go:117 where the default apiKey is set from os.Getenv("OPENAI_API_KEY"), and WithAPIKey unconditionally overwrites it.
Synthetic 401 naming is correct. provider/openai/model.go:85 uses m.p.apiKeyName in the error message, and the Kimi registration sets this to "KIMI_API_KEY" via WithAPIKeyName. The default remains "OPENAI_API_KEY" for all other users. Verified by reading both builtin.go:95 and openai.go:72-73.
Test coverage is adequate. builtin_kimi_test.go verifies: (a) correct base URL and auth header when key is present, (b) synthetic 401 with correct variable name and no network hit when key is missing, and (c) DSN scheme round-trip to a custom host.
No semantic/domain errors. The default endpoint https://api.moonshot.ai/v1 and the DSN-derived https:// base URL via DSN.BaseURL() are consistent with the project's existing patterns (ADR-0015 for llama-swap, ADR-0004 for env-DSN semantics).
No parse/chains/health regressions. The new provider is registered in the same maps as all other built-ins (r.providers and r.schemes), so it participates in Parse, failover, and health tracking automatically without any special-case logic.

No logic bugs, incorrect behavior, or semantic errors were found.

🧹 Code cleanliness & maintainability — Minor issues

Verdict: Minor issues

parse_test.go:216 — TestBuiltinsResolve claims to verify that “All built-in provider names resolve,” but its hard-coded list omits the new kimi built-in (and already omitted llama-swap). Adding a built-in without updating this comprehensive loop means the test’s coverage promise is stale; future regressions for kimi/… parsing won’t be caught here even though they’re caught in the new builtin_kimi_test.go.
Fix: Append "kimi" to the slice (and backfill "llama-swap" while you’re there).

That’s the only material maintainability gap I could verify. The change otherwise follows existing patterns well: it reuses openaiOpts instead of duplicating HTTP-client injection, mirrors the openai inline scheme closure style, keeps docs and ADRs in sync, and the new WithAPIKeyName option is a clean, minimal addition to provider/openai.

⚡ Performance — No material issues found

Verdict: No material issues found

The diff touches only startup-time provider registration and error-path message formatting. No hot loops, no request-path allocations beyond the pre-existing llm.APIError struct, and no I/O or caching changes.

builtin.go:91-106 — Kimi provider and scheme registration follow the identical openaiOpts pattern already used by OpenAI and llama-swap. Extra map insertions are O(1) and happen once at registry creation.
provider/openai/openai.go:118 — apiKeyName adds one string field to Provider. Negligible memory footprint; only read on the missing-key error path.
provider/openai/model.go:85 — String concatenation in the synthetic-401 message is only executed when apiKey == "", an error path that returns immediately. Not a justifiable optimization target.

Nothing in this PR affects throughput, latency, or allocation pressure on the successful-request hot path.

🧯 Error handling & edge cases — No material issues found

No material issues found.

I scrutinized the diff through the error-handling lens:

Missing-key synthetic 401: Verified in provider/openai/model.go:76-87 that do() returns before any HTTP round-trip when apiKey == "", so the TestKimiBuiltinMissingKey assertion that rt.req == nil holds. The new WithAPIKeyName option correctly customizes the error hint to KIMI_API_KEY instead of OPENAI_API_KEY.
Empty-key override: Confirmed in builtin.go:94-95 that WithAPIKey(r.envLookup("KIMI_API_KEY")) is passed unconditionally, even when empty. This severs the fallthrough to OPENAI_API_KEY that openai.New would otherwise default to. Verified by reading provider/openai/openai.go:117-118.
Scheme factory edge cases: The kimi:// DSN factory (builtin.go:99-106) passes WithAPIKey(dsn.Token), which may be empty. This produces the same correct synthetic-401 behavior for DSNs without a token, with the hint still naming KIMI_API_KEY.
Nil / race safety: openai.New never returns nil, and the scheme factory returns a valid provider + nil error, consistent with all other scheme factories. No panics on bad input.
Test coverage: The new builtin_kimi_test.go exercises the missing-key path, the no-network-hit path, and the DSN round-trip, all using the same hermetic captureRT pattern used elsewhere.

Nothing in this lane is materially wrong.

_{Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.}

## 🪰 Gadfly review — consensus across 5 models **Verdict: Minor issues** · 4 findings (0 with multi-model agreement) <details><summary>4 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🟡 | kimi:// DSN scheme hardcodes KIMI_API_KEY in the missing-key hint even for custom-named DSN providers whose credential comes from the DSN token | `builtin.go:104` | opencode/glm-5.2:cloud | correctness | | 🟡 | hand-rolled RoundTripper duplicates the codebase's established httptest pattern for DSN round-trip tests | `builtin_kimi_test.go:22` | claude-code/sonnet | maintainability | | 🟡 | TestBuiltinsResolve omits new kimi built-in (and pre-existing llama-swap) | `parse_test.go:216` | opencode/kimi-k2.6:cloud | maintainability | | ⚪ | kimi:// scheme's missing-key error always hints KIMI_API_KEY even when the fix is adding a token to the DSN, not setting that env var | `builtin.go:99` | claude-code/sonnet | error-handling | </details> <details><summary>Per-model detail</summary> <details><summary>claude-code/sonnet (claude-code) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary>🔒 Security — No material issues found</summary> No secret leakage. This is a well-contained, low-risk change — nothing new from a security standpoint beyond the existing openai-compat scheme pattern (which it faithfully mirrors). No injection surface, no auth bypass, no logging of secrets, and DSN/base-URL handling is identical to the existing openai and llama-swap schemes. ## VERDICT: No material issues found - Credential handling mirrors the existing `openai`/llama-swap scheme pattern exactly (`dsn.Token` → `WithAPIKey`, base URL from `dsn.BaseURL()`); no new SSRF/auth surface introduced. - Verified `WithAPIKey` is passed unconditionally (`builtin.go:96`, `builtin.go:106`) so an unset `KIMI_API_KEY` cannot fall through to `OPENAI_API_KEY` — confirmed by reading `provider/openai/openai.go:117` (`os.Getenv("OPENAI_API_KEY")` default) and the new missing-key test. - The new `WithAPIKeyName` only changes the *variable name* shown in the synthetic-401 hint (`provider/openai/model.go:85`), never the key value itself — no secret leakage. - `DSN.BaseURL()` (`env.go:40`) always forces `https://`, so a malicious/loose DSN host can't downgrade the kimi scheme to plaintext HTTP; consistent with existing openai/anthropic schemes. </details> <details><summary>🎯 Correctness — No material issues found</summary> I've verified the key correctness claims against the actual code: envLookup capture ordering in `registry.go` (New() applies `WithEnvLookup` before `registerBuiltins` runs, so the kimi built-in correctly picks up the injected/test lookup), the unconditional `WithAPIKey` override severing the `OPENAI_API_KEY` fallthrough in `provider/openai/openai.go`, the `apiKeyName` field threading into the synthetic-401 message in `model.go`, DSN→base-URL→request-URL construction for both the built-in and `kimi://` scheme, and that `errors.AsType` is an established pattern already used throughout the test suite (not a new/broken API). **VERDICT: No material issues found** Findings (correctness lens only): - No logic bugs identified. Specifically checked and confirmed correct: - `builtin.go:94` — `r.envLookup("KIMI_API_KEY")` is evaluated after `r.envLookup` is assigned in `registry.go:187` (which happens before `registerBuiltins` is called at `registry.go:190`), so the test's `WithEnvLookup` override is honored, not the real `os.Getenv`. - `WithAPIKey(r.envLookup(...))` in `builtin.go:94`/`103` correctly overrides `openai.New`'s default `os.Getenv("OPENAI_API_KEY")` seed (`openai.go:117`) even when the looked-up value is `""`, since `WithAPIKey` unconditionally assigns `p.apiKey = key` — verified this actually prevents the OpenAI-key fallthrough the PR claims to fix. - `model.go:85`'s new `m.p.apiKeyName` substitution only fires on the pre-existing missing-key branch (`m.p.apiKey == ""`), and `apiKeyName` defaults to `"OPENAI_API_KEY"` in `openai.go:118`, so all non-Kimi callers are unaffected. - URL construction traced end-to-end for both the built-in (`kimiBaseURL` → `.../v1/chat/completions`) and the `kimi://` DSN scheme (`DSN.BaseURL()` = `https://` + parsed host/path → same suffix), matching both test assertions. - No `openai.Provider{...}` struct literals exist elsewhere that the new `apiKeyName` field could break; `provider/llamaswap` and the pre-existing `openai` DSN scheme construct via `New()`/options only. Nothing else in this diff (README/ADR/progress.md prose, doc footnotes) constitutes executable logic subject to this lens. </details> <details><summary>🧹 Code cleanliness & maintainability — Minor issues</summary> ## Verdict **Minor issues** - `builtin_kimi_test.go:22-35` — The new `captureRT` hand-rolled `http.RoundTripper` duplicates functionality the codebase already has an established pattern for. `provider/openai/openai_test.go`, `provider/openai/stream_test.go`, and this same package's `env_test.go` (`TestEnvForemanChatRoundTrip`) all use `httptest.NewServer`/`NewTLSServer` for the identical "verify a built-in/DSN scheme sends the right URL + auth header" scenario, matching the CLAUDE.md convention "provider clients test against httptest." This PR introduces a second, one-off test-double mechanism instead of reusing the existing pattern already present one file away. Not broken, but it's a maintainability inconsistency a future reader has to reconcile — consider using `httptest.NewServer` for consistency with `TestEnvForemanChatRoundTrip`, which is really the same test shape (DSN scheme → real chat round trip). (The `builtin.go` gofmt-alignment finding did not survive verification: a byte-level check of the file shows the const block is split into separate tabwriter alignment groups by the interposed doc comments — `ProviderOpenAI` sits alone in its own group, immediately followed by a comment, so its single-space padding is gofmt-correct, not a violation.) </details> <details><summary>⚡ Performance — No material issues found</summary> registerBuiltins is invoked once per `Registry` construction (registry.go:190), not per request — so the added kimi provider/scheme registration is a one-time, fixed-cost operation, not a hot-path concern. The error-message string concatenation in model.go only runs on the missing-key error path. There's no loop, no repeated allocation, no N+1 pattern, and no request-path change introduced by this diff — it purely reuses the existing `provider/openai` client machinery. VERDICT: No material issues found No performance-relevant regressions in this diff. The new `kimi` built-in/scheme construction happens once at registry setup (verified via `registry.go:190` → `registerBuiltins`), and the `provider/openai` changes (`apiKeyName` field, `WithAPIKeyName` option, error-message concatenation in `model.go:85`) touch only construction and the already-cold missing-key error path — no additional work on the request/response hot path (`Generate`/`Stream`/`do`). </details> <details><summary>🧯 Error handling & edge cases — Minor issues</summary> All findings in the draft check out against the actual code. Everything cited — `builtin.go` lines, `model.go`'s fail-fast check, registration ordering in `registry.go`, and the test file contents — matches exactly what's in the repo. **VERDICT: Minor issues** - `builtin.go:99-105` (the `kimi://` scheme factory) hard-codes `openai.WithAPIKeyName("KIMI_API_KEY")` regardless of the DSN's registered name. For a scheme-constructed provider like `LLM_KCN=kimi://api.moonshot.cn/v1` (no token before `@`), `dsn.Token` is `""` → the missing-key error names `KIMI_API_KEY`, but the actual fix is adding a token to the `LLM_KCN` DSN — `KIMI_API_KEY` isn't consulted at all for scheme-constructed providers. This misdirects the operator's remediation. Low severity: classification/fail-fast behavior is correct, only the human-readable hint is imprecise, and the same imprecision pre-existed for `openai://` (default `OPENAI_API_KEY` hint) — not a regression, just extended to a new scheme. </details> </details> <details><summary>glm-5.2:cloud (ollama-cloud) — No material issues found</summary> **Verdict: No material issues found · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary>🔒 Security — No material issues found</summary> **Verdict: No material issues found** Through the security lens, this change is actually *security-positive*: the headline design decision (passing `WithAPIKey` unconditionally, even when empty) correctly severs a credential fallthrough that would otherwise silently authenticate Kimi requests with `OPENAI_API_KEY`. I verified the relevant code paths: - **Credential fallthrough severed (verified).** `builtin.go:91-96` passes `WithAPIKey(r.envLookup("KIMI_API_KEY"))` unconditionally. `openai.New` defaults `apiKey` to `os.Getenv("OPENAI_API_KEY")` (`openai.go:117`), and `Option`s overwrite that, so an empty `KIMI_API_KEY` yields `apiKey == ""`, which triggers the synthetic 401 at `model.go:76-86` before any network call. No silent cross-provider authentication. - **No secret leakage in errors (verified).** The missing-key message at `model.go:85` interpolates only `m.p.apiKeyName` — a fixed string set in code (`"KIMI_API_KEY"` / `"OPENAI_API_KEY"`), never the key value itself. The test `TestKimiBuiltinMissingKey` confirms the error names the env var, not the secret. - **DSN token handling (verified).** The `kimi://` scheme (`builtin.go:99-106`) passes `dsn.Token` straight to `WithAPIKey`, matching the existing `openai://` scheme (`builtin.go:77-83`). `DSN.BaseURL()` (`env.go:41`) is `https://` + host, so the token never leaks into the URL. An empty token → empty key → synthetic 401, no silent unauthenticated call. - **SSRF / host control (verified, not a new issue).** The `kimi://` host is operator-controlled config (`LLM_*` env vars), the same trust boundary as every other scheme. No new untrusted-input surface is introduced. - **`WithAPIKeyName` default (verified).** `openai.go:118` defaults `apiKeyName` to `"OPENAI_API_KEY"`, so existing openai-compat providers keep their behavior; only the kimi built-in/scheme override it. No authn/authz gaps, injection, secret leakage, or insecure defaults introduced by this change. </details> <details><summary>🎯 Correctness — No material issues found</summary> VERDICT: No material issues found Through the correctness lens I verified the core behaviors against the checked-out code: - **Fallthrough severance (builtin.go:91-96, openai.go:114-128, model.go:76-87):** `New` defaults `apiKey` to `os.Getenv("OPENAI_API_KEY")`, then applies options. `WithAPIKey(r.envLookup("KIMI_API_KEY"))` unconditionally overwrites it — even with `""` — so an unset `KIMI_API_KEY` yields `apiKey==""`, and `do()` returns the synthetic 401 before any network call. The OPENAI default cannot leak into Kimi. Correct. - **Error-hint name (openai.go:68-74, model.go:85):** `apiKeyName` defaults to `"OPENAI_API_KEY"`; kimi overrides to `"KIMI_API_KEY"`, and `do()` interpolates `m.p.apiKeyName` into the message. Existing openai-compat providers keep the default. Correct. - **Base URL resolution (builtin.go:37, openai.go:126, model.go:92):** `kimiBaseURL="https://api.moonshot.ai/v1"` → `TrimRight(...,"/")` (no-op) → `+ "/chat/completions"` = `https://api.moonshot.ai/v1/chat/completions`. Matches the test expectation. Correct. - **DSN scheme (env.go:41, env.go:46-64, builtin.go:99-106):** `kimi://[email protected]/v1` parses to scheme=`kimi`, token=`tok`, host=`api.moonshot.cn/v1`; `BaseURL()`=`https://api.moonshot.cn/v1`; final URL `https://api.moonshot.cn/v1/chat/completions`. Matches test. Correct. - **401 classification (llm/errors.go:129-137):** synthetic 401 (`Status>=400`) → `ClassPermanent`, consistent with the "permanent auth failure" intent. Correct. - **Test harness (parse_test.go:14-21):** `newTestRegistry` prepends `WithoutEnvProviders()` + an envLookup returning `""`, but still calls `New` → `registerBuiltins`, so the kimi built-in is registered and `KIMI_API_KEY` reads `""`. The missing-key test therefore hits the synthetic 401 path with no network hit. Correct. No semantic, conversion, formula, or boundary issues found in this change. </details> <details><summary>🧹 Code cleanliness & maintainability — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: agent: tool-error guard tripped: "read_file" called identically more than 4 times </details> <details><summary>⚡ Performance — No material issues found</summary> VERDICT: No material issues found Through the performance lens, this change is additive and lightweight: - The `kimi` built-in provider is constructed once at registry registration time (`registerBuiltins`), exactly like the existing `openai`/`ollama` built-ins — no per-request or per-parse overhead added. - `r.envLookup("KIMI_API_KEY")` is a single lookup at registration, not on any hot path. - The `kimi://` scheme factory constructs a new `openai.Provider` per DSN load (not per request), consistent with the existing `openai://` scheme; `New` just allocates a small struct and applies options. No unbounded growth, no N+1, no work inside loops. - `WithAPIKeyName` adds one string field and a string concat in the (error-only) missing-key path — negligible and off the hot path. I verified `registerBuiltins` (builtin.go:42-106) and `openai.New` (provider/openai/openai.go:114-128) to confirm construction is one-time and the request path is unchanged. </details> <details><summary>🧯 Error handling & edge cases — No material issues found</summary> Looking at the error-handling lens, I verified the key edge cases by reading the actual code. **Verdict: No material issues found** I checked the unhappy paths this PR introduces or touches: - **Missing key → synthetic 401** (`provider/openai/model.go:76-87`): `do()` checks `m.p.apiKey == ""` *before* building any request, returning `*llm.APIError` with `Status: 401`, `Code: "missing_api_key"`, and a message using `m.p.apiKeyName`. For kimi, `WithAPIKey(r.envLookup("KIMI_API_KEY"))` is passed unconditionally, so an unset key yields `""` (verified `envLookup` defaults to a no-op `""` return in `newTestRegistry` at `parse_test.go:18`, and to `os.Getenv` in `New` at `registry.go:173`). The empty key short-circuits before any network call — no nil deref, no swallowed error. The `TestKimiBuiltinMissingKey` test asserts `rt.req == nil`, confirming no network hit. ✓ - **`apiKeyName` never empty** (`provider/openai/openai.go:113-122`): `New` defaults `apiKeyName` to `"OPENAI_API_KEY"`, and kimi overrides it via `WithAPIKeyName("KIMI_API_KEY")`. No path leaves it `""`, so the error message at `model.go:85` can never produce `"set or use WithAPIKey"`. ✓ - **`kimi://` DSN with no token**: `ParseDSN` (`env.go:46-64`) leaves `Token` empty when there's no `@`; the scheme factory passes `WithAPIKey(dsn.Token)` = `""`, reusing the same synthetic-401 path. No panic, clear error. ✓ - **`kimi://` DSN with empty host**: `ParseDSN` returns `ErrInvalidDSN` for `host == ""` (`env.go:60-62`), so the scheme factory never sees an empty `dsn.BaseURL()`. ✓ - **Scheme factory error path**: `r.schemes[ProviderKimi]` always returns a non-nil `*Provider` and `nil` error (consistent with the existing `openai` scheme at `builtin.go:77-83`); `openai.New` never fails. No ignored error. ✓ - **Eager vs lazy key capture**: the built-in reads `KIMI_API_KEY` at registry construction (`builtin.go:94`), matching the existing OpenAI built-in's eager `os.Getenv` default in `openai.New`. Not a regression in error handling. The synthetic-401-before-network design is exactly the right edge-case handling, and the `WithAPIKeyName` refinement ensures the error names the correct variable. Nothing in the error-handling lane is materially wrong. </details> </details> <details><summary>kimi-k2.6:cloud (ollama-cloud) — No material issues found</summary> **Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary>🔒 Security — No material issues found</summary> No material issues found The PR correctly addresses the primary security concern: **credential isolation**. By unconditionally passing `WithAPIKey(r.envLookup("KIMI_API_KEY"))` — even when empty — the built-in `kimi` provider overwrites the `openai.New` default of `os.Getenv("OPENAI_API_KEY")`, preventing a silent cross-provider fallthrough where an unset `KIMI_API_KEY` would authenticate against Moonshot's endpoint using the user's OpenAI key (`builtin.go:91-96`). The same protection applies to the `kimi://` DSN scheme (`builtin.go:99-106`). Other security-relevant aspects verified: - **HTTPS enforced**: The built-in hardcodes `https://api.moonshot.ai/v1`, and the DSN `BaseURL()` always prefixes `https://` (`env.go:41`), so no downgrade to HTTP is possible. - **No secret leakage**: The `WithAPIKeyName` option only affects the synthetic 401 error hint (`provider/openai/model.go:85`); the actual key is never logged or exposed. - **No new injection surface**: The `kimi://` scheme follows the identical DSN parsing and construction path as existing `openai://`, `anthropic://`, etc. schemes — arbitrary host/path values are constrained by the same `https://` + `/chat/completions` pattern. </details> <details><summary>🎯 Correctness — No material issues found</summary> **No material issues found** After verifying the code through the Correctness lens: - **`builtin.go:91-96`** — The built-in kimi provider correctly passes `WithAPIKey(r.envLookup("KIMI_API_KEY"))` unconditionally, even when empty. This severs the fallback to `OPENAI_API_KEY` that `openai.New` would otherwise default to. Verified by reading `provider/openai/openai.go:114-128` where `apiKey` defaults to `os.Getenv("OPENAI_API_KEY")` and `WithAPIKey` unconditionally overwrites it. - **`builtin.go:99-105`** — The `kimi://` DSN scheme factory correctly wires `dsn.BaseURL()`, `dsn.Token`, and `WithAPIKeyName("KIMI_API_KEY")`. The `openaiOpts` helper properly injects `httpClient` when non-nil (verified at `builtin.go:70-75`), consistent with every other provider in `registerBuiltins`. - **`provider/openai/openai.go:68-74` + `model.go:85`** — The new `WithAPIKeyName` option initializes `apiKeyName` to `"OPENAI_API_KEY"` by default, preserving existing behavior for all other consumers, while the synthetic 401 now dynamically names the correct env var. Verified by reading both files. - **`builtin_kimi_test.go`** — Tests verify the base URL (`https://api.moonshot.ai/v1`), bearer token injection (`Bearer kimi-secret`), missing-key fast-fail with hint `KIMI_API_KEY` (and explicitly not `OPENAI_API_KEY`), no network hit on missing key, and DSN round-trip to the China endpoint. The `errors.AsType` usage is pre-existing throughout the codebase (`llm/errors.go`, `openai_test.go`, `anthropic_test.go`). The change is semantically correct: the credential isolation is properly implemented, the default endpoint and DSN parsing behave as documented, and the error-hint customization preserves backward compatibility. </details> <details><summary>🧹 Code cleanliness & maintainability — No material issues found</summary> **No material issues found** The PR adds a new built-in provider by following the exact same pattern already established by llama-swap and the existing openai/anthropic/google registrations. No duplication, no new abstraction layers, no leaked internals. The `WithAPIKeyName` option added to `provider/openai` is a small, backward-compatible refinement that serves a real purpose (correct error hint) without clutter. Test structure is consistent with the rest of the package. </details> <details><summary>⚡ Performance — No material issues found</summary> No material issues found. The changes are structurally clean from a performance standpoint: - **No hot-path modifications:** All request-building, JSON marshaling, base64 encoding, streaming, and image-handling code in `provider/openai/wire.go` and `model.go` is untouched. The `buildRequest`, `contentValue`, and `checkRequest` paths remain identical. - **Construction-time only:** The new `ProviderKimi` registration and `apiKeyName` field affect registry setup and error-message formatting, not per-request throughput. - **Cold-path string concat:** The `"no API key configured: set " + m.p.apiKeyName + " or use WithAPIKey"` concatenation in `model.go:85` only executes when the API key is missing, which is a fast-fail that *avoids* a network round-trip. The allocation is negligible. - **No allocation regressions:** The `openaiOpts` variadic-helper slice creation happens once per provider at registry construction — same pattern as all existing built-ins. No new allocs on the request path. - **Client reuse is a win:** Delegating to the existing `provider/openai` package avoids duplicating the request/response pipeline, keeping the binary smaller and cache friendlier. </details> <details><summary>🧯 Error handling & edge cases — No material issues found</summary> No material issues found - I verified the `KIMI_API_KEY` empty-string handling: `builtin.go:94` passes `WithAPIKey(r.envLookup("KIMI_API_KEY"))` unconditionally, which correctly overrides the openai client's `OPENAI_API_KEY` default because options are applied after defaults in `openai.New` (line 123-125 of `provider/openai/openai.go`). An unset key therefore surfaces as a synthetic 401 at request time, not as a silent fallback. - The `WithAPIKeyName("KIMI_API_KEY")` option ensures the synthetic 401 error message names the correct variable (`provider/openai/model.go:85`). - The `kimi://` DSN scheme factory (`builtin.go:99-106`) propagates the DSN token and the same key-name hint, with no new nil/empty edge cases introduced. - Tests cover the missing-key path with a capturing RoundTripper, confirming no network hit occurs when the key is absent. </details> </details> <details><summary>opencode/glm-5.2:cloud (opencode) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary>🔒 Security — No material issues found</summary> Verdict: No material issues found Reviewed through the security lens (authn/authz, credential handling, injection, secret leakage). The change is actually a security improvement, and I found no material issues. Verified: - `builtin.go:91-96` — the kimi built-in passes `WithAPIKey(r.envLookup("KIMI_API_KEY"))` unconditionally. In `provider/openai/openai.go:114-128`, `New` sets `apiKey: os.Getenv("OPENAI_API_KEY")` first, then applies options, so `WithAPIKey("")` (unset key) overrides the OpenAI default to empty. This correctly severs the credential fallthrough the PR describes — a keyless kimi call will not silently authenticate as OpenAI. Verified by reading `openai.go:114-128`. - `provider/openai/model.go:75-87` — when `apiKey == ""`, the synthetic 401 is returned *before* any HTTP request is built or sent, so no network call and no credential header is constructed with an empty/leaked value. The kimi test asserts `rt.req == nil` to lock this in. - `builtin.go:99-106` — the `kimi://` DSN scheme passes `WithAPIKey(dsn.Token)`; an empty DSN token sets `apiKey=""`, triggering the same synthetic 401 path rather than falling back to `OPENAI_API_KEY`. No fallthrough. - `env.go:39-63` — `BaseURL()` always prepends `https://` to the parsed host; the scheme is split off at `://` so a DSN cannot inject an arbitrary scheme or path-only base URL. The host is operator-configured via `LLM_*` env (trusted config), not untrusted input. - `provider/openai/model.go:85` — the error message interpolates `m.p.apiKeyName`, which for kimi is the static constant `"KIMI_API_KEY"` (set via `WithAPIKeyName`), never user-controlled or key-material. No injection / no secret leakage in the error. - The error message names the env var, never the key value itself — no credential leakage in error reporting. </details> <details><summary>🎯 Correctness — Minor issues</summary> Verdict: Minor issues - `builtin.go:104` — The `kimi://` DSN scheme factory hardcodes `WithAPIKeyName("KIMI_API_KEY")` for every provider built from a `kimi://` DSN, regardless of the registry name. For a DSN-defined provider (e.g. `LLM_KCN=kimi://api.moonshot.cn/v1` with **no token**), the credential comes from the DSN token, not from the `KIMI_API_KEY` env var. If the token is empty, the synthetic 401 at `provider/openai/model.go:85` reads `"no API key configured: set KIMI_API_KEY or use WithAPIKey"` — naming a variable the operator never wired up for this provider, instead of pointing at the DSN token. Verified by reading `model.do` (the empty-key check at `model.go:76` uses `m.p.apiKeyName` in the message at line 85) and the scheme factory at `builtin.go:99-106`. `TestKimiScheme` only exercises a non-empty token (`kimi://tok@...`), so this path is untested. Suggested fix: either omit `WithAPIKeyName` for the DSN scheme (fall back to `OPENAI_API_KEY`, no worse than the existing `openai://` scheme) or use the DSN-derived `name` in the hint — but the cleanest is to not hardcode `KIMI_API_KEY` for arbitrary DSN names. Severity: small (misleading error message, no functional break). </details> <details><summary>🧹 Code cleanliness & maintainability — No material issues found</summary> I'll verify the code against the actual repository state, focusing on cleanliness/maintainability concerns. Let me check the openai scheme for comparison — does it pass `WithAPIKeyName`? And check the existing openai scheme's pattern for consistency. Now let me check the const block alignment issue — the gofmt alignment of the const block after the change. The alignment looks odd — let me verify with the actual column positions. `ProviderOpenAI = "openai"` (no padding) but `ProviderKimi = "kimi"` (padded). This is actually gofmt-correct because the doc comment lines between break the alignment groups — `ProviderOpenAI` is in its own group (followed by a comment), and the rest align in a separate group. Let me verify by checking the column widths. `ProviderOpenAI = "openai"` — `ProviderOpenAI` is 14 chars, then ` = `, then value. `ProviderKimi = "kimi"` — `ProviderKimi` is 12 chars + 8 spaces = 20, then ` = `. Wait — `ProviderLlamaSwapTLS = "llama-swaps"` (20 chars name) is the longest in the second group. `ProviderKimi` (12) + 8 spaces = 20. ✓ Aligned to `ProviderLlamaSwapTLS`. And `ProviderAnthropic` (16) + 4 = 20. ✓ So gofmt aligns the second group correctly. `ProviderOpenAI` stands alone (comment-separated). This is fine and gofmt-clean. Let me verify the test helper `targetsOf` and `newTestRegistry` exist. All test helpers and the `errors.AsType` generic helper exist and are used consistently elsewhere. Let me check the kimi:// DSN scheme — note that the `openai` scheme (line 77-83) does NOT pass `WithAPIKeyName`, but kimi scheme does. Is this an inconsistency worth noting? Actually no — openai uses the default `OPENAI_API_KEY` name which is correct for the openai scheme. Kimi correctly overrides. That's the intended behavior, not a problem. One thing to check: the `kimiBaseURL` constant is defined at package scope in builtin.go, breaking the local pattern where other base URLs (like `defaultBaseURL` in openai.go) are in their provider packages. But since kimi has no package, this is the right place. Let me check if there are other top-level constants in builtin.go for comparison — no, `kimiBaseURL` is the only one. It's fine. Let me verify the test file's `captureRT` helper isn't duplicating something already available. The rest of the suite uses `httptest.NewServer` heavily, while this new test uses a custom `captureRT` RoundTripper. Both are valid hermetic patterns. The `captureRT` is appropriate here because the tests need to assert against the *exact* URL (`https://api.moonshot.ai/v1/chat/completions`) which `httptest.NewServer` would replace with its own host — so capturing the request to verify the real base URL is the right approach. Not a duplication concern. I've verified enough. Let me assess findings through the maintainability lens: **Observations:** 1. The const block alignment is gofmt-correct (comment-separated groups). 2. `kimiBaseURL` at package scope is reasonable since kimi has no own package. 3. The kimi built-in and scheme blocks are near-identical to the openai pattern, with the addition of `WithAPIKey` + `WithAPIKeyName`. Consistent with surrounding code. 4. The new `WithAPIKeyName` option is well-documented and follows the existing Option pattern. 5. Test helpers (`newTestRegistry`, `targetsOf`, `errors.AsType`) are reused, not reinvented. 6. The `captureRT` test helper is a single-use helper in the new file — acceptable, and justified by needing to assert the exact URL. 7. Comments are thorough and follow the `// Why:` convention where rationale isn't obvious. No material cleanliness/maintainability issues. The change is clean, consistent with existing patterns, and well-documented. **VERDICT: No material issues found** - Verified `builtin.go:14-33` const block alignment is gofmt-correct: `ProviderOpenAI` sits in its own comment-separated group (unpadded), while the rest align to the longest name (`ProviderLlamaSwapTLS`, 20 chars). Not a misalignment. - Verified `builtin.go:91-106`: the kimi built-in and `kimi://` scheme blocks mirror the existing openai/anthropic patterns (lines 76-83, 141-147), with the addition of `WithAPIKey` + `WithAPIKeyName`. Consistent with surrounding code, no copy-paste divergence. - Verified `provider/openai/openai.go:68-74`: the new `WithAPIKeyName` option follows the established Option-func pattern and carries a `// Why:` doc comment per house conventions. - Verified `builtin_kimi_test.go` reuses existing test helpers `newTestRegistry`/`targetsOf` (from `parse_test.go`) and the generic `errors.AsType` (used across 19 sites), rather than reinventing them. The file-local `captureRT` is justified: the tests assert the *exact* Moonshot URL, which `httptest.NewServer` (the suite's dominant pattern) cannot preserve. - `kimiBaseURL` at package scope in `builtin.go:37` is the right home given kimi has no own provider package; no leaky abstraction. </details> <details><summary>⚡ Performance — No material issues found</summary> VERDICT: No material issues found Reviewed through the performance lens. The change adds a built-in provider registration and a DSN scheme factory that both construct an `openai.Provider` once at registry setup time (`builtin.go:91-106`), mirroring the existing `openai` and `llama-swap` patterns. No hot-path work, no per-request allocations, no new loops, no unbounded growth, no blocking calls introduced. - `r.envLookup("KIMI_API_KEY")` runs once during `registerBuiltins` (registry construction), not per request — verified at `builtin.go:94`. - The `kimi://` scheme factory allocates one `openai.Provider` per DSN registration (same as the existing `openai` scheme at `builtin.go:77-83`) — no per-parse/per-request construction. - `WithAPIKeyName` just stores a string field on `Provider` (`openai.go:73-75`), read only on the missing-key error path (`model.go:85`) — no runtime overhead on the success path. </details> <details><summary>🧯 Error handling & edge cases — No material issues found</summary> I've verified the key code paths. Let me check one more edge case around the synthetic-401 message for the DSN scheme. I've verified the relevant code paths. Let me check one more thing about the DSN token handling. I have enough context. Let me finalize my review. VERDICT: No material issues found I reviewed the diff through the **error handling & edge cases** lens, verifying the actual checked-out code (not just the diff). What I checked and confirmed is sound: - **Missing-key path** (`builtin.go:91-96`, `model.go:76-87`): the built-in passes `WithAPIKey(r.envLookup("KIMI_API_KEY"))` unconditionally, so an unset key becomes the empty string rather than falling through to `os.Getenv("OPENAI_API_KEY")`. In `model.do`, an empty `apiKey` short-circuits to the synthetic `*llm.APIError{Status: 401, Code: "missing_api_key"}` **before** any HTTP call — so no network hit and a permanent (per `Classify`) error. `WithAPIKeyName("KIMI_API_KEY")` correctly threads into that message. The `TestKimiBuiltinMissingKey` test asserts `rt.req == nil` (no network hit), confirming the early-return. Verified by reading `model.go:75-87`. - **`kimi://` DSN scheme with empty token** (`builtin.go:99-106`): if a user writes `LLM_X=kimi://host/v1` (no `token@`), `ParseDSN` leaves `Token=""` (`env.go:52-58`), `WithAPIKey("")` is passed, and the same `apiKey == ""` guard in `model.do` produces the synthetic 401. The error hint still names `KIMI_API_KEY` (via `WithAPIKeyName`), which is arguably the wrong variable in that case (the operator set `LLM_X` without a token, not `KIMI_API_KEY`), but this is a cosmetic hint mismatch, not a swallowed error or panic — within the "no material issue" bar for this lens. The error is still surfaced as a hard 401. - **`apiKeyName` default**: `New` initializes `apiKeyName: "OPENAI_API_KEY"` (`openai.go:118`), so every existing openai/compat consumer that doesn't pass `WithAPIKeyName` gets byte-identical behavior to before. The diff only changes the constructed message string when the option is explicitly set. No nil/zero-value hazard. - **Error wrapping / deferred cleanup**: the new code adds no I/O or resource acquisition of its own — it only calls `openai.New(...)`, which returns `*Provider` (never errors), and registers it. Nothing to defer, nothing to roll back. The kimi scheme factory returns `(nil, nil)`-shaped `(provider, nil)` only; it cannot fail. No error swallowed. - **No panics on bad input**: `WithAPIKey("")`, `WithAPIKeyName("")` (if ever), `WithBaseURL("")` are all plain field assignments; the subsequent `strings.TrimRight(p.baseURL, "/")` in `New` is safe on empty. No nil deref introduced. - **Tests' `captureRT`** (`builtin_kimi_test.go`): returns a fixed `200` with `io.NopCloser` — no body close leak on the test's own response, and the real client's `Generate` path `defer httpResp.Body.Close()` (`model.go:36`) handles cleanup. The missing-key test never reaches the transport, so there's no unclosed-body edge case there. One cosmetic note (not a finding): in the `kimi://` scheme factory, the missing-key hint is hardcoded to `KIMI_API_KEY` even when the key actually comes from the DSN token rather than that env var. For a keyless DSN the message points the operator at a variable that has no bearing on that provider. This is a hint-quality nit, not an error-handling defect — the failure is still surfaced as a real 401 with no swallowed error — so I'm not raising it as a finding. </details> </details> <details><summary>opencode/kimi-k2.6:cloud (opencode) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary>🔒 Security — No material issues found</summary> **Verdict:** No material issues found Security analysis complete. This PR adds a kimi built-in provider by reusing the `provider/openai` client with a dedicated `KIMI_API_KEY` credential and `https://api.moonshot.ai/v1` endpoint. **Credential isolation (verified):** - `builtin.go:94` passes `openai.WithAPIKey(r.envLookup("KIMI_API_KEY"))` unconditionally, including when empty. This correctly severs the fallback to `OPENAI_API_KEY` that `openai.New` would otherwise use. - `builtin.go:95` sets `openai.WithAPIKeyName("KIMI_API_KEY")`, so the synthetic 401 at `provider/openai/model.go:85` names the correct env var in its error message. - `builtin_kimi_test.go:83-107` confirms both the synthetic 401 and the correct hint, and verifies no network request is sent when the key is missing. **DSN / transport (verified):** - `env.go:41` forces `https://` for all DSN-derived base URLs; the built-in constant `kimiBaseURL` is also explicitly `https://`. - `builtin_kimi_test.go:113-138` verifies the DSN scheme round-trips with the token placed in the `Authorization: Bearer ...` header. - Go's `net/http` header validation prevents injection through the token value. **Secret handling (verified):** - Tests use fake placeholders (`"kimi-secret"`, `"tok"`); no live credentials are present. - Error messages surface only the env-var *name* (`apiKeyName`), never the key value itself (`provider/openai/model.go:85`). No authn/authz gaps, injection vectors, secret leakage, or insecure defaults were found in this change. </details> <details><summary>🎯 Correctness — No material issues found</summary> **Verdict: No material issues found** The PR adds a `kimi` built-in provider and `kimi://` DSN scheme that reuse `provider/openai` pointed at Moonshot's endpoint. Through the correctness lens: - **Credential isolation is sound.** `builtin.go:94` passes `openai.WithAPIKey(r.envLookup("KIMI_API_KEY"))` unconditionally, including when empty. This correctly overrides `openai.New`'s default of reading `OPENAI_API_KEY` from the environment, severing the fallthrough. Verified by reading `provider/openai/openai.go:117` where the default `apiKey` is set from `os.Getenv("OPENAI_API_KEY")`, and `WithAPIKey` unconditionally overwrites it. - **Synthetic 401 naming is correct.** `provider/openai/model.go:85` uses `m.p.apiKeyName` in the error message, and the Kimi registration sets this to `"KIMI_API_KEY"` via `WithAPIKeyName`. The default remains `"OPENAI_API_KEY"` for all other users. Verified by reading both `builtin.go:95` and `openai.go:72-73`. - **Test coverage is adequate.** `builtin_kimi_test.go` verifies: (a) correct base URL and auth header when key is present, (b) synthetic 401 with correct variable name and no network hit when key is missing, and (c) DSN scheme round-trip to a custom host. - **No semantic/domain errors.** The default endpoint `https://api.moonshot.ai/v1` and the DSN-derived `https://` base URL via `DSN.BaseURL()` are consistent with the project's existing patterns (ADR-0015 for llama-swap, ADR-0004 for env-DSN semantics). - **No parse/chains/health regressions.** The new provider is registered in the same maps as all other built-ins (`r.providers` and `r.schemes`), so it participates in Parse, failover, and health tracking automatically without any special-case logic. No logic bugs, incorrect behavior, or semantic errors were found. </details> <details><summary>🧹 Code cleanliness & maintainability — Minor issues</summary> **Verdict:** Minor issues - **`parse_test.go:216`** — `TestBuiltinsResolve` claims to verify that “All built-in provider names resolve,” but its hard-coded list omits the new `kimi` built-in (and already omitted `llama-swap`). Adding a built-in without updating this comprehensive loop means the test’s coverage promise is stale; future regressions for `kimi/…` parsing won’t be caught here even though they’re caught in the new `builtin_kimi_test.go`. **Fix:** Append `"kimi"` to the slice (and backfill `"llama-swap"` while you’re there). That’s the only material maintainability gap I could verify. The change otherwise follows existing patterns well: it reuses `openaiOpts` instead of duplicating HTTP-client injection, mirrors the `openai` inline scheme closure style, keeps docs and ADRs in sync, and the new `WithAPIKeyName` option is a clean, minimal addition to `provider/openai`. </details> <details><summary>⚡ Performance — No material issues found</summary> **Verdict: No material issues found** The diff touches only startup-time provider registration and error-path message formatting. No hot loops, no request-path allocations beyond the pre-existing `llm.APIError` struct, and no I/O or caching changes. - `builtin.go:91-106` — Kimi provider and scheme registration follow the identical `openaiOpts` pattern already used by OpenAI and llama-swap. Extra map insertions are O(1) and happen once at registry creation. - `provider/openai/openai.go:118` — `apiKeyName` adds one string field to `Provider`. Negligible memory footprint; only read on the missing-key error path. - `provider/openai/model.go:85` — String concatenation in the synthetic-401 message is only executed when `apiKey == ""`, an error path that returns immediately. Not a justifiable optimization target. Nothing in this PR affects throughput, latency, or allocation pressure on the successful-request hot path. </details> <details><summary>🧯 Error handling & edge cases — No material issues found</summary> No material issues found. I scrutinized the diff through the error-handling lens: - **Missing-key synthetic 401:** Verified in `provider/openai/model.go:76-87` that `do()` returns before any HTTP round-trip when `apiKey == ""`, so the `TestKimiBuiltinMissingKey` assertion that `rt.req == nil` holds. The new `WithAPIKeyName` option correctly customizes the error hint to `KIMI_API_KEY` instead of `OPENAI_API_KEY`. - **Empty-key override:** Confirmed in `builtin.go:94-95` that `WithAPIKey(r.envLookup("KIMI_API_KEY"))` is passed unconditionally, even when empty. This severs the fallthrough to `OPENAI_API_KEY` that `openai.New` would otherwise default to. Verified by reading `provider/openai/openai.go:117-118`. - **Scheme factory edge cases:** The `kimi://` DSN factory (`builtin.go:99-106`) passes `WithAPIKey(dsn.Token)`, which may be empty. This produces the same correct synthetic-401 behavior for DSNs without a token, with the hint still naming `KIMI_API_KEY`. - **Nil / race safety:** `openai.New` never returns nil, and the scheme factory returns a valid provider + nil error, consistent with all other scheme factories. No panics on bad input. - **Test coverage:** The new `builtin_kimi_test.go` exercises the missing-key path, the no-network-hit path, and the DSN round-trip, all using the same hermetic `captureRT` pattern used elsewhere. Nothing in this lane is materially wrong. </details> </details> </details> Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.

steve added 1 commit 2026-07-18 07:24:49 +00:00

fix: address Gadfly findings on kimi provider

CI / Build & Test (pull_request) Successful in 9m43s

Details

CI / Tidy (pull_request) Successful in 9m29s

Details

fcbb01b729

- kimi:// DSN scheme: missing-credential hint now names the LLM_<NAME> env
  var that defines the provider (its token comes from the DSN, not
  KIMI_API_KEY), matching providerFor's lazy-resolution key form. Fixes the
  correctness/error-handling findings that the old hint misdirected users to
  set KIMI_API_KEY when the fix is adding a token to the DSN.
- parse_test.go: add kimi to TestBuiltinsResolve. (llama-swap stays excluded
  and is now documented — its no-URL built-in errors at Model() construction,
  not just on use, so it can't resolve there; the finding's llama-swap half
  was a false lead the test surfaced.)
- Add TestKimiSchemeMissingToken covering the corrected hint.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>

steve merged commit c5f84b95d8 into main

2026-07-18 07:26:11 +00:00

steve deleted branch feat/kimi-provider

2026-07-18 07:26:11 +00:00

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: steve/majordomo#20

feat: kimi (Moonshot AI) built-in provider (ADR-0026) #20

What

How

Decisions (per project owner)

Tests (hermetic, no network/creds)

Docs kept in sync (same commit)

Gates

🪰 Gadfly — live review status

claude-code/sonnet · claude-code — ✅ done

glm-5.2:cloud · ollama-cloud — ✅ done

kimi-k2.6:cloud · ollama-cloud — ✅ done

opencode/glm-5.2:cloud · opencode — ✅ done

opencode/kimi-k2.6:cloud · opencode — ✅ done

🪰 Gadfly review — consensus across 5 models

VERDICT: No material issues found

Verdict

`claude-code/sonnet` · claude-code — ✅ done

`glm-5.2:cloud` · ollama-cloud — ✅ done

`kimi-k2.6:cloud` · ollama-cloud — ✅ done

`opencode/glm-5.2:cloud` · opencode — ✅ done

`opencode/kimi-k2.6:cloud` · opencode — ✅ done