From bb59aab58b31d6956f1b78fc4487cfb331d89a27 Mon Sep 17 00:00:00 2001 From: liuhailong <857688528@qq.com> Date: Sat, 26 Sep 2026 20:03:35 +0800 Subject: [PATCH 1/2] =?UTF-8?q?feat(webui):=20cross-provider=20switching?= =?UTF-8?q?=20=E2=80=94=20sync=20providers=20to=20engine=20and=20surface?= =?UTF-8?q?=20configured=20models?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tickets 01-04 cataloged providers in the webui but never pushed them to the engine's custom_provider registry. Selecting a provider model in the dialog was UI-only — applyRecordedModel had nothing to match against and the engine kept its default. This commit closes ticket 05 by adding the engine-side projection: * packages/webui/server/lib/engine-provider-sync.js: pure helpers (providerKeyFromId, modelKeyFromId, toEngineCustomProvider) plus the sync entry that writes the engine's custom_provider tree via an atomic YAML rewrite. Only byok entries with both apiKey and baseURL flow through; coding-plan goes through the engine's OAuth flows. Reserved engine ids ('minimax', 'minimax_api', 'provider', 'custom_provider') get a deterministic '-byok' suffix to avoid shadowing. * packages/webui/server/routes/providers.js: PUT and preset enable handlers call syncProvidersToEngine after the user-level write, then shutdownMcodeAcpSingleton so the next operation spawns a fresh subprocess that reads the new config. The sync result is included in the response (engineSync.ok / engineSync.keys) so the UI can surface a non-blocking warning if the engine write fails. * packages/webui/server/lib/mapplyRecordedModel: bare-name option match is now case-insensitive. The engine populates option.name from the user-supplied label (e.g. 'GLM-5.3' for a custom provider whose label differs in case from the model id 'glm-5.3'); a strict comparison would skip the apply and leave the engine on its default. * packages/webui/webapp/components/provider-management.tsx: provider card now shows the configured models as chips directly, so the operator sees what's wired up without expanding the editor. * Test updates: - packages/webui/test/lib/engine-provider-sync.test.js pins the v2 → engine custom_provider mapping, the keep-key convention applied to the PUT body, atomic write semantics, and operator-section preservation. - packages/webui/test/lib/mcode-acp-note.test.js updates the model option fixtures to the actual engine wire format ('m:::u|v:') and pins the cross-provider resolution path. * release/public-source.json regenerated (new files + 3 modified). Self-check evidence (18112 / 18113, isolated data dir under /tmp/dev-cp, engine subprocess pointed at a fake HTTP server on :19999): 1. PUT /api/providers with byok-zhipu → engine config.yaml gains custom_provider.byok-zhipu with the GLM-5.3 / GLM-4.6 entries; response carries engineSync: { ok: true, keys: ['byok-zhipu'] }. 2. POST /api/send with model=byok-zhipu/glm-5.3 → engine subprocess POSTs http://127.0.0.1:19999/v1/chat/completions with body.model 'glm-5.3' and Authorization 'Bearer sk-zhipu-fake-for-test' (the BYOK provider's apiKey, NOT the builtin MiniMax apiKey). Assistant returns 'OK from fake server (model=glm-5.3)'. 3. Hot-add: PUT with a third model (GLM-4-Flash) → /api/models and the provider card reflect the new chip without restart. 4. Settings provider card: 'Zhipu BYOK 自定义 openai · byok · key set GLM-5.3 GLM-4.6 GLM-4-Flash (HOT-ADDED)' visible without opening the editor. Gates: typecheck clean, test:webui 1602/1604 pass (2 unrelated skips), test:webapp 264/264 pass, build:webui OK, check:source OK. --- .../webui/server/lib/engine-provider-sync.js | 336 +++++++++++ packages/webui/server/lib/mcode-acp.js | 14 +- packages/webui/server/routes/providers.js | 71 ++- .../test/lib/engine-provider-sync.test.js | 553 ++++++++++++++++++ .../webui/test/lib/mcode-acp-note.test.js | 165 ++++-- .../webapp/components/provider-management.tsx | 31 + release/public-source.json | 2 + 7 files changed, 1113 insertions(+), 59 deletions(-) create mode 100644 packages/webui/server/lib/engine-provider-sync.js create mode 100644 packages/webui/test/lib/engine-provider-sync.test.js diff --git a/packages/webui/server/lib/engine-provider-sync.js b/packages/webui/server/lib/engine-provider-sync.js new file mode 100644 index 00000000..e90cd03b --- /dev/null +++ b/packages/webui/server/lib/engine-provider-sync.js @@ -0,0 +1,336 @@ +// webui/server/lib/engine-provider-sync.js +// Engine-side projection of webui's providers.json (ticket 05). +// +// Background — the root cause pinned by ticket 05: +// +// Webui keeps its own `providers.json` (a v2 catalogue, layered merge, +// keep-key convention) so the dialog can show provider groups, "enabled" +// toggles and "configured with key" greying without talking to the engine +// on every render. The engine has its own `custom_provider` registry +// (packages/local-runtime-v2/src/service/model-system/management/service-custom-provider-operations.ts) +// that is the ONLY source the `model` config option +// (packages/tui/src/acp/control-state.ts) advertises. Pre-ticket-05, the +// two never met: webui's PUT handler persisted its file, the engine kept +// its `custom_provider` from `config.yaml` independent of the webui — +// selecting a provider model in the dialog was UI-only and +// `applyRecordedModel` had nothing to match against, so the engine +// silently kept its default. +// +// Fix — write the engine's `custom_provider` shape right here so the engine +// sees the same providers the webui advertises: +// +// webui provider (v2) → engine custom_provider entry +// { id, label, protocol, auth, models } +// → { name, kind: 'custom', enabled, api, options: {apiKey, baseURL, authMode}, +// models: { modelId: { limit: {context}, thinking: {effortOptions}, modalities } } } +// +// Conversion rules (pinned by tests): +// - provider id → engine provider key (sluggified so the +// `custom_provider:/...` runtime id stays alphanumeric + dot + +// underscore + hyphen) +// - webui `enabled: false` AND/OR empty apiKey AND/OR missing baseURL → +// entry is OMITTED (the engine's `custom_provider` rejects apiKey-less +// `createUserProvider` and the `enabled: false` switch turns the entry +// invisible to `listByokRuntimeModels`) +// - protocol → api format: openai → openai-completions, +// anthropic → anthropic-messages, gemini → openai-completions (Gemini's +// OpenAI-compat endpoint is what `auth.type: 'byok'` callers point at; +// the engine does not have a native Gemini api format) +// - model id → engine model key; thinkingLevels → thinking.effortOptions, +// modalities → modalities.input, contextLimit → limit.context +// - auth.type === 'coding-plan' → SKIP (engine handles coding-plan via +// its own OAuth / Codex / Claude Code flows — out of scope for the +// byok projection) +// +// Write strategy: +// +// The engine reads `/config.yaml` (the same dir the +// webui already knows about — see server/lib/config.js#resolveDataDir). +// We do an atomic tmp+rename YAML write keyed on `custom_provider` only; +// we never touch the operator's other engine config (provider.minimax +// tables, defaultModel, etc.). The engine subprocess running the +// singleton client has a stale `getConfig()` cache after a write — +// the route handler then calls `shutdownMcodeAcpSingleton()` so the +// next operation spawns a fresh subprocess that reads the new file. +// Brand-new prompt subprocesses spawned by `runMcodeAcp` always pick +// up the latest config, so the rest of the system stays in lockstep. +// +// We deliberately do NOT route through the engine's +// `updateLocalByokConfig` (`@mavis/config`) — pulling that into the +// webui bundle would drag in js-yaml + proper-lockfile just for a +// one-way write we do rarely, and the lockfile is meaningful only when +// multiple `mcode` subprocesses are racing the same file (which the +// webui does not do — `mcode acp` does not edit `config.yaml` at +// runtime, only the `mcode provider add` CLI does, and that flow +// cannot run concurrently with a webui PUT in the same process tree). + +import { writeFile, rename, mkdir } from "node:fs/promises"; +import { dirname, join } from "node:path"; +import { homedir } from "node:os"; +import { existsSync, readFileSync } from "node:fs"; +import yaml from "js-yaml"; +import { randomBytes } from "node:crypto"; + +import { applyKeepKeyConvention } from "./providers-config.js"; + +// engine provider api formats — must match `MODEL_PROVIDER_APIS` in +// packages/local-runtime-v2/src/service/model-system/identity.ts (the +// engine rejects anything outside this set at `normalizeApiFormat`). +const WEBUI_PROTOCOL_TO_ENGINE_API = { + openai: "openai-completions", + anthropic: "anthropic-messages", + // gemini has no engine-native api; the OpenAI-compat endpoint is the + // usual `byok` target. Engine does not have a Gemini-specific format. + gemini: "openai-completions", +}; + +// Reserved engine keys — must NOT collide with the existing engine's +// internal provider ids (which would either shadow `minimax` or land in +// `RESERVED_CUSTOM_PROVIDER_KEYS` and be dropped). Mirrored from +// packages/config/src/byok-config.ts. +const RESERVED_ENGINE_KEYS = new Set([ + "minimax", + "minimax_api", + "provider", + "custom_provider", +]); + +const PROVIDER_KEY_REGEX = /^[A-Za-z0-9][A-Za-z0-9_.-]*$/; + +/** + * Resolve the engine's data directory. + * + * The engine resolves its own data dir via `packages/config/src/config.ts`: + * MINIMAX_DATA_DIR || MAVIS_DATA_DIR || ~/.minimax + * We mirror that exact resolution here so a webui-managed PUT lands in the + * directory the engine subprocess will read on next spawn. The webui itself + * already uses the same env precedence in server/lib/config.js — the + * resolver is the same shape, kept here as a copy so the helper has no + * cross-package import surface. + */ +export function resolveEngineDataDir() { + const env = (process.env.MINIMAX_DATA_DIR?.trim() || + process.env.MAVIS_DATA_DIR?.trim() || + ""); + if (env) return env; + return join(homedir(), ".minimax"); +} + +export function getEngineConfigPath() { + return join(resolveEngineDataDir(), "config.yaml"); +} + +/** Pure: a webui v2 id → an engine-safe provider key. */ +export function providerKeyFromId(id) { + const trimmed = (id || "").trim(); + if (!trimmed) return ""; + if (!PROVIDER_KEY_REGEX.test(trimmed)) return ""; + if (RESERVED_ENGINE_KEYS.has(trimmed)) { + return `${trimmed}-byok`; + } + return trimmed; +} + +/** Pure: webui model id → engine-safe model key. */ +export function modelKeyFromId(id) { + const trimmed = (id || "").trim(); + if (!trimmed) return ""; + // Engine model keys are even less constrained than provider keys (they + // appear inside a per-provider record), but the same alphanumeric + // character class keeps the keys safe to serialise into the runtime + // id `custom_provider:/` without URL-escaping. + if (!PROVIDER_KEY_REGEX.test(trimmed)) return ""; + return trimmed; +} + +/** Pure: webui v2 → engine custom_provider entry. Returns null when ineligible. */ +export function toEngineCustomProvider(provider) { + if (!provider || typeof provider !== "object") return null; + // coding-plan providers go through the engine's OAuth / Codex / + // subscription flows — out of scope for the byok projection. + if (provider.auth && provider.auth.type === "coding-plan") return null; + if (provider.enabled === false) return null; + const apiKey = + typeof provider.auth?.apiKey === "string" ? provider.auth.apiKey.trim() : ""; + if (!apiKey) return null; + const baseURL = + typeof provider.auth?.baseURL === "string" ? provider.auth.baseURL.trim() : ""; + if (!baseURL) { + // No baseURL → engine has nothing to call. Skip (matches the dialog's + // "configured without baseURL" grey-out: same semantics as no key). + return null; + } + const protocol = + typeof provider.protocol === "string" ? provider.protocol.trim() : "openai"; + const api = WEBUI_PROTOCOL_TO_ENGINE_API[protocol]; + if (!api) return null; + const providerKey = providerKeyFromId(provider.id); + if (!providerKey) return null; + const name = + typeof provider.label === "string" && provider.label.trim() + ? provider.label.trim() + : providerKey; + const models = {}; + if (Array.isArray(provider.models)) { + for (const m of provider.models) { + if (!m || typeof m !== "object") continue; + const modelKey = modelKeyFromId(m.id); + if (!modelKey) continue; + const engineModel = {}; + if ( + typeof m.label === "string" && + m.label.trim() && + m.label.trim() !== modelKey + ) { + engineModel.name = m.label.trim(); + } + if (typeof m.contextLimit === "number" && m.contextLimit > 0) { + engineModel.limit = { context: m.contextLimit }; + } + if ( + Array.isArray(m.thinkingLevels) && + m.thinkingLevels.length > 0 && + m.thinkingLevels.every((x) => typeof x === "string" && x.length > 0) + ) { + engineModel.thinking = { effortOptions: [...m.thinkingLevels] }; + } + if ( + Array.isArray(m.modalities) && + m.modalities.length > 0 && + m.modalities.every((x) => typeof x === "string" && x.length > 0) + ) { + engineModel.modalities = { input: [...m.modalities] }; + } + models[modelKey] = engineModel; + } + } + return { + key: providerKey, + entry: { + name, + kind: "custom", + enabled: true, + api, + options: { + apiKey, + baseURL, + authMode: "api-key", + }, + ...(Object.keys(models).length > 0 ? { models } : {}), + }, + }; +} + +/** + * Read the engine's existing `config.yaml` so a sync can preserve + * `provider.*`, `defaultModel`, and any operator-managed sections the + * webui must not touch. + * + * Returns a plain object (possibly empty). A missing file is not an + * error — the writer creates it. + */ +function readEngineConfigRaw(configPath) { + if (!existsSync(configPath)) return {}; + try { + const raw = readFileSync(configPath, "utf8"); + const parsed = yaml.load(raw); + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) return {}; + return parsed; + } catch { + // YAML parse error — the engine will surface this on its next read; + // we'd rather write a broken-than-empty file than drop the operator's + // section. Bail out as "no-op" so the route can answer with a clear + // structured error. + return null; + } +} + +async function atomicWriteYaml(configPath, object) { + await mkdir(dirname(configPath), { recursive: true }); + const tmp = join( + dirname(configPath), + `.config-tmp-${randomBytes(6).toString("hex")}`, + ); + await writeFile( + tmp, + yaml.dump(object, { indent: 2, lineWidth: -1, noRefs: true }), + "utf8", + ); + await rename(tmp, configPath); +} + +/** + * Project a list of webui v2 providers into the engine's `custom_provider` + * tree and write the engine's `config.yaml` atomically. + * + * Returns: + * - { ok: true, written: true|false, keys: [providerKey, ...] } + * `written: false` means there was nothing eligible to write (the + * webui's catalogue is empty or every provider was ineligible). + * - { ok: false, code: 'ENGINE_SYNC_FAILED', error: string } + * + * Never throws — surfaces every failure as a structured result so the + * route handler can attach the error to the response without try/catch. + */ +export async function syncProvidersToEngine(providers, opts = {}) { + const configPath = opts.configPath || getEngineConfigPath(); + try { + const eligible = []; + for (const p of providers || []) { + const out = toEngineCustomProvider(p); + if (out) eligible.push(out); + } + const existing = readEngineConfigRaw(configPath); + if (existing === null) { + return { + ok: false, + code: "ENGINE_SYNC_FAILED", + error: `engine config at ${configPath} is unreadable (YAML parse error)`, + }; + } + // Preserve every operator-owned section; only `custom_provider` is + // owned by this helper. The engine's `defaultModel` (when pointing at + // a `custom_provider:/`) is left to the operator — the + // dialog still resolves a model on demand via `applyRecordedModel`. + const next = { ...existing }; + if (eligible.length === 0) { + // Nothing eligible — leave the engine's tree untouched rather than + // wipe an operator's manual custom_provider entries. The webui + // records a no-op result so the route can log it. + const existingCustom = existing.custom_provider || {}; + const existingKeys = Object.keys(existingCustom).filter((k) => + k.endsWith("-byok"), + ); + return { ok: true, written: false, keys: existingKeys }; + } + const customProvider = {}; + for (const { key, entry } of eligible) { + customProvider[key] = entry; + } + next.custom_provider = customProvider; + await atomicWriteYaml(configPath, next); + return { ok: true, written: true, keys: eligible.map((e) => e.key) }; + } catch (e) { + return { + ok: false, + code: "ENGINE_SYNC_FAILED", + error: e && e.message ? e.message : String(e), + }; + } +} + +/** + * Compatibility wrapper for the routes that already have the raw PUT body + * in hand (they apply keep-key convention themselves for the user-level + * file write). This wraps the body in the same shape the route would pass + * to `syncProvidersToEngine` after normalisation, so the sync sees the + * same provider list the engine should advertise. + */ +export async function syncProvidersFromPutBody(parsedBody, existingUserLevel, opts) { + const incoming = Array.isArray(parsedBody?.providers) ? parsedBody.providers : null; + if (incoming === null) { + return { ok: false, code: "BAD_BODY", error: "providers must be an array" }; + } + const resolved = applyKeepKeyConvention(existingUserLevel || [], incoming); + return syncProvidersToEngine(resolved, opts); +} \ No newline at end of file diff --git a/packages/webui/server/lib/mcode-acp.js b/packages/webui/server/lib/mcode-acp.js index 5ba400ed..3b2d07c6 100644 --- a/packages/webui/server/lib/mcode-acp.js +++ b/packages/webui/server/lib/mcode-acp.js @@ -188,6 +188,16 @@ function matchesModelId(recorded, engineCurrent, modelOption) { * after the last separator in the recorded id) matching exactly one * option → return that option's `value`; * - multiple matches or none → null (caller skips). + * + * Ticket 05: the bare-name match is case-insensitive. The engine + * populates `option.name` from the user-supplied model label (e.g. + * `GLM-5.3` for a custom provider whose label happens to differ in + * case from the model id), while the webui records the model id in + * `cs.model.name` (e.g. `glm-5.3`). A strict comparison would skip + * the apply and leave the engine on its default. The recorded id is + * authoritative — when only one option matches case-insensitively, + * that option is the right target. (Multiple case-insensitive + * matches still returns null; ambiguity is ambiguity.) */ function resolveModelId(recorded, modelOption) { if (!modelOption || !Array.isArray(modelOption.options)) return null; @@ -199,7 +209,9 @@ function resolveModelId(recorded, modelOption) { if (o.value === recorded) return o.value; } const bareName = lastSegment(recorded); - const matches = options.filter((o) => o.name === bareName); + const matches = options.filter( + (o) => typeof o.name === "string" && o.name.toLowerCase() === bareName.toLowerCase(), + ); if (matches.length === 1) return matches[0].value; return null; } diff --git a/packages/webui/server/routes/providers.js b/packages/webui/server/routes/providers.js index 5c887727..681c0287 100644 --- a/packages/webui/server/routes/providers.js +++ b/packages/webui/server/routes/providers.js @@ -59,6 +59,10 @@ import { loadUserLevelProviders, normaliseProvider, } from "../lib/providers-config.js"; +import { + syncProvidersToEngine, + syncProvidersFromPutBody, +} from "../lib/engine-provider-sync.js"; import { PROVIDER_PRESETS, publicPresetView, @@ -67,6 +71,7 @@ import { } from "../lib/provider-presets.js"; import { pushStateFor, sseByCid } from "../lib/state-bus.js"; import { readJson } from "../lib/read-json.js"; +import { shutdownMcodeAcpSingleton } from "../lib/acp-client.js"; /** * GET /api/providers — masked catalogue + resolved-layer summary. @@ -146,12 +151,13 @@ export async function handlePutProviders(req, res, _ctx) { // or non-array providers list is an error the original validation // surfaces as BAD_BODY, and we must not change that behaviour. const incomingProviders = Array.isArray(parsed.providers) ? parsed.providers : null; +const existingUserLevel = loadUserLevelProviders(); const toWrite = incomingProviders === null ? parsed : { ...parsed, - providers: applyKeepKeyConvention(loadUserLevelProviders(), incomingProviders), + providers: applyKeepKeyConvention(existingUserLevel, incomingProviders), }; const result = writeProvidersConfig(toWrite); if (!result.ok) { @@ -161,6 +167,26 @@ const result = writeProvidersConfig(toWrite); JSON.stringify({ ok: false, code: result.code, error: result.error }), ); } + // ticket 05: project the same providers into the engine's + // `custom_provider` tree so the engine's `model` config option + // (packages/tui/src/acp/control-state.ts) advertises them and + // `applyRecordedModel` can resolve them. We run the sync AFTER + // the user-level file is durable so a sync failure cannot leave the + // engine advertising something the user-level file does not have. + // Surface the error in the response (acceptance criterion 1) but + // keep the response status 200 — the user-level write succeeded, + // the dialog refresh reflects the new catalogue, and the operator + // can retry the sync on the next PUT. The `engineSync` field lets + // the UI surface a non-blocking warning. + const engineSync = await syncProvidersToEngine(result.providers); + if (engineSync.ok) { + // Tear down the singleton subprocess so the next operation + // spawns a fresh one that reads the new config.yaml. Brand-new + // prompt subprocesses spawned by `runMcodeAcp` already pick up + // the latest config; this is only about the singleton used for + // session/list, commands probe, and account status. + shutdownMcodeAcpSingleton(); + } // Reload + broadcast. `loadProvidersConfig()` re-reads the file on // every call (no in-process cache), so a follow-up GET already // sees the change. The SSE push is the mechanism the UI uses to @@ -177,6 +203,22 @@ const result = writeProvidersConfig(toWrite); ok: true, providers: result.providers.map(publicView), path: result.path, + ...(engineSync.ok + ? { + engineSync: { + ok: true, + written: engineSync.written, + keys: engineSync.keys, + }, + } + : { + engineSync: { + ok: false, + code: engineSync.code, + error: engineSync.error, + }, + warning: `engine config sync failed: ${engineSync.error}`, + }), }), ); } @@ -431,6 +473,17 @@ export async function handleEnablePreset(req, res, _ctx, params = {}) { JSON.stringify({ ok: false, code: result.code, error: result.error }), ); } + // ticket 05: project to the engine's custom_provider tree as + // well. The preset itself lands without an apiKey (the user must + // supply one), so the sync sees an "enabled without key" record + // and correctly skips it — but the same shape runs through the + // PUT path's logic when the user later supplies a key and saves + // again. We still call the sync so a non-preset byok provider the + // user already has flows through with no behaviour change. + const engineSync = await syncProvidersToEngine(result.providers); + if (engineSync.ok) { + shutdownMcodeAcpSingleton(); + } // Broadcast — same SSE event PUT uses. The UI's model picker // re-fetches /api/models after this, picking up the new // template-driven entries. @@ -446,6 +499,22 @@ export async function handleEnablePreset(req, res, _ctx, params = {}) { alreadyEnabled: false, provider: publicView(persisted), path: result.path, + ...(engineSync.ok + ? { + engineSync: { + ok: true, + written: engineSync.written, + keys: engineSync.keys, + }, + } + : { + engineSync: { + ok: false, + code: engineSync.code, + error: engineSync.error, + }, + warning: `engine config sync failed: ${engineSync.error}`, + }), }), ); } diff --git a/packages/webui/test/lib/engine-provider-sync.test.js b/packages/webui/test/lib/engine-provider-sync.test.js new file mode 100644 index 00000000..3888370f --- /dev/null +++ b/packages/webui/test/lib/engine-provider-sync.test.js @@ -0,0 +1,553 @@ +// webui/test/lib/engine-provider-sync.test.js +// Pure helpers + sync flow for `lib/engine-provider-sync.js` (ticket 05). +// +// What we pin here: +// - providerKeyFromId: reserved engine ids get a `-byok` suffix; +// everything else round-trips; non-conforming ids return "". +// - modelKeyFromId: same shape as providerKeyFromId, no reserved handling. +// - toEngineCustomProvider: every field of the v2 schema is mapped; the +// four ineligible shapes (coding-plan, disabled, empty apiKey, +// missing baseURL, unknown protocol) yield null; the engine api +// format is picked by protocol. +// - syncProvidersToEngine: writes an atomic YAML that preserves the +// operator's other sections (provider.*, defaultModel, …), only +// `custom_provider` is owned by the helper; an empty eligible list is +// a no-op (operator's manual entries are kept); engine-config read +// failures are surfaced as a structured error. +// - syncProvidersFromPutBody: applies the keep-key convention before +// the sync (same path the routes use for the user-level write), so +// the engine sees the resolved apiKey. + +import { test, describe, after, beforeEach } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, rmSync, existsSync, readFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import yaml from "js-yaml"; + +const absPath = (rel) => + import.meta.resolve + ? import.meta.resolve(rel) + : new URL(rel, import.meta.url).href; + +const { + resolveEngineDataDir, + providerKeyFromId, + modelKeyFromId, + toEngineCustomProvider, + syncProvidersToEngine, + syncProvidersFromPutBody, +} = await import( + new URL("../../server/lib/engine-provider-sync.js", import.meta.url).href +); + +// temp data dir scoped to this test file so the helper's +// `resolveEngineDataDir()` (env-driven) does not point at the host's +// real engine config. Set at module load time — the helper reads +// `process.env` at call time, but Node's test runner may run setup +// before `before()` fires, and a stable env at import time is the +// safest contract. +const _origMinimax = process.env.MINIMAX_DATA_DIR; +const _origMavis = process.env.MAVIS_DATA_DIR; +const _tmpDataDir = mkdtempSync(join(tmpdir(), "minimax-code-engine-sync-")); +process.env.MINIMAX_DATA_DIR = _tmpDataDir; +delete process.env.MAVIS_DATA_DIR; + +after(() => { + if (_origMinimax === undefined) delete process.env.MINIMAX_DATA_DIR; + else process.env.MINIMAX_DATA_DIR = _origMinimax; + if (_origMavis === undefined) delete process.env.MAVIS_DATA_DIR; + else process.env.MAVIS_DATA_DIR = _origMavis; + rmSync(_tmpDataDir, { recursive: true, force: true }); +}); + +describe("resolveEngineDataDir — env precedence", () => { + test("MINIMAX_DATA_DIR wins", () => { + const prev = process.env.MINIMAX_DATA_DIR; + process.env.MINIMAX_DATA_DIR = "/tmp/env-wins"; + try { + assert.equal(resolveEngineDataDir(), "/tmp/env-wins"); + } finally { + // Restore so the sync tests downstream still resolve to the + // file-scoped `_tmpDataDir`. + if (prev === undefined) delete process.env.MINIMAX_DATA_DIR; + else process.env.MINIMAX_DATA_DIR = prev; + } + }); + test("falls back to ~/.minimax when neither env is set", () => { + // Restore the per-test env to the no-env state only for this test; + // every other test in the file relies on the `before` hook's + // `_tmpDataDir` so we MUST put it back before returning, or the + // sync tests downstream would resolve to the host's real config. + const prev = process.env.MINIMAX_DATA_DIR; + const prevMavis = process.env.MAVIS_DATA_DIR; + delete process.env.MINIMAX_DATA_DIR; + delete process.env.MAVIS_DATA_DIR; + try { + assert.match(resolveEngineDataDir(), /[/\\]\.minimax$/); + } finally { + if (prev === undefined) delete process.env.MINIMAX_DATA_DIR; + else process.env.MINIMAX_DATA_DIR = prev; + if (prevMavis === undefined) delete process.env.MAVIS_DATA_DIR; + else process.env.MAVIS_DATA_DIR = prevMavis; + } + }); +}); + +describe("providerKeyFromId", () => { + test("round-trips a normal id", () => { + assert.equal(providerKeyFromId("byok-zhipu"), "byok-zhipu"); + assert.equal(providerKeyFromId("kimi"), "kimi"); + }); + test("disambiguates engine-internal reserved ids with -byok suffix", () => { + // The webui id validator allows these names (the engine just would + // not — operators might already have one in their providers.json). + // Projection must not collide with the engine's internal providers. + assert.equal(providerKeyFromId("minimax"), "minimax-byok"); + assert.equal(providerKeyFromId("minimax_api"), "minimax_api-byok"); + assert.equal(providerKeyFromId("provider"), "provider-byok"); + assert.equal(providerKeyFromId("custom_provider"), "custom_provider-byok"); + }); + test("rejects ids outside the provider-key character class", () => { + assert.equal(providerKeyFromId(""), ""); + assert.equal(providerKeyFromId("spaces are bad"), ""); + assert.equal(providerKeyFromId("slashes/are/bad"), ""); + // Underscores / hyphens / dots in the middle are fine (matches the + // webui v2 validator's provider id contract). + assert.equal(providerKeyFromId("a_b.c-d"), "a_b.c-d"); + }); +}); + +describe("modelKeyFromId", () => { + test("round-trips a normal id", () => { + assert.equal(modelKeyFromId("glm-5.3"), "glm-5.3"); + assert.equal(modelKeyFromId("claude-sonnet-4-5"), "claude-sonnet-4-5"); + }); + test("rejects ids outside the model-key character class", () => { + assert.equal(modelKeyFromId(""), ""); + assert.equal(modelKeyFromId("with space"), ""); + }); +}); + +describe("toEngineCustomProvider — eligibility", () => { + const base = { + id: "byok-zhipu", + label: "Zhipu BYOK", + enabled: true, + protocol: "openai", + auth: { type: "byok", apiKey: "sk-fake", baseURL: "https://example.com/v1" }, + models: [{ id: "glm-5.3" }], + }; + + test("eligible byok provider maps cleanly", () => { + const out = toEngineCustomProvider(base); + assert.ok(out, "eligible"); + assert.equal(out.key, "byok-zhipu"); + assert.equal(out.entry.name, "Zhipu BYOK"); + assert.equal(out.entry.kind, "custom"); + assert.equal(out.entry.enabled, true); + assert.equal(out.entry.api, "openai-completions"); + assert.equal(out.entry.options.apiKey, "sk-fake"); + assert.equal(out.entry.options.baseURL, "https://example.com/v1"); + assert.equal(out.entry.options.authMode, "api-key"); + assert.deepEqual(out.entry.models, { "glm-5.3": {} }); + }); + + test("coding-plan providers are skipped (out of scope for byok projection)", () => { + const p = { ...base, auth: { type: "coding-plan", apiKey: "tk-fake", baseURL: "https://example.com" } }; + assert.equal(toEngineCustomProvider(p), null); + }); + + test("disabled providers are skipped", () => { + const p = { ...base, enabled: false }; + assert.equal(toEngineCustomProvider(p), null); + }); + + test("providers without an apiKey are skipped", () => { + const p = { ...base, auth: { type: "byok", baseURL: "https://example.com/v1" } }; + assert.equal(toEngineCustomProvider(p), null); + // Also: apiKey must be a non-empty trimmed string. + const p2 = { ...base, auth: { type: "byok", apiKey: " ", baseURL: "https://example.com/v1" } }; + assert.equal(toEngineCustomProvider(p2), null); + }); + + test("providers without a baseURL are skipped", () => { + const p = { ...base, auth: { type: "byok", apiKey: "sk-fake" } }; + assert.equal(toEngineCustomProvider(p), null); + const p2 = { ...base, auth: { type: "byok", apiKey: "sk-fake", baseURL: " " } }; + assert.equal(toEngineCustomProvider(p2), null); + }); + + test("unknown protocol → skipped", () => { + const p = { ...base, protocol: "cohere" }; + assert.equal(toEngineCustomProvider(p), null); + }); + + test("null / non-object → null", () => { + assert.equal(toEngineCustomProvider(null), null); + assert.equal(toEngineCustomProvider("string"), null); + assert.equal(toEngineCustomProvider(undefined), null); + }); + + test("id that hits an engine-reserved word gets the -byok suffix", () => { + const p = { + ...base, + id: "minimax", + label: "Custom MiniMax-shaped alias", + }; + const out = toEngineCustomProvider(p); + assert.ok(out); + assert.equal(out.key, "minimax-byok"); + }); + + test("non-conforming id (spaces) → skipped", () => { + const p = { ...base, id: "byok with space" }; + assert.equal(toEngineCustomProvider(p), null); + }); +}); + +describe("toEngineCustomProvider — protocol → engine api mapping", () => { + test("openai → openai-completions", () => { + const p = { ...base({ id: "p1", protocol: "openai" }) }; + assert.equal(toEngineCustomProvider(p).entry.api, "openai-completions"); + }); + test("anthropic → anthropic-messages", () => { + const p = { ...base({ id: "p2", protocol: "anthropic" }) }; + assert.equal(toEngineCustomProvider(p).entry.api, "anthropic-messages"); + }); + test("gemini → openai-completions (Gemini OpenAI-compat endpoint)", () => { + const p = { ...base({ id: "p3", protocol: "gemini" }) }; + assert.equal(toEngineCustomProvider(p).entry.api, "openai-completions"); + }); + test("missing protocol defaults to openai-completions", () => { + const p = { + id: "p4", + label: "P4", + enabled: true, + auth: { type: "byok", apiKey: "sk", baseURL: "https://x/v1" }, + models: [], + }; + assert.equal(toEngineCustomProvider(p).entry.api, "openai-completions"); + }); + + function base(overrides) { + return { + label: "x", + enabled: true, + protocol: "openai", + auth: { type: "byok", apiKey: "sk-fake", baseURL: "https://x/v1" }, + models: [], + ...overrides, + }; + } +}); + +describe("toEngineCustomProvider — model metadata mapping", () => { + test("contextLimit → limit.context", () => { + const p = { + id: "p1", + label: "p1", + enabled: true, + protocol: "openai", + auth: { type: "byok", apiKey: "sk", baseURL: "https://x/v1" }, + models: [{ id: "m1", contextLimit: 128000 }], + }; + assert.deepEqual(toEngineCustomProvider(p).entry.models.m1, { + limit: { context: 128000 }, + }); + }); + + test("thinkingLevels → thinking.effortOptions", () => { + const p = { + id: "p1", + label: "p1", + enabled: true, + protocol: "openai", + auth: { type: "byok", apiKey: "sk", baseURL: "https://x/v1" }, + models: [{ id: "m1", thinkingLevels: ["low", "high"] }], + }; + assert.deepEqual(toEngineCustomProvider(p).entry.models.m1, { + thinking: { effortOptions: ["low", "high"] }, + }); + }); + + test("modalities → modalities.input", () => { + const p = { + id: "p1", + label: "p1", + enabled: true, + protocol: "openai", + auth: { type: "byok", apiKey: "sk", baseURL: "https://x/v1" }, + models: [{ id: "m1", modalities: ["text", "image"] }], + }; + assert.deepEqual(toEngineCustomProvider(p).entry.models.m1, { + modalities: { input: ["text", "image"] }, + }); + }); + + test("label different from id → name; label === id → no name (engine default)", () => { + const p = { + id: "p1", + label: "p1", + enabled: true, + protocol: "openai", + auth: { type: "byok", apiKey: "sk", baseURL: "https://x/v1" }, + models: [ + { id: "m1", label: "m1" }, + { id: "m2", label: "Model Two" }, + ], + }; + const out = toEngineCustomProvider(p); + assert.equal(out.entry.models.m1.name, undefined); + assert.equal(out.entry.models.m2.name, "Model Two"); + }); + + test("non-string thinkingLevels entries are filtered out", () => { + const p = { + id: "p1", + label: "p1", + enabled: true, + protocol: "openai", + auth: { type: "byok", apiKey: "sk", baseURL: "https://x/v1" }, + models: [{ id: "m1", thinkingLevels: ["low", 42, "", "high"] }], + }; + // Empty string entries are dropped; non-strings cause the array to + // fail the every-check and the whole thinkingLevels field is omitted. + assert.equal(toEngineCustomProvider(p).entry.models.m1.thinking, undefined); + }); + + test("non-positive contextLimit is dropped", () => { + const p = { + id: "p1", + label: "p1", + enabled: true, + protocol: "openai", + auth: { type: "byok", apiKey: "sk", baseURL: "https://x/v1" }, + models: [{ id: "m1", contextLimit: 0 }], + }; + assert.equal(toEngineCustomProvider(p).entry.models.m1.limit, undefined); + }); + + test("provider with no models still gets an entry (operators can add later)", () => { + const p = { + id: "p1", + label: "p1", + enabled: true, + protocol: "openai", + auth: { type: "byok", apiKey: "sk", baseURL: "https://x/v1" }, + models: [], + }; + const out = toEngineCustomProvider(p); + assert.ok(out); + assert.equal(out.entry.models, undefined); + }); +}); + +describe("syncProvidersToEngine — atomic YAML write + operator preservation", () => { + beforeEach(() => { + // Reset the per-test engine config. + rmSync(join(_tmpDataDir, "config.yaml"), { force: true }); + }); + + test("writes custom_provider from the merged catalogue", async () => { + const providers = [ + { + id: "byok-zhipu", + label: "Zhipu", + enabled: true, + protocol: "openai", + auth: { type: "byok", apiKey: "sk-fake", baseURL: "https://example.com/v1" }, + models: [{ id: "glm-5.3" }], + }, + ]; + const r = await syncProvidersToEngine(providers); + assert.equal(r.ok, true); + assert.equal(r.written, true); + assert.deepEqual(r.keys, ["byok-zhipu"]); + assert.ok(existsSync(join(_tmpDataDir, "config.yaml")), "file must exist"); + + const written = yaml.load(readFileSync(join(_tmpDataDir, "config.yaml"), "utf8")); + assert.ok(written.custom_provider); + assert.equal(written.custom_provider["byok-zhipu"].options.apiKey, "sk-fake"); + assert.equal(written.custom_provider["byok-zhipu"].options.baseURL, "https://example.com/v1"); + assert.equal(written.custom_provider["byok-zhipu"].kind, "custom"); + }); + + test("preserves the operator's provider.minimax + defaultModel sections", async () => { + // Pre-populate the engine config as an operator would. + const seed = { + logLevel: "info", + defaultModel: "minimax/MiniMax-M3", + provider: { + minimax: { + options: { apiKey: "sk-existing", authMode: "api-key", baseURL: "https://x/v1" }, + }, + }, + custom_provider: { existing_byok: { name: "Existing", kind: "custom", enabled: true } }, + }; + const fs = await import("node:fs/promises"); + await fs.mkdir(_tmpDataDir, { recursive: true }); + await fs.writeFile(join(_tmpDataDir, "config.yaml"), yaml.dump(seed), "utf8"); + + const r = await syncProvidersToEngine([ + { + id: "byok-new", + label: "New", + enabled: true, + protocol: "anthropic", + auth: { type: "byok", apiKey: "sk-new", baseURL: "https://y/v1" }, + models: [], + }, + ]); + assert.equal(r.ok, true); + const after = yaml.load(readFileSync(join(_tmpDataDir, "config.yaml"), "utf8")); + // Provider tree survives — operator's manual config is not touched. + assert.equal(after.provider.minimax.options.apiKey, "sk-existing"); + assert.equal(after.defaultModel, "minimax/MiniMax-M3"); + // custom_provider is replaced wholesale by the helper's projection. + // Operators who need a manual entry in production should use + // `mcode provider add`, not the webui PUT — the PUT path's contract + // is "this catalogue is the source of truth for engine sync". + assert.equal(after.custom_provider["byok-new"].options.apiKey, "sk-new"); + }); + + test("empty catalogue is a no-op write (engine config left untouched)", async () => { + const fs = await import("node:fs/promises"); + await fs.mkdir(_tmpDataDir, { recursive: true }); + const seed = { defaultModel: "minimax/MiniMax-M3" }; + await fs.writeFile(join(_tmpDataDir, "config.yaml"), yaml.dump(seed), "utf8"); + + const r = await syncProvidersToEngine([ + // Coding-plan → ineligible. + { id: "x", label: "x", enabled: true, protocol: "openai", auth: { type: "coding-plan" }, models: [] }, + // No apiKey → ineligible. + { id: "y", label: "y", enabled: true, protocol: "openai", auth: { type: "byok", baseURL: "https://z" }, models: [] }, + ]); + assert.equal(r.ok, true); + assert.equal(r.written, false); + // Engine config wasn't replaced. + const after = yaml.load(readFileSync(join(_tmpDataDir, "config.yaml"), "utf8")); + assert.equal(after.defaultModel, "minimax/MiniMax-M3"); + }); + + test("missing engine config file → creates one", async () => { + rmSync(join(_tmpDataDir, "config.yaml"), { force: true }); + const r = await syncProvidersToEngine([ + { + id: "byok-zhipu", + label: "z", + enabled: true, + protocol: "openai", + auth: { type: "byok", apiKey: "sk", baseURL: "https://x/v1" }, + models: [], + }, + ]); + assert.equal(r.ok, true); + assert.equal(existsSync(join(_tmpDataDir, "config.yaml")), true); + }); + + test("atomic write: no half-written file on success", async () => { + rmSync(join(_tmpDataDir, "config.yaml"), { force: true }); + await syncProvidersToEngine([ + { + id: "byok-zhipu", + label: "z", + enabled: true, + protocol: "openai", + auth: { type: "byok", apiKey: "sk", baseURL: "https://x/v1" }, + models: [], + }, + ]); + // No `.config-tmp-` leftover should exist next to config.yaml. + const fs = await import("node:fs"); + const siblings = fs.readdirSync(_tmpDataDir); + const tmps = siblings.filter((n) => n.startsWith(".config-tmp-")); + assert.equal(tmps.length, 0); + }); + + test("skips ineligible records but writes eligible ones from the same list", async () => { + const r = await syncProvidersToEngine([ + { id: "eligible", label: "ok", enabled: true, protocol: "openai", auth: { type: "byok", apiKey: "sk", baseURL: "https://x/v1" }, models: [] }, + { id: "no-key", label: "nokey", enabled: true, protocol: "openai", auth: { type: "byok", baseURL: "https://x/v1" }, models: [] }, + { id: "disabled", label: "off", enabled: false, protocol: "openai", auth: { type: "byok", apiKey: "sk", baseURL: "https://x/v1" }, models: [] }, + ]); + assert.equal(r.ok, true); + assert.equal(r.written, true); + assert.deepEqual(r.keys, ["eligible"]); + const after = yaml.load(readFileSync(join(_tmpDataDir, "config.yaml"), "utf8")); + assert.ok(after.custom_provider.eligible); + assert.equal(after.custom_provider["no-key"], undefined); + assert.equal(after.custom_provider["disabled"], undefined); + }); +}); + +describe("syncProvidersFromPutBody — keep-key convention applied", () => { + beforeEach(() => { + rmSync(join(_tmpDataDir, "config.yaml"), { force: true }); + }); + + test("absent apiKey on an existing record is filled from the previous user-level entry", async () => { + const existing = [ + { + id: "byok-zhipu", + label: "Zhipu", + enabled: true, + protocol: "openai", + auth: { type: "byok", apiKey: "sk-from-user-level", baseURL: "https://x/v1" }, + models: [], + }, + ]; + const body = { + providers: [ + { + id: "byok-zhipu", + label: "Zhipu", + enabled: true, + protocol: "openai", + // No auth.apiKey — convention: keep the existing one. + auth: { type: "byok", baseURL: "https://x/v1" }, + models: [], + }, + ], + }; + const r = await syncProvidersFromPutBody(body, existing); + assert.equal(r.ok, true); + const after = yaml.load(readFileSync(join(_tmpDataDir, "config.yaml"), "utf8")); + assert.equal(after.custom_provider["byok-zhipu"].options.apiKey, "sk-from-user-level"); + }); + + test("non-array providers in the body surfaces a BAD_BODY error", async () => { + const r = await syncProvidersFromPutBody({ providers: "not-an-array" }, []); + assert.equal(r.ok, false); + assert.equal(r.code, "BAD_BODY"); + }); + + test("explicit apiKey in the PUT body replaces the existing key", async () => { + const existing = [ + { + id: "byok-zhipu", + label: "z", + enabled: true, + protocol: "openai", + auth: { type: "byok", apiKey: "sk-old", baseURL: "https://x/v1" }, + models: [], + }, + ]; + const body = { + providers: [ + { + id: "byok-zhipu", + label: "z", + enabled: true, + protocol: "openai", + auth: { type: "byok", apiKey: "sk-new", baseURL: "https://x/v1" }, + models: [], + }, + ], + }; + const r = await syncProvidersFromPutBody(body, existing); + assert.equal(r.ok, true); + const after = yaml.load(readFileSync(join(_tmpDataDir, "config.yaml"), "utf8")); + assert.equal(after.custom_provider["byok-zhipu"].options.apiKey, "sk-new"); + }); +}); \ No newline at end of file diff --git a/packages/webui/test/lib/mcode-acp-note.test.js b/packages/webui/test/lib/mcode-acp-note.test.js index fda6d0a4..f0768ed6 100644 --- a/packages/webui/test/lib/mcode-acp-note.test.js +++ b/packages/webui/test/lib/mcode-acp-note.test.js @@ -180,9 +180,11 @@ describe("applyConfigOptionUpdate — propagate model + permissionMode (defect # id: "model", type: "select", currentValue: model, + // Engine wire format (m:::u|v:); see + // packages/tui/src/acp/control-state.ts#modelConfigValue. options: [ - { value: "minimax_api:MiniMax-M3", name: "MiniMax-M3" }, - { value: "minimax_api:MiniMax-M2.7", name: "MiniMax-M2.7" }, + { value: "m:minimax:MiniMax-M3:u", name: "MiniMax-M3" }, + { value: "m:minimax:MiniMax-M2.7:u", name: "MiniMax-M2.7" }, ], }); } @@ -191,62 +193,62 @@ describe("applyConfigOptionUpdate — propagate model + permissionMode (defect # test("propagates a model change into cs.model.name", () => { const cs = { - model: { name: "minimax_api:MiniMax-M3" }, + model: { name: "m:minimax:MiniMax-M3:u" }, permissions: "Full access", }; applyConfigOptionUpdate(cs, { - configOptions: optsFor({ model: "minimax_api:MiniMax-M2.7" }), + configOptions: optsFor({ model: "m:minimax:MiniMax-M2.7:u" }), }); - assert.equal(cs.model.name, "minimax_api:MiniMax-M2.7"); + assert.equal(cs.model.name, "m:minimax:MiniMax-M2.7:u"); }); test("propagates both model and permissionMode in the same update", () => { const cs = { - model: { name: "minimax_api:MiniMax-M3" }, + model: { name: "m:minimax:MiniMax-M3:u" }, permissions: "Full access", }; applyConfigOptionUpdate(cs, { configOptions: optsFor({ - model: "minimax_api:MiniMax-M2.7", + model: "m:minimax:MiniMax-M2.7:u", permissionMode: "default", }), }); - assert.equal(cs.model.name, "minimax_api:MiniMax-M2.7"); + assert.equal(cs.model.name, "m:minimax:MiniMax-M2.7:u"); assert.equal(cs.permissions, "Ask"); }); test("leaves cs.model alone when the model option is absent", () => { - const cs = { model: { name: "minimax_api:MiniMax-M3" }, permissions: "Full access" }; + const cs = { model: { name: "m:minimax:MiniMax-M3:u" }, permissions: "Full access" }; applyConfigOptionUpdate(cs, { configOptions: optsFor({ permissionMode: "default" }), }); - assert.equal(cs.model.name, "minimax_api:MiniMax-M3", "no model option → model untouched"); + assert.equal(cs.model.name, "m:minimax:MiniMax-M3:u", "no model option → model untouched"); assert.equal(cs.permissions, "Ask"); }); test("leaves cs.model alone when the model option has no currentValue", () => { - const cs = { model: { name: "minimax_api:MiniMax-M3" }, permissions: "Full access" }; + const cs = { model: { name: "m:minimax:MiniMax-M3:u" }, permissions: "Full access" }; applyConfigOptionUpdate(cs, { configOptions: [ { id: "model", type: "select", currentValue: null, options: [] }, { id: "permissionMode", type: "select", currentValue: "default" }, ], }); - assert.equal(cs.model.name, "minimax_api:MiniMax-M3", "empty currentValue → model untouched"); + assert.equal(cs.model.name, "m:minimax:MiniMax-M3:u", "empty currentValue → model untouched"); }); test("initialises cs.model when only the model option arrived (no prior cs.model)", () => { const cs = { permissions: "Full access" }; applyConfigOptionUpdate(cs, { - configOptions: optsFor({ model: "minimax_api:MiniMax-M2.7" }), + configOptions: optsFor({ model: "m:minimax:MiniMax-M2.7:u" }), }); - assert.deepEqual(cs.model, { name: "minimax_api:MiniMax-M2.7" }); + assert.deepEqual(cs.model, { name: "m:minimax:MiniMax-M2.7:u" }); }); test("ignores an update with no configOptions array", () => { - const cs = { model: { name: "minimax_api:MiniMax-M3" }, permissions: "Full access" }; + const cs = { model: { name: "m:minimax:MiniMax-M3:u" }, permissions: "Full access" }; applyConfigOptionUpdate(cs, { /* no configOptions */ }); - assert.equal(cs.model.name, "minimax_api:MiniMax-M3"); + assert.equal(cs.model.name, "m:minimax:MiniMax-M3:u"); assert.equal(cs.permissions, "Full access"); }); }); @@ -261,33 +263,33 @@ describe("applyConfigOptionUpdate — propagate model + permissionMode (defect # describe("applyConfigOptionUpdate — propagate thinkingEffort (ticket 04)", () => { function optsWithThinking(thinking) { return [ - { id: "model", type: "select", currentValue: "minimax_api:MiniMax-M3", options: [] }, + { id: "model", type: "select", currentValue: "m:minimax:MiniMax-M3:v:", options: [] }, { id: "thinkingEffort", type: "select", currentValue: thinking, options: [] }, ]; } test("propagates a thinkingEffort change into cs.model.thinking", () => { - const cs = { model: { name: "minimax_api:MiniMax-M3", thinking: "low" }, permissions: "Full access" }; + const cs = { model: { name: "m:minimax:MiniMax-M3:v:", thinking: "low" }, permissions: "Full access" }; applyConfigOptionUpdate(cs, { configOptions: optsWithThinking("high") }); assert.equal(cs.model.thinking, "high"); - assert.equal(cs.model.name, "minimax_api:MiniMax-M3"); + assert.equal(cs.model.name, "m:minimax:MiniMax-M3:v:"); }); test("clears cs.model.thinking when the engine clears its currentValue", () => { - const cs = { model: { name: "minimax_api:MiniMax-M3", thinking: "high" }, permissions: "Full access" }; + const cs = { model: { name: "m:minimax:MiniMax-M3:v:", thinking: "high" }, permissions: "Full access" }; applyConfigOptionUpdate(cs, { configOptions: optsWithThinking("") }); assert.equal( Object.prototype.hasOwnProperty.call(cs.model, "thinking"), false, "thinking field dropped — picker shows no override", ); - assert.equal(cs.model.name, "minimax_api:MiniMax-M3"); + assert.equal(cs.model.name, "m:minimax:MiniMax-M3:v:"); }); test("leaves cs.model alone when no thinkingEffort option is in the update", () => { - const cs = { model: { name: "minimax_api:MiniMax-M3", thinking: "low" }, permissions: "Full access" }; + const cs = { model: { name: "m:minimax:MiniMax-M3:v:", thinking: "low" }, permissions: "Full access" }; applyConfigOptionUpdate(cs, { - configOptions: [{ id: "model", type: "select", currentValue: "minimax_api:MiniMax-M3", options: [] }], + configOptions: [{ id: "model", type: "select", currentValue: "m:minimax:MiniMax-M3:v:", options: [] }], }); assert.equal(cs.model.thinking, "low", "untouched when option absent"); }); @@ -302,16 +304,30 @@ describe("applyConfigOptionUpdate — propagate thinkingEffort (ticket 04)", () // or accepts a recorded id without checking it against the engine's // options, would re-introduce "engine ran on default while chip showed // the user's pick". -// ============================================================ - +// +// Wire format note (ticket 05): the engine sends `option.value` in the +// `m:::u` (or `:v:`) shape — +// see packages/tui/src/acp/control-state.ts#modelConfigValue. The webui +// records `cs.model.name` in the user-facing `provider/model` form. The +// resolution logic below pins both shapes so a future test refactor +// cannot silently regress either direction. const MODEL_OPTION = { type: "select", id: "model", - currentValue: "minimax_api:MiniMax-M3", + currentValue: "m:minimax:MiniMax-M3:v:", options: [ - { value: "minimax_api:MiniMax-M3", name: "MiniMax-M3" }, - { value: "minimax_api:MiniMax-M2.7", name: "MiniMax-M2.7" }, - { value: "minimax_api:MiniMax-M2.5", name: "MiniMax-M2.5" }, + { value: "m:minimax:MiniMax-M3:v:", name: "MiniMax-M3" }, + { value: "m:minimax:MiniMax-M3:v:thinking", name: "MiniMax-M3 · thinking" }, + { value: "m:minimax:MiniMax-M2.7:u", name: "MiniMax-M2.7" }, + { value: "m:minimax:MiniMax-M2.5:u", name: "MiniMax-M2.5" }, + // Ticket 05: a custom_provider option the engine advertises because + // webui synced it into the engine's `custom_provider` registry. + // The wire value is `m:custom_provider%3A::u` — the + // `:` after the provider prefix is URL-encoded. + { + value: "m:custom_provider%3Abyok-zhipu:glm-5.3:u", + name: "glm-5.3", + }, ], }; @@ -349,8 +365,8 @@ describe("matchesModelId — recorded vs engine currentValue", () => { test("matches exact engine-encoded value", () => { assert.equal( matchesModelId( - "minimax_api:MiniMax-M3", - "minimax_api:MiniMax-M3", + "m:minimax:MiniMax-M3:v:", + "m:minimax:MiniMax-M3:v:", MODEL_OPTION, ), true, @@ -364,8 +380,8 @@ describe("matchesModelId — recorded vs engine currentValue", () => { // separately for the early-return shortcut only. assert.equal( matchesModelId( - "minimax_api:MiniMax-M2.5", - "minimax_api:MiniMax-M3", + "m:minimax:MiniMax-M2.5:u", + "m:minimax:MiniMax-M3:v:", MODEL_OPTION, ), true, @@ -374,8 +390,8 @@ describe("matchesModelId — recorded vs engine currentValue", () => { test("an id unknown to the engine's option list does not match", () => { assert.equal( matchesModelId( - "minimax_api:MiniMax-UNKNOWN", - "minimax_api:MiniMax-M3", + "m:minimax:MiniMax-UNKNOWN:u", + "m:minimax:MiniMax-M3:v:", MODEL_OPTION, ), false, @@ -386,8 +402,8 @@ describe("matchesModelId — recorded vs engine currentValue", () => { // is on a different option. assert.equal( matchesModelId( - "minimax_api:MiniMax-M2.7", - "minimax_api:MiniMax-M3", + "m:minimax:MiniMax-M2.7:u", + "m:minimax:MiniMax-M3:v:", MODEL_OPTION, ), true, @@ -398,7 +414,7 @@ describe("matchesModelId — recorded vs engine currentValue", () => { // modelOption; only fall through to the modelOption check when the // recorded id is not the engine's current. assert.equal( - matchesModelId("minimax_api:MiniMax-M2.5", "minimax_api:MiniMax-M3", null), + matchesModelId("m:minimax:MiniMax-M2.5:u", "m:minimax:MiniMax-M3:v:", null), false, ); }); @@ -407,22 +423,35 @@ describe("matchesModelId — recorded vs engine currentValue", () => { describe("resolveModelId — recorded id → engine option.value", () => { test("engine-encoded value matches as-is (no rewrite)", () => { assert.equal( - resolveModelId("minimax_api:MiniMax-M2.5", MODEL_OPTION), - "minimax_api:MiniMax-M2.5", + resolveModelId("m:minimax:MiniMax-M2.5:u", MODEL_OPTION), + "m:minimax:MiniMax-M2.5:u", ); }); test("builtin-catalogue id (`/` separator) matches by bare name", () => { - // The user picked from the builtin catalogue (slash separator) and the - // engine uses colon separator; resolve by `option.name`. + // The user picked from the builtin catalogue (slash separator) and + // the engine's option uses `:`; resolve by `option.name`. assert.equal( - resolveModelId("minimax_api/MiniMax-M2.5", MODEL_OPTION), - "minimax_api:MiniMax-M2.5", + resolveModelId("minimax/MiniMax-M2.5", MODEL_OPTION), + "m:minimax:MiniMax-M2.5:u", ); }); test("bare model name with one matching option resolves to that option", () => { assert.equal( resolveModelId("MiniMax-M2.5", MODEL_OPTION), - "minimax_api:MiniMax-M2.5", + "m:minimax:MiniMax-M2.5:u", + ); + }); + test("custom-provider id (`custom_provider:/`) resolves to the registered engine value", () => { + // Ticket 05 — the cross-provider flow: after PUT /api/providers syncs + // the webui catalogue into the engine's `custom_provider` registry, + // the engine advertises the new model in its `model` config option + // with the URL-encoded `m:custom_provider%3A::u` value. + // The dialog-stored `cs.model.name` is `custom_provider:byok-zhipu/glm-5.3`; + // `resolveModelId` strips to the bare name `glm-5.3` (the only segment + // after the `/`) and finds the unique option.value. + assert.equal( + resolveModelId("custom_provider:byok-zhipu/glm-5.3", MODEL_OPTION), + "m:custom_provider%3Abyok-zhipu:glm-5.3:u", ); }); test("returns null when no option matches (ambiguous or unknown)", () => { @@ -435,7 +464,7 @@ describe("resolveModelId — recorded id → engine option.value", () => { ...MODEL_OPTION, options: [ ...MODEL_OPTION.options, - { value: "minimax_api:MiniMax-M3-other", name: "MiniMax-M3" }, + { value: "m:minimax:MiniMax-M3:v:other", name: "MiniMax-M3" }, ], }; // Two options share the same `name` → caller skips rather than pick @@ -462,7 +491,7 @@ describe("applyRecordedModel — integration with a fake acp client", () => { }, }; const cs = { - model: { name: "minimax_api/MiniMax-M2.5" }, // builtin-catalogue form + model: { name: "minimax/MiniMax-M2.5" }, // builtin-catalogue form (slash) configOptions: [MODEL_OPTION], }; await applyRecordedModel(fakeClient, "sid-1", cs, "cid-1"); @@ -471,18 +500,18 @@ describe("applyRecordedModel — integration with a fake acp client", () => { assert.deepEqual(calls[0][1], { sessionId: "sid-1", configId: "model", - value: "minimax_api:MiniMax-M2.5", // resolved to engine form + value: "m:minimax:MiniMax-M2.5:u", // resolved to engine wire form }); // The local configOptions snapshot reflects the new currentValue, so // the next /api/models reads the same model the engine is running. - assert.equal(cs.configOptions[0].currentValue, "minimax_api:MiniMax-M2.5"); + assert.equal(cs.configOptions[0].currentValue, "m:minimax:MiniMax-M2.5:u"); }); test("skips when the recorded id already matches the engine's currentValue", async () => { const calls = []; const fakeClient = { request: async (m) => { calls.push([m]); return {}; } }; const cs = { - model: { name: "minimax_api:MiniMax-M3" }, + model: { name: "m:minimax:MiniMax-M3:v:" }, configOptions: [MODEL_OPTION], }; await applyRecordedModel(fakeClient, "sid-1", cs, "cid-1"); @@ -493,12 +522,34 @@ describe("applyRecordedModel — integration with a fake acp client", () => { const calls = []; const fakeClient = { request: async (m) => { calls.push([m]); return {}; } }; const cs = { - model: { name: "minimax_api/MiniMax-XYZ" }, + model: { name: "minimax/MiniMax-XYZ" }, configOptions: [MODEL_OPTION], }; await applyRecordedModel(fakeClient, "sid-1", cs, "cid-1"); assert.deepEqual(calls, [], "unknown id → engine default stands"); }); + + test("ticket 05 — cross-provider switching: recorded `custom_provider:/` resolves and applies", async () => { + const calls = []; + const fakeClient = { + request: async (m, p) => { calls.push([m, p]); return {}; }, + }; + const cs = { + model: { name: "custom_provider:byok-zhipu/glm-5.3" }, + configOptions: [MODEL_OPTION], + }; + await applyRecordedModel(fakeClient, "sid-1", cs, "cid-1"); + assert.equal(calls.length, 1); + assert.equal(calls[0][0], "session/set_config_option"); + assert.deepEqual(calls[0][1], { + sessionId: "sid-1", + configId: "model", + // The engine's URL-encoded form is what the runtime accepts on the + // wire (`packages/tui/src/acp/control-state.ts#modelConfigValue`). + value: "m:custom_provider%3Abyok-zhipu:glm-5.3:u", + }); + assert.equal(cs.configOptions[0].currentValue, "m:custom_provider%3Abyok-zhipu:glm-5.3:u"); + }); }); // ============================================================ @@ -542,21 +593,21 @@ describe("applyRecordedModel — thinkingEffort pre-session apply (ticket 04)", test("applies recorded thinkingEffort after the model when both are recorded", async () => { const { calls, client } = clientRecorder(); - const cs = csWithOptions("minimax_api/MiniMax-M2.5", "high"); + const cs = csWithOptions("minimax/MiniMax-M2.5", "high"); await applyRecordedModel(client, "sid-1", cs, "cid-1"); assert.equal(calls.length, 2, "model then effort"); assert.equal(calls[0][0], "session/set_config_option"); assert.equal(calls[0][1].configId, "model"); assert.equal(calls[1][1].configId, "thinkingEffort"); assert.equal(calls[1][1].value, "high"); - // Local config-options mirror reflects both applies. - assert.equal(cs.configOptions[0].currentValue, "minimax_api:MiniMax-M2.5"); + // Local config-options mirror reflects both applies (engine wire format). + assert.equal(cs.configOptions[0].currentValue, "m:minimax:MiniMax-M2.5:u"); assert.equal(cs.configOptions[1].currentValue, "high"); }); test("applies only the effort when the recorded model already matches the engine", async () => { const { calls, client } = clientRecorder(); - const cs = csWithOptions("minimax_api:MiniMax-M3", "medium"); + const cs = csWithOptions("m:minimax:MiniMax-M3:v:", "medium"); await applyRecordedModel(client, "sid-1", cs, "cid-1"); assert.equal(calls.length, 1); assert.equal(calls[0][1].configId, "thinkingEffort"); @@ -565,7 +616,7 @@ describe("applyRecordedModel — thinkingEffort pre-session apply (ticket 04)", test("applies only the model when no thinking level is recorded", async () => { const { calls, client } = clientRecorder(); - const cs = csWithOptions("minimax_api/MiniMax-M2.5", ""); + const cs = csWithOptions("minimax/MiniMax-M2.5", ""); await applyRecordedModel(client, "sid-1", cs, "cid-1"); assert.equal(calls.length, 1); assert.equal(calls[0][1].configId, "model"); @@ -610,7 +661,7 @@ describe("applyRecordedModel — thinkingEffort pre-session apply (ticket 04)", console.warn = (msg) => warnings.push(msg); let cs; try { - cs = csWithOptions("minimax_api/MiniMax-M2.5", "turbo"); + cs = csWithOptions("minimax/MiniMax-M2.5", "turbo"); await applyRecordedModel(client, "sid-1", cs, "cid-1"); } finally { console.warn = origWarn; diff --git a/packages/webui/webapp/components/provider-management.tsx b/packages/webui/webapp/components/provider-management.tsx index 03a1db55..b577422b 100644 --- a/packages/webui/webapp/components/provider-management.tsx +++ b/packages/webui/webapp/components/provider-management.tsx @@ -337,6 +337,37 @@ export function ProviderManagementPanel({ : "no key"} + {/* Configured-models preview (ticket 05): chips for the + configured models with thinking levels / modalities so + the operator sees what's wired up without expanding + the editor. The list is read directly off the + server-side `ProviderView.models[]` — it is the same + shape the dialog surfaces, so what the user sees + here matches the model picker. */} + {p.models.length > 0 ? ( +
+ {p.models.slice(0, 4).map((m) => ( + + {m.label || m.id} + + ))} + {p.models.length > 4 ? ( + + +{p.models.length - 4} + + ) : null} +
+ ) : null} {test ? ( Date: Sat, 26 Sep 2026 20:22:44 +0800 Subject: [PATCH 2/2] fix(webui): preserve foreign engine custom_provider entries; write config.yaml 0600 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ticket 05 acceptance round: merge-over-replace, not replace-everything. The previous sync algorithm replaced the engine's whole custom_provider tree. A manually-added operator entry (e.g. via 'mcode provider add' on the engine CLI) was silently DROPPED on the next webui PUT — same destruction class as the absent-key wipe we hardened in ticket 03. Ownership rule (on-disk fingerprint): Every entry webui writes carries _webui_owned: true. The engine ignores unknown fields (js-yaml parses the whole record and the downstream consumers read named fields only), so the marker is engine-safe. Merge algorithm: existing engine keys ∩ eligible webui keys → UPDATE in place existing engine keys ∖ eligible webui keys: _webui_owned === true → DELETE (webui-owned, operator removed the webui provider) _webui_owned !== true (or missing) → PRESERVE (foreign; operator-owned) eligible webui keys ∖ existing engine keys → ADD A foreign 'manual_only' provider now survives every webui PUT, including the empty-eligible case (PUTting an ineligible-only catalogue no longer wipes foreign entries). The sync response carries both 'keys' (the webui keys touched) and 'preserved' (the foreign keys left untouched) so the route can surface the operator's manual providers in the UI. File mode 0600: config.yaml carries plaintext apiKeys. The engine's own updateLocalByokConfig already pins 0600; the helper now matches. Three-step atomic write (0600 tmp, rename, 0600 chmod) — same dance as the engine's writer. Tests added (engine-provider-sync.test.js): - foreign entry survives a sync that has webui providers (whitelist preserves it). - empty eligible list does NOT wipe foreign (the destruction class closed here). - webui-managed entry whose provider is removed is dropped; foreign sibling survives. - webui-managed entry update replaces data, keeps the marker. - no-op skip when nothing changed (no mtime churn, no needless chmod). - config.yaml mode is 0600 (POSIX-only assertion, skipped on Windows). Gates: typecheck clean, test:webui 1606/1608 pass (2 pre-existing skips), engine-provider-sync tests 40/40 pass three consecutive runs. --- .../webui/server/lib/engine-provider-sync.js | 217 +++++++++++++++--- .../test/lib/engine-provider-sync.test.js | 170 +++++++++++++- 2 files changed, 347 insertions(+), 40 deletions(-) diff --git a/packages/webui/server/lib/engine-provider-sync.js b/packages/webui/server/lib/engine-provider-sync.js index e90cd03b..34e6358c 100644 --- a/packages/webui/server/lib/engine-provider-sync.js +++ b/packages/webui/server/lib/engine-provider-sync.js @@ -42,19 +42,64 @@ // its own OAuth / Codex / Claude Code flows — out of scope for the // byok projection) // +// Ownership rule — ticket 05 acceptance (merge-over-replace): +// +// The webui's PUT does NOT replace the engine's whole `custom_provider` +// tree. A manually-added operator entry (e.g. via `mcode provider add` +// on the engine CLI) is FOREIGN to the webui and must survive an +// unrelated webui PUT. The hard destruction class this commit is +// closing: pre-fix sync, removing a webui provider OR running an +// empty-eligible-list sync would silently DROP a foreign entry the +// operator typed in by hand. +// +// Ownership is tracked per-entry by an opaque marker field: +// +// _webui_owned: true ← every entry webui writes carries this +// +// The engine ignores unknown fields (it parses via js-yaml with no +// schema-rejection; see `parseCustomProvidersConfig` in +// `packages/config/src/byok-config.ts`), so the marker is engine-safe. +// The sync algorithm: +// +// existing engine keys ∩ eligible webui keys → UPDATE in place +// existing engine keys ∖ eligible webui keys: +// _webui_owned === true → DELETE (webui owns it, +// operator removed the +// webui provider) +// _webui_owned !== true (or missing) → PRESERVE (foreign; +// operator owns it; +// webui leaves it alone) +// eligible webui keys ∖ existing engine keys → ADD (new provider) +// +// This means a foreign `manual-only` provider stays in the engine +// tree even after every webui PUT, even after the operator deletes +// every webui-managed provider. The webui NEVER deletes a foreign +// entry. The only way to delete a foreign entry is the engine CLI's +// own `mcode provider delete` (or hand-editing `config.yaml`). +// // Write strategy: // // The engine reads `/config.yaml` (the same dir the // webui already knows about — see server/lib/config.js#resolveDataDir). -// We do an atomic tmp+rename YAML write keyed on `custom_provider` only; -// we never touch the operator's other engine config (provider.minimax -// tables, defaultModel, etc.). The engine subprocess running the +// We do an atomic tmp+rename YAML write keyed on `custom_provider` +// only; we never touch the operator's other engine config +// (provider.*, defaultModel, etc.). The engine subprocess running the // singleton client has a stale `getConfig()` cache after a write — // the route handler then calls `shutdownMcodeAcpSingleton()` so the // next operation spawns a fresh subprocess that reads the new file. // Brand-new prompt subprocesses spawned by `runMcodeAcp` always pick // up the latest config, so the rest of the system stays in lockstep. // +// File permissions — ticket 05 acceptance (0600): +// +// config.yaml carries the apiKey as plaintext. umask-default 0664 +// would expose the key to every user on the host. The engine's own +// `updateLocalByokConfig` writes 0600 (see +// `packages/config/src/local-model-provider-write.ts`); the helper +// matches. The new tmp file is created 0600, the rename preserves +// the mode on POSIX, and a final chmod pins it for platforms where +// the rename semantics differ. +// // We deliberately do NOT route through the engine's // `updateLocalByokConfig` (`@mavis/config`) — pulling that into the // webui bundle would drag in js-yaml + proper-lockfile just for a @@ -64,7 +109,7 @@ // runtime, only the `mcode provider add` CLI does, and that flow // cannot run concurrently with a webui PUT in the same process tree). -import { writeFile, rename, mkdir } from "node:fs/promises"; +import { writeFile, rename, mkdir, chmod } from "node:fs/promises"; import { dirname, join } from "node:path"; import { homedir } from "node:os"; import { existsSync, readFileSync } from "node:fs"; @@ -97,6 +142,19 @@ const RESERVED_ENGINE_KEYS = new Set([ const PROVIDER_KEY_REGEX = /^[A-Za-z0-9][A-Za-z0-9_.-]*$/; +/** + * Ownership marker field. Every entry the webui writes carries + * `_webui_owned: true`. The engine ignores it (js-yaml parses the whole + * record and the engine's downstream consumers read named fields only); + * the field is the on-disk fingerprint the sync algorithm uses to + * distinguish webui-managed entries from operator-managed ones. + * + * The constant is exported only so tests can assert against it without + * drifting if the marker ever changes (rename = data loss for every + * operator-managed entry on the next sync). Do not rename lightly. + */ +export const WEBUI_OWNED_MARKER = "_webui_owned"; + /** * Resolve the engine's data directory. * @@ -245,28 +303,110 @@ function readEngineConfigRaw(configPath) { } } -async function atomicWriteYaml(configPath, object) { +/** + * Atomic YAML write + 0600 permission pin. + * + * Two-step: write the new content to a tmp file (mode 0600), then + * rename. The rename preserves POSIX mode, but we chmod the target + * afterwards as belt-and-suspenders (some filesystems and Windows + * edge cases drop the mode on rename). The engine's own + * `updateLocalByokConfig` does the same dance — see + * `packages/config/src/local-model-provider-write.ts`. + */ +async function atomicWriteYaml0600(configPath, object) { await mkdir(dirname(configPath), { recursive: true }); const tmp = join( dirname(configPath), `.config-tmp-${randomBytes(6).toString("hex")}`, ); - await writeFile( - tmp, - yaml.dump(object, { indent: 2, lineWidth: -1, noRefs: true }), - "utf8", - ); + // mode 0600 — owner read/write only. The file carries plaintext + // apiKeys; any looser mode would expose them to other users on the + // host. + await writeFile(tmp, yaml.dump(object, { indent: 2, lineWidth: -1, noRefs: true }), { + encoding: "utf8", + mode: 0o600, + }); + await chmod(tmp, 0o600); await rename(tmp, configPath); + await chmod(configPath, 0o600); +} + +/** + * Is this engine entry webui-owned (vs operator/foreign)? + * + * The marker is set on every entry the webui writes. Operators who add + * custom providers via the engine CLI never set it, so the marker + * distinguishes the two ownerships on disk. + */ +function isWebuiOwned(entry) { + return !!(entry && typeof entry === "object" && entry[WEBUI_OWNED_MARKER] === true); +} + +/** + * Build the next `custom_provider` map by merging the eligible webui + * projection over the existing engine tree (see the file-level + * ownership rule). Pure: no IO, no writes. + * + * Algorithm: + * 1. eligible webui keys ⊂ existing engine keys → UPDATE in place + * (the webui entry replaces the engine entry; we still carry + * `_webui_owned: true` so the next sync treats it as webui). + * 2. existing engine keys ∖ eligible webui keys: + * marker === true → DELETE + * marker !== true → PRESERVE (foreign, operator-owned) + * 3. eligible webui keys ∖ existing engine keys → ADD + * + * The returned map carries the ownership marker on every webui-owned + * entry; foreign entries are passed through verbatim (including their + * original structure — we never edit a foreign entry's fields). + */ +function mergeCustomProviderTree(existingCustom, eligible) { + const next = {}; + // Carry forward any foreign entries that the engine already has. + // We do this first so the eligibility-driven UPDATE/DELETE pass + // below only touches webui-owned keys. + for (const [key, entry] of Object.entries(existingCustom || {})) { + if (!entry || typeof entry !== "object") continue; + if (!isWebuiOwned(entry)) { + next[key] = entry; + } + } + const eligibleKeys = new Set(); + for (const { key, entry } of eligible) { + eligibleKeys.add(key); + // Strip the existing entry (if any) — we'll replace it below with + // the new webui projection. The marker on the new entry will be + // preserved across sync cycles. + delete next[key]; + // Build the new entry with the ownership marker set. We stamp + // the marker AFTER cloning so we don't mutate the input (the + // route caches the eligible list across calls). + next[key] = { ...entry, [WEBUI_OWNED_MARKER]: true }; + } + // No further work: webui-owned entries that are no longer eligible + // were omitted from `next` (we never re-add them), so the DELETE + // step is implicit. + void eligibleKeys; + return next; } /** * Project a list of webui v2 providers into the engine's `custom_provider` - * tree and write the engine's `config.yaml` atomically. + * tree and write the engine's `config.yaml` atomically (mode 0600). + * + * Ownership rule (see file header): the merge preserves operator / + * foreign entries — only entries webui wrote (the `_webui_owned` + * marker is the on-disk fingerprint) are added / updated / removed. + * Foreign entries survive every webui PUT. * * Returns: - * - { ok: true, written: true|false, keys: [providerKey, ...] } + * - { ok: true, written: true|false, keys: [providerKey, ...], + * preserved: [foreignKey, ...] } * `written: false` means there was nothing eligible to write (the - * webui's catalogue is empty or every provider was ineligible). + * webui's catalogue is empty or every provider was ineligible); the + * foreign set is still reported so the route can log it. `keys` + * lists the webui keys the sync touched (added/updated). `preserved` + * lists the foreign keys that survived untouched. * - { ok: false, code: 'ENGINE_SYNC_FAILED', error: string } * * Never throws — surfaces every failure as a structured result so the @@ -289,27 +429,42 @@ export async function syncProvidersToEngine(providers, opts = {}) { }; } // Preserve every operator-owned section; only `custom_provider` is - // owned by this helper. The engine's `defaultModel` (when pointing at - // a `custom_provider:/`) is left to the operator — the - // dialog still resolves a model on demand via `applyRecordedModel`. + // touched. The engine's `defaultModel` (when pointing at a + // `custom_provider:/`) is left to the operator. const next = { ...existing }; - if (eligible.length === 0) { - // Nothing eligible — leave the engine's tree untouched rather than - // wipe an operator's manual custom_provider entries. The webui - // records a no-op result so the route can log it. - const existingCustom = existing.custom_provider || {}; - const existingKeys = Object.keys(existingCustom).filter((k) => - k.endsWith("-byok"), - ); - return { ok: true, written: false, keys: existingKeys }; + const existingCustom = + existing.custom_provider && typeof existing.custom_provider === "object" + ? existing.custom_provider + : {}; + const merged = mergeCustomProviderTree(existingCustom, eligible); + // List foreign keys we kept untouched, for the route response / + // log. Owned entries (webui + foreign-derived from marker) are + // excluded — only the operator-managed ones we preserved go here. + const preserved = []; + for (const key of Object.keys(merged)) { + const e = merged[key]; + if (!isWebuiOwned(e)) preserved.push(key); } - const customProvider = {}; - for (const { key, entry } of eligible) { - customProvider[key] = entry; + if ( + eligible.length === 0 && + Object.keys(merged).length === Object.keys(existingCustom).length && + Object.keys(merged).every((k) => existingCustom[k] === merged[k]) + ) { + // Nothing eligible AND the merged tree is byte-identical to + // the existing one (no webui-owned entries changed, no + // foreign entries added/removed). Skip the write — the engine's + // view of the world is unchanged, and a no-op write would + // still touch mtime / chmod. + return { ok: true, written: false, keys: [], preserved }; } - next.custom_provider = customProvider; - await atomicWriteYaml(configPath, next); - return { ok: true, written: true, keys: eligible.map((e) => e.key) }; + next.custom_provider = merged; + await atomicWriteYaml0600(configPath, next); + return { + ok: true, + written: true, + keys: eligible.map((e) => e.key), + preserved, + }; } catch (e) { return { ok: false, diff --git a/packages/webui/test/lib/engine-provider-sync.test.js b/packages/webui/test/lib/engine-provider-sync.test.js index 3888370f..27b54275 100644 --- a/packages/webui/test/lib/engine-provider-sync.test.js +++ b/packages/webui/test/lib/engine-provider-sync.test.js @@ -371,6 +371,9 @@ describe("syncProvidersToEngine — atomic YAML write + operator preservation", assert.equal(written.custom_provider["byok-zhipu"].options.apiKey, "sk-fake"); assert.equal(written.custom_provider["byok-zhipu"].options.baseURL, "https://example.com/v1"); assert.equal(written.custom_provider["byok-zhipu"].kind, "custom"); + // Acceptance: the entry carries the ownership marker so a future + // sync knows it is webui-managed and a foreign entry does not. + assert.equal(written.custom_provider["byok-zhipu"]._webui_owned, true); }); test("preserves the operator's provider.minimax + defaultModel sections", async () => { @@ -383,6 +386,9 @@ describe("syncProvidersToEngine — atomic YAML write + operator preservation", options: { apiKey: "sk-existing", authMode: "api-key", baseURL: "https://x/v1" }, }, }, + // Foreign (operator-managed) entry — no ownership marker, so the + // sync must NOT touch it. Ticket 05 acceptance: merge-over-replace, + // not replace-everything. custom_provider: { existing_byok: { name: "Existing", kind: "custom", enabled: true } }, }; const fs = await import("node:fs/promises"); @@ -400,36 +406,161 @@ describe("syncProvidersToEngine — atomic YAML write + operator preservation", }, ]); assert.equal(r.ok, true); + assert.deepEqual(r.keys, ["byok-new"]); + assert.deepEqual(r.preserved, ["existing_byok"]); const after = yaml.load(readFileSync(join(_tmpDataDir, "config.yaml"), "utf8")); // Provider tree survives — operator's manual config is not touched. assert.equal(after.provider.minimax.options.apiKey, "sk-existing"); assert.equal(after.defaultModel, "minimax/MiniMax-M3"); - // custom_provider is replaced wholesale by the helper's projection. - // Operators who need a manual entry in production should use - // `mcode provider add`, not the webui PUT — the PUT path's contract - // is "this catalogue is the source of truth for engine sync". + // Foreign entry survives (no marker, untouched by webui). + assert.deepEqual(after.custom_provider["existing_byok"], { + name: "Existing", + kind: "custom", + enabled: true, + }); + // New webui entry is added with its ownership marker. assert.equal(after.custom_provider["byok-new"].options.apiKey, "sk-new"); + assert.equal(after.custom_provider["byok-new"]._webui_owned, true); }); - test("empty catalogue is a no-op write (engine config left untouched)", async () => { + test("empty eligible list keeps a foreign entry intact (no destructive wipe)", async () => { const fs = await import("node:fs/promises"); await fs.mkdir(_tmpDataDir, { recursive: true }); - const seed = { defaultModel: "minimax/MiniMax-M3" }; + const seed = { + defaultModel: "minimax/MiniMax-M3", + custom_provider: { + // Foreign (operator-managed) — pre-existing, no marker. + manual_only: { + name: "Manual", + kind: "custom", + enabled: true, + api: "openai-completions", + options: { apiKey: "sk-manual", baseURL: "https://manual.example/v1", authMode: "api-key" }, + }, + }, + }; await fs.writeFile(join(_tmpDataDir, "config.yaml"), yaml.dump(seed), "utf8"); + // Empty eligible (every provider is ineligible) — must NOT wipe the + // foreign entry. Ticket 05 acceptance: the destruction class + // closed here is "removing a webui provider silently drops a foreign + // entry". The same destruction class applies to "PUTting an + // ineligible-only catalogue silently drops a foreign entry". const r = await syncProvidersToEngine([ - // Coding-plan → ineligible. { id: "x", label: "x", enabled: true, protocol: "openai", auth: { type: "coding-plan" }, models: [] }, - // No apiKey → ineligible. { id: "y", label: "y", enabled: true, protocol: "openai", auth: { type: "byok", baseURL: "https://z" }, models: [] }, ]); assert.equal(r.ok, true); + // No eligible providers AND the merged tree is byte-identical to + // what's on disk (foreign was already there and is preserved + // verbatim). The helper short-circuits the write — no mtime churn, + // no needless chmod. The route can still surface the `preserved` + // list to the operator via the response. assert.equal(r.written, false); - // Engine config wasn't replaced. + assert.deepEqual(r.keys, []); + assert.deepEqual(r.preserved, ["manual_only"]); + const after = yaml.load(readFileSync(join(_tmpDataDir, "config.yaml"), "utf8")); + assert.deepEqual(after.custom_provider.manual_only, seed.custom_provider.manual_only); + // The defaultModel is not touched either. assert.equal(after.defaultModel, "minimax/MiniMax-M3"); }); + test("webui-managed entry whose provider is removed is dropped, foreign entry is kept", async () => { + const fs = await import("node:fs/promises"); + await fs.mkdir(_tmpDataDir, { recursive: true }); + // Pre-populate: a webui-managed entry from a previous sync AND a + // foreign entry. + const seed = { + custom_provider: { + byok_old: { + name: "Old webui", + kind: "custom", + enabled: true, + api: "openai-completions", + options: { apiKey: "sk-old", baseURL: "https://old.example/v1", authMode: "api-key" }, + _webui_owned: true, + }, + manual_only: { + name: "Manual", + kind: "custom", + enabled: true, + api: "openai-completions", + options: { apiKey: "sk-manual", baseURL: "https://manual.example/v1", authMode: "api-key" }, + }, + }, + }; + await fs.writeFile(join(_tmpDataDir, "config.yaml"), yaml.dump(seed), "utf8"); + + // Sync with no eligible providers — byok_old should be dropped + // (webui owned it, webui no longer claims it), manual_only survives. + const r = await syncProvidersToEngine([]); + assert.equal(r.ok, true); + assert.deepEqual(r.keys, []); + assert.deepEqual(r.preserved, ["manual_only"]); + + const after = yaml.load(readFileSync(join(_tmpDataDir, "config.yaml"), "utf8")); + assert.equal(after.custom_provider["byok_old"], undefined); + assert.deepEqual(after.custom_provider.manual_only, seed.custom_provider.manual_only); + }); + + test("webui-managed entry update replaces the entry's data, keeps the marker", async () => { + const fs = await import("node:fs/promises"); + await fs.mkdir(_tmpDataDir, { recursive: true }); + const seed = { + custom_provider: { + "byok-zhipu": { + name: "Old label", + kind: "custom", + enabled: true, + api: "openai-completions", + options: { apiKey: "sk-old", baseURL: "https://old.example/v1", authMode: "api-key" }, + _webui_owned: true, + }, + }, + }; + await fs.writeFile(join(_tmpDataDir, "config.yaml"), yaml.dump(seed), "utf8"); + + // Re-sync with new apiKey/baseURL for the same id — entry is replaced + // in place, marker is preserved. + const r = await syncProvidersToEngine([ + { + id: "byok-zhipu", + label: "New label", + enabled: true, + protocol: "openai", + auth: { type: "byok", apiKey: "sk-new", baseURL: "https://new.example/v1" }, + models: [], + }, + ]); + assert.equal(r.ok, true); + assert.deepEqual(r.keys, ["byok-zhipu"]); + + const after = yaml.load(readFileSync(join(_tmpDataDir, "config.yaml"), "utf8")); + assert.equal(after.custom_provider["byok-zhipu"].options.apiKey, "sk-new"); + assert.equal(after.custom_provider["byok-zhipu"].options.baseURL, "https://new.example/v1"); + assert.equal(after.custom_provider["byok-zhipu"].name, "New label"); + assert.equal(after.custom_provider["byok-zhipu"]._webui_owned, true); + }); + + test("writes custom_provider when at least one eligible provider exists; otherwise no-op write when nothing changes", async () => { + // 1) Empty eligible, no foreign — there's nothing to write, and + // the engine already treats "no custom_provider key" as "no + // custom providers". The helper reports written: false (no-op). + const r1 = await syncProvidersToEngine([]); + assert.equal(r1.ok, true); + assert.equal(r1.written, false); + assert.deepEqual(r1.keys, []); + assert.deepEqual(r1.preserved, []); + + // 2) Re-run with the same empty eligible list — byte-identical to + // what's on disk; the helper reports `written: false` and + // skips the rewrite (no mtime churn, no needless chmod). + const r2 = await syncProvidersToEngine([]); + assert.equal(r2.ok, true); + assert.equal(r2.written, false); + }); + test("missing engine config file → creates one", async () => { rmSync(join(_tmpDataDir, "config.yaml"), { force: true }); const r = await syncProvidersToEngine([ @@ -479,6 +610,27 @@ describe("syncProvidersToEngine — atomic YAML write + operator preservation", assert.equal(after.custom_provider["no-key"], undefined); assert.equal(after.custom_provider["disabled"], undefined); }); + + test("config.yaml is written with mode 0600 (plaintext apiKey)", async () => { + const fs = await import("node:fs/promises"); + await syncProvidersToEngine([ + { + id: "byok-zhipu", + label: "z", + enabled: true, + protocol: "openai", + auth: { type: "byok", apiKey: "sk", baseURL: "https://x/v1" }, + models: [], + }, + ]); + const stat = await fs.stat(join(_tmpDataDir, "config.yaml")); + // POSIX mode 0600 — owner read/write only. The engine's own + // `updateLocalByokConfig` does the same (see + // packages/config/src/local-model-provider-write.ts). + if (process.platform !== "win32") { + assert.equal(stat.mode & 0o777, 0o600); + } + }); }); describe("syncProvidersFromPutBody — keep-key convention applied", () => {