diff --git a/apps/memos-local-plugin/core/config/writer.ts b/apps/memos-local-plugin/core/config/writer.ts index 512d2e052..c7804e574 100644 --- a/apps/memos-local-plugin/core/config/writer.ts +++ b/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), ""); - 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): 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): Record { + // 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; + 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, 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)[key]; + } + return cursor; +} + +function deleteDottedPath(obj: Record, dotted: string): void { + const keys = dotted.split("."); + const stack: Array<{ parent: Record; key: string }> = []; + let cursor: Record = 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; + if (Object.keys(target).length === 0) { + delete frame.parent[frame.key]; + } else { + break; + } + } +} + function isPlainObject(v: unknown): v is Record { return typeof v === "object" && v !== null && !Array.isArray(v); } diff --git a/apps/memos-local-plugin/tests/unit/config/writer.test.ts b/apps/memos-local-plugin/tests/unit/config/writer.test.ts index fd89ea053..d9737761c 100644 --- a/apps/memos-local-plugin/tests/unit/config/writer.test.ts +++ b/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 })) + // `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 })) .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, + }), + ).rejects.toThrow(/schema validation/); + }); });