From cb391f4bdc75f069ccf642842b25966932068ff2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=B8=8D=E5=81=9A=E4=BA=86=E7=9D=A1=E5=A4=A7=E8=A7=89?= <64798754+stakeswky@users.noreply.github.com> Date: Mon, 16 Feb 2026 21:13:51 +0800 Subject: [PATCH] fix(config): prevent config.patch from destroying arrays when patch entries lack id (#18030) Merged via /review-pr -> /prepare-pr -> /merge-pr. Prepared head SHA: a857df9e328f17774bc345a9567aaa39f94038ae Co-authored-by: stakeswky <64798754+stakeswky@users.noreply.github.com> Co-authored-by: sebslight <19554889+sebslight@users.noreply.github.com> Reviewed-by: @sebslight --- CHANGELOG.md | 1 + src/config/merge-patch.test.ts | 70 +++++++++++++++++++++ src/config/merge-patch.ts | 25 +++++--- src/gateway/server.config-patch.e2e.test.ts | 41 ++++++++++++ 4 files changed, 130 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e49d930be46..235981b8b07 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ Docs: https://docs.openclaw.ai - Skills/Linux: harden go installer fallback on apt-based systems by handling root/no-sudo environments safely, doing best-effort apt index refresh, and returning actionable errors instead of failing with spawn errors. (#17687) Thanks @mcrolly. - Web Fetch/Security: cap downloaded response body size before HTML parsing to prevent memory exhaustion from oversized or deeply nested pages. Thanks @xuemian168. - Config/Gateway: make sensitive-key whitelist suffix matching case-insensitive while preserving `passwordFile` path exemptions, preventing accidental redaction of non-secret config values like `maxTokens` and IRC password-file paths. (#16042) Thanks @akramcodez. +- Gateway/Config: prevent `config.patch` from falling back to full-array replacement when an object-array patch includes entries without `id`, so partial `agents.list` updates no longer risk deleting unrelated agents. (#17989) Thanks @stakeswky. - Dev tooling: harden git `pre-commit` hook against option injection from malicious filenames (for example `--force`), preventing accidental staging of ignored files. Thanks @mrthankyou. - Gateway/Agent: reject malformed `agent:`-prefixed session keys (for example, `agent:main`) in `agent` and `agent.identity.get` instead of silently resolving them to the default agent, preventing accidental cross-session routing. (#15707) Thanks @rodrigouroz. - Gateway/Chat: harden `chat.send` inbound message handling by rejecting null bytes, stripping unsafe control characters, and normalizing Unicode to NFC before dispatch. (#8593) Thanks @fr33d3m0n. diff --git a/src/config/merge-patch.test.ts b/src/config/merge-patch.test.ts index 750d743d684..203227c33e4 100644 --- a/src/config/merge-patch.test.ts +++ b/src/config/merge-patch.test.ts @@ -59,6 +59,76 @@ describe("applyMergePatch", () => { expect(secondary?.workspace).toBe("/tmp/two"); }); + it("merges by id even when patch entries lack id (appends them)", () => { + const base = { + agents: { + list: [ + { id: "primary", workspace: "/tmp/one" }, + { id: "secondary", workspace: "/tmp/two" }, + ], + }, + }; + const patch = { + agents: { + list: [ + { id: "primary", model: "new-model" }, + { workspace: "/tmp/orphan" }, // no id + ], + }, + }; + + const merged = applyMergePatch(base, patch, { + mergeObjectArraysById: true, + }) as { + agents?: { + list?: Array<{ id?: string; workspace?: string; model?: string }>; + }; + }; + // Both original entries preserved, patch without id appended + expect(merged.agents?.list).toHaveLength(3); + const primary = merged.agents?.list?.find((entry) => entry.id === "primary"); + expect(primary?.workspace).toBe("/tmp/one"); + expect(primary?.model).toBe("new-model"); + expect(merged.agents?.list?.[1]?.id).toBe("secondary"); + expect(merged.agents?.list?.[2]?.workspace).toBe("/tmp/orphan"); + }); + + it("does not destroy agents list when patching a single agent by id", () => { + const base = { + agents: { + list: [ + { id: "main", default: true, workspace: "/home/main" }, + { id: "ota", workspace: "/home/ota" }, + { id: "trading", workspace: "/home/trading" }, + { id: "codex", workspace: "/home/codex" }, + ], + }, + }; + const patch = { + agents: { + list: [{ id: "main", model: "claude-opus-4-20250918" }], + }, + }; + + const merged = applyMergePatch(base, patch, { + mergeObjectArraysById: true, + }) as { + agents?: { + list?: Array<{ id?: string; workspace?: string; model?: string; default?: boolean }>; + }; + }; + // All 4 agents must survive + expect(merged.agents?.list).toHaveLength(4); + const main = merged.agents?.list?.find((e) => e.id === "main"); + expect(main?.model).toBe("claude-opus-4-20250918"); + expect(main?.default).toBe(true); + expect(main?.workspace).toBe("/home/main"); + // Others untouched + expect(merged.agents?.list?.find((e) => e.id === "ota")?.workspace).toBe("/home/ota"); + expect(merged.agents?.list?.find((e) => e.id === "trading")?.workspace).toBe("/home/trading"); + expect(merged.agents?.list?.find((e) => e.id === "codex")?.workspace).toBe("/home/codex"); + }); + it("falls back to replacement for non-id arrays even when enabled", () => { const base = { channels: { diff --git a/src/config/merge-patch.ts b/src/config/merge-patch.ts index 5878fc57e16..6e7627cde4b 100644 --- a/src/config/merge-patch.ts +++ b/src/config/merge-patch.ts @@ -14,23 +14,34 @@ function isObjectWithStringId(value: unknown): value is Record } function mergeObjectArraysById(base: unknown[], patch: unknown[], options: MergePatchOptions) { - if (!base.every(isObjectWithStringId) || !patch.every(isObjectWithStringId)) { + // Require all *base* entries to have string ids — if the existing array + // isn't id-keyed there's nothing sensible to merge against. + if (!base.every(isObjectWithStringId)) { return undefined; } + const merged = [...base] as Array & { id: string }>; const indexById = new Map(); for (const [index, entry] of merged.entries()) { indexById.set(entry.id, index); } - for (const entry of patch) { - const existingIndex = indexById.get(entry.id); - if (existingIndex === undefined) { - merged.push(structuredClone(entry)); - indexById.set(entry.id, merged.length - 1); + for (const patchEntry of patch) { + // Patch entries without a valid id are appended as-is (best-effort). + // This prevents the entire merge from falling back to full replacement + // just because one patch element is missing an id field. + if (!isObjectWithStringId(patchEntry)) { + merged.push(structuredClone(patchEntry) as Record & { id: string }); continue; } - merged[existingIndex] = applyMergePatch(merged[existingIndex], entry, options) as Record< + + const existingIndex = indexById.get(patchEntry.id); + if (existingIndex === undefined) { + merged.push(structuredClone(patchEntry)); + indexById.set(patchEntry.id, merged.length - 1); + continue; + } + merged[existingIndex] = applyMergePatch(merged[existingIndex], patchEntry, options) as Record< string, unknown > & { id: string }; diff --git a/src/gateway/server.config-patch.e2e.test.ts b/src/gateway/server.config-patch.e2e.test.ts index 4ea6f08810e..19fdd5ad376 100644 --- a/src/gateway/server.config-patch.e2e.test.ts +++ b/src/gateway/server.config-patch.e2e.test.ts @@ -91,6 +91,47 @@ describe("gateway config methods", () => { expect(primary?.workspace).toBe("/tmp/primary-updated"); expect(secondary?.workspace).toBe("/tmp/secondary"); }); + + it("rejects mixed-id agents.list patches without mutating persisted config", async () => { + const setRes = await rpcReq<{ ok?: boolean }>(ws, "config.set", { + raw: JSON.stringify({ + agents: { + list: [ + { id: "primary", default: true, workspace: "/tmp/primary" }, + { id: "secondary", workspace: "/tmp/secondary" }, + ], + }, + }), + }); + expect(setRes.ok).toBe(true); + + const beforeRes = await rpcReq<{ hash?: string }>(ws, "config.get", {}); + expect(beforeRes.ok).toBe(true); + expect(typeof beforeRes.payload?.hash).toBe("string"); + + const patchRes = await rpcReq<{ ok?: boolean }>(ws, "config.patch", { + baseHash: beforeRes.payload?.hash, + raw: JSON.stringify({ + agents: { + list: [ + { + id: "primary", + workspace: "/tmp/primary-updated", + }, + { + workspace: "/tmp/orphan-no-id", + }, + ], + }, + }), + }); + expect(patchRes.ok).toBe(false); + expect(patchRes.error?.message ?? "").toContain("invalid config"); + + const afterRes = await rpcReq<{ hash?: string }>(ws, "config.get", {}); + expect(afterRes.ok).toBe(true); + expect(afterRes.payload?.hash).toBe(beforeRes.payload?.hash); + }); }); describe("gateway server sessions", () => {