From 34fbf56e1eb32c1298a2fc3a125250a5f57fbbd8 Mon Sep 17 00:00:00 2001 From: MemOS AutoDev Date: Wed, 5 Aug 2026 14:36:06 +0800 Subject: [PATCH 1/3] fix(memos-local-plugin): protect adapter-owned viewer keys on config PATCH MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sanitize incoming PATCH /api/v1/config bodies inside core/config/writer.ts so viewer.port and viewer.bindHost — owned at runtime by the Hermes/OpenClaw adapters via bridge.mts AGENT_DEFAULT_PORTS — are silently stripped before the deep-merge writes them to disk. Also drop empty-string patches on embedding/llm/l3Llm/skillEvolver endpoint fields to prevent the UI's rehydrated placeholder from wiping a previously-configured endpoint. Fixes #2212: on Hermes, saving from the Memory Viewer used to clobber viewer.port with the UI default 18799 (the OpenClaw port), breaking the bridge until config.yaml was hand-edited back to 18800. - Added sanitizePatch() helper with ADAPTER_OWNED_PATCH_PATHS and NON_EMPTY_PATCH_PATHS whitelists; prunes now-empty parent maps. - 4 new tests in tests/unit/config/writer.test.ts covering both guards plus the openOnFirstTurn-still-patchable invariant. - Updated existing schema-validation test to use bridge.port since viewer.port now bypasses validation via strip. Test evidence: 11/11 writer tests pass, 1265/1265 unit tests pass, tsc --noEmit clean. --- apps/memos-local-plugin/core/config/writer.ts | 94 ++++++++++++++- .../tests/unit/config/writer.test.ts | 110 +++++++++++++++++- 2 files changed, 202 insertions(+), 2 deletions(-) diff --git a/apps/memos-local-plugin/core/config/writer.ts b/apps/memos-local-plugin/core/config/writer.ts index 512d2e052..da601944d 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,97 @@ 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", + "viewer.bindHost", +]); + +/** + * Empty-string patches on these fields are silently dropped (like the + * secret-field treatment in `stripEmptySecrets` on the API layer). This + * covers UI rehydration paths where an untouched form field would + * otherwise send `""` and overwrite a previously-configured value — the + * companion symptom from #2212 where `embedding.endpoint` was clobbered + * with a stray value after save. Secret fields are handled upstream. + */ +const NON_EMPTY_PATCH_PATHS: readonly string[] = Object.freeze([ + "embedding.endpoint", + "llm.endpoint", + "l3Llm.endpoint", + "skillEvolver.endpoint", +]); + +/** + * Strip adapter-owned keys and empty-string 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 { + const cloned = JSON.parse(JSON.stringify(patch)) as Record; + for (const dotted of ADAPTER_OWNED_PATCH_PATHS) { + deleteDottedPath(cloned, dotted); + } + for (const dotted of NON_EMPTY_PATCH_PATHS) { + const value = readDottedPath(cloned, dotted); + if (value === "") { + 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..83091dcb9 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,110 @@ 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 adapter-owned viewer fields + * (`viewer.port`, `viewer.bindHost`) and preserve whatever is already + * on disk instead. + */ + 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("ignores viewer.bindHost in the incoming patch to protect adapter ownership", 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("127.0.0.1"); + const text = await fs.readFile(ctx.home.configFile, "utf8"); + expect(text).not.toMatch(/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/bindHost fields. + */ + 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); + }); + + /** + * Companion of the viewer.port guard: the reporter also observed a + * placeholder-looking `embedding.endpoint: "mem os"` slipping into the + * on-disk config after a save. Empty-string patches on `embedding.endpoint` + * (rehydrated by the UI when the field is untouched) must not clobber a + * previously-configured endpoint — mirror the treatment `stripEmptySecrets` + * gives to `apiKey` fields. + */ + it("does not overwrite embedding.endpoint with an empty-string patch", 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("https://api.openai.com/v1"); + }); }); From b3e21738e23c53840afa535d578a891c46deda1f Mon Sep 17 00:00:00 2001 From: MemOS AutoDev Date: Wed, 5 Aug 2026 14:59:36 +0800 Subject: [PATCH 2/3] fix(memos-local-plugin): harden config PATCH sanitiser (OCR follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address two Open Code Review findings on the #2212 fix: - writer.ts:171 — replace `JSON.parse(JSON.stringify(patch))` with `structuredClone(patch)`. The JSON round-trip silently drops `undefined` leaves, so a caller passing `{ llm: { endpoint: undefined } }` would have that key vanish before `applyPatch` could act on it — a silent no-op instead of a visible error. structuredClone is available on Node 17+ (package engines require >=20) and preserves the full object graph. - writer.ts:177 — trim before comparing the endpoint guard so whitespace-only strings (`" "`, `"\t\n"`) submitted by UI forms are dropped the same way exact `""` is. Also adds a `typeof` guard so a non-string value at one of these paths cannot crash the check. Mirrors the treatment used by `stripEmptySecrets` on secret fields. Test evidence: - 3 new tests in tests/unit/config/writer.test.ts (whitespace guard, structured-clone semantics — via schema-rejection assertion — and the previously-added empty-string guard remains green). - 13/13 writer tests pass. - 1267/1267 unit tests pass, tsc --noEmit clean. --- apps/memos-local-plugin/core/config/writer.ts | 15 ++++- .../tests/unit/config/writer.test.ts | 58 +++++++++++++++++++ 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/apps/memos-local-plugin/core/config/writer.ts b/apps/memos-local-plugin/core/config/writer.ts index da601944d..8a8e8d07b 100644 --- a/apps/memos-local-plugin/core/config/writer.ts +++ b/apps/memos-local-plugin/core/config/writer.ts @@ -168,13 +168,24 @@ const NON_EMPTY_PATCH_PATHS: readonly string[] = Object.freeze([ * noisier in debug logs). */ function sanitizePatch(patch: Record): Record { - const cloned = JSON.parse(JSON.stringify(patch)) as 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 NON_EMPTY_PATCH_PATHS) { const value = readDottedPath(cloned, dotted); - if (value === "") { + // Trim before comparing so whitespace-only strings (e.g. `" "` from + // a UI form) are treated the same as `""` — otherwise they'd slip past + // this guard and get written to disk as broken endpoint values. The + // typeof guard also keeps this safe if a non-string value shows up at + // one of these paths. + if (typeof value === "string" && value.trim() === "") { deleteDottedPath(cloned, dotted); } } 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 83091dcb9..302886c84 100644 --- a/apps/memos-local-plugin/tests/unit/config/writer.test.ts +++ b/apps/memos-local-plugin/tests/unit/config/writer.test.ts @@ -226,4 +226,62 @@ llm: const reloaded = await loadConfig(ctx.home); expect(reloaded.config.embedding.endpoint).toBe("https://api.openai.com/v1"); }); + + /** + * Same rationale as the `""` guard above: a UI form can also submit a + * whitespace-only string (`" "`) when the user tabs through the field + * without changing it — some HTML pickers pad the value. The guard must + * trim before comparing so those inputs are treated the same as empty + * strings, otherwise they'd get written to disk verbatim and yield a + * broken endpoint configuration with no visible error. + */ + 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/); + }); }); From eb4ad040f50aee1824c8fe3370487c1f0c933b85 Mon Sep 17 00:00:00 2001 From: jiachengzhen Date: Fri, 7 Aug 2026 01:02:19 +0800 Subject: [PATCH 3/3] fix(plugin): preserve user-owned config patches --- apps/memos-local-plugin/core/config/writer.ts | 27 +++++-------- .../tests/unit/config/writer.test.ts | 38 +++++++------------ 2 files changed, 24 insertions(+), 41 deletions(-) diff --git a/apps/memos-local-plugin/core/config/writer.ts b/apps/memos-local-plugin/core/config/writer.ts index 8a8e8d07b..c7804e574 100644 --- a/apps/memos-local-plugin/core/config/writer.ts +++ b/apps/memos-local-plugin/core/config/writer.ts @@ -127,7 +127,7 @@ function removeUnsupportedUserConfig(doc: ReturnType): void { } /** - * Adapter-owned config keys the client is NEVER allowed to patch via + * 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 @@ -142,18 +142,14 @@ function removeUnsupportedUserConfig(doc: ReturnType): void { */ const ADAPTER_OWNED_PATCH_PATHS: readonly string[] = Object.freeze([ "viewer.port", - "viewer.bindHost", ]); /** - * Empty-string patches on these fields are silently dropped (like the - * secret-field treatment in `stripEmptySecrets` on the API layer). This - * covers UI rehydration paths where an untouched form field would - * otherwise send `""` and overwrite a previously-configured value — the - * companion symptom from #2212 where `embedding.endpoint` was clobbered - * with a stray value after save. Secret fields are handled upstream. + * 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 NON_EMPTY_PATCH_PATHS: readonly string[] = Object.freeze([ +const ENDPOINT_PATCH_PATHS: readonly string[] = Object.freeze([ "embedding.endpoint", "llm.endpoint", "l3Llm.endpoint", @@ -161,7 +157,7 @@ const NON_EMPTY_PATCH_PATHS: readonly string[] = Object.freeze([ ]); /** - * Strip adapter-owned keys and empty-string endpoints from an incoming + * 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 @@ -178,14 +174,11 @@ function sanitizePatch(patch: Record): Record for (const dotted of ADAPTER_OWNED_PATCH_PATHS) { deleteDottedPath(cloned, dotted); } - for (const dotted of NON_EMPTY_PATCH_PATHS) { + for (const dotted of ENDPOINT_PATCH_PATHS) { const value = readDottedPath(cloned, dotted); - // Trim before comparing so whitespace-only strings (e.g. `" "` from - // a UI form) are treated the same as `""` — otherwise they'd slip past - // this guard and get written to disk as broken endpoint values. The - // typeof guard also keeps this safe if a non-string value shows up at - // one of these paths. - if (typeof value === "string" && value.trim() === "") { + // 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); } } 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 302886c84..d9737761c 100644 --- a/apps/memos-local-plugin/tests/unit/config/writer.test.ts +++ b/apps/memos-local-plugin/tests/unit/config/writer.test.ts @@ -129,9 +129,8 @@ skillEvolver: "" * 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 adapter-owned viewer fields - * (`viewer.port`, `viewer.bindHost`) and preserve whatever is already - * on disk instead. + * 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: @@ -159,7 +158,7 @@ llm: expect(text).toMatch(/port:\s*18800/); }); - it("ignores viewer.bindHost in the incoming patch to protect adapter ownership", async () => { + it("allows patching viewer.bindHost because the server honors it", async () => { const original = `viewer: port: 18800 bindHost: 127.0.0.1 @@ -172,16 +171,15 @@ llm: }); const reloaded = await loadConfig(ctx.home); - expect(reloaded.config.viewer.bindHost).toBe("127.0.0.1"); + expect(reloaded.config.viewer.bindHost).toBe("0.0.0.0"); const text = await fs.readFile(ctx.home.configFile, "utf8"); - expect(text).not.toMatch(/0\.0\.0\.0/); + 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/bindHost fields. + * 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: @@ -201,15 +199,8 @@ llm: expect(reloaded.config.viewer.port).toBe(18800); }); - /** - * Companion of the viewer.port guard: the reporter also observed a - * placeholder-looking `embedding.endpoint: "mem os"` slipping into the - * on-disk config after a save. Empty-string patches on `embedding.endpoint` - * (rehydrated by the UI when the field is untouched) must not clobber a - * previously-configured endpoint — mirror the treatment `stripEmptySecrets` - * gives to `apiKey` fields. - */ - it("does not overwrite embedding.endpoint with an empty-string patch", async () => { + /** 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" @@ -224,16 +215,15 @@ llm: }); const reloaded = await loadConfig(ctx.home); - expect(reloaded.config.embedding.endpoint).toBe("https://api.openai.com/v1"); + expect(reloaded.config.embedding.endpoint).toBe(""); + const text = await fs.readFile(ctx.home.configFile, "utf8"); + expect(text).toMatch(/endpoint:\s*""/); }); /** - * Same rationale as the `""` guard above: a UI form can also submit a - * whitespace-only string (`" "`) when the user tabs through the field - * without changing it — some HTML pickers pad the value. The guard must - * trim before comparing so those inputs are treated the same as empty - * strings, otherwise they'd get written to disk verbatim and yield a - * broken endpoint configuration with no visible error. + * 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: