Fix #2212: Bug: viewer config save overwrites viewer.port with UI default (18799) on Hermes

apps/memos-local-plugin/core/config/writer.ts

@@ -55,7 +55,8 @@ export async function patchConfig(
 
   // Parse (or seed) the YAML document.
   const doc = existingText ? parseDoc(existingText, home.configFile) : parseDoc(stringifyYaml(DEFAULT_CONFIG), "<defaults>");
-  applyPatch(doc, patch);
+  const sanitized = sanitizePatch(patch);
+  applyPatch(doc, sanitized);
   removeUnsupportedUserConfig(doc);
 
   // Validate against schema using the merged JS view.
@@ -125,6 +126,101 @@ function removeUnsupportedUserConfig(doc: ReturnType<typeof parseDoc>): void {
   }
 }
 
+/**
+ * Adapter-owned config keys the client is never allowed to patch via
+ * `PATCH /api/v1/config`. See #2212: the Hermes adapter hardcodes the
+ * viewer port to :18800 via `bridge.mts::AGENT_DEFAULT_PORTS`, but the
+ * shared UI defaults surface :18799 (the OpenClaw port) in the resolved
+ * config the viewer reads back. If the client mirrors that value into a
+ * subsequent PATCH — either because the settings form rehydrated a
+ * "dirty" viewer block or because a third-party tool round-tripped GET
+ * into PATCH — we used to write 18799 to disk verbatim, silently
+ * breaking the bridge until the user hand-edited config.yaml.
+ *
+ * Sanitising once here means every PATCH path (routes, direct calls,
+ * hub-triggered rewrites) inherits the guard.
+ */
+const ADAPTER_OWNED_PATCH_PATHS: readonly string[] = Object.freeze([
+  "viewer.port",
+]);
+
+/**
+ * Non-empty whitespace-only patches on these fields are silently dropped.
+ * An exact empty string remains meaningful: it clears a custom endpoint
+ * and restores the provider's default base URL.
+ */
+const ENDPOINT_PATCH_PATHS: readonly string[] = Object.freeze([
+  "embedding.endpoint",
+  "llm.endpoint",
+  "l3Llm.endpoint",
+  "skillEvolver.endpoint",
+]);
+
+/**
+ * Strip adapter-owned keys and whitespace-only endpoints from an incoming
+ * patch before it reaches the YAML writer. Never mutates the caller's
+ * object. Prunes now-empty parent maps so we don't leave dangling
+ * `viewer: {}` in the patch (which would still be a no-op but is
+ * noisier in debug logs).
+ */
+function sanitizePatch(patch: Record<string, unknown>): Record<string, unknown> {
+  // Use `structuredClone` (Node 17+) rather than a JSON round-trip because
+  // the latter silently drops `undefined` values — a caller may legitimately
+  // pass `{ llm: { endpoint: undefined } }` and expect `applyPatch` to see
+  // that leaf (`doc.setIn` handles the write). JSON.stringify would delete
+  // the key before it ever reached the writer, silently suppressing the
+  // intended patch. `structuredClone` preserves the full object graph.
+  const cloned = structuredClone(patch) as Record<string, unknown>;
+  for (const dotted of ADAPTER_OWNED_PATCH_PATHS) {
+    deleteDottedPath(cloned, dotted);
+  }
+  for (const dotted of ENDPOINT_PATCH_PATHS) {
+    const value = readDottedPath(cloned, dotted);
+    // Preserve "" so users can reset a custom endpoint. Reject only
+    // non-empty strings that contain no usable characters.
+    if (typeof value === "string" && value.length > 0 && value.trim() === "") {
+      deleteDottedPath(cloned, dotted);
+    }
+  }
+  return cloned;
+}
+
+function readDottedPath(obj: Record<string, unknown>, dotted: string): unknown {
+  const keys = dotted.split(".");
+  let cursor: unknown = obj;
+  for (const key of keys) {
+    if (!isPlainObject(cursor)) return undefined;
+    cursor = (cursor as Record<string, unknown>)[key];
+  }
+  return cursor;
+}
+
+function deleteDottedPath(obj: Record<string, unknown>, dotted: string): void {
+  const keys = dotted.split(".");
+  const stack: Array<{ parent: Record<string, unknown>; key: string }> = [];
+  let cursor: Record<string, unknown> = obj;
+  for (let i = 0; i < keys.length - 1; i++) {
+    const key = keys[i]!;
+    const next = cursor[key];
+    if (!isPlainObject(next)) return;
+    stack.push({ parent: cursor, key });
+    cursor = next;
+  }
+  const leaf = keys[keys.length - 1]!;
+  if (!(leaf in cursor)) return;
+  delete cursor[leaf];
+  // Prune now-empty parent maps back up the stack.
+  for (let i = stack.length - 1; i >= 0; i--) {
+    const frame = stack[i]!;
+    const target = frame.parent[frame.key] as Record<string, unknown>;
+    if (Object.keys(target).length === 0) {
+      delete frame.parent[frame.key];
+    } else {
+      break;
+    }
+  }
+}
+
 function isPlainObject(v: unknown): v is Record<string, unknown> {
   return typeof v === "object" && v !== null && !Array.isArray(v);
 }

apps/memos-local-plugin/tests/unit/config/writer.test.ts

@@ -53,7 +53,9 @@ llm:
   it("validates after merge — invalid patches are rejected", async () => {
     const ctx = await makeTmpHome({ agent: "openclaw" });
     cleanup = ctx.cleanup;
-    await expect(patchConfig(ctx.home, { viewer: { port: -3 } as Record<string, unknown> }))
+    // `viewer.port` is adapter-owned and silently stripped from patches
+    // (see #2212), so pick a still-validated field for the schema check.
+    await expect(patchConfig(ctx.home, { bridge: { port: -3 } as Record<string, unknown> }))
       .rejects.toThrow(/schema validation/);
   });
 
@@ -118,4 +120,158 @@ skillEvolver: ""
     expect(reloaded.config.skillEvolver.provider).toBe("gemini");
     expect(reloaded.config.skillEvolver.model).toBe("gemini-2.5-flash");
   });
+
+  /**
+   * Regression: #2212. On Hermes the viewer daemon is hardcoded to :18800
+   * (see bridge.mts::AGENT_DEFAULT_PORTS), but the shared UI default in
+   * defaults.ts is :18799 (the OpenClaw port). A PATCH body that carries
+   * `viewer.port: 18799` — from a viewer form that rehydrated the
+   * cross-agent default, or from any third-party client that mirrors GET
+   * back into PATCH — used to be written to disk verbatim, silently
+   * corrupting the Hermes config so the bridge could not find the viewer
+   * on next start. The writer must protect the fixed `viewer.port` while
+   * leaving other viewer settings patchable.
+   */
+  it("ignores viewer.port in the incoming patch to protect adapter ownership", async () => {
+    const original = `viewer:
+  port: 18800
+  bindHost: 127.0.0.1
+llm:
+  provider: openai_compatible
+`;
+    const ctx = await makeTmpHome({ agent: "hermes", configYaml: original });
+    cleanup = ctx.cleanup;
+
+    await patchConfig(ctx.home, {
+      viewer: { port: 18799 },
+      llm: { temperature: 0.4 },
+    });
+
+    const reloaded = await loadConfig(ctx.home);
+    // viewer.port must survive untouched — the adapter owns it.
+    expect(reloaded.config.viewer.port).toBe(18800);
+    // Sibling patches still land normally.
+    expect(reloaded.config.llm.temperature).toBe(0.4);
+    // On-disk YAML must not contain the rejected 18799 value under viewer.
+    const text = await fs.readFile(ctx.home.configFile, "utf8");
+    expect(text).not.toMatch(/port:\s*18799/);
+    expect(text).toMatch(/port:\s*18800/);
+  });
+
+  it("allows patching viewer.bindHost because the server honors it", async () => {
+    const original = `viewer:
+  port: 18800
+  bindHost: 127.0.0.1
+`;
+    const ctx = await makeTmpHome({ agent: "hermes", configYaml: original });
+    cleanup = ctx.cleanup;
+
+    await patchConfig(ctx.home, {
+      viewer: { bindHost: "0.0.0.0" },
+    });
+
+    const reloaded = await loadConfig(ctx.home);
+    expect(reloaded.config.viewer.bindHost).toBe("0.0.0.0");
+    const text = await fs.readFile(ctx.home.configFile, "utf8");
+    expect(text).toMatch(/bindHost:\s*0\.0\.0\.0/);
+  });
+
+  /**
+   * viewer.openOnFirstTurn is an actual user-facing preference (the UI
+   * exposes it via the settings page), so it must remain patchable even
+   * though it sits under the same `viewer:` map as the adapter-owned port.
+   */
+  it("still allows patching non-adapter viewer fields (openOnFirstTurn)", async () => {
+    const original = `viewer:
+  port: 18800
+  bindHost: 127.0.0.1
+  openOnFirstTurn: false
+`;
+    const ctx = await makeTmpHome({ agent: "hermes", configYaml: original });
+    cleanup = ctx.cleanup;
+
+    await patchConfig(ctx.home, {
+      viewer: { openOnFirstTurn: true, port: 18799 },
+    });
+
+    const reloaded = await loadConfig(ctx.home);
+    expect(reloaded.config.viewer.openOnFirstTurn).toBe(true);
+    expect(reloaded.config.viewer.port).toBe(18800);
+  });
+
+  /** Empty means "use the provider default" and must remain patchable. */
+  it("allows clearing embedding.endpoint to restore the provider default", async () => {
+    const original = `embedding:
+  provider: openai_compatible
+  endpoint: "https://api.openai.com/v1"
+  model: text-embedding-3-small
+  apiKey: "sk-existing"
+`;
+    const ctx = await makeTmpHome({ agent: "hermes", configYaml: original });
+    cleanup = ctx.cleanup;
+
+    await patchConfig(ctx.home, {
+      embedding: { endpoint: "" },
+    });
+
+    const reloaded = await loadConfig(ctx.home);
+    expect(reloaded.config.embedding.endpoint).toBe("");
+    const text = await fs.readFile(ctx.home.configFile, "utf8");
+    expect(text).toMatch(/endpoint:\s*""/);
+  });
+
+  /**
+   * A non-empty whitespace-only string is never a usable endpoint. Ignore
+   * that accidental form value without conflating it with the valid empty
+   * reset above.
+   */
+  it("does not overwrite endpoint fields with whitespace-only patches", async () => {
+    const original = `embedding:
+  provider: openai_compatible
+  endpoint: "https://api.openai.com/v1"
+llm:
+  provider: openai_compatible
+  endpoint: "https://api.openai.com/v1"
+`;
+    const ctx = await makeTmpHome({ agent: "hermes", configYaml: original });
+    cleanup = ctx.cleanup;
+
+    await patchConfig(ctx.home, {
+      embedding: { endpoint: "   " },
+      llm: { endpoint: "\t\n" },
+    });
+
+    const reloaded = await loadConfig(ctx.home);
+    expect(reloaded.config.embedding.endpoint).toBe("https://api.openai.com/v1");
+    expect(reloaded.config.llm.endpoint).toBe("https://api.openai.com/v1");
+  });
+
+  /**
+   * Regression: the sanitiser used to clone the patch via
+   * `JSON.parse(JSON.stringify(...))`, which silently deletes keys whose
+   * value is `undefined`. A caller that legitimately passes
+   * `{ llm: { temperature: undefined } }` (e.g. a form that meant to unset
+   * the override, or a mis-serialised client payload) would have that leaf
+   * disappear before `applyPatch` could act on it, meaning the writer
+   * would silently no-op instead of surfacing the invalid state. Switching
+   * to `structuredClone` preserves the full object graph so the schema
+   * validator sees the invalid leaf and rejects the patch, giving the
+   * caller a clear error instead of silent success.
+   */
+  it("preserves undefined leaves in the patch (structured-clone semantics)", async () => {
+    const ctx = await makeTmpHome({ agent: "openclaw" });
+    cleanup = ctx.cleanup;
+
+    // Old JSON-clone behaviour: `undefined` dropped, patch becomes
+    // `{ llm: {} }`, applyPatch no-ops, schema passes → test would pass.
+    // New structuredClone behaviour: `undefined` survives, applyPatch
+    // calls `doc.setIn(['llm','temperature'], undefined)` which writes a
+    // null scalar, schema then rejects "Expected number" → this assertion
+    // captures the shift.
+    await expect(
+      patchConfig(ctx.home, {
+        llm: { temperature: undefined } as Record<string, unknown>,
+      }),
+    ).rejects.toThrow(/schema validation/);
+  });
 });