feat(webui): provider management UI with keep-key semantics and hot-apply wiring

packages/webui/server/lib/providers-config.js

@@ -658,4 +658,120 @@ export function _extractPlaintextKey(provider) {
   return provider && provider.auth && typeof provider.auth.apiKey === "string"
     ? provider.auth.apiKey
     : "";
+}
+
+// =====================================================================
+// Keep-existing-key convention (ticket 03 cross-branch API note)
+// =====================================================================
+//
+// Background: GET /api/providers returns `apiKeyMasked` (e.g. "sk-aa***bb")
+// rather than the plaintext, so a UI that PUTs back what it has on screen
+// would send the masked value as the new apiKey — the plaintext would be
+// lost on every edit. There is no "unchanged" semantics in the v2 PUT
+// contract (ticket 01 deliberately kept the body a full replacement so
+// the validation/normalisation path is simple), so the management UI
+// ships a convention on top:
+//
+//   * incoming `auth.apiKey` is the empty string OR is absent
+//     (`undefined`) — both are interpreted as "do not change the
+//     existing key for this provider id". The handler copies the
+//     existing key onto the incoming record before
+//     validation/normalisation. Treating the absent-field case the
+//     same as empty is intentional: the v2 normaliser coerces a
+//     missing `auth.apiKey` to `""` anyway, and silently writing
+//     `""` to disk would wipe a credential the operator never
+//     intended to change (the field's HTML placeholder carries the
+//     masked value, not the controlled value, so a UI that drops the
+//     field is the normal "no change" gesture).
+//
+//   * anything non-empty — including the masked placeholder — is
+//     treated as the new value. The UI must therefore blank the field
+//     when the user does not want to overwrite it (the editor renders
+//     the masked placeholder as the input's placeholder, not its
+//     value).
+//
+//   * DELIBERATE KEY CLEARING IS NOT POSSIBLE. There is no wire shape
+//     that results in a stored key becoming empty once one has been
+//     written. Operators who need to rotate a credential PUT a new
+//     value; the convention preserves the previous key only when the
+//     incoming record signals "no change". The documentation
+//     (`webapp/lib/i18n.ts` "API key" placeholder) carries the same
+//     caveat in user-visible form.
+//
+// Layer scope: the convention reads `loadUserLevelProviders()`
+// (user-level file only — NOT the merged result), so the env / cwd
+// layers' secrets are never materialised into the user-level file
+// when an operator edits an env-defined provider. The merged view
+// still wins for the engine — `loadProvidersConfig()` keeps the env
+// priority — so the operator's edit does not "pin" an env secret to
+// disk by accident.
+//
+// Cross-branch API note: the convention lives on this branch
+// (ticket 03) because ticket 01's PUT contract was already merged
+// without it. The convention is opt-in — a UI that always sends the
+// plaintext only sees normal replacement behaviour. The PUT handler
+// is the only place this helper runs, so the rest of the validation
+// surface is unchanged.
+
+/**
+ * Read JUST the user-level providers file, without the env/cwd merge.
+ *
+ * Used by `applyKeepKeyConvention` so the convention's "previous key"
+ * lookup is scoped to the layer the operator owns — the env/cwd
+ * layers are deployment-managed, and silently copying one of THEIR
+ * keys into the user-level file would materialise a deployment
+ * secret onto operator-managed disk (where the convention can no
+ * longer rotate it). Returns `[]` when the file is missing or
+ * malformed, matching the `loadProvidersConfig()` contract.
+ */
+export function loadUserLevelProviders() {
+  const userPath = getUserLevelPath();
+  const parsed = safeReadJson(userPath);
+  if (!parsed) return [];
+  const norm = normaliseConfig(parsed);
+  return norm && Array.isArray(norm.providers) ? norm.providers : [];
+}
+
+/**
+ * Apply the keep-existing-key convention.
+ *
+ * For every incoming provider whose `auth.apiKey` is empty OR absent
+ * (the sentinel):
+ *   - if a same-id provider exists in `existing` with a non-empty
+ *     `auth.apiKey`, copy it onto the incoming record;
+ *   - if no such existing provider exists (the incoming record is
+ *     brand new), the empty stays empty and the normal validation
+ *     flow rejects it for `byok` (which is the right behaviour: a
+ *     new byok provider with no key cannot pass a test probe).
+ *
+ * The function is pure (no IO). Returns a NEW array — `incoming` is
+ * not mutated, so the original body still exists for error reporting
+ * if the caller wants to surface it.
+ */
+export function applyKeepKeyConvention(existing, incoming) {
+  const existingById = new Map();
+  for (const p of existing || []) {
+    if (p && p.id) existingById.set(p.id, p);
+  }
+  return (incoming || []).map((p) => {
+    if (!p || typeof p !== "object") return p;
+    const auth = p.auth && typeof p.auth === "object" ? p.auth : {};
+    // The sentinel: empty string (explicit "I typed nothing") OR
+    // absent (the field was never sent). Both mean "do not change
+    // the existing key". A non-string apiKey (number, boolean) is
+    // left untouched — those are upstream mistakes that the normaliser
+    // will surface as a type mismatch on its own.
+    const apiKeyIsSentinel =
+      typeof auth.apiKey === "undefined" || auth.apiKey === "";
+    if (!apiKeyIsSentinel) return p;
+    const previous = existingById.get(p.id);
+    const previousKey =
+      previous && previous.auth && typeof previous.auth.apiKey === "string"
+        ? previous.auth.apiKey
+        : "";
+    return {
+      ...p,
+      auth: { ...auth, apiKey: previousKey },
+    };
+  });
 }

