fix: reconcile LLM API keys after out-of-band llmPorts edits (PATCH route + load-time)

server/index.js

@@ -247,7 +247,11 @@ app.patch("/api/sparks/:id", (req, res) => {
       return res.json({ success: true, spark, hasPassword: true });
     }
 
-    const spark = registry.updateSpark(req.params.id, body);
+    // LLM API keys: an llmPorts change is the second bypass besides out-of-band
+    // sparks.json writes. patchSpark() arms the reconcile on the llmPorts
+    // own-property (any shape — [] and the legacy scalar are applied by the
+    // normalizer too) and syncs against the post-normalize ports.
+    const { spark } = registry.patchSpark(req.params.id, body);
     // Restart monitor so collectors pick up host/auth/isLocal changes
     stopMonitor(req.params.id);
     startMonitor(spark);

server/sparks/SparkRegistry.js

@@ -250,6 +250,40 @@ export class SparkRegistry {
         this._sparks = [];
       }
     }
+
+    // Out-of-band config edits (sparks.json edited directly without resyncing
+    // secrets) can rename llmPorts underneath stored API keys. Reconcile once
+    // here — before the registry is handed to anything — and never delete key
+    // material at load.
+    this._reconcileLlmApiKeysAtLoad();
+  }
+
+  /**
+   * Load-time LLM key/port reconcile.
+   * An out-of-band sparks.json edit that renames llmPorts (without going
+   * through PATCH / PUT llm-ports) leaves keys keyed on ports the spark no
+   * longer exposes. When exactly one keyed port is orphaned and exactly one
+   * configured port lacks a key, the rename shape is unambiguous → MOVE the
+   * key. Any other mismatch shape is warn-only: never prune, never delete
+   * stored key material at load.
+   */
+  _reconcileLlmApiKeysAtLoad() {
+    for (const spark of this._sparks) {
+      const configured = Array.isArray(spark.llmPorts) ? spark.llmPorts : [];
+      const keyed = this.llmApiKeyPorts(spark.id);
+      const orphans = keyed.filter((p) => !configured.includes(p));
+      const missing = configured.filter((p) => !this.hasLlmApiKey(spark.id, p));
+      if (orphans.length === 1 && missing.length === 1) {
+        this.moveLlmApiKey(spark.id, orphans[0], missing[0]);
+        console.warn(
+          `[SparkRegistry] migrated LLM API key for spark ${spark.id}: port ${orphans[0]} -> port ${missing[0]} after out-of-band config change`
+        );
+      } else if (orphans.length > 0 || missing.length > 0) {
+        console.warn(
+          `[SparkRegistry] spark ${spark.id} LLM key/port mismatch: keyed=<${orphans.join(", ")}> missing=<${missing.join(", ")}>`
+        );
+      }
+    }
   }
 
   _save() {
@@ -434,6 +468,35 @@ export class SparkRegistry {
    * @param {number[]} prevPorts
    * @param {number[]} nextPorts
    */
+  /**
+   * updateSpark() plus LLM API key reconcile for PATCH /api/sparks/:id.
+   *
+   * Armed whenever the patch carries an `llmPorts` own-property — including
+   * `[]` and the legacy single-value shape — because _normalizeLlmPorts()
+   * applies those too (empty/invalid -> [LLM_PORT], scalar -> [n]). The sync
+   * runs against the POST-normalize ports on the stored spark, so it sees the
+   * same rename the persisted record does, not the raw request body.
+   * @param {string} id
+   * @param {object} updates PATCH body
+   * @returns {{ spark: object, llmPortsSynced: boolean }}
+   */
+  patchSpark(id, updates) {
+    const body = updates || {};
+    const armed = Object.prototype.hasOwnProperty.call(body, "llmPorts");
+    let prevPorts = null;
+    if (armed) {
+      const existing = this.getSpark(id);
+      if (!existing) throw new Error(`Spark ${id} not found`);
+      prevPorts = Array.isArray(existing.llmPorts) ? [...existing.llmPorts] : [];
+    }
+    const updated = this.updateSpark(id, body);
+    const llmPortsSynced = armed && Array.isArray(updated.llmPorts);
+    if (llmPortsSynced) {
+      this.syncLlmApiKeysToPorts(id, prevPorts, updated.llmPorts);
+    }
+    return { spark: llmPortsSynced ? this.getSpark(id) : updated, llmPortsSynced };
+  }
+
   syncLlmApiKeysToPorts(id, prevPorts, nextPorts) {
     const prev = Array.isArray(prevPorts) ? prevPorts : [];
     const next = Array.isArray(nextPorts) ? nextPorts : [];

server/sparks/__tests__/SparkRegistry.llmApiKeys.test.js

@@ -0,0 +1,181 @@
+/**
+ * SparkRegistry LLM API key sync guard (follow-up to #25/#26).
+ *
+ * An out-of-band sparks.json edit can rename llmPorts without touching the
+ * encrypted secrets store; PATCH /api/sparks/:id was the other bypass. Result:
+ * the registry kept an orphaned key on the old port (probe then hit the host
+ * without auth → 401s). Covered here:
+ *   - load-time reconcile: unambiguous single-port rename MOVES the key
+ *   - load-time reconcile: every other mismatch shape is warn-only and NEVER
+ *     deletes stored key material at load
+ *   - syncLlmApiKeysToPorts: rename move + prune semantics (the rails the
+ *     PATCH llmPorts path and PUT /api/sparks/:id/llm-ports drive)
+ *
+ * Run: npm test
+ */
+import test from "node:test";
+import assert from "node:assert/strict";
+import fs from "node:fs";
+import os from "node:os";
+import path from "node:path";
+
+// Env must be set before the first import of config.js (paths are captured as
+// consts at module evaluation); each node --test file runs in its own process.
+const TMPDIR = fs.mkdtempSync(path.join(os.tmpdir(), "sparkdash-registry-keys-"));
+process.env.SPARKS_JSON_PATH = path.join(TMPDIR, "sparks.json");
+process.env.SPARKS_SECRETS_PATH = path.join(TMPDIR, "sparks-secrets.json");
+process.env.SECRETS_KEY_PATH = path.join(TMPDIR, ".secrets-key");
+process.env.LLM_PORT = "8888";
+
+const { SparkRegistry } = await import("../SparkRegistry.js");
+const { saveSecrets } = await import("../../secretsStore.js");
+const { LLM_PORT } = await import("../../config.js");
+
+/** Synthetic test key only — never real key material. */
+const TEST_KEY = "sk-test-0000";
+
+const SPARK_ID = "t";
+
+/**
+ * Seed sparks.json + the encrypted secrets store, then construct the registry
+ * through its real load path so the load-time reconcile provably runs.
+ * @param {number[]} llmPorts configured ports
+ * @param {Record<string, string>} llmApiKeys port -> key in the secrets store
+ * @param {{ reset?: boolean }} [opts] reset wipes seeded state first
+ */
+function loadRegistry(llmPorts, llmApiKeys = {}, { reset = false } = {}) {
+  if (reset) {
+    fs.rmSync(process.env.SPARKS_JSON_PATH, { force: true });
+    fs.rmSync(process.env.SPARKS_SECRETS_PATH, { force: true });
+  }
+  fs.writeFileSync(
+    process.env.SPARKS_JSON_PATH,
+    JSON.stringify({
+      sparks: [{ id: SPARK_ID, name: "T", lanIp: "127.0.0.1", llmPorts }],
+    })
+  );
+  saveSecrets(
+    new Map(),
+    new Map([[SPARK_ID, Object.fromEntries(Object.entries(llmApiKeys))]])
+  );
+  return new SparkRegistry();
+}
+
+/** Capture `[SparkRegistry]` warns for the duration of fn(). */
+function captureRegistryWarns(fn) {
+  const lines = [];
+  const orig = console.warn;
+  console.warn = (...args) => {
+    if (typeof args[0] === "string" && args[0].includes("[SparkRegistry]")) {
+      lines.push(args[0]);
+    }
+  };
+  try {
+    fn();
+  } finally {
+    console.warn = orig;
+  }
+  return lines;
+}
+
+// ─── Load-time reconcile ─────────────────────────────────
+
+test("load reconcile: unambiguous single-port rename moves the key", () => {
+  const r = loadRegistry([8899], { "8888": TEST_KEY });
+  assert.deepEqual(r.llmApiKeyPorts(SPARK_ID), [8899]);
+  assert.equal(r.hasLlmApiKey(SPARK_ID, 8888), false);
+  assert.equal(r.getSpark(SPARK_ID).llmApiKeys["8899"], TEST_KEY);
+});
+
+test("load reconcile: single-port rename warns with migration message", () => {
+  const warns = captureRegistryWarns(() => loadRegistry([8899], { "8888": TEST_KEY }));
+  assert.equal(warns.length, 1);
+  assert.equal(
+    warns[0],
+    "[SparkRegistry] migrated LLM API key for spark t: port 8888 -> port 8899 after out-of-band config change"
+  );
+});
+
+test("load reconcile: ambiguous shape never prunes — key on 8888 survives", () => {
+  const r = loadRegistry([8015, 8899], { "8888": TEST_KEY });
+  // No destructive load path: the orphaned key must still exist, unmoved.
+  assert.equal(r.hasLlmApiKey(SPARK_ID, 8888), true);
+  assert.deepEqual(r.llmApiKeyPorts(SPARK_ID), [8888]);
+  assert.equal(r.getSpark(SPARK_ID).llmApiKeys["8888"], TEST_KEY);
+});
+
+test("load reconcile: ambiguous shape warns listing port numbers only", () => {
+  const warns = captureRegistryWarns(() => loadRegistry([8015, 8899], { "8888": TEST_KEY }));
+  assert.equal(warns.length, 1);
+  assert.equal(
+    warns[0],
+    "[SparkRegistry] spark t LLM key/port mismatch: keyed=<8888> missing=<8015, 8899>"
+  );
+  assert.doesNotMatch(warns[0], /sk-test-0000/);
+});
+
+test("load reconcile: aligned shape stays silent and unchanged", () => {
+  const warns = captureRegistryWarns(() => loadRegistry([8899], { "8899": TEST_KEY }));
+  assert.deepEqual(warns, []);
+  const r = loadRegistry([8899], { "8899": TEST_KEY });
+  assert.deepEqual(r.llmApiKeyPorts(SPARK_ID), [8899]);
+});
+
+// ─── syncLlmApiKeysToPorts (PATCH llmPorts / PUT llm-ports rails) ──
+
+function registryWithKey() {
+  const r = loadRegistry([8899], { "8899": TEST_KEY }, { reset: true });
+  r.addSpark({ id: "sp", name: "SP", lanIp: "127.0.0.1", llmPorts: [8888] });
+  r.setLlmApiKey("sp", 8888, TEST_KEY);
+  return r;
+}
+
+test("syncLlmApiKeysToPorts: single rename moves the key with value intact", () => {
+  const r = registryWithKey();
+  r.syncLlmApiKeysToPorts("sp", [8888], [8899]);
+  assert.deepEqual(r.llmApiKeyPorts("sp"), [8899]);
+  assert.equal(r.getSpark("sp").llmApiKeys["8899"], TEST_KEY);
+  assert.equal(r.hasLlmApiKey("sp", 8888), false);
+});
+
+test("syncLlmApiKeysToPorts: no-shape change is a no-op", () => {
+  const r = registryWithKey();
+  r.syncLlmApiKeysToPorts("sp", [8888], [8888]);
+  assert.equal(r.hasLlmApiKey("sp", 8888), true);
+  assert.equal(r.getSpark("sp").llmApiKeys["8888"], TEST_KEY);
+});
+
+test("syncLlmApiKeysToPorts: removed port without rename is pruned", () => {
+  const r = registryWithKey();
+  r.setLlmApiKey("sp", 9001, TEST_KEY);
+  r.syncLlmApiKeysToPorts("sp", [8888, 9001], [8888]);
+  assert.deepEqual(r.llmApiKeyPorts("sp"), [8888]);
+});
+
+// ─── patchSpark (PATCH /api/sparks/:id path) ─────────────────────
+
+test("patchSpark: llmPorts [] normalizes to the default port and moves the key", () => {
+  const r = loadRegistry([8899], { "8899": TEST_KEY }, { reset: true });
+  const { spark, llmPortsSynced } = r.patchSpark(SPARK_ID, { llmPorts: [] });
+  assert.equal(llmPortsSynced, true);
+  assert.deepEqual(spark.llmPorts, [LLM_PORT]);
+  assert.deepEqual(r.llmApiKeyPorts(SPARK_ID), [LLM_PORT]);
+  assert.equal(r.getSpark(SPARK_ID).llmApiKeys[String(LLM_PORT)], TEST_KEY);
+  assert.equal(r.hasLlmApiKey(SPARK_ID, 8899), false);
+});
+
+test("patchSpark: legacy scalar llmPorts is applied and the key follows", () => {
+  const r = loadRegistry([8888], { "8888": TEST_KEY }, { reset: true });
+  const { spark } = r.patchSpark(SPARK_ID, { llmPorts: "8899" });
+  assert.deepEqual(spark.llmPorts, [8899]);
+  assert.deepEqual(r.llmApiKeyPorts(SPARK_ID), [8899]);
+  assert.equal(r.getSpark(SPARK_ID).llmApiKeys["8899"], TEST_KEY);
+});
+
+test("patchSpark: a body without llmPorts never touches keys", () => {
+  const r = loadRegistry([8888], { "8888": TEST_KEY }, { reset: true });
+  const { llmPortsSynced } = r.patchSpark(SPARK_ID, { name: "renamed" });
+  assert.equal(llmPortsSynced, false);
+  assert.deepEqual(r.llmApiKeyPorts(SPARK_ID), [8888]);
+  assert.equal(r.getSpark(SPARK_ID).name, "renamed");
+});