feat(agent-core-v2): support api_key_env for provider credentials - #3762
Conversation
Providers can declare api_key_env = "VAR" in config.toml to read their Bearer token from an environment variable instead of storing it inline. The value is resolved per request (rotation-friendly); an unset or empty variable fails with config.invalid naming the provider and variable. apiKey+apiKeyEnv and apiKeyEnv+oauth are rejected as conflicts. The /models refresh path resolves the same chain (inline apiKey -> apiKeyEnv -> env sub-table); a provider whose variable is unset lands in failed[] without affecting others, and open-platform providers keep the env declaration instead of persisting the resolved secret. Custom registries: applyCustomRegistryProvider consumes the api.json env field — imported providers get apiKeyEnv, the legacy inline apiKey is dropped, and a hand-edited apiKeyEnv survives refreshes (tracked via source.envKey). kimi provider add --api-key and the TUI registry token field are now optional so public registries import without a key.
🦋 Changeset detectedLatest commit: 7e2a582 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b661a5a702
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| kind: 'apiJson', | ||
| url, | ||
| apiKey, | ||
| ...(typeof envKey === 'string' && envKey.length > 0 ? { envKey } : {}), |
There was a problem hiding this comment.
envKey directly instead of conditionally spreading it
Normalize envKey to string | undefined and pass it directly as an optional property rather than using a conditional spread. The repository explicitly prohibits conditional spreads for optional object properties, so this newly added source-deserialization path violates the required construction convention.
AGENTS.md reference: AGENTS.md:L52-L54
Useful? React with 👍 / 👎.
| type: ManagedKimiCodeProtocol; | ||
| baseUrl?: string | undefined; | ||
| apiKey?: string | undefined; | ||
| apiKeyEnv?: string | undefined; |
There was a problem hiding this comment.
undefined from the optional field
Declare this as apiKeyEnv?: string; the optional marker already permits omission, and the repository explicitly forbids additionally including undefined in optional property types.
AGENTS.md reference: AGENTS.md:L55-L57
Useful? React with 👍 / 👎.
| config.providers[providerKey] = { | ||
| type: entry.type, | ||
| baseUrl: entry.api, | ||
| apiKey: source.apiKey, |
There was a problem hiding this comment.
env
When a custom-registry entry has no env field—or stops declaring one—this branch unconditionally rebuilds the provider with source.apiKey, discarding any hand-edited apiKeyEnv. Because startup refresh calls this helper automatically for source-backed providers, such a user override is silently lost and subsequent LLM requests use the registry token instead; preserve an existing untracked apiKeyEnv in this branch as well.
Useful? React with 👍 / 👎.
|
|
||
| - **Known third-party provider**: fetches the model catalog from [models.dev](https://models.dev/), select a provider → enter an API key → select a default model. Vendors whose protocol the catalog does not declare (e.g. xai, openrouter, and other vendor-specific SDKs) are imported as OpenAI-compatible with a "guessed" note; when the catalog provides no usable endpoint, a base URL prompt appears first; proprietary protocols (Amazon Bedrock, Cohere) and unrecognized explicit protocols are refused. Deprecated and alpha-status models are excluded from the import list. If the public catalog is unreachable, the CLI falls back to a built-in snapshot of the catalog, so the import still works offline or in blocked networks | ||
| - **Custom registry (api.json)**: paste a custom registry URL and Bearer token; the CLI automatically creates the `providers` / `models` entries. On later startup, providers from the same registry URL are refreshed together, so upstream provider additions, removals, and model metadata changes are synced. | ||
| - **Custom registry (api.json)**: paste a custom registry URL and, for private registries, a Bearer token; the CLI automatically creates the `providers` / `models` entries. When a registry entry declares the `env` field (the name of the environment variable holding the API key), the imported provider is written with `api_key_env` instead of a plain-text key — the registry token no longer doubles as the LLM credential, and no secret lands in `config.toml`. On later startup, providers from the same registry URL are refreshed together, so upstream provider additions, removals, and model metadata changes are synced. |
There was a problem hiding this comment.
For private registries, applyCustomRegistryProvider still stores the supplied Bearer token in source.apiKey, and the provider TOML serializer persists that source object. Only the LLM credential moves to api_key_env, so saying that “no secret lands in config.toml” can mislead users about storage of their registry token; document that distinction explicitly.
Useful? React with 👍 / 👎.
…dress review nits
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5369cce28a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (typeof provider.apiKey === 'string' && provider.apiKey.length > 0) { | ||
| return provider.apiKey; | ||
| } | ||
| const apiKeyEnv = declaredApiKeyEnv(provider); |
There was a problem hiding this comment.
When a provider declares both api_key_env and either api_key or oauth, runtime model resolution rejects the configuration, but this refresh helper returns the inline key before examining apiKeyEnv and ignores OAuth entirely. The automatic refresh can therefore send a request with credentials that should have been rejected and, for an open-platform provider whose models changed, rewrite the provider to silently discard the other credential; validate the same conflicts here before selecting a key.
Useful? React with 👍 / 👎.
| // `applyOpenPlatformConfig` persists the fetch key inline; an | ||
| // api_key_env provider must keep its env declaration instead, or the | ||
| // secret would leak from the environment into config.toml. | ||
| next.providers[providerId] = { type: 'kimi', baseUrl: platform.baseUrl, apiKeyEnv }; |
There was a problem hiding this comment.
When an api_key_env open-platform provider has custom fields such as customHeaders, a refresh that detects any model change persists this newly reconstructed three-field object and drops those fields. Since customHeaders is consumed when assembling outbound requests, the next request can lose required gateway headers; replace only the resolved inline apiKey while retaining the rest of providerConfig.
Useful? React with 👍 / 👎.
| if (auth.apiKey !== undefined) return; | ||
| if (auth.apiKeyEnv !== undefined) { | ||
| if (nonEmpty(process.env[auth.apiKeyEnv]) !== undefined) return; | ||
| throw new AuthTokenMissingError(providerName); |
There was a problem hiding this comment.
When the declared variable is unset, common entry paths such as apps/kimi-code/src/cli/v2/run-v2-print.ts and kap-server session actions call ensureReady() before credential resolution, so this generic auth.token_missing exception prevents the later config.invalid error that names auth.apiKeyEnv from ever reaching the user. A typo or missing export is therefore reported only as “provider … has no credential configured”; include the declared variable in this readiness diagnostic.
Useful? React with 👍 / 👎.
| 3. The matching key inside the `[providers.<name>.env]` sub-table (`KIMI_API_KEY`, `ANTHROPIC_API_KEY`, etc.): consulted only when both fields above are empty | ||
| 4. If all are absent, startup fails with an error indicating the provider is missing credentials | ||
|
|
||
| `api_key_env` is the one deliberate exception to "no shell environment variables for credentials": the variable is re-read on every request, so rotating the key only takes a fresh `export` — no restart, and no secret in `config.toml`. Declaring `api_key_env` while the variable is unset or empty fails the request with an error naming the variable; it is not a startup error, and the CLI never silently falls back to another credential source. Setting `api_key` together with `api_key_env`, or `api_key_env` together with `oauth`, is rejected as a configuration conflict. |
There was a problem hiding this comment.
For a long-running TUI or kap-server process, running a fresh export in the parent shell cannot modify that already-running process's process.env, so re-reading it per request does not make the new value visible without restarting the process. This instruction can leave users believing a rotated key is active when requests still use the old value; document the required process restart in both locale mirrors.
AGENTS.md reference: docs/AGENTS.md:L19-L21
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb99da2068
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| type?: ProviderType; | ||
| apiKey?: string; | ||
| apiKeyEnv?: string; |
There was a problem hiding this comment.
Add apiKeyEnv to packages/klient/src/contract/global/providers.ts and its public provider-auth input. Klient routes providerService through a Zod-validated contract whose schema still omits this new engine field, so set strips the declaration and get/list silently remove it from results over both memory and IPC transports. This leaves the contract-driven client facade unable to round-trip the newly supported credential configuration.
AGENTS.md reference: AGENTS.md:L30-L30
Useful? React with 👍 / 👎.
| if (apiKeyEnv !== undefined && provider.oauth !== undefined) { | ||
| throw new Error( | ||
| 'Provider has both apiKeyEnv and oauth set in config.toml - they are mutually exclusive. Remove one.', | ||
| ); |
There was a problem hiding this comment.
Apply this new apiKeyEnv/OAuth conflict validation to the managed-provider branch as well. A managed:kimi-code provider containing both fields never calls this helper because branch 1 selects it directly by oauth; refresh therefore succeeds with OAuth and, when models change, applyManagedKimiCodeConfig rewrites the provider without apiKeyEnv, even though normal model resolution rejects the same configuration. Fresh evidence after the earlier comment is that the added check remains reachable only from the open-platform and API-key branches.
Useful? React with 👍 / 👎.
| const inlineApiKey = | ||
| typeof provider.apiKey === 'string' && provider.apiKey.length > 0 | ||
| ? provider.apiKey | ||
| : undefined; |
There was a problem hiding this comment.
Trim the inline key before treating it as configured, matching resolveModelAuthMaterial's nonEmpty behavior. With api_key = " " and a valid api_key_env, normal requests correctly regard the inline value as empty and use the environment variable, but automatic model refresh treats it as present and reports a mutually-exclusive-credentials failure instead.
Useful? React with 👍 / 👎.
…en refresh validation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 521950da0a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const apiKeyEnv = | ||
| existingApiKeyEnv === undefined || existingApiKeyEnv === trackedEnvKey | ||
| ? envKey | ||
| : existingApiKeyEnv; | ||
| config.providers[providerKey] = { | ||
| type: entry.type, | ||
| baseUrl: entry.api, | ||
| apiKeyEnv, |
There was a problem hiding this comment.
A custom registry controls both entry.env and entry.api, so this automatically persists an arbitrary remotely supplied environment-variable name as apiKeyEnv. A malicious or compromised registry can declare a sensitive variable such as AWS_SECRET_ACCESS_KEY; the request path then reads that variable in ModelCatalog.buildCredentials() and sends its value as the credential to the registry-controlled API endpoint. Require explicit user approval or restrict registry-derived names rather than directly binding untrusted metadata to process.env.
Useful? React with 👍 / 👎.
| "@moonshot-ai/kimi-code": minor | ||
| --- | ||
|
|
||
| Providers can read their API key from an environment variable instead of storing it in `config.toml`: set `api_key_env = "YOUR_KEY_NAME"` on the provider. Custom registries declaring `env` import this way automatically, and `kimi provider add` no longer requires `--api-key` for public registries. |
There was a problem hiding this comment.
Rewrite this two-sentence changelog entry as one short user-facing sentence; the repository explicitly requires every changeset to contain exactly one such sentence.
AGENTS.md reference: AGENTS.md:L86-L87
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1eff24b174
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const existing = config.providers[providerKey]; | ||
| const existingApiKeyEnv = nonEmptyString(existing?.['apiKeyEnv']); |
There was a problem hiding this comment.
When an existing custom-registry provider is re-imported through either /provider or kimi provider add, the caller deletes the provider before this preservation check: applyCustomRegistryEntries() calls removeCustomRegistryProvider(), while the CLI calls harness.removeProvider(). Therefore existing is undefined and a hand-edited api_key_env is replaced by source.apiKey—often the empty string for a public registry—leaving subsequent model requests unauthenticated. Fresh evidence beyond the earlier refresh-path report is that both explicit re-import paths bypass the newly added preservation branch; retain the environment binding before deletion or update the provider in place.
Useful? React with 👍 / 👎.
|
|
||
| type?: ProviderType; | ||
| apiKey?: string; | ||
| apiKeyEnv?: string; |
There was a problem hiding this comment.
When an environment-backed provider is returned through /api/v1/config, toConfigResponse() still delegates to hasProviderCredential() in packages/kap-server/src/routes/config.ts, whose ProviderLike and predicate inspect only apiKey and oauth. The response therefore reports has_api_key: false for a provider whose declared variable contains a valid key, causing clients that consume this projection to present a configured provider as credentialless; mirror apiKeyEnv into this projection and apply the intended configured/readiness semantics.
Useful? React with 👍 / 👎.
| 3. The matching key inside the `[providers.<name>.env]` sub-table (`KIMI_API_KEY`, `ANTHROPIC_API_KEY`, etc.): consulted only when both fields above are empty | ||
| 4. If all are absent, startup fails with an error indicating the provider is missing credentials | ||
|
|
||
| `api_key_env` is the one deliberate exception to "no shell environment variables for credentials": the value is re-read from the process's own environment on every request, so it is never cached beyond the process lifetime and no secret lands in `config.toml`. Note that a running process only sees the environment it started with — rotating the variable takes a restart of the `kimi` / TUI or kap-server process; a fresh `export` in the parent shell only affects newly spawned processes. Declaring `api_key_env` while the variable is unset or empty fails the request with an error naming the variable; it is not a startup error, and the CLI never silently falls back to another credential source. Setting `api_key` together with `api_key_env`, or `api_key_env` together with `oauth`, is rejected as a configuration conflict. |
There was a problem hiding this comment.
Remove the claim that an unset api_key_env is not a startup error. Common entry paths call IAuthSummaryService.ensureReady() before beginning work, and the new branch in packages/agent-core-v2/src/app/auth/authService.ts explicitly throws AuthTokenMissingError when the variable is absent, so print mode and server session creation can fail before any model request is sent. The same correction is needed in the mirrored Chinese page.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a2e257aa2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| source, | ||
| }; | ||
| const existing = config.providers[providerKey]; | ||
| const existingApiKeyEnv = nonEmptyString(existing?.['apiKeyEnv']); |
There was a problem hiding this comment.
Although this version stopped consuming untrusted entry.env, applyCustomRegistryProvider now preserves apiKeyEnv from any provider with the same ID, without verifying that its source.url matches. During an unscoped automatic refresh, a registry can add an entry whose ID collides with a manually configured or different-registry provider; this replaces baseUrl with the registry-controlled entry.api while retaining the victim provider's environment binding, so the next request sends that environment secret to the new endpoint. Preserve the binding only when the existing provider belongs to the same registry source, and otherwise reject or replace the collision without carrying credentials across.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f72f61f23d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "@moonshot-ai/kimi-code": minor | ||
| --- | ||
|
|
||
| Providers can read their API key from a named environment variable via `api_key_env` in `config.toml`, and `kimi provider add` no longer requires `--api-key` for public registries. |
There was a problem hiding this comment.
Although the earlier two-sentence entry was condensed, the current sentence now combines two independently shipped behaviors: provider authentication through api_key_env and unauthenticated fetching of public registries. The required changeset workflow allows only one logical change per changeset, so split these into separate entries rather than joining them into one sentence.
AGENTS.md reference: AGENTS.md:L86-L87
Useful? React with 👍 / 👎.
|
|
||
| type?: ProviderType; | ||
| apiKey?: string; | ||
| apiKeyEnv?: string; |
There was a problem hiding this comment.
When an existing models.dev provider uses this field and is refreshed through :import_catalog without api_key, ModelsDevImportService.doImportModelsDevProvider() reconstructs the provider at lines 157–160 but copies only existing.apiKey. The refresh therefore silently drops apiKeyEnv, and the next session fails authentication; preserve the existing environment binding when no replacement inline key is supplied.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: decc8e7b1e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| type?: ProviderType; | ||
| apiKey?: string; | ||
| apiKeyEnv?: string; |
There was a problem hiding this comment.
When an environment-backed provider is edited through PUT /api/v1/providers/{provider_id} with a non-empty api_key, the replacement path in packages/kap-server/src/routes/modelCatalog.ts starts from { ...target } and then assigns provider.apiKey, leaving this new apiKeyEnv field intact. The route reports success, but resolveModelAuthMaterial() rejects the resulting provider because both credentials are set, so the next session fails with config.invalid; remove the old environment binding when this route receives a replacement inline key.
Useful? React with 👍 / 👎.
| ``` | ||
|
|
||
| Priority: `api_key` field > `env` sub-table key > if both are absent, startup fails with an error. | ||
| Priority: `api_key` field > `api_key_env` (variable name) > `env` sub-table key > if all are absent, startup fails with an error. During a `/models` refresh, a provider whose declared variable is unset or empty is reported as failed without affecting other providers. |
There was a problem hiding this comment.
This priority statement tells users that api_key wins over api_key_env, even though the new field table above and resolveModelAuthMaterial() reject any provider where both non-empty fields are configured. A user following the documented precedence therefore gets a startup/request failure rather than the promised fallback behavior; describe the fields as alternatives instead, and make the same correction in the mirrored Chinese pages and the other provider-credential summaries.
Useful? React with 👍 / 👎.
…es a replacement key
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08a3c4fd1a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@moonshot-ai/kimi-code": minor | |||
There was a problem hiding this comment.
Change this bump to patch: the required changeset workflow classifies configuration additions to existing features as patch releases, while minor is reserved for capabilities such as new commands or modes. Leaving this as minor unnecessarily advances the CLI's minor version.
AGENTS.md reference: AGENTS.md:L86-L88
Useful? React with 👍 / 👎.
|
|
||
| type: ProviderTypeSchema.optional(), | ||
| apiKey: z.string().optional(), | ||
| apiKeyEnv: z.string().optional(), |
There was a problem hiding this comment.
When global.providers.set/IProviderService.set replaces an existing inline-key or OAuth provider with { type, apiKeyEnv }, providersToToml() starts from the previous raw provider and only visits fields present in the replacement, so the old api_key or oauth remains in config.toml. The current process temporarily sees only apiKeyEnv, but after reload or restart both credentials are loaded and resolveModelAuthMaterial() rejects the provider as conflicting; serialization must remove omitted known credential fields when this replacement is persisted.
Useful? React with 👍 / 👎.
| 2. The matching key inside the `[providers.<name>.env]` sub-table (`KIMI_API_KEY`, `ANTHROPIC_API_KEY`, etc.): consulted only when `api_key` is empty | ||
| 3. If both are absent, startup fails with an error indicating the provider is missing credentials | ||
| 1. `[providers.<name>].api_key`: key written directly in the config file | ||
| 2. `[providers.<name>].api_key_env`: name of a shell environment variable to read the key from |
There was a problem hiding this comment.
This new api_key_env instruction directly contradicts the warning at line 29, which still says the explicit KIMI_MODEL_* channel is the only way provider credentials can come from exported shell variables. Readers following the page top-to-bottom are therefore told the newly documented mechanism does not exist; update that warning and its Chinese mirror to name api_key_env as the second explicit exception.
AGENTS.md reference: docs/AGENTS.md:L19-L21
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76e1f1969d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const existingApiKeyEnv = | ||
| existing !== undefined && readCustomRegistrySource(existing)?.url === source.url | ||
| ? nonEmptyString(existing['apiKeyEnv']) |
There was a problem hiding this comment.
During an automatic refresh, matching only source.url preserves the environment binding even if the registry changes this provider's entry.api; the rebuilt record then points baseUrl at that new registry-controlled endpoint while still reading the user's environment secret. A compromised registry can therefore redirect an already opted-in credential to an attacker endpoint. Preserve apiKeyEnv only when the provider endpoint is unchanged, or require explicit approval before carrying it across an endpoint change.
Useful? React with 👍 / 👎.
Reject reserved provider collisions and credential endpoint drift before mutation. Replace registry-owned sections atomically with compare-and-set guards, and carry the replacement contract through klient and node-sdk.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d38f6f7a69
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| type: providerTypeSchema.optional(), | ||
| apiKey: z.string().optional(), | ||
| apiKeyEnv: z.string().optional(), |
There was a problem hiding this comment.
Although the wire schema now round-trips apiKeyEnv, the public global.kosong.addProvider(id, config) path still accepts only ProviderAuth methods api-key and oauth, and its mapping writes only apiKey. Klient consumers therefore cannot create an environment-backed provider through the typed provider-management facade over either memory or IPC. Fresh evidence after the earlier comment is that this commit added the contract field but left the facade input and mapping unchanged; extend the named-provider auth input and mapping to emit apiKeyEnv.
AGENTS.md reference: packages/klient/AGENTS.md:L9-L18
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8411328f3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (stripped === undefined) { | ||
| delete stagedRaw[domain]; |
There was a problem hiding this comment.
When sections[domain] is undefined and exactKeys is supplied, this deletes the entire staged domain; applySectionToToml() subsequently removes the whole raw TOML section, undoing the selective raw-key deletion in persistDomains(). For example, the registry-import path clears thinking this way when its default model disappears, so an unrelated future field in [thinking] is lost—the newly added “preserves unknown fields while clearing exact fields” test exercises this exact case and cannot pass. Treat an undefined submission with exactKeys as deletion of only those keys rather than deletion of the domain.
Useful? React with 👍 / 👎.
| const apiKeyEnv = nonEmptyString(existing['apiKeyEnv']); | ||
| if ( | ||
| apiKeyEnv !== undefined && | ||
| normalizedEndpoint(existing['baseUrl']) !== normalizedEndpoint(entry.api) |
There was a problem hiding this comment.
When a same-registry provider has both a non-empty apiKey and apiKeyEnv, explicit re-imports through the CLI, TUI, or importCustomRegistry() reach this helper without the conflict validation used by automatic refresh. It retains apiKeyEnv, and applyCustomRegistryProvider() then reconstructs the provider without the inline key, silently rewriting a configuration that normal model resolution rejects; fresh evidence beyond the earlier refresh finding is that applyCustomRegistryEntries() still calls this helper directly and never invokes declaredProviderCredential(). Validate the existing provider before choosing which credential to preserve.
Useful? React with 👍 / 👎.
Route CLI and TUI imports through the engine via the typed SDK and Klient operation. Keep registry replacement policy inside the engine and preserve credential edits made while fetching the registry. Cover the shared entry point with real configuration storage and memory/IPC transport tests.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52f68eb275
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| selectedModel, | ||
| thinking: false, | ||
| apiKey, | ||
| credential: declared.kind === 'env' ? { apiKeyEnv: declared.apiKeyEnv } : { apiKey }, |
There was a problem hiding this comment.
When an open-platform provider's api_key_env is changed while /models is being fetched and the upstream model metadata also changes, declared still contains the pre-fetch variable name; rebaseSelectionAfterFetch() only refreshes the default/thinking fields, and this stale credential is then persisted over the user's edit. Reload or revalidate the provider credential after the fetch before applying the refreshed model list.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f766b9e3a3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ); | ||
| deps.exit(1); | ||
| } | ||
| const apiKey = resolveApiKey(opts.apiKey, deps.env) ?? ''; |
There was a problem hiding this comment.
When an existing private registry is re-imported without --api-key or KIMI_REGISTRY_API_KEY, converting the missing credential to '' prevents ModelsDevImportService.doImportCustomRegistry() from falling back to registryKeyFromExisting(...); the fetch is therefore sent without authorization and typically fails with 401/403 even though the registry token is already stored. Pass undefined when no credential was supplied (the TUI path likewise currently passes an empty string); new public-registry imports will still resolve to '' after the service's stored-key fallback.
Useful? React with 👍 / 👎.
Both first-party clients encoded "no key given" as an empty string, but the engine's fallback chain (options.apiKey ?? registryKeyFromExisting) only reuses the key parked in the provider's source blob when the option is undefined. A keyless re-import of a private registry therefore went out without an Authorization header and failed with 401 from the CLI and the TUI dialog, while the kap-server REST route (which passes undefined) reused the stored key fine. Pass undefined from the CLI handler and the TUI import dialog instead of normalizing to ''. The dialog's result type now marks apiKey optional, matching the engine contract.
The coverage trim went past redundancy in three places, leaving the PR's headline guards untested. Restore the compare-and-set rejection path (guarded value changed on disk -> retry error, no write) and the absent-section guard normalization in config.test.ts, the apiKey+apiKeyEnv / apiKeyEnv+oauth conflict rejections at runtime resolution (modelAuth) and at the refresh orchestrator (discovery, with per-provider failure isolation), and the open-platform refresh through api_key_env proving the resolved secret never lands in the provider record. Also add the first direct unit tests for the shared declaredProviderCredential / reconcileProviderCredentialUpdate primitives.
Signed-off-by: 7Sageer <sag77r@hotmail.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e2a58294f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const declared = declaredProviderCredential(managedProvider, KIMI_CODE_PROVIDER_NAME); | ||
| if (declared.kind === 'conflict') { | ||
| throw new Error(declared.message); | ||
| } |
There was a problem hiding this comment.
If managed:kimi-code is switched from OAuth to apiKeyEnv while this network request is in flight and the upstream model list also changes, rebaseSelectionAfterFetch() loads the edited provider but applyManagedKimiCodeConfig() immediately rebuilds it using the stale pre-fetch OAuth reference, silently undoing the credential change. Fresh evidence beyond the earlier open-platform race is that this managed branch validates only before the fetch and never checks the rebased provider; revalidate it after line 447 and abort if its credential declaration changed.
Useful? React with 👍 / 👎.
2 participants
Related Issue
N/A — internal feature request (no tracking issue).
Problem
Providers need an explicit way to read an API key from a named environment variable without storing the secret in
config.toml. Registry imports must preserve that binding while replacing obsolete providers and model aliases.Explicit registry imports also duplicated configuration mutation rules between the CLI/TUI and the engine. Clients had to calculate replacement keys, restore defaults, and assemble persistence guards, making fixes easy to miss across entry points.
What changed
api_key_envthroughout configuration, runtime credential resolution, catalog responses, and provider contracts. The variable is resolved per request; unset or empty values fail withconfig.invalidwithout falling back to another credential.envnames remain hints: users explicitly choose the binding, and resolved environment secrets are never persisted.IModelsDevImportService.importCustomRegistry. CLI and TUI now call a typed SDK/Klient operation, and REST uses the same engine implementation. The CLI/TUI still defer default-model selection; REST retains its existing first-model default on fresh setup.preserveUnknown,exactKeys, andexpectedValuesfrom the SDK/Klient configuration interface. Registry replacement policy stays inside the engine.Follow-ups
345cf3818fix(cli): the CLI and the TUI import dialog encoded "no key given" as'', which short-circuited the engine's stored-key reuse (apiKey ?? registryKeyFromExisting) and made a keyless re-import of a private registry fail with 401 from both first-party clients while the REST route reused the stored key fine. Both now passundefined; regression test included.80a8960c0test: the coverage trim had also removed the only tests for the compare-and-set rejection path, theapiKey+apiKeyEnv/apiKeyEnv+oauthconflict rejections at runtime resolution and at the refresh orchestrator, and the open-platform refresh throughapi_key_env(resolved secret never persisted). Restored those, and added the first direct unit tests for the shareddeclaredProviderCredential/reconcileProviderCredentialUpdateprimitives.Verification
pnpm run build:packagesandpnpm --filter @moonshot-ai/kimi-code buildpassed.pnpm lintpassed with 0 errors, including the comment-free check;git diff --checkpassed.Known limitations
api_key_envwhen the existing provider also has an inline key; normal requests and automatic refresh reject that conflict.Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.