diff --git a/docs/byok-environment-credentials.md b/docs/byok-environment-credentials.md new file mode 100644 index 00000000..6b60346c --- /dev/null +++ b/docs/byok-environment-credentials.md @@ -0,0 +1,81 @@ +# BYOK environment-variable credentials + +Store a reference in the active profile's `config.yaml` to keep an API key out of +that file. The TUI, `mcode exec`, and ACP resolve the reference from their own +process environment when they use the credential. + +## Configure an existing provider + +Set `WORK_API_KEY` locally in the shell that launches `mcode`. For a POSIX shell, +read the key without displaying it or putting its value in shell history: + +```bash +read -s WORK_API_KEY +export WORK_API_KEY +``` + +In PowerShell: + +```powershell +$secureKey = Read-Host 'API Key' -AsSecureString +$env:WORK_API_KEY = [System.Net.NetworkCredential]::new('', $secureKey).Password +``` + +Edit the existing provider entry in `config.yaml`, keeping its endpoint, API +format, and model definitions: + +```yaml +custom_provider: + work: + options: + apiKey: '${WORK_API_KEY}' +``` + +The official MiniMax API key supports the same syntax: + +```yaml +minimax_api: + apiKey: '${MINIMAX_API_KEY}' +``` + +Launch `mcode` from the configured shell. Restart a running TUI, exec process, or +ACP host after changing its launch environment; changes in another terminal do +not update an existing process. A launcher or editor that starts ACP must pass +the variable to the child process too. + +`provider add --api-key-env NAME` continues to read and save the variable's value. +To retain a reference, edit the saved credential field as shown above. + +## Supported syntax and errors + +Only a complete `${NAME}` string expands. Names begin with a letter or underscore, +followed by letters, digits, or underscores. `$NAME`, `Bearer ${NAME}`, and +`prefix-${NAME}` remain literal values. A mapping such as `{env: NAME}` is rejected +as the wrong credential type. This feature does not expand Base URLs or custom +request headers. + +Missing or whitespace-only variables fail before the model request, with an error +that identifies the provider, credential field, and variable name. Check whether +the variable exists without printing its value. Saved-provider discovery and +connection tests use the same credential resolution; cached connection status +tracks the resolved key, so changing the key invalidates a previous result. + +## Configuration and logs + +Saving another setting preserves credential reference text and existing YAML +comments. When a configuration value changes, YAML aliases and merge fields are +expanded to independent values: editing one provider cannot change another +provider through a shared anchor, and clearing an inherited key stays cleared +when the file is read again. A no-op write keeps the original text. Changed +files may have normalized whitespace or indentation. + +Numeric provider and model keys retain their identity during updates. YAML version +directives do not change the application loader's scalar interpretation during +alias expansion. Multiline plain strings retain their folded line breaks when +unrelated settings are saved. Each changed document is checked with that loader +before the configuration file is replaced. + +Malformed configuration is rejected without replacing the existing file. POSIX +configuration permissions remain private. Runtime log fields and common +credential text are redacted, while token usage counters remain readable. This +protection does not remove credentials from older logs or files. diff --git a/docs/examples.md b/docs/examples.md index 677cbb75..5dcb050d 100644 --- a/docs/examples.md +++ b/docs/examples.md @@ -68,6 +68,11 @@ pnpm mcode provider list --json `--context-limit` and `--output-limit` each accept a positive safe integer (at most `9007199254740991`). Either flag can be used independently. The same limits apply to every repeated `--model`; only the first model is tested and selected by `--use`. The JSON list shows the configured values as `contextLimit` and `maxOutputTokens`. Without these flags, the existing defaults remain unchanged (unknown custom models currently fall back to 200,000 context tokens and 16,384 output tokens). Model discovery does not infer your local server's context size. +To keep the key out of `config.yaml`, replace the saved `apiKey` with a quoted +`${MCODE_PROVIDER_API_KEY}` reference and launch `mcode` from the shell where the +variable is exported. See [environment-variable credentials](byok-environment-credentials.md) +for both supported credential fields, restart behavior, and configuration writes. + `--api-key-env` reads the current environment variable value and stores that value in the active profile's `config.yaml`; it does not save an environment-variable reference. The file still contains plaintext credentials. On POSIX systems, config writes and temporary copies use `0600`. When loading existing files, MCode removes group/other access while preserving the owner's permissions; already-private files such as `0400` or `0600` do not require a permission change. Loading fails if an unsafe main config cannot be restricted. Older migration backups are also checked, but inspection or repair failures produce a warning identifying the directory or backup that needs manual attention rather than preventing the main config from loading. Windows file modes do not provide equivalent ACL protection; restrict access to the profile directory using Windows permissions. ### Third-party relays and custom auth headers diff --git a/packages/config/package.json b/packages/config/package.json index 82bc08fa..a94c1b6f 100644 --- a/packages/config/package.json +++ b/packages/config/package.json @@ -28,7 +28,8 @@ "dependencies": { "@mavis/shared": "workspace:^", "js-yaml": "^4", - "proper-lockfile": "^4" + "proper-lockfile": "^4", + "yaml": "^2.9.0" }, "devDependencies": { "@types/js-yaml": "^4", diff --git a/packages/config/src/comment-preserving-config-write.ts b/packages/config/src/comment-preserving-config-write.ts new file mode 100644 index 00000000..30668498 --- /dev/null +++ b/packages/config/src/comment-preserving-config-write.ts @@ -0,0 +1,237 @@ +import { isDeepStrictEqual } from 'node:util'; + +import yaml from 'js-yaml'; +import { + isMap, + isNode, + isScalar, + parseDocument, + visit, + type Alias, + type Document, + type Node, + type YAMLMap, +} from 'yaml'; + +/** + * Runtime rewrites of config.yaml must not throw away what the user wrote. + * + * The previous path parsed the file, mutated a plain object, and dumped that + * object back, which dropped every comment and re-indented the whole file. Start + * from the original document — which keeps comments and original formatting on + * every node we do not touch — and write only the paths that actually changed. + * + * The diff is bounded: nested plain objects are walked key by key so comments + * survive inside them, while anything else — a changed node kind, or a list — is + * replaced as one value. That keeps the rewrite narrow instead of inventing a + * merge rule for shapes this file does not use. + */ + +type ConfigEdit = + | { readonly kind: 'set'; readonly path: string[]; readonly value: unknown } + | { readonly kind: 'delete'; readonly path: string[] }; + +/** + * Parses config text into the plain-object "before" state used for the diff. + * Callers keep this separate from the object they mutate, because several write + * paths hand that mutable object to a callback that aliases into it. + */ +export function parseConfigText(text: string): Record { + try { + const parsed = yaml.load(text); + return isPlainRecord(parsed) ? parsed : {}; + } catch { + return {}; + } +} + +/** + * Serializes `next` over `previous` while keeping comments and formatting from + * `originalText`. Falls back to a full dump when the original cannot be parsed + * into a document, so a hand-broken file still round-trips instead of vanishing. + */ +export function serializeConfigPreservingComments( + originalText: string, + previous: Record, + next: Record, +): string { + const document = parseDocument(originalText, { + schema: 'core', + compat: 'yaml-1.1', + customTags: ['timestamp'], + merge: true, + }); + if (document.errors.length > 0 || (!isMap(document.contents) && originalText.trim() !== '')) { + return dumpConfig(next); + } + + try { + const edits = collectConfigEdits(previous, next); + if (edits.length === 0) return originalText; + useConfigLoaderScalarValues(document); + materializeConfigReferences(document); + for (const edit of edits) { + const path = existingConfigPath(document, edit.path); + if (edit.kind === 'delete') { + document.deleteIn(path); + } else { + document.setIn(path, edit.value); + } + } + const serialized = document.toString({ indent: 2, lineWidth: -1 }); + // The application loader is the final authority. Reject an invalid or + // semantically different result before the atomic writer replaces the file. + if (!isDeepStrictEqual(yaml.load(serialized), yaml.load(dumpConfig(next)))) { + throw new Error('The serialized configuration does not match the intended values'); + } + return serialized; + } catch (error) { + // A self-referential anchor (`&a { self: *a }`) makes the emitter recurse + // without bound. That input cannot be written back in any form, so report + // it as a config problem instead of letting a stack overflow escape. + throw new Error( + `config.yaml could not be rewritten: ${ + error instanceof Error ? error.message : String(error) + }${error instanceof RangeError ? '. Remove the self-referential YAML anchor and retry.' : ''}`, + ); + } +} + +/** Keep alias expansion independent of the AST parser's implicit scalar rules. */ +function useConfigLoaderScalarValues(document: Document): void { + visit(document, { + Scalar(_key, node) { + if (node.addToJSMap || typeof node.source !== 'string') return; + if (node.type !== 'PLAIN' && !node.tag) return; + // A mapping wrapper keeps values such as "---" from becoming directives. + // node.source already contains YAML's folded multiline value. Quote it + // before reparsing so its line breaks keep their meaning and indentation. + // Explicit tags still apply to quoted values. + const source = + node.type === 'PLAIN' && !node.source.includes('\n') + ? node.source + : JSON.stringify(node.source); + const tag = node.tag ? `!<${node.tag}> ` : ''; + const value = (yaml.load(`value: ${tag}${source}`) as { value: unknown }).value; + if (!isDeepStrictEqual(node.value, value)) { + node.value = value; + delete node.format; + } + }, + }); +} + +/** JavaScript config keys are strings, while YAML keeps numeric/boolean keys typed. */ +function existingConfigKey(map: YAMLMap, key: string): unknown { + return map.items.find((pair) => isScalar(pair.key) && String(pair.key.value) === key)?.key ?? key; +} + +function existingConfigPath(document: Document, path: readonly string[]): unknown[] { + const resolved: unknown[] = []; + let parent: unknown = document.contents; + for (const key of path) { + resolved.push(isMap(parent) ? existingConfigKey(parent, key) : key); + parent = document.getIn(resolved, true); + } + return resolved; +} + +/** + * Provider updates can replace one options object while leaving its former + * aliases unchanged. Snapshot references before editing their sources, so YAML + * sharing cannot reintroduce coupling the application has already removed. + * Materialize merge fields too: deleting an inherited key must not expose the + * value again through `<<`. Explicit nodes retain their comments and styles. + */ +function materializeConfigReferences(document: Document): void { + const aliases = new Map(); + visit(document, { + Alias(_key, alias) { + const value: unknown = alias.toJS(document); + const node = createConfigNode(document, value); + node.comment = alias.comment; + node.commentBefore = alias.commentBefore; + node.spaceBefore = alias.spaceBefore; + aliases.set(alias, node); + }, + }); + visit(document, { Alias: (_key, alias) => aliases.get(alias) }); + visit(document, { + Map(_key, map) { + const merges = map.items.filter((pair) => isScalar(pair.key) && pair.key.addToJSMap); + if (merges.length === 0) return; + const values = map.toJS(document) as Record; + const comments = merges.flatMap((pair) => + [pair.key, pair.value].flatMap((node) => + isNode(node) ? [node.commentBefore, node.comment] : [], + ), + ); + map.commentBefore = [map.commentBefore, ...comments].filter(Boolean).join('\n') || undefined; + map.items = map.items.filter((pair) => !merges.includes(pair)); + for (const [key, value] of Object.entries(values)) { + if (!map.has(existingConfigKey(map, key))) { + map.set(key, createConfigNode(document, value)); + } + } + }, + }); + // All merge pairs are gone. Disable merge emission, and quote strings that + // either scalar schema could interpret (including the loader's 0b integers). + document.setSchema(document.directives?.yaml.version ?? '1.2', { + schema: 'core', + compat: 'yaml-1.1', + customTags: ['timestamp'], + merge: false, + }); +} + +function createConfigNode(document: Document, value: unknown): Node { + const node = document.createNode(value, { aliasDuplicateObjects: false }); + // A resolved object's literal "<<" property has already lost its YAML quote + // metadata. Quote it again so materialization cannot turn it into a merge. + visit(node, { + Pair(_key, pair) { + if (isScalar(pair.key) && pair.key.value === '<<') pair.key.type = 'QUOTE_DOUBLE'; + }, + }); + return node; +} + +function collectConfigEdits(previous: unknown, next: unknown, path: string[] = []): ConfigEdit[] { + if (!isPlainRecord(previous) || !isPlainRecord(next)) { + return valuesEqual(previous, next) ? [] : [{ kind: 'set', path, value: next }]; + } + const edits: ConfigEdit[] = []; + for (const key of new Set([...Object.keys(previous), ...Object.keys(next)])) { + const childPath = [...path, key]; + // A caller that assigns undefined is removing the value; writing it back + // would emit `key: null`, which reads as a configured-but-empty field. + if (!(key in next) || next[key] === undefined) { + edits.push({ kind: 'delete', path: childPath }); + } else if (!(key in previous)) { + edits.push({ kind: 'set', path: childPath, value: next[key] }); + } else { + edits.push(...collectConfigEdits(previous[key], next[key], childPath)); + } + } + return edits; +} + +function valuesEqual(left: unknown, right: unknown): boolean { + if (left === right) return true; + if (left === null || right === null) return false; + if (typeof left !== 'object' || typeof right !== 'object') return false; + try { + return JSON.stringify(left) === JSON.stringify(right); + } catch { + return false; + } +} + +function dumpConfig(next: Record): string { + return yaml.dump(next, { indent: 2, lineWidth: -1, noRefs: true }); +} + +function isPlainRecord(value: unknown): value is Record { + return Boolean(value) && typeof value === 'object' && !Array.isArray(value); +} diff --git a/packages/config/src/config.ts b/packages/config/src/config.ts index 75bccea1..70bb6fa3 100644 --- a/packages/config/src/config.ts +++ b/packages/config/src/config.ts @@ -1,3 +1,7 @@ +import { + parseConfigText, + serializeConfigPreservingComments, +} from './comment-preserving-config-write.js'; import { resolveRunawayGuardConfig, type RunawayGuardSettings, @@ -1654,7 +1658,11 @@ function syncManagedPresetBaseUrl(configPath: string): void { try { writePrivateConfigFileSync( configPath, - yaml.dump(raw, { indent: 2, lineWidth: -1, noRefs: true }), + serializeConfigPreservingComments( + originalContent.toString('utf-8'), + parseConfigText(originalContent.toString('utf-8')), + raw, + ), ); } catch (error) { // This on-disk sync is optional, but a failure after truncation is not safe diff --git a/packages/config/src/credential-reference.ts b/packages/config/src/credential-reference.ts new file mode 100644 index 00000000..31793b36 --- /dev/null +++ b/packages/config/src/credential-reference.ts @@ -0,0 +1,93 @@ +/** + * Provider credentials may name an environment variable instead of carrying the + * key itself, so a plaintext secret never has to reach config.yaml. + * + * A whole-string `${NAME}` reference is resolved when the credential is read, so + * rotating the key in the environment takes effect without touching the file. The + * resolved value belongs to the request in flight; nothing in the config write + * path ever receives it. + * + * The accepted form is deliberately narrow. Partial interpolation (`Bearer ${T}`) + * and the unbraced `$NAME` shorthand stay plaintext, because a partial expansion + * cannot be rejected at the config boundary without guessing at a prefix rule. + */ + +const ENV_REFERENCE_PATTERN = /^\$\{([A-Za-z_][A-Za-z0-9_]*)\}$/; + +export interface ProviderCredentialInput { + /** Config path the credential was read from, quoted in errors. */ + readonly field: string; + /** Provider the credential belongs to, quoted in errors. */ + readonly provider: string; + /** Raw configured value: plaintext, a `${NAME}` reference, or the wrong type. */ + readonly value: unknown; + readonly env?: NodeJS.ProcessEnv; +} + +/** Environment variable name when `value` is exactly a `${NAME}` reference. */ +export function credentialEnvReferenceName(value: unknown): string | undefined { + if (typeof value !== 'string') return undefined; + return ENV_REFERENCE_PATTERN.exec(value.trim())?.[1]; +} + +export function resolveProviderCredential(input: ProviderCredentialInput): string | undefined { + const { field, provider, value } = input; + if (value === undefined || value === null) return undefined; + if (typeof value !== 'string') { + throw new Error( + `Provider "${provider}": ${field} must be a string or a \`\${NAME}\` environment variable reference, received ${describeConfiguredType(value)}.`, + ); + } + const configured = value.trim(); + if (!configured) return undefined; + + const envName = credentialEnvReferenceName(configured); + if (!envName) return configured; + + const resolved = (input.env ?? process.env)[envName]?.trim(); + if (!resolved) { + throw new Error( + `Provider "${provider}": ${field} references environment variable "${envName}", which is not set.`, + ); + } + return resolved; +} + +export type ProviderCredentialSource = 'plaintext' | 'env'; + +export interface ProviderCredentialProbe { + /** Whether the provider declares a credential, regardless of whether it resolves. */ + readonly configured: boolean; + readonly source?: ProviderCredentialSource; + /** Resolved secret. Absent when the reference is broken or the system store is unavailable. */ + readonly secret?: string; +} + +/** + * Non-throwing counterpart to {@link resolveProviderCredential}, for call sites + * that only need to know whether a credential is configured and where it comes + * from — provider views, connection fingerprints. A missing environment + * variable reads as "configured but unresolved" here; the strict + * resolver still reports it when a request is actually made. + */ +export function probeProviderCredential(input: ProviderCredentialInput): ProviderCredentialProbe { + const hasPlain = typeof input.value === 'string' && input.value.trim().length > 0; + if (!hasPlain) return { configured: false }; + + const source: ProviderCredentialSource = isEnvReference(input.value) ? 'env' : 'plaintext'; + try { + const secret = resolveProviderCredential(input); + return { configured: true, source, ...(secret ? { secret } : {}) }; + } catch { + return { configured: true, source }; + } +} + +function isEnvReference(value: unknown): boolean { + return credentialEnvReferenceName(value) !== undefined; +} + +function describeConfiguredType(value: unknown): string { + if (Array.isArray(value)) return 'a list'; + return `a ${typeof value}`; +} diff --git a/packages/config/src/index.ts b/packages/config/src/index.ts index 6b3563f6..ac99130e 100644 --- a/packages/config/src/index.ts +++ b/packages/config/src/index.ts @@ -83,6 +83,20 @@ export { isManagedProviderBaseUrl, resolveProviderAuthMode, } from './provider-auth-mode.js'; +export { + credentialEnvReferenceName, + probeProviderCredential, + resolveProviderCredential, +} from './credential-reference.js'; +export type { + ProviderCredentialInput, + ProviderCredentialProbe, + ProviderCredentialSource, +} from './credential-reference.js'; +export { + parseConfigText, + serializeConfigPreservingComments, +} from './comment-preserving-config-write.js'; export { compareAndSetLocalModelContext, removeLocalProviderConfig, diff --git a/packages/config/src/local-model-provider-write.ts b/packages/config/src/local-model-provider-write.ts index 9a0bb656..e7c157d9 100644 --- a/packages/config/src/local-model-provider-write.ts +++ b/packages/config/src/local-model-provider-write.ts @@ -1,5 +1,5 @@ import { randomBytes } from 'node:crypto'; -import fs from 'node:fs'; +import fs, { readFileSync } from 'node:fs'; import { dirname, join } from 'node:path'; import yaml from 'js-yaml'; @@ -12,6 +12,7 @@ import { resetConfig, type Config, } from './config.js'; +import { serializeConfigPreservingComments } from './comment-preserving-config-write.js'; import { MANAGED_MINIMAX_PROVIDER_ID, MINIMAX_API_PROVIDER_ID } from './model-availability.js'; const LOCAL_CONFIG_FILE_MODE = 0o600; @@ -234,12 +235,16 @@ async function withLockedConfig( stale: 10_000, retries: { retries: 20, factor: 1, minTimeout: 5, maxTimeout: 25 }, }); - const raw = readLocalRawConfig(configPath); + const previousRaw = readLocalRawConfig(configPath); + const previous = structuredClone(previousRaw); resetConfig(); - const outcome = await operation(raw, getConfig()); + const outcome = await operation(previous, getConfig()); if (outcome.write) { - assertSafeConfigRecord(raw); - await atomicWriteFile(configPath, yaml.dump(raw, { indent: 2, lineWidth: -1, noRefs: true })); + assertSafeConfigRecord(previous); + await atomicWriteFile( + configPath, + serializeConfigPreservingComments(readFileSync(configPath, 'utf-8'), previousRaw, previous), + ); resetConfig(); } return { config: getConfig(), value: outcome.value }; diff --git a/packages/config/test/comment-preserving-config-write.test.ts b/packages/config/test/comment-preserving-config-write.test.ts new file mode 100644 index 00000000..5d8eb0fe --- /dev/null +++ b/packages/config/test/comment-preserving-config-write.test.ts @@ -0,0 +1,393 @@ +import yaml from 'js-yaml'; +import { describe, expect, it } from 'vitest'; + +import { + parseConfigText, + serializeConfigPreservingComments, +} from '../src/comment-preserving-config-write.js'; + +function apply(originalText: string, mutate: (config: Record) => void): string { + const previous = parseConfigText(originalText); + const next = structuredClone(previous); + mutate(next); + return serializeConfigPreservingComments(originalText, previous, next); +} + +const ORIGINAL = `# my providers +logLevel: info + +custom_provider: + # third party, owned by platform team + mafia: + options: + baseURL: https://api.example.com/v1 + apiKey: "\${MAFIA_API_KEY}" # keep this note + models: + gpt-x: + limit: + context: 128000 +`; + +describe('comment-preserving config serialization', () => { + it.each(['123', '0x10', 'true', 'null'])( + 'updates and deletes existing scalar mapping key %s', + (sourceKey) => { + const original = `custom_provider:\n ${sourceKey}: # existing provider\n options: { apiKey: old-placeholder }\n`; + const previous = parseConfigText(original); + const providers = previous.custom_provider as Record; + const key = Object.keys(providers)[0]; + const updated = apply(original, (config) => { + const tree = config.custom_provider as Record }>; + tree[key].options.apiKey = 'new-placeholder'; + }); + expect(yaml.load(updated)).toEqual({ + custom_provider: { [key]: { options: { apiKey: 'new-placeholder' } } }, + }); + expect(updated).toContain('# existing provider'); + + const removed = apply(original, (config) => { + delete (config.custom_provider as Record)[key]; + }); + expect(yaml.load(removed)).toEqual({ custom_provider: {} }); + }, + ); + + it('preserves numeric explicit keys alongside merged defaults', () => { + const original = `defaults: &defaults { 123: inherited, 456: retained } +settings: + <<: *defaults + 123: explicit # local override +`; + const written = apply(original, (config) => { + (config.settings as Record)['123'] = 'updated'; + }); + expect(yaml.load(written)).toEqual({ + defaults: { '123': 'inherited', '456': 'retained' }, + settings: { '123': 'updated', '456': 'retained' }, + }); + expect(written).toContain('# local override'); + }); + + it.each(['', '%YAML 1.1\n---\n'])( + 'uses loader scalar semantics when materializing aliases and merges with directive %j', + (directive) => { + const original = `${directive}defaults: &defaults + on: on + yes: yes + no: no + zero: 012 + binary: 0b10 + binaryText: "0b10" + decimalText: "012" + separated: 1_000 + date: 2020-01-01 + tagged: !!str 012 + folded: > + first + second +alias: *defaults +merged: + <<: *defaults + local: retained +`; + const expected = { ...parseConfigText(original), logLevel: 'debug' }; + const written = apply(original, (config) => { + config.logLevel = 'debug'; + }); + expect(yaml.load(written)).toEqual(expected); + }, + ); + + it.each(['', '%YAML 1.1\n---\n'])( + 'preserves multiline plain strings when saving unrelated fields with directive %j', + (directive) => { + const original = `${directive}defaults: &defaults + note: first + + second + + + third + tagged: !!str alpha + + beta +alias: *defaults +logLevel: info +`; + const previous = parseConfigText(original); + const written = apply(original, (config) => { + config.logLevel = 'debug'; + }); + expect(yaml.load(written)).toEqual({ ...previous, logLevel: 'debug' }); + expect((previous.defaults as Record).note).toBe('first\nsecond\n\nthird'); + }, + ); + + it('rejects a serialized document that no longer matches the intended configuration', () => { + expect(() => + serializeConfigPreservingComments( + 'logLevel: info\n', + { logLevel: 'debug' }, + { logLevel: 'debug', defaultModel: 'work/new' }, + ), + ).toThrow(/serialized configuration does not match/u); + }); + + it('keeps reference syntax unchanged when no configuration values change', () => { + const original = 'first: &options { apiKey: placeholder }\nsecond: *options\n'; + const previous = parseConfigText(original); + + expect(serializeConfigPreservingComments(original, previous, structuredClone(previous))).toBe( + original, + ); + }); + + it('keeps an alias value when its anchor owner is removed', () => { + const original = + 'first: &options { apiKey: placeholder }\nsecond: *options # retained provider\n'; + const previous = parseConfigText(original); + const next = { second: previous.second }; + + const written = serializeConfigPreservingComments(original, previous, next); + + expect(parseConfigText(written)).toEqual(next); + expect(written).toContain('# retained provider'); + }); + + it.each(['primary', 'secondary'])( + 'isolates edits to the %s provider with shared options', + (key) => { + const original = `# provider settings +custom_provider: + primary: + options: &options + baseURL: https://original.example/v1 # original endpoint + apiKey: shared-placeholder + secondary: + options: *options # secondary settings +`; + const previous = parseConfigText(original); + const next = structuredClone(previous); + const providers = next.custom_provider as Record< + string, + { options: Record } + >; + providers[key].options = { ...providers[key].options, baseURL: 'https://updated.example/v1' }; + + const written = serializeConfigPreservingComments(original, previous, next); + + expect(parseConfigText(written)).toEqual(next); + expect(written).toContain('# provider settings'); + expect(written).toContain('# original endpoint'); + expect(written).toContain('# secondary settings'); + }, + ); + + it.each(['', ' apiKey: override-placeholder\n'])( + 'clears a merged credential with local override %j', + (override) => { + const original = `defaults: &defaults + apiKey: inherited-placeholder + baseURL: https://original.example/v1 +custom_provider: + work: + options: + <<: *defaults # inherited connection settings +${override} authMode: api-key # preserve local settings +`; + const previous = parseConfigText(original); + const next = structuredClone(previous); + const providers = next.custom_provider as Record< + string, + { options: Record } + >; + providers.work.options = { ...providers.work.options }; + delete providers.work.options.apiKey; + + const written = serializeConfigPreservingComments(original, previous, next); + + expect(parseConfigText(written)).toEqual(next); + expect(written).toContain('# inherited connection settings'); + expect(written).toContain('# preserve local settings'); + }, + ); + + it('keeps merge precedence, nested aliases and quoted merge-like keys when clearing a key', () => { + const original = `first: &first + apiKey: first-placeholder + baseURL: https://first.example +second: &second + apiKey: second-placeholder + authMode: api-key +options: &options + <<: [*first, *second] +literal: &literal + "<<": literal +custom_provider: + work: + options: *options +copiedLiteral: *literal +`; + const previous = parseConfigText(original); + const next = structuredClone(previous); + const providers = next.custom_provider as Record }>; + providers.work.options = { ...providers.work.options }; + delete providers.work.options.apiKey; + + const written = serializeConfigPreservingComments(original, previous, next); + + expect(parseConfigText(written)).toEqual(next); + }); + + it('keeps comments and formatting while changing one leaf', () => { + const next = apply(ORIGINAL, (config) => { + const provider = config.custom_provider as Record< + string, + Record> + >; + provider.mafia.options.baseURL = 'https://api.new.example.com/v1'; + }); + + expect(next).toContain('# my providers'); + expect(next).toContain('# third party, owned by platform team'); + expect(next).toContain('# keep this note'); + expect(next).toContain('https://api.new.example.com/v1'); + expect(next).not.toContain('api.example.com/v1'); + // Untouched siblings keep their original shape. + expect(next).toContain('logLevel: info'); + expect(next).toContain('context: 128000'); + }); + + it('does not re-serialize an untouched reference into a nested map', () => { + const next = apply(ORIGINAL, (config) => { + config.defaultModel = 'mafia/gpt-x'; + }); + + // The old dump-then-write path round-tripped the reference through the + // parser and wrote a nested map here; the file must keep the reference. + expect(next).toContain(['$', '{MAFIA_API_KEY}'].join('')); + expect(next).not.toMatch(/env:/u); + }); + + it('deletes a removed key without disturbing its siblings', () => { + const next = apply(ORIGINAL, (config) => { + const provider = config.custom_provider as Record< + string, + Record> + >; + delete provider.mafia.options.baseURL; + }); + + expect(next).not.toContain('baseURL'); + // The emitter normalises the spaces before a trailing comment; the comment + // itself is what has to survive. + expect(next).toMatch(/apiKey: "\$\{MAFIA_API_KEY\}"\s+# keep this note/u); + expect(next).toContain('# my providers'); + }); + + it('adds a missing key under an existing parent', () => { + const next = apply(ORIGINAL, (config) => { + const provider = config.custom_provider as Record< + string, + Record> + >; + provider.mafia.options.authMode = 'api-key'; + }); + + expect(next).toContain('authMode: api-key'); + expect(next).toContain('# keep this note'); + }); + + it('produces a parseable document', () => { + const next = apply(ORIGINAL, (config) => { + config.defaultModel = 'mafia/gpt-x'; + }); + + expect(parseConfigText(next)).toMatchObject({ defaultModel: 'mafia/gpt-x' }); + }); + + it('falls back to a full dump when the original is unparseable', () => { + const broken = 'custom_provider: [unclosed\n'; + const next = apply(broken, (config) => { + config.defaultModel = 'mafia/gpt-x'; + }); + + expect(parseConfigText(next)).toMatchObject({ defaultModel: 'mafia/gpt-x' }); + }); + + it('preserves changes the caller made through a shared object', () => { + const aliased = 'a: &p\n x: 1\nb: *p\n'; + const next = apply(aliased, (config) => { + (config.b as Record).x = 2; + }); + + expect(parseConfigText(next)).toEqual({ a: { x: 2 }, b: { x: 2 } }); + }); + + it('writes when the alias sits in the middle of the path', () => { + // `mafia: *d` puts the alias above the leaf being written, so resolving + // only the final node left getIn unable to descend past it. + const midAlias = 'defaults: &d\n baseURL: https://a\ncustom_provider:\n mafia: *d\n'; + const next = apply(midAlias, (config) => { + (config.custom_provider as Record>).mafia.apiKey = 'sk-x'; + }); + + expect(parseConfigText(next)).toEqual({ + defaults: { baseURL: 'https://a', apiKey: 'sk-x' }, + custom_provider: { mafia: { baseURL: 'https://a', apiKey: 'sk-x' } }, + }); + }); + + it('follows a merge-key alias', () => { + const merged = 'a: &a\n x: 1\nb: &b\n <<: *a\n y: 2\n'; + const next = apply(merged, (config) => { + (config.b as Record).x = 5; + }); + + expect(next).toContain('x: 5'); + }); + + it('reports a self-referential anchor instead of overflowing the stack', () => { + // The edit is on an unrelated key, so the circular anchor is emitted + // untouched and the writer recurses through it. + const circular = 'a: &s\n self: *s\n x: 1\nc: 1\n'; + const previous = parseConfigText(circular); + const next = { ...previous, c: 2 }; + + expect(() => serializeConfigPreservingComments(circular, previous, next)).toThrow( + /self-referential YAML anchor/u, + ); + }); + + it('writes through a nested alias node', () => { + const aliased = 'a: &p\n x: 1\nc:\n d: *p\n'; + const next = apply(aliased, (config) => { + const c = config.c as { d: Record }; + c.d.x = 5; + }); + + expect(next).toContain('x: 5'); + }); + + it('removes a key a caller set to undefined instead of writing a null', () => { + const next = apply(ORIGINAL, (config) => { + const provider = config.custom_provider as Record< + string, + Record> + >; + provider.mafia.options.apiKey = undefined; + }); + + // `apiKey: null` reads as a configured-but-empty credential. + expect(next).not.toContain('apiKey'); + expect(next).not.toContain('null'); + }); + + it('handles an empty config file', () => { + const next = apply('', (config) => { + config.defaultModel = 'mafia/gpt-x'; + }); + + expect(parseConfigText(next)).toMatchObject({ defaultModel: 'mafia/gpt-x' }); + }); +}); diff --git a/packages/config/test/config-update-preservation.test.ts b/packages/config/test/config-update-preservation.test.ts index 175c9da3..e157794f 100644 --- a/packages/config/test/config-update-preservation.test.ts +++ b/packages/config/test/config-update-preservation.test.ts @@ -144,6 +144,31 @@ describe.each(writers)("%s config updates", (_name, write) => { }); describe("valid local config updates", () => { + + it("preserves comments and credential references through general settings writes", async () => { + const reference = "${WORK_API_KEY}"; + fs.writeFileSync(configPath, [ + "# provider credential stays external", + "permissionMode: auto # approval setting", + "custom_provider:", + " work:", + " options:", + " apiKey: '" + reference + "' # inherited from the launcher", + "", + ].join("\n")); + + await updateLocalConfigFile({ permissionMode: "default" }); + + const written = fs.readFileSync(configPath, "utf8"); + expect(written).toContain("# provider credential stays external"); + expect(written).toContain("# approval setting"); + expect(written).toContain("# inherited from the launcher"); + expect(yaml.load(written)).toMatchObject({ + permissionMode: "default", + custom_provider: { work: { options: { apiKey: reference } } }, + }); + }); + it.each([undefined, "", "# empty config\n", "{}\n", "null\n"])( "initializes a missing or empty document: %s", async (source) => { diff --git a/packages/config/test/credential-reference.test.ts b/packages/config/test/credential-reference.test.ts new file mode 100644 index 00000000..3631ae75 --- /dev/null +++ b/packages/config/test/credential-reference.test.ts @@ -0,0 +1,88 @@ +import { describe, expect, it } from 'vitest'; + +import { credentialEnvReferenceName, resolveProviderCredential } from '../src/index.js'; + +const FIELD = 'custom_provider options.apiKey'; +const PROVIDER = 'mafia'; + +describe('provider credential environment reference', () => { + it('returns plaintext credentials untouched', () => { + expect( + resolveProviderCredential({ field: FIELD, provider: PROVIDER, value: 'sk-plaintext-value' }), + ).toBe('sk-plaintext-value'); + }); + + it('resolves a ${NAME} reference from the environment at read time', () => { + expect( + resolveProviderCredential({ + field: FIELD, + provider: PROVIDER, + value: ' ${MAFIA_API_KEY} ', + env: { MAFIA_API_KEY: 'sk-from-env' }, + }), + ).toBe('sk-from-env'); + }); + + it('picks up a rotated key without the config file changing', () => { + const env = { MAFIA_API_KEY: 'sk-rotation-one' }; + expect(resolveProviderCredential({ field: FIELD, provider: PROVIDER, value: '${MAFIA_API_KEY}', env })).toBe( + 'sk-rotation-one', + ); + env.MAFIA_API_KEY = 'sk-rotation-two'; + expect(resolveProviderCredential({ field: FIELD, provider: PROVIDER, value: '${MAFIA_API_KEY}', env })).toBe( + 'sk-rotation-two', + ); + }); + + it('names the unset variable instead of reporting a missing credential', () => { + expect(() => + resolveProviderCredential({ field: FIELD, provider: PROVIDER, value: '${MISSING_KEY}', env: {} }), + ).toThrow(/"mafia".*"MISSING_KEY"/u); + }); + + it('treats an unset variable that resolves to blanks as missing', () => { + expect(() => + resolveProviderCredential({ + field: FIELD, + provider: PROVIDER, + value: '${BLANK_KEY}', + env: { BLANK_KEY: ' ' }, + }), + ).toThrow(/"BLANK_KEY"/u); + }); + + it('explains the supported forms when a nested map reaches the resolver', () => { + // YAML reads `apiKey: {env: VAR}` into an object; the previous path called + // `.trim()` on it and surfaced a TypeError with no mention of any field. + expect(() => + resolveProviderCredential({ field: FIELD, provider: PROVIDER, value: { env: 'VAR' } }), + ).toThrow('${NAME}'); + }); + + it('treats a partial interpolation as plaintext rather than expanding it', () => { + expect( + resolveProviderCredential({ field: FIELD, provider: PROVIDER, value: 'Bearer ${TOKEN}' }), + ).toBe('Bearer ${TOKEN}'); + }); + + it('treats the unbraced shorthand as plaintext rather than expanding it', () => { + expect( + resolveProviderCredential({ field: FIELD, provider: PROVIDER, value: '$MAFIA_API_KEY' }), + ).toBe('$MAFIA_API_KEY'); + }); + + it('reports an absent credential as absent', () => { + expect(resolveProviderCredential({ field: FIELD, provider: PROVIDER, value: undefined })).toBeUndefined(); + expect(resolveProviderCredential({ field: FIELD, provider: PROVIDER, value: ' ' })).toBeUndefined(); + }); +}); + +describe('credentialEnvReferenceName', () => { + it('accepts only a whole-string ${NAME} reference', () => { + expect(credentialEnvReferenceName('${A_B}')).toBe('A_B'); + expect(credentialEnvReferenceName('${9BAD}')).toBeUndefined(); + expect(credentialEnvReferenceName('${}')).toBeUndefined(); + expect(credentialEnvReferenceName('prefix-${NAME}')).toBeUndefined(); + expect(credentialEnvReferenceName(42)).toBeUndefined(); + }); +}); diff --git a/packages/config/test/local-model-provider-write.test.ts b/packages/config/test/local-model-provider-write.test.ts new file mode 100644 index 00000000..8e4adad0 --- /dev/null +++ b/packages/config/test/local-model-provider-write.test.ts @@ -0,0 +1,441 @@ +import fs from 'node:fs'; +import { mkdtemp, rm } from 'node:fs/promises'; +import os, { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +import yaml from 'js-yaml'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { + compareAndSetLocalModelContext, + getConfigPath, + getConfig, + removeLocalProviderConfig, + replaceLocalManagedMinimaxProvider, + resetConfig, + resetGitDetect, + updateLocalByokConfig, + updateLocalModelSelection, +} from '../src/index.js'; + +let dataDir: string; + +function writeConfig(raw: Record): void { + fs.writeFileSync(getConfigPath(), yaml.dump(raw), 'utf-8'); + resetConfig(); +} + +function readConfig(): Record { + return yaml.load(fs.readFileSync(getConfigPath(), 'utf-8')) as Record; +} + +describe('local model-provider config writes', () => { + beforeEach(async () => { + dataDir = await mkdtemp(join(tmpdir(), 'model-provider-config-write-')); + vi.spyOn(os, 'homedir').mockReturnValue(dataDir); + vi.stubEnv('MINIMAX_DATA_DIR', ''); + vi.stubEnv('MAVIS_DATA_DIR', dataDir); + vi.stubEnv('__MAVIS_RUNTIME_DISABLE_GIT_AUTO_CONFIG', '1'); + resetGitDetect(); + resetConfig(); + }); + + afterEach(async () => { + vi.restoreAllMocks(); + vi.unstubAllEnvs(); + resetGitDetect(); + resetConfig(); + await rm(dataDir, { recursive: true, force: true }); + }); + + it('updates a numeric provider key without corrupting the persisted configuration', async () => { + fs.writeFileSync( + getConfigPath(), + `# provider settings +custom_provider: + 123: # numeric provider + options: + baseURL: https://fixture.example/v1 + apiKey: old-placeholder +`, + 'utf-8', + ); + resetConfig(); + + await updateLocalByokConfig((draft) => { + const providers = draft.custom_provider as Record< + string, + { options: Record } + >; + providers['123'].options = { + ...providers['123'].options, + apiKey: 'new-placeholder', + }; + }); + + expect(readConfig()).toEqual({ + custom_provider: { + '123': { + options: { + baseURL: 'https://fixture.example/v1', + apiKey: 'new-placeholder', + }, + }, + }, + }); + expect(fs.readFileSync(getConfigPath(), 'utf-8')).toContain('# numeric provider'); + resetConfig(); + expect(getConfig().custom_provider?.['123'].options?.apiKey).toBe('new-placeholder'); + }); + + it('preserves YAML 1.1 aliased headers when changing only the default model', async () => { + fs.writeFileSync( + getConfigPath(), + `%YAML 1.1 +--- +defaultModel: custom_provider:primary/old +custom_provider: + primary: + options: &options + baseURL: https://fixture.example/v1 + apiKey: placeholder + headers: + X-Feature: on # retain the header value + X-Binary: "0b10" + secondary: + options: *options +`, + 'utf-8', + ); + resetConfig(); + const expected = { + ...readConfig(), + defaultModel: 'custom_provider:primary/new', + }; + + await updateLocalModelSelection({ + modelKey: 'custom_provider:primary/new', + }); + + expect(readConfig()).toEqual(expected); + expect(fs.readFileSync(getConfigPath(), 'utf-8')).toContain('# retain the header value'); + resetConfig(); + expect(getConfig().custom_provider?.secondary.options?.headers?.['X-Feature']).toBe('on'); + }); + + it.each(['primary', 'secondary'])( + 'persists an isolated %s provider update with YAML aliases', + async (key) => { + fs.writeFileSync( + getConfigPath(), + `# local providers +custom_provider: + primary: + options: &options + baseURL: https://original.example/v1 + apiKey: shared-placeholder + secondary: + options: *options +`, + 'utf-8', + ); + resetConfig(); + const previous = readConfig(); + const expected = structuredClone(previous); + const providers = expected.custom_provider as Record< + string, + { options: Record } + >; + providers[key].options = { ...providers[key].options, apiKey: 'updated-placeholder' }; + + await updateLocalByokConfig((draft) => { + const tree = draft.custom_provider as Record }>; + tree[key].options = { ...tree[key].options, apiKey: 'updated-placeholder' }; + }); + + expect(readConfig()).toEqual(expected); + expect(fs.readFileSync(getConfigPath(), 'utf-8')).toContain('# local providers'); + }, + ); + + it('persists clearing a credential inherited through a YAML merge', async () => { + fs.writeFileSync( + getConfigPath(), + `defaults: &defaults + apiKey: inherited-placeholder + baseURL: https://original.example/v1 +custom_provider: + work: + options: + <<: *defaults + authMode: api-key +`, + 'utf-8', + ); + resetConfig(); + + await updateLocalByokConfig((draft) => { + const tree = draft.custom_provider as Record }>; + tree.work.options = { ...tree.work.options }; + delete tree.work.options.apiKey; + }); + + expect(readConfig()).toMatchObject({ + defaults: { apiKey: 'inherited-placeholder' }, + custom_provider: { + work: { options: { baseURL: 'https://original.example/v1', authMode: 'api-key' } }, + }, + }); + expect(getConfig().custom_provider?.work.options?.apiKey).toBeUndefined(); + expect( + (readConfig().custom_provider as Record }>).work + .options, + ).not.toHaveProperty('apiKey'); + }); + + it('persists the default model and variant without rewriting provider secrets', async () => { + writeConfig({ + provider: { work: { options: { apiKey: 'sk-secret' } } }, + defaultModel: 'work/old', + defaultModelVariant: 'high', + }); + + await updateLocalModelSelection({ modelKey: 'work/new', variant: 'low' }); + + expect(readConfig()).toEqual({ + provider: { work: { options: { apiKey: 'sk-secret' } } }, + defaultModel: 'work/new', + defaultModelVariant: 'low', + }); + }); + + it('restores complete default selection after restart and clears omitted overrides', async () => { + writeConfig({ provider: { minimax: { models: { 'MiniMax-M3.1': {} } } } }); + await updateLocalModelSelection({ + modelKey: 'minimax/MiniMax-M3.1', + contextLimit: 1_000_000, + thinking: { effort: 'max' }, + variant: 'thinking', + }); + resetConfig(); + expect(getConfig()).toMatchObject({ + defaultModel: 'minimax/MiniMax-M3.1', + defaultModelContextWindow: 1_000_000, + defaultModelThinking: { effort: 'max' }, + defaultModelVariant: 'thinking', + }); + expect(readConfig()).toMatchObject({ + defaultModelContextWindow: 1_000_000, + defaultModelThinking: { effort: 'max' }, + }); + await updateLocalModelSelection({ modelKey: 'minimax/MiniMax-M3.1' }); + resetConfig(); + expect(getConfig().defaultModelThinking).toBeUndefined(); + expect(getConfig().defaultModelContextWindow).toBeUndefined(); + expect(readConfig()).not.toHaveProperty('defaultModelThinking'); + }); + + it('keeps model defaults isolated between runtime profiles', async () => { + await updateLocalModelSelection({ + modelKey: 'minimax/MiniMax-M3.1', + contextLimit: 1_000_000, + thinking: { effort: 'max' }, + }); + const otherProfile = await mkdtemp(join(tmpdir(), 'model-provider-other-profile-')); + try { + vi.stubEnv('MAVIS_DATA_DIR', otherProfile); + resetConfig(); + expect(getConfig().defaultModelThinking).toBeUndefined(); + expect(getConfig().defaultModelContextWindow).toBeUndefined(); + await updateLocalModelSelection({ + modelKey: 'minimax/MiniMax-M3.1', + contextLimit: 512_000, + thinking: { effort: 'low' }, + }); + vi.stubEnv('MAVIS_DATA_DIR', dataDir); + resetConfig(); + expect(getConfig()).toMatchObject({ + defaultModelContextWindow: 1_000_000, + defaultModelThinking: { effort: 'max' }, + }); + } finally { + vi.stubEnv('MAVIS_DATA_DIR', dataDir); + resetConfig(); + await rm(otherProfile, { recursive: true, force: true }); + } + }); + + it('updates the default model and clears its stale variant in the BYOK transaction', async () => { + writeConfig({ + defaultModel: 'custom_provider:work/gpt-custom', + defaultModelVariant: 'max', + defaultModelThinking: { effort: 'max' }, + defaultModelContextWindow: 1_000_000, + }); + + await updateLocalByokConfig((draft) => { + draft.defaultModel = 'minimax/MiniMax-M3'; + draft.defaultModelVariant = undefined; + }); + + expect(readConfig()).toEqual({ defaultModel: 'minimax/MiniMax-M3' }); + }); + + it('removes the migrated OAuth provider while preserving unrelated provider entries', async () => { + writeConfig({ + provider: { + 'openai-codex': { options: { apiKey: 'legacy' } }, + minimax: { options: { apiKey: 'managed' } }, + }, + custom_provider: { + 'openai-codex': { kind: 'oauth', models: { 'gpt-5': { name: 'gpt-5' } } }, + }, + }); + + await removeLocalProviderConfig('openai-codex'); + + expect(readConfig()).toEqual({ + provider: { minimax: { options: { apiKey: 'managed' } } }, + custom_provider: { + 'openai-codex': { kind: 'oauth', models: { 'gpt-5': { name: 'gpt-5' } } }, + }, + }); + }); + + it('keeps the managed context selection across official snapshot refreshes', async () => { + writeConfig({ + provider: { + minimax: { + models: { + 'MiniMax-M4': { + name: 'MiniMax-M4', + limit: { context: 256_000, output: 64_000 }, + contextWindowOptions: [256_000, 768_000], + }, + }, + }, + }, + }); + + const result = await compareAndSetLocalModelContext( + { + providerId: 'minimax', + modelId: 'MiniMax-M4', + expectedContextLimit: 256_000, + contextLimit: 768_000, + }, + async () => true, + ); + + expect(result.updated).toBe(true); + expect(readConfig()).toMatchObject({ + provider: { + minimax: { models: { 'MiniMax-M4': { limit: { context: 256_000 } } } }, + }, + minimaxModelContextLimits: { 'MiniMax-M4': 768_000 }, + }); + expect(result.config.provider.minimax?.models?.['MiniMax-M4']?.limit?.context).toBe(768_000); + + const refreshed = await replaceLocalManagedMinimaxProvider({ + model_order: ['MiniMax-M4'], + models: { + 'MiniMax-M4': { + name: 'MiniMax-M4', + limit: { context: 256_000, output: 64_000 }, + contextWindowOptions: [256_000, 768_000], + }, + }, + }); + + expect(readConfig()).toMatchObject({ + provider: { + minimax: { + model_order: ['MiniMax-M4'], + models: { 'MiniMax-M4': { limit: { context: 256_000 } } }, + }, + }, + minimaxModelContextLimits: { 'MiniMax-M4': 768_000 }, + }); + expect(refreshed.config.provider.minimax?.models?.['MiniMax-M4']?.limit?.context).toBe(768_000); + + const restricted = await replaceLocalManagedMinimaxProvider({ + models: { + 'MiniMax-M4': { + name: 'MiniMax-M4', + limit: { context: 256_000, output: 64_000 }, + contextWindowOptions: [256_000], + }, + }, + }); + + expect(restricted.config.provider.minimax?.models?.['MiniMax-M4']?.limit?.context).toBe( + 256_000, + ); + }); + + it('persists a BYOK context override without changing the managed snapshot', async () => { + writeConfig({ + provider: { + minimax: { + models: { + 'MiniMax-M3': { + limit: { context: 512_000 }, + contextWindowOptions: [512_000, 1_000_000], + }, + }, + }, + }, + minimax_api: { apiKey: 'sk-user-key' }, + }); + + const result = await compareAndSetLocalModelContext( + { + providerId: 'minimax_api', + modelId: 'MiniMax-M3', + expectedContextLimit: 512_000, + contextLimit: 1_000_000, + }, + async () => true, + ); + + expect(result.updated).toBe(true); + expect(readConfig()).toMatchObject({ + provider: { + minimax: { models: { 'MiniMax-M3': { limit: { context: 512_000 } } } }, + }, + minimax_api: { + apiKey: 'sk-user-key', + modelContextLimits: { 'MiniMax-M3': 1_000_000 }, + }, + }); + + await replaceLocalManagedMinimaxProvider({ + models: { 'Remote-Only-M4': { limit: { context: 256_000 } } }, + }); + + expect(readConfig()).toMatchObject({ + provider: { minimax: { models: { 'Remote-Only-M4': { limit: { context: 256_000 } } } } }, + minimax_api: { + apiKey: 'sk-user-key', + modelContextLimits: { 'MiniMax-M3': 1_000_000 }, + }, + }); + }); + + it('does not write a BYOK context override after a stale comparison', async () => { + writeConfig({ minimax_api: { apiKey: 'sk-user-key' } }); + + const result = await compareAndSetLocalModelContext( + { + providerId: 'minimax_api', + modelId: 'MiniMax-M3', + expectedContextLimit: 1_000_000, + contextLimit: 512_000, + }, + async () => true, + ); + + expect(result.updated).toBe(false); + expect(readConfig()).toEqual({ minimax_api: { apiKey: 'sk-user-key' } }); + }); +}); diff --git a/packages/local-runtime-v2/assets/agents/mavis/skills/minimax-code-product/SKILL.md b/packages/local-runtime-v2/assets/agents/mavis/skills/minimax-code-product/SKILL.md index 884c2dd1..0d90ab96 100644 --- a/packages/local-runtime-v2/assets/agents/mavis/skills/minimax-code-product/SKILL.md +++ b/packages/local-runtime-v2/assets/agents/mavis/skills/minimax-code-product/SKILL.md @@ -104,8 +104,9 @@ global entitlement without saying so. - workflows → `references/workflows.md` - Agent/Session/Memory/Team → `references/agents.md` - Skill/Plugin/MCP → `references/extensions.md` - - plans/models/media → `references/account-models.md` -5. Use only the current runtime region's sources described above. + - plans/models/media, BYOK API keys, or environment-variable credential configuration → `references/account-models.md` +5. For BYOK environment variables, follow that reference for configuration syntax, process inheritance, and safe checks. Never request or echo the key. +6. Use only the current runtime region's sources described above. ## Response rules diff --git a/packages/local-runtime-v2/assets/agents/mavis/skills/minimax-code-product/references/account-models.md b/packages/local-runtime-v2/assets/agents/mavis/skills/minimax-code-product/references/account-models.md index a3e61635..7a0a844c 100644 --- a/packages/local-runtime-v2/assets/agents/mavis/skills/minimax-code-product/references/account-models.md +++ b/packages/local-runtime-v2/assets/agents/mavis/skills/minimax-code-product/references/account-models.md @@ -9,6 +9,37 @@ Use this reference for MiniMax Code or MiniMax Open Platform accounts, Token Pla - Media support and credit eligibility are separate questions: a model may exist in the catalog without being enabled for the user's product, plan, or requested operation. - The Agent cannot change balance, entitlement, quota, permissions, or server-side validation. +## BYOK environment-variable credentials + +For an API key supplied through the environment, locate the active profile's +`config.yaml` and change the existing credential field to a whole `${NAME}` +reference. Preserve its API format, endpoint, models, and other options: + +```yaml +custom_provider: + work: + options: + apiKey: '${WORK_API_KEY}' +minimax_api: + apiKey: '${MINIMAX_API_KEY}' +``` + +- Supported credential fields are `custom_provider..options.apiKey` and + `minimax_api.apiKey`. Variable names start with a letter or underscore and may + then contain letters, digits, or underscores. +- `$NAME`, `Bearer ${NAME}`, and partial interpolation remain literal strings; + `{env: NAME}` is an invalid credential type. +- Have the user set the variable locally in the shell that launches `mcode`. + Restart TUI, exec, or ACP after changing its launch environment. An already + running process keeps its existing environment. +- Missing or blank variables produce an error naming the provider, field, and + variable. Check only whether the variable is set and nonempty; never echo it + or ask the user to paste a secret into the conversation. +- Model requests, saved-provider discovery and connection tests use the resolved + key. Saving settings preserves the reference text in the configuration. +- `provider add --api-key-env` reads and saves a value; edit `config.yaml` to keep + a reference. Do not assume custom headers or Base URLs expand variables. + ## Official source discovery Use only the current region's sources: diff --git a/packages/local-runtime-v2/src/service/model-system/catalog/config-fingerprint.ts b/packages/local-runtime-v2/src/service/model-system/catalog/config-fingerprint.ts index 28f6ef32..d75b0e05 100644 --- a/packages/local-runtime-v2/src/service/model-system/catalog/config-fingerprint.ts +++ b/packages/local-runtime-v2/src/service/model-system/catalog/config-fingerprint.ts @@ -1,3 +1,5 @@ +import { probeProviderCredential } from '@mavis/config'; + import type { LocalModelConfig, LocalRuntimeConfig } from '../contracts.js'; import { MINIMAX_API_PROVIDER_ID, parseProviderId } from '../resolution/model-key.js'; import { minimaxApiBaseUrl, minimaxApiModels } from './minimax-api.js'; @@ -38,7 +40,11 @@ function minimaxModelTestStatus( cache: ModelCacheData, modelId: string, ): ModelCacheStatusEntry | undefined { - const apiKey = config.minimax_api?.apiKey?.trim(); + const apiKey = probeProviderCredential({ + field: 'minimax_api.apiKey', + provider: 'minimax_api', + value: config.minimax_api?.apiKey, + }).secret; const model = minimaxApiModels(config)[modelId]; if (!apiKey || !model) return undefined; const fingerprint = modelConnectionTestFingerprint( @@ -62,7 +68,11 @@ function customModelTestStatus(input: { }): ModelCacheStatusEntry | undefined { const { config, cache, providerId, modelId } = input; const provider = config.custom_provider?.[input.providerKey]; - const apiKey = provider?.options?.apiKey?.trim(); + const apiKey = probeProviderCredential({ + field: 'options.apiKey', + provider: providerId, + value: provider?.options?.apiKey, + }).secret; const baseUrl = provider?.options?.baseURL?.trim(); const model = provider?.models?.[modelId]; if (!provider || provider.enabled === false || !apiKey || !baseUrl || !model) return undefined; diff --git a/packages/local-runtime-v2/src/service/model-system/catalog/list-models.ts b/packages/local-runtime-v2/src/service/model-system/catalog/list-models.ts index 331b4cd0..f8725109 100644 --- a/packages/local-runtime-v2/src/service/model-system/catalog/list-models.ts +++ b/packages/local-runtime-v2/src/service/model-system/catalog/list-models.ts @@ -1,4 +1,9 @@ -import { getRuntimePresetKey, listRouteModelIds, resolveProviderAuthMode } from '@mavis/config'; +import { + getRuntimePresetKey, + listRouteModelIds, + probeProviderCredential, + resolveProviderAuthMode, +} from '@mavis/config'; import type { LocalCustomProviderConfig, @@ -70,7 +75,11 @@ export function builtinProviderKind( } export function hasMinimaxApiKey(config: LocalRuntimeConfig): boolean { - return Boolean(config.minimax_api?.apiKey?.trim()); + return probeProviderCredential({ + field: 'minimax_api.apiKey', + provider: 'minimax_api', + value: config.minimax_api?.apiKey, + }).configured; } /** Models the builtin provider can route under the active runtime preset. */ diff --git a/packages/local-runtime-v2/src/service/model-system/catalog/provider-views.ts b/packages/local-runtime-v2/src/service/model-system/catalog/provider-views.ts index b18b7648..8f4a4ce2 100644 --- a/packages/local-runtime-v2/src/service/model-system/catalog/provider-views.ts +++ b/packages/local-runtime-v2/src/service/model-system/catalog/provider-views.ts @@ -16,6 +16,8 @@ import type { ModelProviderSource, ModelProviderView, } from '../contracts.js'; +import { probeProviderCredential } from '@mavis/config'; + import { modelConnectionTestFingerprint } from './config-fingerprint.js'; import { MINIMAX_API_FORMAT, @@ -44,14 +46,37 @@ import { export type { ModelProviderView } from '../contracts.js'; +/** + * Credential state for a provider view. An environment reference counts as + * configured even when the variable is unset right now, so the user sees what + * is configured rather than a provider that flickers to keyless between runs. + * The resolved value is masked like any other key; an unresolved reference has + * no masked form to show, because masking the literal `${VAR}` would imply a + * secret that is not there. + */ +function credentialViewFields( + options: { readonly apiKey?: string } | undefined, + providerId: string, +): { hasApiKey: boolean; maskedApiKey?: string } { + const probe = probeProviderCredential({ + field: 'options.apiKey', + provider: providerId, + value: options?.apiKey, + }); + if (!probe.configured) return { hasApiKey: false }; + return probe.secret + ? { hasApiKey: true, maskedApiKey: maskSecret(probe.secret) } + : { hasApiKey: true }; +} + export function buildBuiltinProviderView( config: LocalRuntimeConfig, cache: ModelCacheData, providerId: string, provider: LocalProviderConfig, ): ModelProviderView { - const apiKey = provider.options?.apiKey?.trim(); const providerName = provider.name ?? providerId; + const credential = credentialViewFields(provider.options, providerId); const providerKind = builtinProviderKind(config, providerId, provider); const models = providerId === 'minimax' && config.minimaxModelSource === 'minimax_api_key' @@ -64,8 +89,7 @@ export function buildBuiltinProviderView( kind: providerKind, enabled: true, ...(provider.options?.baseURL ? { baseUrl: provider.options.baseURL } : {}), - hasApiKey: Boolean(apiKey), - ...(apiKey ? { maskedApiKey: maskSecret(apiKey) } : {}), + ...credential, models: providerModelEntries(config, cache, { providerId, models, @@ -83,7 +107,7 @@ export function buildMinimaxProviderView( config: LocalRuntimeConfig, cache: ModelCacheData, ): ModelProviderView { - const apiKey = config.minimax_api?.apiKey?.trim(); + const credential = credentialViewFields(config.minimax_api, MINIMAX_API_PROVIDER_ID); return { providerId: MINIMAX_API_PROVIDER_ID, name: MINIMAX_API_PROVIDER_NAME, @@ -92,8 +116,7 @@ export function buildMinimaxProviderView( enabled: true, baseUrl: minimaxApiBaseUrl(config), apiFormat: MINIMAX_API_FORMAT, - hasApiKey: Boolean(apiKey), - ...(apiKey ? { maskedApiKey: maskSecret(apiKey) } : {}), + ...credential, models: providerModelEntries(config, cache, { providerId: MINIMAX_API_PROVIDER_ID, models: minimaxApiModels(config), @@ -112,8 +135,8 @@ export function buildCustomProviderView( provider: LocalCustomProviderConfig, ): ModelProviderView { const providerId = `${CUSTOM_PROVIDER_ID_PREFIX}${providerKey}`; - const apiKey = provider.options?.apiKey?.trim(); const providerName = provider.name ?? providerKey; + const credential = credentialViewFields(provider.options, providerId); const providerKind = customProviderKind(provider); return { providerId, @@ -123,8 +146,7 @@ export function buildCustomProviderView( enabled: provider.enabled !== false, ...(provider.options?.baseURL ? { baseUrl: provider.options.baseURL } : {}), ...(provider.api ? { apiFormat: provider.api } : {}), - hasApiKey: Boolean(apiKey), - ...(apiKey ? { maskedApiKey: maskSecret(apiKey) } : {}), + ...credential, ...(provider.options?.headers ? { headerNames: Object.keys(provider.options.headers).sort() } : {}), @@ -211,7 +233,11 @@ function customModelFingerprint( modelId: string, model: LocalModelConfig, ): string | undefined { - const apiKey = provider.options?.apiKey?.trim(); + const apiKey = probeProviderCredential({ + field: 'options.apiKey', + provider: provider.name ?? 'custom_provider', + value: provider.options?.apiKey, + }).secret; const baseUrl = provider.options?.baseURL?.trim(); if (!apiKey || !baseUrl) return undefined; const api = (provider.api?.trim() || 'anthropic-messages') as ModelProviderApi; diff --git a/packages/local-runtime-v2/src/service/model-system/management/service-context.test.ts b/packages/local-runtime-v2/src/service/model-system/management/service-context.test.ts index 587494eb..c39b8f01 100644 --- a/packages/local-runtime-v2/src/service/model-system/management/service-context.test.ts +++ b/packages/local-runtime-v2/src/service/model-system/management/service-context.test.ts @@ -103,10 +103,43 @@ beforeEach(async () => { }); afterEach(async () => { + vi.unstubAllEnvs(); await rm(dataDir, { recursive: true, force: true }); }); describe('LocalModelProviderService context', () => { + + it('uses the environment credential for tests and invalidates status after rotation', async () => { + vi.stubEnv('WORK_API_KEY', CUSTOM_KEY); + const reference = '${WORK_API_KEY}'; + const harness = createHarness(); + const provider = await harness.service.createUserProvider({ + name: 'Work', + baseUrl: 'https://api.example.com/v1', + apiKey: reference, + apiFormat: 'openai-completions', + models: [{ modelId: 'model-a' }], + }); + + await harness.service.testModel(provider.providerId, 'model-a'); + expect(harness.testCalls[0]?.target.apiKey).toBe(CUSTOM_KEY); + expect(harness.config.custom_provider?.work?.options?.apiKey).toBe(reference); + expect(harness.service.listUserProviders()[0]?.models[0]?.status?.state).toBe('available'); + expect(JSON.stringify(harness.service.listUserProviders())).not.toContain(CUSTOM_KEY); + + vi.stubEnv('WORK_API_KEY', 'fixture-rotated-key'); + expect(harness.service.listUserProviders()[0]?.models[0]?.status).toBeUndefined(); + await harness.service.testModel(provider.providerId, 'model-a'); + expect(harness.testCalls[1]?.target.apiKey).toBe('fixture-rotated-key'); + expect(harness.service.listUserProviders()[0]?.models[0]?.status?.state).toBe('available'); + + vi.stubEnv('WORK_API_KEY', ''); + expect(harness.service.listUserProviders()[0]).toMatchObject({ hasApiKey: true }); + expect(harness.service.listUserProviders()[0]?.maskedApiKey).toBeUndefined(); + await expect(harness.service.testModel(provider.providerId, 'model-a')).rejects.toThrow('WORK_API_KEY'); + expect(harness.testCalls).toHaveLength(2); + }); + it('stores MiniMax credentials while exposing only their masked view', async () => { const harness = createHarness(); diff --git a/packages/local-runtime-v2/src/service/model-system/management/service-context.ts b/packages/local-runtime-v2/src/service/model-system/management/service-context.ts index b6f39216..9a444f64 100644 --- a/packages/local-runtime-v2/src/service/model-system/management/service-context.ts +++ b/packages/local-runtime-v2/src/service/model-system/management/service-context.ts @@ -1,3 +1,5 @@ +import { resolveProviderCredential } from '@mavis/config'; + import { CUSTOM_PROVIDER_ID_PREFIX, MINIMAX_API_PROVIDER_ID, @@ -27,7 +29,8 @@ import { import { mergeModelsFromInputs, normalizeApiFormat, - normalizeApiKeyUpdate, + applyProviderCredentialUpdate, + normalizeProviderCredentialUpdate, normalizeHeaderNames, normalizeHeaders, removeHeaderCaseInsensitive, @@ -85,13 +88,15 @@ function candidateOptions( input: UserModelProviderCandidateView, baseUrl: string, ): NonNullable { - const apiKeyUpdate = normalizeApiKeyUpdate(input.apiKey); + const apiKeyUpdate = normalizeProviderCredentialUpdate(input); const options = { ...(current?.options ?? {}), baseURL: baseUrl, authMode: 'api-key' as const }; + // A new provider needs a credential it can actually use; an existing one + // keeps whatever it already has when the request carries none. `keep` on an + // edit is a no-op, not a missing-key error. if (!current && apiKeyUpdate.kind !== 'set') { throw new LocalModelProviderError(400, 'API key must not be empty', 'INVALID_API_KEY'); } - if (apiKeyUpdate.kind === 'set') options.apiKey = apiKeyUpdate.apiKey; - if (apiKeyUpdate.kind === 'clear') delete options.apiKey; + applyProviderCredentialUpdate(options, apiKeyUpdate); applyCandidateHeaderUpdates(options, current, input); return options; } @@ -190,8 +195,19 @@ function connectionTestFailureMessage(result: ModelConnectionTestResult): string function requireCustomProviderCredentials( provider: LocalCustomProviderConfig | undefined, apiKeyOverride: string | undefined, + providerId: string, ): { apiKey: string; baseUrl: string } { - const apiKey = apiKeyOverride?.trim() || provider?.options?.apiKey?.trim(); + // A request about to leave the machine must surface the resolver's own + // diagnosis. The probe is the view's tool: it reports a broken reference as + // "configured" and returns no secret, which would downgrade "environment + // variable MAFIA_KEY is not set" to a generic missing key. + const apiKey = + apiKeyOverride?.trim() || + resolveProviderCredential({ + field: 'options.apiKey', + provider: providerId, + value: provider?.options?.apiKey, + }); if (!apiKey) { throw new LocalModelProviderError(400, 'Provider API key is not configured', 'NO_API_KEY'); } @@ -294,7 +310,13 @@ export class ModelProviderServiceContext { } discoveryTargetForProvider(provider: LocalCustomProviderConfig): ModelDiscoveryTarget { - const apiKey = provider.options?.apiKey?.trim(); + // Discovery talks to the upstream, so it needs the same credential the turn + // path uses. Reading the raw field sent a `${VAR}` literal as the key. + const apiKey = resolveProviderCredential({ + field: 'options.apiKey', + provider: provider.name ?? 'custom_provider', + value: provider.options?.apiKey, + }); if (!apiKey) { throw new LocalModelProviderError(400, 'Provider API key is not configured', 'NO_API_KEY'); } @@ -375,7 +397,11 @@ export class ModelProviderServiceContext { ? customProviderKeyFromId(providerId) : this.requireExistingProviderKey(providerId); const provider = options.customProviderOverride ?? config.custom_provider?.[providerKey]; - const { apiKey, baseUrl } = requireCustomProviderCredentials(provider, options.apiKeyOverride); + const { apiKey, baseUrl } = requireCustomProviderCredentials( + provider, + options.apiKeyOverride, + `${CUSTOM_PROVIDER_ID_PREFIX}${providerKey}`, + ); const chosenModelId = requireCustomProviderModelId(provider, modelId); const fullProviderId = `${CUSTOM_PROVIDER_ID_PREFIX}${providerKey}`; const api = normalizeApiFormat(provider?.api) ?? 'anthropic-messages'; @@ -404,7 +430,13 @@ export class ModelProviderServiceContext { modelId: string | undefined, options: ResolveMinimaxTestTargetOptions = {}, ): ResolvedConnectionTestTarget { - const apiKey = options.apiKeyOverride?.trim() || config.minimax_api?.apiKey?.trim(); + const apiKey = + options.apiKeyOverride?.trim() || + resolveProviderCredential({ + field: 'minimax_api.apiKey', + provider: 'minimax_api', + value: config.minimax_api?.apiKey, + }); if (!apiKey) { throw new LocalModelProviderError(400, 'MiniMax API key is not configured', 'NO_API_KEY'); } diff --git a/packages/local-runtime-v2/src/service/model-system/management/service-custom-provider-operations.ts b/packages/local-runtime-v2/src/service/model-system/management/service-custom-provider-operations.ts index 848d3998..f5570056 100644 --- a/packages/local-runtime-v2/src/service/model-system/management/service-custom-provider-operations.ts +++ b/packages/local-runtime-v2/src/service/model-system/management/service-custom-provider-operations.ts @@ -19,7 +19,7 @@ import { mergeModelsFromInputs, modelsFromInputs, normalizeApiFormat, - normalizeApiKeyUpdate, + normalizeProviderCredentialUpdate, normalizeHeaderNames, normalizeHeaders, removeHeaderCaseInsensitive, @@ -39,7 +39,7 @@ interface UserProviderUpdateInput { } interface PreparedUserProviderUpdate { - apiKeyUpdate: ReturnType; + apiKeyUpdate: ReturnType; baseUrl?: string; apiFormat?: ReturnType; headers?: Record; @@ -115,7 +115,7 @@ export async function updateUserProvider( const providerKey = context.requireExistingProviderKey(input.providerId); // Tri-state api_key: absent keeps the stored key, empty string clears it, // masked placeholders are rejected so a sanitized read can't be written back. - const apiKeyUpdate = normalizeApiKeyUpdate(input.apiKey); + const apiKeyUpdate = normalizeProviderCredentialUpdate({ apiKey: input.apiKey }); const baseUrl = input.baseUrl?.trim(); if (input.baseUrl !== undefined && !baseUrl) { throw new LocalModelProviderError(400, 'base_url must not be empty', 'VALIDATION_ERROR'); diff --git a/packages/local-runtime-v2/src/service/model-system/management/service-input.test.ts b/packages/local-runtime-v2/src/service/model-system/management/service-input.test.ts index b1c6ed9b..b466980c 100644 --- a/packages/local-runtime-v2/src/service/model-system/management/service-input.test.ts +++ b/packages/local-runtime-v2/src/service/model-system/management/service-input.test.ts @@ -2,13 +2,14 @@ import { describe, expect, it } from 'vitest'; import { LocalModelProviderError, type LocalModelConfig } from '../contracts.js'; import { + applyProviderCredentialUpdate, assertValidRawApiKey, mergeModelsFromInputs, modelsFromInputs, normalizeApiFormat, - normalizeApiKeyUpdate, normalizeHeaderNames, normalizeHeaders, + normalizeProviderCredentialUpdate, removeHeaderCaseInsensitive, } from './service-input.js'; @@ -44,9 +45,12 @@ describe('model provider input normalization', () => { ); expect(() => assertValidRawApiKey('sk-a****z')).toThrow(LocalModelProviderError); expect(assertValidRawApiKey(' sk-raw ')).toBe('sk-raw'); - expect(normalizeApiKeyUpdate(undefined)).toEqual({ kind: 'keep' }); - expect(normalizeApiKeyUpdate(' ')).toEqual({ kind: 'clear' }); - expect(normalizeApiKeyUpdate(' sk-next ')).toEqual({ kind: 'set', apiKey: 'sk-next' }); + expect(normalizeProviderCredentialUpdate({})).toEqual({ kind: 'keep' }); + expect(normalizeProviderCredentialUpdate({ apiKey: ' ' })).toEqual({ kind: 'clear' }); + expect(normalizeProviderCredentialUpdate({ apiKey: ' sk-next ' })).toEqual({ + kind: 'set', + apiKey: 'sk-next', + }); expect(normalizeApiFormat(undefined)).toBeUndefined(); expect(normalizeApiFormat(' ')).toBeUndefined(); @@ -244,3 +248,19 @@ describe('model provider input normalization', () => { ); }); }); + +describe('provider credential application', () => { + it('writes the resolved key and clears it', () => { + const options: Record = {}; + applyProviderCredentialUpdate(options, { kind: 'set', apiKey: 'sk-new' }); + expect(options).toEqual({ apiKey: 'sk-new' }); + applyProviderCredentialUpdate(options, { kind: 'clear' }); + expect(options).toEqual({}); + }); + + it('leaves the stored key untouched on keep', () => { + const options: Record = { apiKey: 'sk-stored' }; + applyProviderCredentialUpdate(options, { kind: 'keep' }); + expect(options).toEqual({ apiKey: 'sk-stored' }); + }); +}); diff --git a/packages/local-runtime-v2/src/service/model-system/management/service-input.ts b/packages/local-runtime-v2/src/service/model-system/management/service-input.ts index e987cc40..67a17865 100644 --- a/packages/local-runtime-v2/src/service/model-system/management/service-input.ts +++ b/packages/local-runtime-v2/src/service/model-system/management/service-input.ts @@ -8,7 +8,8 @@ import { const MASKED_KEY_MARKER = '****'; -export function assertValidRawApiKey(apiKey: string): string { +/** Absent and blank both fail: a new provider needs one usable credential source. */ +export function assertValidRawApiKey(apiKey: string | undefined): string { const trimmed = typeof apiKey === 'string' ? apiKey.trim() : ''; if (!trimmed) { throw new LocalModelProviderError(400, 'API key must not be empty', 'INVALID_API_KEY'); @@ -23,12 +24,33 @@ export function assertValidRawApiKey(apiKey: string): string { return trimmed; } -export function normalizeApiKeyUpdate( - apiKey: string | undefined, -): { kind: 'keep' } | { kind: 'clear' } | { kind: 'set'; apiKey: string } { - if (apiKey === undefined) return { kind: 'keep' }; - if (apiKey.trim() === '') return { kind: 'clear' }; - return { kind: 'set', apiKey: assertValidRawApiKey(apiKey) }; +/** + * Credential source states a provider write can produce. `keep` on an existing + * provider means "leave the stored key alone"; `clear` removes it. + */ +export type ProviderCredentialUpdate = + | { readonly kind: 'keep' } + | { readonly kind: 'clear' } + | { readonly kind: 'set'; readonly apiKey: string }; + +export function normalizeProviderCredentialUpdate(input: { + readonly apiKey?: string; +}): ProviderCredentialUpdate { + if (input.apiKey === undefined) return { kind: 'keep' }; + if (input.apiKey.trim() === '') return { kind: 'clear' }; + return { kind: 'set', apiKey: assertValidRawApiKey(input.apiKey) }; +} + +/** Writes the resolved credential source onto provider options. */ +export function applyProviderCredentialUpdate( + options: Record, + update: ProviderCredentialUpdate, +): void { + if (update.kind === 'set') { + options.apiKey = update.apiKey; + return; + } + if (update.kind === 'clear') delete options.apiKey; } export function normalizeApiFormat( diff --git a/packages/local-runtime-v2/src/service/model-system/management/service-minimax-operations.ts b/packages/local-runtime-v2/src/service/model-system/management/service-minimax-operations.ts index dee37daf..4f516933 100644 --- a/packages/local-runtime-v2/src/service/model-system/management/service-minimax-operations.ts +++ b/packages/local-runtime-v2/src/service/model-system/management/service-minimax-operations.ts @@ -1,3 +1,4 @@ +import { probeProviderCredential } from '@mavis/config'; import { maskSecret } from '../secret.js'; import { MANAGED_MINIMAX_PROVIDER_ID, MINIMAX_API_PROVIDER_ID } from '../identity.js'; import type { LocalModelConfig, ModelContextUpdateOutcome } from '../contracts.js'; @@ -38,7 +39,11 @@ export function getMinimaxApiKeyStatus(context: ModelProviderServiceContext): { maskedApiKey?: string; cachedStatus?: ModelCacheStatusView; } { - const apiKey = context.deps.configGetter().minimax_api?.apiKey?.trim(); + const apiKey = probeProviderCredential({ + field: 'minimax_api.apiKey', + provider: 'minimax_api', + value: context.deps.configGetter().minimax_api?.apiKey, + }).secret; if (!apiKey) return { hasApiKey: false }; const cache = context.deps.cache.load(); const target = context.resolveTestTarget(MINIMAX_API_PROVIDER_ID, undefined); diff --git a/packages/local-runtime-v2/src/service/model-system/resolution/byok-error-attribution.ts b/packages/local-runtime-v2/src/service/model-system/resolution/byok-error-attribution.ts index c07ad1d6..28705f29 100644 --- a/packages/local-runtime-v2/src/service/model-system/resolution/byok-error-attribution.ts +++ b/packages/local-runtime-v2/src/service/model-system/resolution/byok-error-attribution.ts @@ -6,6 +6,7 @@ import { type AssistantMessageEventStream, } from '@earendil-works/pi-ai'; import { classifyLLMErrorToCode, LLM_ERROR_CODES } from '@mavis/shared/llm-error-classifier'; +import { redactSecretText } from '@mavis/shared/logging/redact-log-secret'; const BYOK_ERROR_PREFIX = 'BYOK upstream error'; const MAX_ERROR_MESSAGE_LENGTH = 1_200; @@ -43,12 +44,7 @@ function formatByokErrorMessage(raw: unknown, providerId: string): string { } function redactByokErrorMessage(message: string): string { - return message - .slice(0, MAX_ERROR_MESSAGE_LENGTH) - .replace(/\bsk-[A-Za-z0-9._-]{8,}\b/gu, 'sk-***') - .replace(/(authorization\s*[:=]\s*bearer\s+)[^\s"',}]+/giu, '$1***') - .replace(/((?:api[_-]?key|x-api-key)\s*[:=]\s*)[^\s"',}]+/giu, '$1***') - .replace(/\b(Bearer\s+)[A-Za-z0-9._~+/=-]{16,}/giu, '$1***'); + return redactSecretText(message).slice(0, MAX_ERROR_MESSAGE_LENGTH); } function wrapStream( diff --git a/packages/local-runtime-v2/src/service/model-system/resolution/local-model-resolver.ts b/packages/local-runtime-v2/src/service/model-system/resolution/local-model-resolver.ts index a58c2bc7..e6c7e90b 100644 --- a/packages/local-runtime-v2/src/service/model-system/resolution/local-model-resolver.ts +++ b/packages/local-runtime-v2/src/service/model-system/resolution/local-model-resolver.ts @@ -9,6 +9,7 @@ import type { StreamFn, ThinkingLevel as PiThinkingLevel } from '@earendil-works import { isFirstPartyMinimaxMessagesRoute, resolveProviderAuthMode, + resolveProviderCredential, type ProviderAuthMode, type ProviderAuthModeSource, } from '@mavis/config'; @@ -727,7 +728,7 @@ export function resolveLocalProviderCredentials( const token = authContext?.accessToken?.trim(); const headers = resolveCredentialHeaders(optionHeaders, modelHeaders, token, auth.authMode); return { - apiKey: resolveCredentialApiKey(modelRef, options, auth.authMode), + apiKey: resolveCredentialApiKey(provider, modelRef, options, auth.authMode), baseUrl: resolveCredentialBaseUrl(configuredBaseUrl, auth.managedBaseURL), ...(headers ? { headers } : {}), ...(options ? { rawProviderOptions: options } : {}), @@ -739,11 +740,22 @@ export function resolveLocalProviderCredentials( } function resolveCredentialApiKey( + provider: string, modelRef: IModelRef, options: LocalProviderOptions | undefined, authMode: ProviderAuthMode, ): string | undefined { - const configured = modelRef.api_key?.trim() || options?.apiKey?.trim(); + const configured = + resolveProviderCredential({ + field: 'api_key', + provider, + value: modelRef.api_key, + }) ?? + resolveProviderCredential({ + field: 'options.apiKey', + provider, + value: options?.apiKey, + }); if (configured) return configured; return authMode === 'managed-login' ? MANAGED_PROVIDER_API_KEY_PLACEHOLDER : undefined; } diff --git a/packages/local-runtime-v2/src/service/model-system/resolution/model-resolver-byok.test.ts b/packages/local-runtime-v2/src/service/model-system/resolution/model-resolver-byok.test.ts index 38cb6b7f..22463a26 100644 --- a/packages/local-runtime-v2/src/service/model-system/resolution/model-resolver-byok.test.ts +++ b/packages/local-runtime-v2/src/service/model-system/resolution/model-resolver-byok.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from 'vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; import { firstBuiltinModel, @@ -24,6 +24,8 @@ const MESSAGES_API_COMPAT_PATH = String.fromCodePoint( 0x63, ); +afterEach(() => vi.unstubAllEnvs()); + describe('MiniMax API BYOK planning', () => { it('returns absent when the source is not configured and fails closed without a key', () => { expect( @@ -95,6 +97,47 @@ describe('MiniMax API BYOK planning', () => { }); describe('custom BYOK planning', () => { + const planningBase = { provider: 'custom_provider:work', providerKey: 'work', modelId: 'model' }; + + it('resolves an environment-variable credential reference without the file holding the key', () => { + // Stub the variable instead of depending on the shell: a clean CI worker has + // none of these set, and an inherited value from a developer would make the + // assertion pass for the wrong reason. + vi.stubEnv('WORK_API_KEY', 'sk-from-env'); + const apiKeyReference = ['$', '{WORK_API_KEY}'].join(''); + const plan = planCustomProviderResolution({ + ...planningBase, + byok: { + custom_provider: { + work: { + options: { apiKey: apiKeyReference, baseURL: 'https://api.example.com/v1' }, + models: { model: {} }, + }, + }, + }, + }); + expect(plan?.apiKey).toBe('sk-from-env'); + }); + + it('names the provider and field when a nested map reaches the credential reader', () => { + // An unquoted `apiKey: {env: VAR}` is valid YAML, so it reaches the reader as an object. + // The cast is the point: the declared type says string, the file can still say otherwise. + const nestedEnvForm = { env: 'WORK_API_KEY' } as unknown as string; + expect(() => + planCustomProviderResolution({ + ...planningBase, + byok: { + custom_provider: { + work: { + options: { apiKey: nestedEnvForm, baseURL: 'https://api.example.com/v1' }, + models: { model: {} }, + }, + }, + }, + }), + ).toThrow('custom_provider:work'); + }); + it('returns absent for missing, disabled, and unknown model configurations', () => { const base = { provider: 'custom_provider:work', providerKey: 'work', modelId: 'model' }; expect(planCustomProviderResolution({ ...base, byok: undefined })).toBeUndefined(); diff --git a/packages/local-runtime-v2/src/service/model-system/resolution/model-resolver-byok.ts b/packages/local-runtime-v2/src/service/model-system/resolution/model-resolver-byok.ts index 3da72fb2..09727de7 100644 --- a/packages/local-runtime-v2/src/service/model-system/resolution/model-resolver-byok.ts +++ b/packages/local-runtime-v2/src/service/model-system/resolution/model-resolver-byok.ts @@ -1,5 +1,9 @@ import type { Api } from '@earendil-works/pi-ai'; -import { MINIMAX_API_MODEL_CATALOG, getRuntimeRegion } from '@mavis/config'; +import { + resolveProviderCredential, + MINIMAX_API_MODEL_CATALOG, + getRuntimeRegion, +} from '@mavis/config'; import type { LocalByokProviderConfig, @@ -46,7 +50,11 @@ export function planMinimaxApiResolution(input: { }): ByokResolutionPlan | undefined { const config = input.byok?.minimax_api; if (!config) return undefined; - const apiKey = config.apiKey?.trim(); + const apiKey = resolveProviderCredential({ + field: 'minimax_api.apiKey', + provider: MINIMAX_API_PROVIDER_ID, + value: config.apiKey, + }); if (!apiKey) { throw new Error('LocalModelResolver: minimax_api apiKey is not configured.'); } @@ -104,7 +112,11 @@ function resolveCustomProviderCredentials( ): Pick { const authProvider = config.kind === 'oauth' || config.options?.authMode === 'oauth' ? input.providerKey : undefined; - const apiKey = config.options?.apiKey?.trim(); + const apiKey = resolveProviderCredential({ + field: 'custom_provider options.apiKey', + provider: input.provider, + value: config.options?.apiKey, + }); if (!apiKey && !authProvider) { throw new Error(`LocalModelResolver: api_key not configured for provider "${input.provider}".`); } diff --git a/packages/local-runtime/src/config/update.ts b/packages/local-runtime/src/config/update.ts index c4d29214..a70c2b82 100644 --- a/packages/local-runtime/src/config/update.ts +++ b/packages/local-runtime/src/config/update.ts @@ -1,7 +1,13 @@ import { randomBytes } from 'node:crypto'; import fs from 'node:fs'; import { dirname, join } from 'node:path'; -import { getConfig, getConfigPath, MINIMAX_API_MODEL_CATALOG, resetConfig } from '@mavis/config'; +import { + getConfig, + getConfigPath, + MINIMAX_API_MODEL_CATALOG, + resetConfig, + serializeConfigPreservingComments, +} from '@mavis/config'; import yaml from 'js-yaml'; import lockfile from 'proper-lockfile'; @@ -86,7 +92,9 @@ export async function updateLocalConfigFile( stale: 10_000, retries: { retries: 20, factor: 1, minTimeout: 5, maxTimeout: 25 }, }); - const raw = readLocalRawConfig(configPath); + const originalText = fs.readFileSync(configPath, 'utf-8'); + const previous = parseLocalRawConfig(originalText); + const raw = structuredClone(previous); if (preparedCommitPayload) { assertSafeConfigRecord({ ...preparedCommitPayload }); const invalidRoot = Object.keys(preparedCommitPayload).find( @@ -101,7 +109,7 @@ export async function updateLocalConfigFile( } else { applyLocalConfigUpdate(raw, body); } - await atomicWriteFile(configPath, yaml.dump(raw, { indent: 2, lineWidth: -1, noRefs: true })); + await atomicWriteFile(configPath, serializeConfigPreservingComments(originalText, previous, raw)); resetConfig(); return { config: getConfig() }; } catch (err) { @@ -144,7 +152,9 @@ export async function compareAndSetLocalModelContext( stale: 10_000, retries: { retries: 20, factor: 1, minTimeout: 5, maxTimeout: 25 }, }); - const raw = readLocalRawConfig(configPath); + const originalText = fs.readFileSync(configPath, 'utf-8'); + const previous = parseLocalRawConfig(originalText); + const raw = structuredClone(previous); resetConfig(); const currentConfig = getConfig() as LocalRuntimeConfig; const currentContext = @@ -176,7 +186,7 @@ export async function compareAndSetLocalModelContext( }; } assertSafeConfigRecord(raw); - await atomicWriteFile(configPath, yaml.dump(raw, { indent: 2, lineWidth: -1, noRefs: true })); + await atomicWriteFile(configPath, serializeConfigPreservingComments(originalText, previous, raw)); resetConfig(); return { updated: true, config: getConfig() as LocalRuntimeConfig }; } catch (err) { @@ -236,7 +246,9 @@ export async function updateLocalByokConfig( stale: 10_000, retries: { retries: 20, factor: 1, minTimeout: 5, maxTimeout: 25 }, }); - const raw = readLocalRawConfig(configPath); + const originalText = fs.readFileSync(configPath, 'utf-8'); + const previous = parseLocalRawConfig(originalText); + const raw = structuredClone(previous); const draft: LocalByokConfigDraft = { minimax_api: isPlainRecord(raw.minimax_api) ? raw.minimax_api : undefined, custom_provider: withoutEmptyEntries( @@ -279,7 +291,7 @@ export async function updateLocalByokConfig( } else if (draft.defaultModelVariant === undefined && 'defaultModelVariant' in raw) { delete raw.defaultModelVariant; } - await atomicWriteFile(configPath, yaml.dump(raw, { indent: 2, lineWidth: -1, noRefs: true })); + await atomicWriteFile(configPath, serializeConfigPreservingComments(originalText, previous, raw)); resetConfig(); return { config: getConfig() }; } catch (err) { @@ -309,10 +321,7 @@ export async function atomicWriteFile(filePath: string, content: string): Promis } } -function readLocalRawConfig(configPath: string): Record { - // Callers create missing files before locking. A read failure must abort the - // update rather than turn an existing configuration into an empty document. - const source = fs.readFileSync(configPath, 'utf-8'); +function parseLocalRawConfig(source: string): Record { let parsed: unknown; try { parsed = yaml.load(source); diff --git a/packages/local-runtime/src/model-provider/config-fingerprint.ts b/packages/local-runtime/src/model-provider/config-fingerprint.ts index 0d5122e5..b1108e87 100644 --- a/packages/local-runtime/src/model-provider/config-fingerprint.ts +++ b/packages/local-runtime/src/model-provider/config-fingerprint.ts @@ -1,3 +1,5 @@ +import { probeProviderCredential } from '@mavis/config'; + import type { LocalModelConfig, LocalRuntimeConfig } from '../config/types.js'; import { MINIMAX_API_PROVIDER_ID, parseProviderId } from '../config/model-key.js'; import { minimaxApiBaseUrl, minimaxApiModels } from './minimax-api.js'; @@ -21,7 +23,11 @@ export function byokModelTestStatus( ): ModelCacheStatusEntry | undefined { const parsed = parseProviderId(providerId); if (parsed?.source === 'minimax_api') { - const apiKey = config.minimax_api?.apiKey?.trim(); + const apiKey = probeProviderCredential({ + field: 'minimax_api.apiKey', + provider: 'minimax_api', + value: config.minimax_api?.apiKey, + }).secret; const model = minimaxApiModels(config)[modelId]; if (!apiKey || !model) return undefined; const fingerprint = modelConnectionTestFingerprint( @@ -37,7 +43,11 @@ export function byokModelTestStatus( } if (parsed?.source !== 'custom_provider') return undefined; const provider = config.custom_provider?.[parsed.providerKey]; - const apiKey = provider?.options?.apiKey?.trim(); + const apiKey = probeProviderCredential({ + field: 'options.apiKey', + provider: providerId, + value: provider?.options?.apiKey, + }).secret; const baseUrl = provider?.options?.baseURL?.trim(); const model = provider?.models?.[modelId]; if (!provider || provider.enabled === false || !apiKey || !baseUrl || !model) return undefined; diff --git a/packages/local-runtime/src/model-provider/list-models.ts b/packages/local-runtime/src/model-provider/list-models.ts index 76f9794f..d4ce1fd7 100644 --- a/packages/local-runtime/src/model-provider/list-models.ts +++ b/packages/local-runtime/src/model-provider/list-models.ts @@ -1,4 +1,9 @@ -import { getRuntimePresetKey, listRouteModelIds, resolveProviderAuthMode } from '@mavis/config'; +import { + getRuntimePresetKey, + listRouteModelIds, + resolveProviderAuthMode, + probeProviderCredential, +} from '@mavis/config'; import type { LocalCustomProviderConfig, @@ -54,7 +59,11 @@ export function builtinProviderKind( } export function hasMinimaxApiKey(config: LocalRuntimeConfig): boolean { - return Boolean(config.minimax_api?.apiKey?.trim()); + return probeProviderCredential({ + field: 'minimax_api.apiKey', + provider: 'minimax_api', + value: config.minimax_api?.apiKey, + }).configured; } /** Models the builtin provider can route under the active runtime preset. */ diff --git a/packages/local-runtime/src/runtime/model-resolver-byok.ts b/packages/local-runtime/src/runtime/model-resolver-byok.ts index 3c550177..2d371825 100644 --- a/packages/local-runtime/src/runtime/model-resolver-byok.ts +++ b/packages/local-runtime/src/runtime/model-resolver-byok.ts @@ -4,7 +4,7 @@ // tree path. Custom providers never consult the Pi catalog by name; missing // limits use the dedicated BYOK fallbacks (not the legacy 2048 default). import type { Api } from '@earendil-works/pi-ai'; -import { MINIMAX_API_MODEL_CATALOG } from '@mavis/config'; +import { MINIMAX_API_MODEL_CATALOG, resolveProviderCredential } from '@mavis/config'; import type { LocalCustomProvidersConfig, @@ -73,7 +73,11 @@ export function planMinimaxApiResolution(input: { }): ByokResolutionPlan | undefined { const cfg = input.byok?.minimax_api; if (!cfg) return undefined; - const apiKey = cfg.apiKey?.trim(); + const apiKey = resolveProviderCredential({ + field: 'minimax_api.apiKey', + provider: 'minimax_api', + value: cfg.apiKey, + }); if (!apiKey) { throw new Error('LocalModelResolver: minimax_api apiKey is not configured.'); } @@ -118,7 +122,11 @@ export function planCustomProviderResolution(input: { if (!modelConfig) return undefined; const authProvider = cfg.kind === 'oauth' || cfg.options?.authMode === 'oauth' ? input.providerKey : undefined; - const apiKey = cfg.options?.apiKey?.trim(); + const apiKey = resolveProviderCredential({ + field: 'custom_provider options.apiKey', + provider: input.provider, + value: cfg.options?.apiKey, + }); if (!apiKey && !authProvider) { throw new Error(`LocalModelResolver: api_key not configured for provider "${input.provider}".`); } diff --git a/packages/shared/package.json b/packages/shared/package.json index 57e7636a..0c200ca2 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -191,6 +191,10 @@ "./safety-check-v2": { "types": "./src/safety-check-v2.ts", "import": "./dist/safety-check-v2.js" + }, + "./logging/redact-log-secret": { + "types": "./src/logging/redact-log-secret.ts", + "import": "./dist/logging/redact-log-secret.js" } }, "types": "./src/index.ts", diff --git a/packages/shared/src/logging/redact-log-secret.ts b/packages/shared/src/logging/redact-log-secret.ts new file mode 100644 index 00000000..094f568e --- /dev/null +++ b/packages/shared/src/logging/redact-log-secret.ts @@ -0,0 +1,152 @@ +/** + * Secret redaction for anything that leaves the process as text: runtime logs, + * error messages surfaced to the user, and diagnostic bundles built from them. + * + * One vocabulary lives here so the log arm, the BYOK error arm, and any future + * artifact writer cannot drift into covering different key names or patterns. + * + * The key list is deliberately a fixed set of unambiguous credential names. Bare + * `token` / `tokens` / `key` are excluded on purpose: the runtime logs token + * accounting fields (`context_usage`, `cache_read`, `tokens`) that users read for + * throughput numbers, and blanket-redacting them destroys the signal. + */ + +export const REDACTED = '***'; + +const SENSITIVE_KEYS = new Set([ + 'authorization', + 'proxyauthorization', + 'apikey', + 'apisecret', + 'accesstoken', + 'refreshtoken', + 'idtoken', + 'authtoken', + 'bearertoken', + 'sessiontoken', + 'bearertokens', + 'clientsecret', + 'clientsecrets', + 'clientappsecret', + 'accesskey', + 'accesskeyid', + 'accesskeysecret', + 'secretaccesskey', + 'privatekey', + 'signingkey', + 'credential', + 'credentials', + 'password', + 'passwd', + 'pwd', + 'cookie', + 'cookies', + 'jwt', + 'secret', + 'secrets', +]); + +const API_KEY_LITERAL = /\bsk-[A-Za-z0-9._-]{8,}\b/gu; +// A credential header is `Authorization: `. The scheme and the +// payload are separate tokens, so consuming only the first leaves the payload in +// clear: `Basic dXNlcjpwYXNz` used to lose `Basic` and keep the base64. +// The header runs to the end of its value, not to the next space: `Digest +// username="x", nonce="y"` is one credential spread over several fields. The +// scheme word stays so the line still says which auth was attempted. +const AUTHORIZATION_VALUE = /\b(authorization\s*[:=]\s*)((?:[A-Za-z][A-Za-z-]*)\s+)?[^,\n}]*/giu; +// Quoted JSON pairs sit next to a quote, which the unquoted value pattern cannot +// reach: `{"x-api-key":"..."}` matched nothing at all before this rule. +const QUOTED_NAMED_VALUE = + /("(?:api[_-]?key|apikey|x-api-key|authorization|access[_-]?token|refresh[_-]?token|client[_-]?secret)"\s*:\s*)"(?:\\.|[^"\\])*"/giu; +const NAMED_KEY_VALUE = /((?:api[_-]?key|x-api-key|client[_-]?secret)\s*[:=]\s*)[^\s"',}]+/giu; +const BEARER_TOKEN = /\b(Bearer\s+)[A-Za-z0-9._~+/=-]{8,}/giu; + +/** Credential terms that also cover vendor-prefixed headers (`x-api-key`, …). */ +const SENSITIVE_KEY_SUFFIXES = [ + 'authorization', + 'apikey', + 'apisecret', + 'accesstoken', + 'refreshtoken', + 'idtoken', + 'authtoken', + 'bearertoken', + 'sessiontoken', + 'clientsecret', + 'privatekey', + 'signingkey', + 'password', + 'passwd', + 'secret', + 'credential', +]; + +/** True when a structured field name denotes a credential. */ +export function isSecretKey(key: string): boolean { + const normalized = normalizeKey(key); + return ( + SENSITIVE_KEYS.has(normalized) || + SENSITIVE_KEY_SUFFIXES.some((suffix) => normalized.endsWith(suffix)) + ); +} + +/** Strips credential literals and `key: value` credential headers from a string. */ +export function redactSecretText(text: string): string { + return text + .replace(API_KEY_LITERAL, `sk-${REDACTED}`) + .replace(QUOTED_NAMED_VALUE, `$1"${REDACTED}"`) + .replace(AUTHORIZATION_VALUE, (_match, prefix: string, scheme: string | undefined) => + `${prefix}${scheme ?? ''}${REDACTED}`, + ) + .replace(NAMED_KEY_VALUE, `$1${REDACTED}`) + .replace(BEARER_TOKEN, `$1${REDACTED}`); +} + +const MAX_REDACTION_DEPTH = 8; +/** Branch was too deep to walk, or could not be enumerated. */ +const TRUNCATED = '[truncated]'; +/** Reference back to an ancestor already in this branch. */ +const CIRCULAR = '[circular]'; + +/** + * Returns a copy of `value` with credential fields and credential literals + * removed. A branch that cannot be walked — past the depth cap, back to an + * ancestor, or un-enumerable — becomes a marker string rather than the original + * value, so no unredacted subtree can reach the log line. + */ +export function redactSecretValue(value: T): T { + return redactValue(value, new WeakSet(), 0) as T; +} + +/** + * Every path that cannot finish redacting a branch yields a marker, never the + * original value. Handing back the input at the depth cap, on a cycle, or when + * enumeration throws would put an unredacted subtree straight into the log + * line, which is the one outcome this function exists to prevent. + */ +function redactValue(value: unknown, seen: WeakSet, depth: number): unknown { + if (typeof value === 'string') return redactSecretText(value); + if (value === null || typeof value !== 'object') return value; + if (depth >= MAX_REDACTION_DEPTH) return TRUNCATED; + if (seen.has(value)) return CIRCULAR; + seen.add(value); + + try { + if (Array.isArray(value)) { + return value.map((entry) => redactValue(entry, seen, depth + 1)); + } + const record: Record = {}; + for (const [key, entry] of Object.entries(value)) { + record[key] = isSecretKey(key) ? REDACTED : redactValue(entry, seen, depth + 1); + } + return record; + } catch { + return TRUNCATED; + } finally { + seen.delete(value); + } +} + +function normalizeKey(key: string): string { + return key.toLowerCase().replace(/[^a-z0-9]/gu, ''); +} diff --git a/packages/shared/src/logging/structured-logger.ts b/packages/shared/src/logging/structured-logger.ts index 60bdee95..14dbab91 100644 --- a/packages/shared/src/logging/structured-logger.ts +++ b/packages/shared/src/logging/structured-logger.ts @@ -5,6 +5,7 @@ import pino from 'pino'; import pinoPretty from 'pino-pretty'; import type { DiskLogTransport } from './disk-transport.js'; +import { redactSecretText, redactSecretValue } from './redact-log-secret.js'; export interface TraceContextLike { traceId: string; @@ -34,6 +35,15 @@ export interface StructuredLoggerOptions { disk?: DiskLogTransport; } +/** Extras stay serializable the way the caller built them; an exotic payload degrades to its keys. */ +function stringifyExtras(extras: Record): string { + try { + return JSON.stringify(extras); + } catch { + return JSON.stringify(Object.keys(extras)); + } +} + function getCallerLocation(): string | undefined { const { stack } = new Error(); if (!stack) return undefined; @@ -87,9 +97,13 @@ function createPrettyStream( for (const key of Object.keys(log)) { if (!standardKeys.has(key)) extras[key] = log[key]; } - const jsonStr = Object.keys(extras).length > 0 ? ` ${JSON.stringify(extras)}` : ''; + // Redact here, at the single point where a log line becomes text. Every + // level and both the terminal and disk arms pass through it, so no call + // site has to remember to strip credentials before logging. + const safeExtras = redactSecretValue(extras); + const jsonStr = Object.keys(safeExtras).length > 0 ? ` ${stringifyExtras(safeExtras)}` : ''; const traceSuffix = traceId ? ` [${traceId}]` : ''; - return `[${String(source)}]${traceSuffix} ${String(msg)}${jsonStr}\n`; + return `[${String(source)}]${traceSuffix} ${redactSecretText(String(msg))}${jsonStr}\n`; }, }); } diff --git a/packages/shared/test/unit/redact-log-secret.test.ts b/packages/shared/test/unit/redact-log-secret.test.ts new file mode 100644 index 00000000..99c78f12 --- /dev/null +++ b/packages/shared/test/unit/redact-log-secret.test.ts @@ -0,0 +1,200 @@ +import { PassThrough } from 'node:stream'; +import { describe, expect, it } from 'vitest'; + +import { createStructuredLogger } from '../../src/logging/index.js'; +import { + isSecretKey, + redactSecretText, + redactSecretValue, +} from '../../src/logging/redact-log-secret.js'; + +function collectLogLines(log: (logger: ReturnType) => void): string { + const dest = new PassThrough(); + const chunks: string[] = []; + dest.on('data', (chunk) => chunks.push(chunk.toString('utf8'))); + const logger = createStructuredLogger({ + destination: dest, + level: 'info', + env: { NODE_ENV: 'production' }, + }); + log(logger); + return chunks.join(''); +} + +describe('redactSecretText', () => { + it('removes an api key literal', () => { + expect(redactSecretText('upstream failed with key sk-fixturefixturefixture')).toBe( + 'upstream failed with key sk-***', + ); + }); + + it('removes a bearer credential from a header form', () => { + expect(redactSecretText('Authorization: Bearer fixturefixture00')).toBe( + 'Authorization: Bearer ***', + ); + }); + + it('removes an api-key header value', () => { + expect(redactSecretText('x-api-key: fixturefixture00')).toBe('x-api-key: ***'); + }); + + it('removes the credential payload, not just the scheme word', () => { + // `Basic`/`Digest` are scheme names, not the secret. Stopping after the + // scheme left the payload — the base64 or the nonce — in clear. The scheme + // stays so the line still says which auth was attempted. + expect(redactSecretText('Authorization: Basic dXNlcjpwYXNz')).toBe('Authorization: Basic ***'); + expect(redactSecretText('authorization=Bearer fixturefixture00, next=1')).toBe( + 'authorization=Bearer ***, next=1', + ); + expect(redactSecretText('Authorization: Digest username="x", nonce="y"')).toBe( + 'Authorization: Digest ***, nonce="y"', + ); + }); + + it('removes a credential inside a quoted JSON pair', () => { + // The value sits between quotes, so the unquoted pattern could not reach it + // and the whole string passed through untouched. + expect(redactSecretText('detail: {"x-api-key":"fixturefixture00"}')).toBe( + 'detail: {"x-api-key":"***"}', + ); + expect(redactSecretText('{"access_token":"tok-fixturefixture"}')).toBe( + '{"access_token":"***"}', + ); + expect(redactSecretText('{"apiKey":"sk-secret-value-here","model":"gpt-x"}')).toBe( + '{"apiKey":"***","model":"gpt-x"}', + ); + }); + + it('leaves text with no credential alone', () => { + expect(redactSecretText('model=gpt-x resolved in 42ms')).toBe('model=gpt-x resolved in 42ms'); + }); + + it('removes a bare bearer token', () => { + expect(redactSecretText('send Bearer fixturefixture00 now')).toBe('send Bearer *** now'); + }); + + it('leaves unrelated text alone', () => { + expect(redactSecretText('model resolve finished in 42ms')).toBe( + 'model resolve finished in 42ms', + ); + }); +}); + +describe('redactSecretValue', () => { + it('removes credential fields at any depth', () => { + const result = redactSecretValue({ + provider: 'mafia', + options: { + baseURL: 'https://api.example.com', + apiKey: 'sk-fixturefixture00', + headers: { 'x-api-key': 'fixturefixture00', 'x-trace': 'keep' }, + }, + nested: { list: [{ password: 'hunter2' }] }, + }); + + expect(result).toEqual({ + provider: 'mafia', + options: { + baseURL: 'https://api.example.com', + apiKey: '***', + headers: { 'x-api-key': '***', 'x-trace': 'keep' }, + }, + nested: { list: [{ password: '***' }] }, + }); + }); + + it('keeps the token accounting fields users read for throughput', () => { + // The tok/s panel is a headline feature; redacting these would remove the + // exact signal the log line exists to carry. + const result = redactSecretValue({ + context_usage: { components: [{ kind: 'TOOLS', tokens: 21170 }] }, + cache_read: 113792, + input_tokens: 8165, + total_tokens: 122154, + }); + + expect(result).toEqual({ + context_usage: { components: [{ kind: 'TOOLS', tokens: 21170 }] }, + cache_read: 113792, + input_tokens: 8165, + total_tokens: 122154, + }); + }); + + it('keeps a credential environment variable name readable', () => { + expect(redactSecretValue({ apiKeyEnv: 'MAFIA_API_KEY' })).toEqual({ apiKeyEnv: 'MAFIA_API_KEY' }); + }); + + it('strips a credential literal that only appears inside a leaf string', () => { + expect(redactSecretValue({ detail: 'request used sk-fixturefixture00' })).toEqual({ + detail: 'request used sk-***', + }); + }); + + it('replaces a cycle with a marker instead of re-emitting the original', () => { + const cyclic: Record = { name: 'root', apiKey: 'sk-inside-cycle' }; + cyclic.self = cyclic; + + const result = redactSecretValue(cyclic) as Record; + + expect(result.name).toBe('root'); + expect(result.apiKey).toBe('***'); + expect(result.self).toBe('[circular]'); + expect(JSON.stringify(result)).not.toContain('sk-inside-cycle'); + }); + + it('redacts a credential nested past the depth cap', () => { + // The cap must truncate the branch, not hand the original object back: + // anything below the cap would otherwise reach the log unredacted. + let deep: Record = { apiKey: 'sk-past-the-cap' }; + for (let level = 0; level < 12; level += 1) deep = { nested: deep }; + + const serialized = JSON.stringify(redactSecretValue(deep)); + + expect(serialized).not.toContain('sk-past-the-cap'); + expect(serialized).toContain('[truncated]'); + }); +}); + +describe('isSecretKey', () => { + it('matches credential names regardless of case and separator', () => { + for (const key of ['apiKey', 'api_key', 'API-KEY', 'Authorization', 'clientSecret', 'jwt']) { + expect(isSecretKey(key)).toBe(true); + } + }); + + it('does not match unrelated fields', () => { + for (const key of ['token', 'tokens', 'apiKeyEnv', 'monkey', 'token_usage', 'id']) { + expect(isSecretKey(key)).toBe(false); + } + }); +}); + +describe('structured logger redaction', () => { + it('never writes a credential to the destination', () => { + const output = collectLogLines((logger) => { + logger.info( + { + provider: 'mafia', + options: { apiKey: 'sk-fixturefixture00', baseURL: 'https://api.example.com' }, + }, + 'provider request failed with Authorization: Bearer fixturefixture00', + ); + }); + + expect(output).not.toContain('sk-fixturefixture00'); + expect(output).not.toContain('fixturefixture00'); + expect(output).toContain('provider request failed'); + // Non-secret context must survive so the line stays useful. + expect(output).toContain('api.example.com'); + }); + + it('redacts on the error level too', () => { + const output = collectLogLines((logger) => { + logger.error({ authorization: 'Bearer fixturefixture00' }, 'upstream rejected the request'); + }); + + expect(output).not.toContain('fixturefixture00'); + expect(output).toContain('upstream rejected the request'); + }); +}); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 8014dab7..d3e604f7 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -478,6 +478,9 @@ importers: proper-lockfile: specifier: ^4 version: 4.1.2 + yaml: + specifier: ^2.9.0 + version: 2.9.0 devDependencies: '@types/js-yaml': specifier: ^4 diff --git a/release/dependency-licenses.json b/release/dependency-licenses.json index 854c5200..c0003915 100644 --- a/release/dependency-licenses.json +++ b/release/dependency-licenses.json @@ -1374,7 +1374,7 @@ { "name": "dependency-cruiser", "versions": [ - "18.2.0" + "18.3.1" ], "license": "MIT", "homepage": "https://github.com/sverweij/dependency-cruiser" @@ -1454,7 +1454,7 @@ { "name": "enhanced-resolve", "versions": [ - "5.24.5" + "5.25.1" ], "license": "MIT", "homepage": "https://github.com/webpack/enhanced-resolve#readme" @@ -2056,7 +2056,7 @@ "name": "ignore", "versions": [ "7.0.5", - "7.0.6" + "7.0.9" ], "license": "MIT", "homepage": "https://github.com/kaelzhang/node-ignore#readme" @@ -2114,7 +2114,7 @@ { "name": "is-core-module", "versions": [ - "2.16.2" + "2.17.0" ], "license": "MIT", "homepage": "https://github.com/inspect-js/is-core-module" @@ -2274,7 +2274,7 @@ { "name": "jscpd", "versions": [ - "5.2.0" + "5.2.1" ], "license": "MIT", "homepage": "https://jscpd.dev" @@ -2282,7 +2282,7 @@ { "name": "jscpd-darwin-arm64", "versions": [ - "5.2.0" + "5.2.1" ], "license": "MIT", "homepage": "https://github.com/kucherenko/jscpd#readme" @@ -2863,7 +2863,6 @@ "name": "picomatch", "versions": [ "2.3.2", - "4.0.5", "4.0.7" ], "license": "MIT", @@ -3441,21 +3440,21 @@ "homepage": "https://github.com/unjs/std-env#readme" }, { - "name": "string_decoder", + "name": "string-width", "versions": [ - "1.1.1", - "1.3.0" + "4.2.3" ], "license": "MIT", - "homepage": "https://github.com/nodejs/string_decoder" + "homepage": "https://github.com/sindresorhus/string-width#readme" }, { - "name": "string-width", + "name": "string_decoder", "versions": [ - "4.2.3" + "1.1.1", + "1.3.0" ], "license": "MIT", - "homepage": "https://github.com/sindresorhus/string-width#readme" + "homepage": "https://github.com/nodejs/string_decoder" }, { "name": "strip-ansi", diff --git a/release/public-source.json b/release/public-source.json index addd3d8c..78f8017c 100644 --- a/release/public-source.json +++ b/release/public-source.json @@ -49,6 +49,7 @@ "docs/assets/tui-demo.png", "docs/assets/wordmark-dark.svg", "docs/assets/wordmark-light.svg", + "docs/byok-environment-credentials.md", "docs/demo.md", "docs/examples.md", "docs/installation.md", @@ -438,7 +439,9 @@ "packages/config/src/asr.ts", "packages/config/src/browser-config.ts", "packages/config/src/byok-config.ts", + "packages/config/src/comment-preserving-config-write.ts", "packages/config/src/config.ts", + "packages/config/src/credential-reference.ts", "packages/config/src/cu-backend-io.ts", "packages/config/src/cu-backend.ts", "packages/config/src/data-dir.ts", @@ -466,8 +469,11 @@ "packages/config/src/tool-result-compaction-config.ts", "packages/config/src/tui-config.ts", "packages/config/src/tui-status-line-write.ts", + "packages/config/test/comment-preserving-config-write.test.ts", "packages/config/test/config-file-permissions.test.ts", "packages/config/test/config-update-preservation.test.ts", + "packages/config/test/credential-reference.test.ts", + "packages/config/test/local-model-provider-write.test.ts", "packages/config/test/managed-preset-sync.test.ts", "packages/config/test/private-config-file.test.ts", "packages/local-runtime-v2/assets/agents/_default/prompt-base-all.md", @@ -2747,6 +2753,7 @@ "packages/shared/src/local-runtime-paths.ts", "packages/shared/src/logging/disk-transport.ts", "packages/shared/src/logging/index.ts", + "packages/shared/src/logging/redact-log-secret.ts", "packages/shared/src/logging/structured-logger.ts", "packages/shared/src/markdown-source-citation.ts", "packages/shared/src/mavis-tags.ts", @@ -2786,6 +2793,7 @@ "packages/shared/src/watch-interval.ts", "packages/shared/src/web-source-evidence.ts", "packages/shared/src/windows-file-system.ts", + "packages/shared/test/unit/redact-log-secret.test.ts", "packages/tui/CHANGELOG.md", "packages/tui/CHANGELOG.zh-CN.md", "packages/tui/THIRD_PARTY_NOTICES.md", diff --git a/test/byok.test.mjs b/test/byok.test.mjs index 15feaf54..7c151e36 100644 --- a/test/byok.test.mjs +++ b/test/byok.test.mjs @@ -300,6 +300,34 @@ test( assert.equal(selected.models[0].contextLimit, undefined); assert.equal(selected.models[0].maxOutputTokens, undefined); + // Exercise the built public CLI with a saved environment reference, including + // missing-variable failure, key rotation, and a later unrelated config write. + const referenceConfig = savedConfig(); + referenceConfig.custom_provider.fixture.options.apiKey = '${MCODE_PROVIDER_API_KEY}'; + writeFileSync(configPath, '# external provider credential\n' + stringifyYaml(referenceConfig)); + const beforeReference = requests.length; + await run(['provider', 'test', selected.providerId, '--model', 'fixture-model']); + assert.ok(requests.length > beforeReference); + assert.equal(requests[beforeReference].auth, 'Bearer fixture-only-key'); + assert.match(await run([ + 'exec', 'ENV_REFERENCE_TEST', '--model', `${selected.providerId}/fixture-model`, + '--timeout', '20s', '--max-steps', '1', + ]), /LOCAL_BYOK_OK/); + env.MCODE_PROVIDER_API_KEY = 'fixture-rotated-key'; + const beforeRotation = requests.length; + await run(['provider', 'test', selected.providerId, '--model', 'fixture-model']); + assert.equal(requests[beforeRotation].auth, 'Bearer fixture-rotated-key'); + const afterRotation = requests.length; + env.MCODE_PROVIDER_API_KEY = ''; + const beforeMissing = requests.length; + await assert.rejects(run([ + 'exec', 'MISSING_ENV_REFERENCE_TEST', '--model', `${selected.providerId}/fixture-model`, + '--timeout', '20s', '--max-steps', '1', + ]), /MCODE_PROVIDER_API_KEY.*not set/s); + assert.equal(requests.length, beforeMissing, 'Missing credentials must fail before an upstream request'); + env.MCODE_PROVIDER_API_KEY = 'fixture-only-key'; + assert.equal(savedConfig().custom_provider.fixture.options.apiKey, '${MCODE_PROVIDER_API_KEY}'); + // The selected plan path must survive YAML persistence and actual inference. const codingUrl = `${new URL(baseUrl).origin}/api/coding/paas/v4`; const beforeCoding = requests.length; @@ -308,6 +336,8 @@ test( "--api-format", "openai-completions", "--model", "glm-5.3", "--use", ]); assert.equal(savedConfig().custom_provider.coding.options.baseURL, codingUrl); + assert.equal(savedConfig().custom_provider.fixture.options.apiKey, '${MCODE_PROVIDER_API_KEY}'); + assert.match(readFileSync(configPath, 'utf8'), /# external provider credential/); assert.equal(savedConfig().defaultModel, "custom_provider:coding/glm-5.3"); assert.match(await run([ "exec", "CODING_ENDPOINT_TEST", "--timeout", "20s", "--max-steps", "1", @@ -494,7 +524,11 @@ test( "The real read tool must return file contents to the provider", ); assert.ok(requests.length >= 2); - assert.ok(requests.every((r) => r.auth === "Bearer fixture-only-key")); + assert.ok(requests.every((r, index) => r.auth === ( + index >= beforeRotation && index < afterRotation + ? 'Bearer fixture-rotated-key' + : 'Bearer fixture-only-key' + ))); assert.equal( requests.some((r) => r.body.tools?.some( (tool) => tool.function?.name === "workspace_semantic_search", diff --git a/test/vitest-suites.json b/test/vitest-suites.json index 0601656f..32ccac27 100644 --- a/test/vitest-suites.json +++ b/test/vitest-suites.json @@ -165,7 +165,11 @@ "packages/local-runtime-v2/test/integration/btw-settled-history.integration.test.ts", "packages/tui/test/unit/tui/controller/side-session-flow.test.ts", "packages/tui/test/unit/observability.test.ts", - "packages/local-runtime-v2/src/service/session-system/fork/side-history-boundary.test.ts" + "packages/local-runtime-v2/src/service/session-system/fork/side-history-boundary.test.ts", + "packages/config/test/comment-preserving-config-write.test.ts", + "packages/config/test/credential-reference.test.ts", + "packages/config/test/local-model-provider-write.test.ts", + "packages/shared/test/unit/redact-log-secret.test.ts" ], "status-contract": [ "packages/tui/test/unit/tui-build-mode-contract.test.ts" diff --git a/tsconfig.standalone.json b/tsconfig.standalone.json index 1ad29d2b..6f5ad7d1 100644 --- a/tsconfig.standalone.json +++ b/tsconfig.standalone.json @@ -307,6 +307,9 @@ "@mavis/shared/logging": [ "./packages/shared/src/logging/index.ts" ], + "@mavis/shared/logging/redact-log-secret": [ + "./packages/shared/src/logging/redact-log-secret.ts" + ], "@mavis/shared/mavis-tags": [ "./packages/shared/src/mavis-tags.ts" ],