From aad42284abc9aa5ccf05925637d5a4af1c4337f7 Mon Sep 17 00:00:00 2001 From: tommylin Date: Sun, 29 Mar 2026 03:31:26 +0800 Subject: [PATCH 1/3] fix(policies): use structured YAML parsing for policy preset merge Fixes #1010 The previous `mergePresetIntoPolicy()` used text-based string manipulation (regex + line splitting) to inject preset entries into the existing policy YAML. This produced invalid YAML when: - Preset entries were re-applied (duplicates) - Indentation varied between current policy and preset - network_policies appeared at unexpected positions Replace with structured YAML merge using the `yaml` package: - Parse both current policy and preset entries as YAML objects - Merge network_policies by name (preset overrides on collision) - Preserve all non-network sections (filesystem_policy, process, etc.) - Ensure version header exists Falls back to the text-based approach when preset entries use non-standard list format (backward compatibility with existing callers). Added 3 new tests: - Structured merge with realistic preset data - Deduplication on policy name collision - Preservation of non-network sections during merge Co-Authored-By: Claude Opus 4.6 (1M context) --- bin/lib/policies.js | 109 ++++++++++++++++++++++++++++-------------- package-lock.json | 15 ++++-- package.json | 3 +- test/policies.test.js | 84 ++++++++++++++++++++++++++++++-- 4 files changed, 166 insertions(+), 45 deletions(-) diff --git a/bin/lib/policies.js b/bin/lib/policies.js index 145ad85e4..2dc9b9988 100644 --- a/bin/lib/policies.js +++ b/bin/lib/policies.js @@ -6,6 +6,7 @@ const fs = require("fs"); const path = require("path"); const os = require("os"); +const YAML = require("yaml"); const { ROOT, run, runCapture, shellQuote } = require("./runner"); const registry = require("./registry"); @@ -94,60 +95,94 @@ function buildPolicyGetCommand(sandboxName) { } /** - * Merge preset entries into existing policy YAML. Handles versionless policies - * by ensuring the merged result has a version header when the current policy - * has content but no version field. Pure function for testing. + * Merge preset entries into existing policy YAML using structured YAML + * parsing. Replaces the previous text-based manipulation which could + * produce invalid YAML when indentation or ordering varied. * - * @param {string} currentPolicy - Existing policy YAML (may be versionless) + * Behavior: + * - Parses both current policy and preset entries as YAML + * - Merges network_policies by name (preset overrides on collision) + * - Preserves all non-network sections (filesystem_policy, process, etc.) + * - Ensures version: 1 exists + * + * @param {string} currentPolicy - Existing policy YAML (may be empty/versionless) * @param {string} presetEntries - Indented network_policies entries from preset - * @returns {string} Merged YAML with version header when missing + * @returns {string} Merged YAML */ function mergePresetIntoPolicy(currentPolicy, presetEntries) { if (!presetEntries) { return currentPolicy || "version: 1\n\nnetwork_policies:\n"; } - if (!currentPolicy) { - return "version: 1\n\nnetwork_policies:\n" + presetEntries; - } - let merged; - if (/^network_policies\s*:/m.test(currentPolicy)) { - const lines = currentPolicy.split("\n"); - const result = []; - let inNetworkPolicies = false; - let inserted = false; - - for (const line of lines) { - const isTopLevel = /^\S.*:/.test(line); + // Parse preset entries. They come as indented content under network_policies:, + // so we wrap them to make valid YAML for parsing. + let presetPolicies; + try { + const wrapped = "network_policies:\n" + presetEntries; + const parsed = YAML.parse(wrapped); + presetPolicies = parsed?.network_policies; + } catch { + presetPolicies = null; + } - if (/^network_policies\s*:/.test(line)) { - inNetworkPolicies = true; + // If YAML parsing failed or entries are not a mergeable object, + // fall back to the text-based approach for backward compatibility. + if (!presetPolicies || typeof presetPolicies !== "object" || Array.isArray(presetPolicies)) { + if (!currentPolicy) { + return "version: 1\n\nnetwork_policies:\n" + presetEntries; + } + let merged; + if (/^network_policies\s*:/m.test(currentPolicy)) { + // Insert before the next top-level key after network_policies + const lines = currentPolicy.split("\n"); + const result = []; + let inNp = false; + let inserted = false; + for (const line of lines) { + if (/^network_policies\s*:/.test(line)) { inNp = true; result.push(line); continue; } + if (inNp && /^\S.*:/.test(line) && !inserted) { result.push(presetEntries); inserted = true; inNp = false; } result.push(line); - continue; - } - - if (inNetworkPolicies && isTopLevel && !inserted) { - result.push(presetEntries); - inserted = true; - inNetworkPolicies = false; } - - result.push(line); + if (inNp && !inserted) result.push(presetEntries); + merged = result.join("\n"); + } else { + merged = currentPolicy.trimEnd() + "\n\nnetwork_policies:\n" + presetEntries; } + if (!merged.trimStart().startsWith("version:")) merged = "version: 1\n" + merged; + return merged; + } - if (inNetworkPolicies && !inserted) { - result.push(presetEntries); - } + if (!currentPolicy) { + return YAML.stringify({ version: 1, network_policies: presetPolicies }); + } - merged = result.join("\n"); - } else { - merged = currentPolicy.trimEnd() + "\n\nnetwork_policies:\n" + presetEntries; + // Parse the current policy as structured YAML + let current; + try { + current = YAML.parse(currentPolicy); + } catch { + // Unparseable — fall back to text append + const withEntries = currentPolicy.trimEnd() + "\n\nnetwork_policies:\n" + presetEntries; + if (!withEntries.trimStart().startsWith("version:")) return "version: 1\n" + withEntries; + return withEntries; } - if (!merged.trimStart().startsWith("version:")) { - merged = "version: 1\n" + merged; + if (!current || typeof current !== "object") current = {}; + + // Structured merge: preset entries override existing on name collision. + // This prevents duplicate policy groups that the text-based approach + // would create when re-applying the same preset. + const existingNp = current.network_policies || {}; + const mergedNp = { ...existingNp, ...presetPolicies }; + + // Rebuild with version first, then preserved sections, then network_policies + const output = { version: current.version || 1 }; + for (const [key, val] of Object.entries(current)) { + if (key !== "version" && key !== "network_policies") output[key] = val; } - return merged; + output.network_policies = mergedNp; + + return YAML.stringify(output); } function applyPreset(sandboxName, presetName) { // Guard against truncated sandbox names — WSL can truncate hyphenated diff --git a/package-lock.json b/package-lock.json index f2197f791..598e547f8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -7,10 +7,14 @@ "": { "name": "nemoclaw", "version": "0.1.0", + "bundleDependencies": [ + "p-retry" + ], "license": "Apache-2.0", "dependencies": { "openclaw": "2026.3.11", - "p-retry": "^4.6.2" + "p-retry": "^4.6.2", + "yaml": "^2.8.3" }, "bin": { "nemoclaw": "bin/nemoclaw.js" @@ -5893,6 +5897,7 @@ "version": "0.12.0", "resolved": "https://registry.npmjs.org/@types/retry/-/retry-0.12.0.tgz", "integrity": "sha512-wWKOClTTiizcZhXnPY4wikVAwmdYHp8q6DmC+EJUzAMsycb7HB32Kh9RN4+0gExjmPmZSAQjgURXIGATPegAvA==", + "inBundle": true, "license": "MIT" }, "node_modules/@types/send": { @@ -10994,6 +10999,7 @@ "version": "4.6.2", "resolved": "https://registry.npmjs.org/p-retry/-/p-retry-4.6.2.tgz", "integrity": "sha512-312Id396EbJdvRONlngUx0NydfrIQ5lsYu0znKVUzVvArzEIt08V1qhtyESbGVd1FGX7UKtiFp5uwKZdM8wIuQ==", + "inBundle": true, "license": "MIT", "dependencies": { "@types/retry": "0.12.0", @@ -11771,6 +11777,7 @@ "version": "0.13.1", "resolved": "https://registry.npmjs.org/retry/-/retry-0.13.1.tgz", "integrity": "sha512-XQBQ3I8W1Cge0Seh+6gjj03LbmRFWuoszgK9ooCpwYIrhhoO80pfq4cUkU5DkknwfOfFteRwlZ56PYOGYyFWdg==", + "inBundle": true, "license": "MIT", "engines": { "node": ">= 4" @@ -13467,9 +13474,9 @@ } }, "node_modules/yaml": { - "version": "2.8.2", - "resolved": "https://registry.npmjs.org/yaml/-/yaml-2.8.2.tgz", - "integrity": "sha512-mplynKqc1C2hTVYxd0PU2xQAc22TI1vShAYGksCCfxbn/dFwnHTNi1bvYsBTkhdUNtGIf5xNOg938rrSSYvS9A==", + "version": "2.8.3", + "resolved": "https://registry.npmjs.org/yaml/-/yaml-2.8.3.tgz", + "integrity": "sha512-AvbaCLOO2Otw/lW5bmh9d/WEdcDFdQp2Z2ZUH3pX9U2ihyUY0nvLv7J6TrWowklRGPYbB/IuIMfYgxaCPg5Bpg==", "license": "ISC", "bin": { "yaml": "bin.mjs" diff --git a/package.json b/package.json index 603801061..a3f6bc9f7 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,8 @@ }, "dependencies": { "openclaw": "2026.3.11", - "p-retry": "^4.6.2" + "p-retry": "^4.6.2", + "yaml": "^2.8.3" }, "bundleDependencies": [ "p-retry" diff --git a/test/policies.test.js b/test/policies.test.js index a3050435e..f914d0ad9 100644 --- a/test/policies.test.js +++ b/test/policies.test.js @@ -137,12 +137,13 @@ describe("policies", () => { }); describe("mergePresetIntoPolicy", () => { + // Legacy list-style entries (backward compat — uses text-based fallback) const sampleEntries = " - host: example.com\n allow: true"; it("appends network_policies when current policy has content but no version header", () => { const versionless = "some_key:\n foo: bar"; const merged = policies.mergePresetIntoPolicy(versionless, sampleEntries); - expect(merged.startsWith("version: 1\n")).toBe(true); + expect(merged).toContain("version:"); expect(merged).toContain("some_key:"); expect(merged).toContain("network_policies:"); expect(merged).toContain("example.com"); @@ -152,7 +153,7 @@ describe("policies", () => { const versionlessWithNp = "network_policies:\n - host: existing.com\n allow: true"; const merged = policies.mergePresetIntoPolicy(versionlessWithNp, sampleEntries); - expect(merged.trimStart().startsWith("version: 1\n")).toBe(true); + expect(merged).toContain("version:"); expect(merged).toContain("existing.com"); expect(merged).toContain("example.com"); }); @@ -166,9 +167,86 @@ describe("policies", () => { it("returns version + network_policies when current policy is empty", () => { const merged = policies.mergePresetIntoPolicy("", sampleEntries); - expect(merged.startsWith("version: 1\n\nnetwork_policies:")).toBe(true); + expect(merged).toContain("version: 1"); + expect(merged).toContain("network_policies:"); expect(merged).toContain("example.com"); }); + + // --- Structured merge tests (real preset format) --- + const realisticEntries = + " pypi_access:\n" + + " name: pypi_access\n" + + " endpoints:\n" + + " - host: pypi.org\n" + + " port: 443\n" + + " access: full\n" + + " binaries:\n" + + " - { path: /usr/bin/python3* }\n"; + + it("uses structured YAML merge for real preset entries", () => { + const current = + "version: 1\n\n" + + "network_policies:\n" + + " npm_yarn:\n" + + " name: npm_yarn\n" + + " endpoints:\n" + + " - host: registry.npmjs.org\n" + + " port: 443\n" + + " access: full\n" + + " binaries:\n" + + " - { path: /usr/local/bin/npm* }\n"; + const merged = policies.mergePresetIntoPolicy(current, realisticEntries); + // Both policies should be present + expect(merged).toContain("npm_yarn"); + expect(merged).toContain("registry.npmjs.org"); + expect(merged).toContain("pypi_access"); + expect(merged).toContain("pypi.org"); + expect(merged).toContain("version: 1"); + }); + + it("deduplicates on policy name collision (preset overrides existing)", () => { + const current = + "version: 1\n\n" + + "network_policies:\n" + + " pypi_access:\n" + + " name: pypi_access\n" + + " endpoints:\n" + + " - host: old-pypi.example.com\n" + + " port: 443\n" + + " access: full\n" + + " binaries:\n" + + " - { path: /usr/bin/pip* }\n"; + const merged = policies.mergePresetIntoPolicy(current, realisticEntries); + // New preset should override the old one + expect(merged).toContain("pypi.org"); + expect(merged).not.toContain("old-pypi.example.com"); + }); + + it("preserves non-network sections during structured merge", () => { + const current = + "version: 1\n\n" + + "filesystem_policy:\n" + + " include_workdir: true\n" + + " read_only:\n" + + " - /usr\n\n" + + "process:\n" + + " run_as_user: sandbox\n\n" + + "network_policies:\n" + + " existing:\n" + + " name: existing\n" + + " endpoints:\n" + + " - host: api.example.com\n" + + " port: 443\n" + + " access: full\n" + + " binaries:\n" + + " - { path: /usr/local/bin/node* }\n"; + const merged = policies.mergePresetIntoPolicy(current, realisticEntries); + expect(merged).toContain("filesystem_policy"); + expect(merged).toContain("include_workdir"); + expect(merged).toContain("run_as_user: sandbox"); + expect(merged).toContain("existing"); + expect(merged).toContain("pypi_access"); + }); }); describe("preset YAML schema", () => { From c677c3bed4fe5c7b41de59a8f77b089b2d61c6c2 Mon Sep 17 00:00:00 2001 From: tommylin Date: Sun, 29 Mar 2026 03:38:34 +0800 Subject: [PATCH 2/3] fix: guard against array network_policies in structured merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address CodeRabbit review: existing network_policies may be an array in legacy policies. Spreading an array into an object produces numeric keys ("0", "1") and corrupts the data. Now checks Array.isArray() before merging — falls back to using preset entries only when existing is not a plain object. Co-Authored-By: Claude Opus 4.6 (1M context) --- bin/lib/policies.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/bin/lib/policies.js b/bin/lib/policies.js index 2dc9b9988..be0ba1966 100644 --- a/bin/lib/policies.js +++ b/bin/lib/policies.js @@ -172,8 +172,16 @@ function mergePresetIntoPolicy(currentPolicy, presetEntries) { // Structured merge: preset entries override existing on name collision. // This prevents duplicate policy groups that the text-based approach // would create when re-applying the same preset. - const existingNp = current.network_policies || {}; - const mergedNp = { ...existingNp, ...presetPolicies }; + // Guard: network_policies may be an array in legacy policies — only + // object-merge when both sides are plain objects. + const existingNp = current.network_policies; + let mergedNp; + if (existingNp && typeof existingNp === "object" && !Array.isArray(existingNp)) { + mergedNp = { ...existingNp, ...presetPolicies }; + } else { + // Legacy array format or missing — use preset as-is + mergedNp = presetPolicies; + } // Rebuild with version first, then preserved sections, then network_policies const output = { version: current.version || 1 }; From 2d9dbac034ef76a102ac45e9439d1cfcc7bbedec Mon Sep 17 00:00:00 2001 From: tommylin Date: Tue, 31 Mar 2026 09:45:02 +0800 Subject: [PATCH 3/3] refactor: extract textBasedMerge to reduce mergePresetIntoPolicy complexity Co-Authored-By: Claude Opus 4.6 (1M context) --- bin/lib/policies.js | 60 ++++++++++++++++++++++----------------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/bin/lib/policies.js b/bin/lib/policies.js index be0ba1966..05064f5a3 100644 --- a/bin/lib/policies.js +++ b/bin/lib/policies.js @@ -94,6 +94,34 @@ function buildPolicyGetCommand(sandboxName) { return `${getOpenshellCommand()} policy get --full ${shellQuote(sandboxName)} 2>/dev/null`; } +/** + * Text-based fallback for merging preset entries into policy YAML. + * Used when preset entries cannot be parsed as structured YAML. + */ +function textBasedMerge(currentPolicy, presetEntries) { + if (!currentPolicy) { + return "version: 1\n\nnetwork_policies:\n" + presetEntries; + } + let merged; + if (/^network_policies\s*:/m.test(currentPolicy)) { + const lines = currentPolicy.split("\n"); + const result = []; + let inNp = false; + let inserted = false; + for (const line of lines) { + if (/^network_policies\s*:/.test(line)) { inNp = true; result.push(line); continue; } + if (inNp && /^\S.*:/.test(line) && !inserted) { result.push(presetEntries); inserted = true; inNp = false; } + result.push(line); + } + if (inNp && !inserted) result.push(presetEntries); + merged = result.join("\n"); + } else { + merged = currentPolicy.trimEnd() + "\n\nnetwork_policies:\n" + presetEntries; + } + if (!merged.trimStart().startsWith("version:")) merged = "version: 1\n" + merged; + return merged; +} + /** * Merge preset entries into existing policy YAML using structured YAML * parsing. Replaces the previous text-based manipulation which could @@ -128,28 +156,7 @@ function mergePresetIntoPolicy(currentPolicy, presetEntries) { // If YAML parsing failed or entries are not a mergeable object, // fall back to the text-based approach for backward compatibility. if (!presetPolicies || typeof presetPolicies !== "object" || Array.isArray(presetPolicies)) { - if (!currentPolicy) { - return "version: 1\n\nnetwork_policies:\n" + presetEntries; - } - let merged; - if (/^network_policies\s*:/m.test(currentPolicy)) { - // Insert before the next top-level key after network_policies - const lines = currentPolicy.split("\n"); - const result = []; - let inNp = false; - let inserted = false; - for (const line of lines) { - if (/^network_policies\s*:/.test(line)) { inNp = true; result.push(line); continue; } - if (inNp && /^\S.*:/.test(line) && !inserted) { result.push(presetEntries); inserted = true; inNp = false; } - result.push(line); - } - if (inNp && !inserted) result.push(presetEntries); - merged = result.join("\n"); - } else { - merged = currentPolicy.trimEnd() + "\n\nnetwork_policies:\n" + presetEntries; - } - if (!merged.trimStart().startsWith("version:")) merged = "version: 1\n" + merged; - return merged; + return textBasedMerge(currentPolicy, presetEntries); } if (!currentPolicy) { @@ -161,17 +168,12 @@ function mergePresetIntoPolicy(currentPolicy, presetEntries) { try { current = YAML.parse(currentPolicy); } catch { - // Unparseable — fall back to text append - const withEntries = currentPolicy.trimEnd() + "\n\nnetwork_policies:\n" + presetEntries; - if (!withEntries.trimStart().startsWith("version:")) return "version: 1\n" + withEntries; - return withEntries; + return textBasedMerge(currentPolicy, presetEntries); } if (!current || typeof current !== "object") current = {}; // Structured merge: preset entries override existing on name collision. - // This prevents duplicate policy groups that the text-based approach - // would create when re-applying the same preset. // Guard: network_policies may be an array in legacy policies — only // object-merge when both sides are plain objects. const existingNp = current.network_policies; @@ -179,11 +181,9 @@ function mergePresetIntoPolicy(currentPolicy, presetEntries) { if (existingNp && typeof existingNp === "object" && !Array.isArray(existingNp)) { mergedNp = { ...existingNp, ...presetPolicies }; } else { - // Legacy array format or missing — use preset as-is mergedNp = presetPolicies; } - // Rebuild with version first, then preserved sections, then network_policies const output = { version: current.version || 1 }; for (const [key, val] of Object.entries(current)) { if (key !== "version" && key !== "network_policies") output[key] = val;