packages/webui/server/routes/providers.js

@@ -55,6 +55,8 @@ import {
   writeProvidersConfig,
   testProvider as runProbe,
   getUserLevelPath,
+  applyKeepKeyConvention,
+  loadUserLevelProviders,
   normaliseProvider,
 } from "../lib/providers-config.js";
 import {
@@ -122,7 +124,36 @@ export async function handlePutProviders(req, res, _ctx) {
       JSON.stringify({ ok: false, code: "BAD_BODY", error: "body must be a JSON object" }),
     );
   }
-  const result = writeProvidersConfig(parsed);
+  // Keep-existing-key convention (ticket 03 cross-branch API note):
+//   `auth.apiKey` empty OR absent on an incoming provider means "don't
+//   change the existing key". We copy the user-level file's apiKey
+//   onto those records before validation, so the masked placeholder
+//   the UI sends back (and an absent-field body) does not silently
+//   wipe the plaintext on every edit. See
+//   lib/providers-config.js#applyKeepKeyConvention.
+//
+// Layer scope: the "previous key" lookup reads the user-level file
+// ONLY (`loadUserLevelProviders`), not the merged catalogue. Without
+// this scoping, editing a provider whose key is sourced from the env
+// or cwd layer would materialise the deployment secret into the
+// user-level file — once written there, the deployment layer can no
+// longer rotate it. The merged view still wins for the engine
+// (`loadProvidersConfig` priority order), so the visible behaviour
+// for the operator is unchanged: an env-defined key still wins at
+// read time even after the user edits the provider.
+//
+// Only applied when `parsed.providers` is actually an array — a missing
+// 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 toWrite =
+  incomingProviders === null
+    ? parsed
+    : {
+        ...parsed,
+        providers: applyKeepKeyConvention(loadUserLevelProviders(), incomingProviders),
+      };
+const result = writeProvidersConfig(toWrite);
   if (!result.ok) {
     const status = result.code === "WRITE_FAILED" ? 500 : 400;
     res.writeHead(status, { "Content-Type": "application/json; charset=utf-8" });

packages/webui/test/lib/providers-config.test.js

@@ -773,4 +773,228 @@ describe("testProvider — no network for malformed inputs", () => {
     assert.equal(r.ok, false);
     assert.equal(r.code, "PROBE_FAILED", "validation passed → fetch attempted → probe failed");
   });
+});
+
+// ---------------------------------------------------------------------
+// applyKeepKeyConvention — ticket 03 keep-existing-key convention.
+// ---------------------------------------------------------------------
+//
+// The UI GETs the masked catalogue (apiKey is `apiKeyMasked: "sk-aa***bb"`),
+// then PUTs the same shape back. Without a convention, the masked
+// placeholder would replace the plaintext on every edit. The
+// convention: an incoming `auth.apiKey === ""` means "do not change the
+// existing key for this provider id". The PUT handler is the only caller.
+
+describe("applyKeepKeyConvention — ticket 03 keep-existing-key convention", () => {
+  test("incoming apiKey='' copies the existing key when one is on disk", () => {
+    const existing = [
+      {
+        id: "p1",
+        label: "P1",
+        protocol: "openai",
+        auth: { type: "byok", apiKey: "sk-realkey-on-disk-aaaa" },
+        models: [],
+      },
+    ];
+    const incoming = [
+      {
+        id: "p1",
+        label: "P1 renamed",
+        protocol: "openai",
+        auth: { type: "byok", apiKey: "" }, // sentinel
+        models: [{ id: "m" }],
+      },
+    ];
+    const merged = providersConfig.applyKeepKeyConvention(existing, incoming);
+    assert.equal(merged.length, 1);
+    assert.equal(merged[0].auth.apiKey, "sk-realkey-on-disk-aaaa");
+    // Other fields untouched by the convention.
+    assert.equal(merged[0].label, "P1 renamed");
+    assert.deepEqual(merged[0].models, [{ id: "m" }]);
+  });
+
+  test("incoming apiKey non-empty is NOT replaced (the user is overwriting)", () => {
+    const existing = [
+      { id: "p1", auth: { type: "byok", apiKey: "sk-on-disk" } },
+    ];
+    const incoming = [
+      { id: "p1", auth: { type: "byok", apiKey: "sk-new-plaintext" } },
+    ];
+    const merged = providersConfig.applyKeepKeyConvention(existing, incoming);
+    assert.equal(merged[0].auth.apiKey, "sk-new-plaintext");
+  });
+
+  test("incoming apiKey='' with NO existing record keeps empty (new provider fails byok validation)", () => {
+    // The provider is brand new — there is nothing to keep. The empty
+    // key stays, and the normal validation rejects a `byok` record
+    // with an empty apiKey. This is the right behaviour: a UI that
+    // hits Save without filling the key must not silently inherit
+    // some other provider's credential.
+    const merged = providersConfig.applyKeepKeyConvention(
+      [{ id: "other", auth: { type: "byok", apiKey: "sk-something" } }],
+      [{ id: "brand-new", auth: { type: "byok", apiKey: "" } }],
+    );
+    assert.equal(merged[0].id, "brand-new");
+    assert.equal(merged[0].auth.apiKey, "");
+  });
+
+  test("absent auth.apiKey on an incoming record copies the user-level key", () => {
+    // The convention treats BOTH `auth.apiKey === ""` AND a missing
+    // `auth.apiKey` field as the sentinel. A PUT that drops the key
+    // field is the normal "no change" gesture from the editor form
+    // (which only ever sets the controlled value when the user types).
+    // Without this, the API would silently wipe a stored credential
+    // whenever a caller forgot to send the field — the v2 normaliser
+    // coerces a missing apiKey to "" anyway, so the on-disk write
+    // would land as empty. Pinning the behaviour here so a future
+    // "absent !== empty" change does not regress to the silent wipe.
+    const merged = providersConfig.applyKeepKeyConvention(
+      [{ id: "p1", auth: { type: "byok", apiKey: "sk-disk" } }],
+      [{ id: "p1", auth: { type: "byok" } }],
+    );
+    assert.equal(merged[0].auth.apiKey, "sk-disk", "absent key inherits from user-level");
+  });
+
+  test("absent auth.apiKey with NO existing record keeps the empty key", () => {
+    // Brand-new provider with no key — the empty stays empty, and
+    // the normal validation flow rejects a `byok` record with an
+    // empty key. Coding-plan records (which allow empty keys) pass
+    // through unchanged.
+    const merged = providersConfig.applyKeepKeyConvention(
+      [{ id: "other", auth: { type: "byok", apiKey: "sk-something" } }],
+      [{ id: "brand-new", auth: { type: "byok" } }],
+    );
+    assert.equal(merged[0].id, "brand-new");
+    assert.equal(merged[0].auth.apiKey, "");
+  });
+
+  test("returns a new array — the incoming body is not mutated", () => {
+    const incoming = [
+      { id: "p1", auth: { type: "byok", apiKey: "" } },
+    ];
+    const merged = providersConfig.applyKeepKeyConvention(
+      [{ id: "p1", auth: { type: "byok", apiKey: "sk-disk" } }],
+      incoming,
+    );
+    assert.notEqual(merged, incoming, "new array");
+    assert.equal(incoming[0].auth.apiKey, "", "incoming untouched");
+  });
+
+  test("a provider with no matching id in existing keeps its empty key", () => {
+    const merged = providersConfig.applyKeepKeyConvention(
+      [{ id: "other", auth: { type: "byok", apiKey: "sk-disk" } }],
+      [{ id: "brand-new", auth: { type: "byok", apiKey: "" } }],
+    );
+    assert.equal(merged[0].auth.apiKey, "");
+  });
+
+  test("absent auth.apiKey on existing record: copies the user-layer key", () => {
+    // Acceptance hardening (ticket 03 round 2): a PUT whose body
+    // omits the `auth.apiKey` field used to fall through validation
+    // and silently land on disk as "" — wiping the stored key. The
+    // convention now treats absent the same as empty.
+    const merged = providersConfig.applyKeepKeyConvention(
+      [{ id: "p1", auth: { type: "byok", apiKey: "sk-on-disk-aaaa" } }],
+      [{ id: "p1", auth: { type: "byok" } }],
+    );
+    assert.equal(merged[0].auth.apiKey, "sk-on-disk-aaaa");
+  });
+
+  test("absent auth.apiKey with auth itself omitted: still copies the user-layer key", () => {
+    // Defensive: the helper must not throw when `auth` is entirely
+    // absent from the incoming record. A bug here would 500 the
+    // entire PUT handler on a malformed body.
+    const merged = providersConfig.applyKeepKeyConvention(
+      [{ id: "p1", auth: { type: "byok", apiKey: "sk-on-disk" } }],
+      [{ id: "p1", protocol: "openai", models: [] }],
+    );
+    assert.equal(merged[0].auth.apiKey, "sk-on-disk");
+  });
+
+  test("deliberate key clearing is impossible: no input shape wipes a stored key", () => {
+    // The contract: once a key is on disk, no wire shape can clear
+    // it. The convention copies the previous key onto EVERY incoming
+    // shape that lacks an explicit value ("" or absent). A non-empty
+    // value replaces — there is no API call that means "delete the
+    // stored key and accept the consequence". Operators who need to
+    // rotate put a new key; the convention preserves only on "no
+    // change" gestures.
+    const existing = [{ id: "p1", auth: { type: "byok", apiKey: "sk-stored" } }];
+    const shapes = [
+      // explicit empty
+      { id: "p1", auth: { type: "byok", apiKey: "" } },
+      // absent field
+      { id: "p1", auth: { type: "byok" } },
+      // absent auth entirely
+      { id: "p1", protocol: "openai" },
+    ];
+    for (const shape of shapes) {
+      const merged = providersConfig.applyKeepKeyConvention(existing, [shape]);
+      assert.equal(
+        merged[0].auth.apiKey,
+        "sk-stored",
+        `shape ${JSON.stringify(shape.auth)} should keep existing key`,
+      );
+    }
+  });
+});
+
+// ---------------------------------------------------------------------
+// loadUserLevelProviders — user-layer-only loader (ticket 03 round 2).
+// ---------------------------------------------------------------------
+//
+// The convention reads THIS, not the merged `loadProvidersConfig()`
+// result, so editing an env-defined provider does not materialise
+// the deployment secret onto the operator-managed user-level file.
+// The merged view still wins for the engine — only the on-disk
+// write is scoped to the user layer.
+
+describe("loadUserLevelProviders — user-layer-only loader", () => {
+  test("returns [] when the user-level file is missing", () => {
+    // beforeEach cleared it. Sanity check on the contract.
+    assert.deepEqual(providersConfig.loadUserLevelProviders(), []);
+  });
+
+  test("returns only the user-level records (not env/cwd)", async () => {
+    // Seed the user-level file. The cwd layer is empty in this test
+    // because MCODE_WEBUI_MODELS_CONFIG is unset, so what we seed
+    // IS the only thing loadUserLevelProviders can see — and it
+    // must NOT include any merged material from other layers.
+    writeFileSync(
+      providersConfig.getUserLevelPath(),
+      JSON.stringify({
+        version: 2,
+        providers: [
+          { id: "u1", auth: { type: "byok", apiKey: "sk-user-only-aaaa" } },
+        ],
+      }),
+    );
+    const result = providersConfig.loadUserLevelProviders();
+    assert.equal(result.length, 1);
+    assert.equal(result[0].id, "u1");
+    assert.equal(result[0].auth.apiKey, "sk-user-only-aaaa");
+  });
+
+  test("env-layer key is NOT visible to loadUserLevelProviders", () => {
+    // Set an env-layer file with a key. The user-level file is
+    // empty (cleared by beforeEach). loadUserLevelProviders must
+    // return [] — the env secret does not leak into the user-layer
+    // loader. The convention then has nothing to copy.
+    const envPath = join(_tmpCwd, "env-only.json");
+    writeFileSync(
+      envPath,
+      JSON.stringify({
+        providers: [
+          { id: "envprov", auth: { type: "byok", apiKey: "sk-env-only-aaaa" } },
+        ],
+      }),
+    );
+    process.env.MCODE_WEBUI_MODELS_CONFIG = envPath;
+    try {
+      const result = providersConfig.loadUserLevelProviders();
+      assert.deepEqual(result, [], "env-only secrets must not be visible here");
+    } finally {
+      delete process.env.MCODE_WEBUI_MODELS_CONFIG;
+    }
+  });
 });