From 55c576bbb1451727d5344e099ef640e8ac5a836d Mon Sep 17 00:00:00 2001 From: dtzp555-max Date: Mon, 11 May 2026 03:41:53 +1000 Subject: [PATCH] fix(setup): preserve user-customised plist/systemd env vars on re-setup (#90) * fix(setup): merge plist/systemd env vars instead of overwriting setup.mjs previously wrote the launchd plist and the Linux systemd unit with writeFileSync(path, NEW_TEMPLATE_STRING), which silently dropped any user-customised env vars (CLAUDE_HEARTBEAT_INTERVAL, CLAUDE_CACHE_TTL, etc.) on every re-setup or upgrade. This change introduces scripts/lib/plist-merge.mjs which preserves keys present only in the existing file and lets the template's known keys win. Linux variant uses the same logic against Environment=KEY=VALUE lines. Tests added in test-features.mjs cover preserve / override / first-install for both formats. No cli.js citation needed: this is OCP-internal installer behaviour with no corresponding cli.js operation. Co-Authored-By: Claude Sonnet 4.6 * fix(setup): plist-merge code-quality follow-up Three issues raised by the code-quality reviewer on 0fd1838: 1. mergeSystemdEnv now early-returns when the template has no Environment= anchor, instead of silently dropping preserved lines (defensive guard for a future template change). 2. Both parsers (parsePlistEnv, parseSystemdEnv) now Buffer-normalise their input, removing the TypeError-vs-coerce asymmetry. 3. Two idempotency tests added (calling merge twice on the same input/output pair must be stable). Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: dtzp555 Co-authored-by: Claude Sonnet 4.6 --- scripts/lib/plist-merge.mjs | 86 ++++++++++++++++++++++++++ setup.mjs | 21 +++++-- test-features.mjs | 120 ++++++++++++++++++++++++++++++++++++ 3 files changed, 223 insertions(+), 4 deletions(-) create mode 100644 scripts/lib/plist-merge.mjs diff --git a/scripts/lib/plist-merge.mjs b/scripts/lib/plist-merge.mjs new file mode 100644 index 0000000..52683f5 --- /dev/null +++ b/scripts/lib/plist-merge.mjs @@ -0,0 +1,86 @@ +// scripts/lib/plist-merge.mjs +// +// Preserves user-customised env vars when setup.mjs rewrites the unit file. +// +// Rule: +// - keys present in NEW template → template value wins (template is source of truth) +// - keys ONLY in EXISTING (not in template) → preserved verbatim +// +// No new dependencies — regex-based, plist XY shape +// is stable enough for our hand-written templates in setup.mjs. + +const PLIST_KV_RE = /([^<]+)<\/key>\s*([^<]*)<\/string>/g; + +export function parsePlistEnv(plistContent) { + if (!plistContent) return {}; + if (Buffer.isBuffer(plistContent)) plistContent = plistContent.toString("utf8"); + // Restrict to the EnvironmentVariables dict to avoid catching Label, etc. + const envBlock = plistContent.match(/EnvironmentVariables<\/key>\s*([\s\S]*?)<\/dict>/); + if (!envBlock) return {}; + const out = {}; + let m; + PLIST_KV_RE.lastIndex = 0; + while ((m = PLIST_KV_RE.exec(envBlock[1])) !== null) { + out[m[1]] = m[2]; + } + return out; +} + +export function mergePlistEnv(existing, template) { + if (!existing) return template; + const existingEnv = parsePlistEnv(existing); + const templateEnv = parsePlistEnv(template); + const KNOWN = new Set(Object.keys(templateEnv)); + + const preserved = {}; + for (const [k, v] of Object.entries(existingEnv)) { + if (!KNOWN.has(k)) preserved[k] = v; + } + if (Object.keys(preserved).length === 0) return template; + + const lines = Object.entries(preserved) + .map(([k, v]) => ` ${k}\n ${v}`) + .join("\n"); + + // Inject before the closing of EnvironmentVariables + return template.replace( + /(EnvironmentVariables<\/key>\s*[\s\S]*?)(\n\s*<\/dict>)/, + `$1\n${lines}$2` + ); +} + +const SYSTEMD_KV_RE = /^Environment=([^=]+)=(.*)$/gm; + +export function parseSystemdEnv(serviceContent) { + if (!serviceContent) return {}; + if (Buffer.isBuffer(serviceContent)) serviceContent = serviceContent.toString("utf8"); + const out = {}; + let m; + SYSTEMD_KV_RE.lastIndex = 0; + while ((m = SYSTEMD_KV_RE.exec(serviceContent)) !== null) { + out[m[1]] = m[2]; + } + return out; +} + +export function mergeSystemdEnv(existing, template) { + if (!existing) return template; + const existingEnv = parseSystemdEnv(existing); + const templateEnv = parseSystemdEnv(template); + const KNOWN = new Set(Object.keys(templateEnv)); + + const preservedLines = Object.entries(existingEnv) + .filter(([k]) => !KNOWN.has(k)) + .map(([k, v]) => `Environment=${k}=${v}`); + if (preservedLines.length === 0) return template; + + // Guard: if template has no Environment= anchor, cannot inject — return template as-is. + // (In practice the OCP systemd template always has Environment= lines.) + if (!/^Environment=/m.test(template)) return template; + + // Inject after the last existing Environment= line in the template + return template.replace( + /(^Environment=[^\n]+\n)((?!Environment=).*$)/ms, + `$1${preservedLines.join("\n")}\n$2` + ); +} diff --git a/setup.mjs b/setup.mjs index 39b064e..68959ac 100755 --- a/setup.mjs +++ b/setup.mjs @@ -13,6 +13,7 @@ * 5. Optionally starts the proxy */ import { readFileSync, writeFileSync, existsSync, mkdirSync, unlinkSync, readdirSync, chmodSync } from "node:fs"; +import { mergePlistEnv, mergeSystemdEnv } from "./scripts/lib/plist-merge.mjs"; import { execSync } from "node:child_process"; import { join, dirname } from "node:path"; import { homedir } from "node:os"; @@ -424,9 +425,15 @@ if (!DRY_RUN) { `; - writeFileSync(plistPath, plistXml); + const existingPlist = existsSync(plistPath) ? readFileSync(plistPath, "utf8") : null; + const finalPlistXml = mergePlistEnv(existingPlist, plistXml); + writeFileSync(plistPath, finalPlistXml); chmodSync(plistPath, 0o600); - log(`Plist written: ${plistPath} (mode 600)`); + if (existingPlist && finalPlistXml !== plistXml) { + log(`Plist written: ${plistPath} (mode 600, preserved user env vars)`); + } else { + log(`Plist written: ${plistPath} (mode 600)`); + } // Bootout first (in case it was already loaded) then bootstrap try { execSync(`launchctl bootout gui/$(id -u) "${plistPath}" 2>/dev/null`); } catch { /* ignore */ } @@ -459,9 +466,15 @@ StandardError=append:${logPath} WantedBy=default.target `; - writeFileSync(servicePath, serviceUnit); + const existingService = existsSync(servicePath) ? readFileSync(servicePath, "utf8") : null; + const finalServiceUnit = mergeSystemdEnv(existingService, serviceUnit); + writeFileSync(servicePath, finalServiceUnit); chmodSync(servicePath, 0o600); - log(`Service file written: ${servicePath} (mode 600)`); + if (existingService && finalServiceUnit !== serviceUnit) { + log(`Service file written: ${servicePath} (mode 600, preserved user env vars)`); + } else { + log(`Service file written: ${servicePath} (mode 600)`); + } execSync(`systemctl --user daemon-reload`); execSync(`systemctl --user enable ocp-proxy`); diff --git a/test-features.mjs b/test-features.mjs index 63be8dc..5d9b2b6 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -454,6 +454,126 @@ async function runSingleflightTests() { await runSingleflightTests(); +// ── Plist Env Merge Tests ── +import { mergePlistEnv, mergeSystemdEnv } from "./scripts/lib/plist-merge.mjs"; + +console.log("\nPlist env merge:"); + +const SAMPLE_TEMPLATE_PLIST = ` + + + + Label + dev.ocp.proxy + EnvironmentVariables + + CLAUDE_PROXY_PORT + 3478 + CLAUDE_BIND + 127.0.0.1 + CLAUDE_AUTH_MODE + multi + + +`; + +const SAMPLE_EXISTING_PLIST = ` + + + + Label + dev.ocp.proxy + EnvironmentVariables + + CLAUDE_PROXY_PORT + 3456 + CLAUDE_BIND + 127.0.0.1 + CLAUDE_AUTH_MODE + none + CLAUDE_HEARTBEAT_INTERVAL + 2000 + CLAUDE_CACHE_TTL + 600 + + +`; + +test("mergePlistEnv preserves unknown user keys", () => { + const merged = mergePlistEnv(SAMPLE_EXISTING_PLIST, SAMPLE_TEMPLATE_PLIST); + assert.match(merged, /CLAUDE_HEARTBEAT_INTERVAL<\/key>\s*2000<\/string>/); + assert.match(merged, /CLAUDE_CACHE_TTL<\/key>\s*600<\/string>/); +}); + +test("mergePlistEnv overrides known template keys", () => { + const merged = mergePlistEnv(SAMPLE_EXISTING_PLIST, SAMPLE_TEMPLATE_PLIST); + assert.match(merged, /CLAUDE_PROXY_PORT<\/key>\s*3478<\/string>/); + assert.match(merged, /CLAUDE_AUTH_MODE<\/key>\s*multi<\/string>/); +}); + +test("mergePlistEnv first-install returns template unchanged when existing is null", () => { + const merged = mergePlistEnv(null, SAMPLE_TEMPLATE_PLIST); + assert.equal(merged, SAMPLE_TEMPLATE_PLIST); +}); + +test("mergePlistEnv first-install returns template unchanged when existing is empty", () => { + const merged = mergePlistEnv("", SAMPLE_TEMPLATE_PLIST); + assert.equal(merged, SAMPLE_TEMPLATE_PLIST); +}); + +const SAMPLE_TEMPLATE_SYSTEMD = `[Unit] +Description=OCP — Open Claude Proxy +After=network.target + +[Service] +ExecStart=/usr/bin/node /home/u/ocp/server.mjs +Environment=CLAUDE_PROXY_PORT=3478 +Environment=CLAUDE_BIND=127.0.0.1 +Environment=CLAUDE_AUTH_MODE=multi +Restart=always +`; + +const SAMPLE_EXISTING_SYSTEMD = `[Unit] +Description=OCP — Open Claude Proxy +After=network.target + +[Service] +ExecStart=/usr/bin/node /home/u/ocp/server.mjs +Environment=CLAUDE_PROXY_PORT=3456 +Environment=CLAUDE_BIND=127.0.0.1 +Environment=CLAUDE_AUTH_MODE=none +Environment=CLAUDE_HEARTBEAT_INTERVAL=2000 +Environment=CLAUDE_CACHE_TTL=600 +Restart=always +`; + +test("mergeSystemdEnv preserves unknown user Environment lines", () => { + const merged = mergeSystemdEnv(SAMPLE_EXISTING_SYSTEMD, SAMPLE_TEMPLATE_SYSTEMD); + assert.match(merged, /Environment=CLAUDE_HEARTBEAT_INTERVAL=2000/); + assert.match(merged, /Environment=CLAUDE_CACHE_TTL=600/); +}); + +test("mergeSystemdEnv overrides known template keys", () => { + const merged = mergeSystemdEnv(SAMPLE_EXISTING_SYSTEMD, SAMPLE_TEMPLATE_SYSTEMD); + assert.match(merged, /Environment=CLAUDE_PROXY_PORT=3478/); + assert.match(merged, /Environment=CLAUDE_AUTH_MODE=multi/); +}); + +test("mergeSystemdEnv first-install returns template unchanged", () => { + assert.equal(mergeSystemdEnv(null, SAMPLE_TEMPLATE_SYSTEMD), SAMPLE_TEMPLATE_SYSTEMD); + assert.equal(mergeSystemdEnv("", SAMPLE_TEMPLATE_SYSTEMD), SAMPLE_TEMPLATE_SYSTEMD); +}); + +test("mergePlistEnv is idempotent", () => { + const r1 = mergePlistEnv(SAMPLE_EXISTING_PLIST, SAMPLE_TEMPLATE_PLIST); + assert.equal(mergePlistEnv(r1, SAMPLE_TEMPLATE_PLIST), r1); +}); + +test("mergeSystemdEnv is idempotent", () => { + const r1 = mergeSystemdEnv(SAMPLE_EXISTING_SYSTEMD, SAMPLE_TEMPLATE_SYSTEMD); + assert.equal(mergeSystemdEnv(r1, SAMPLE_TEMPLATE_SYSTEMD), r1); +}); + // ── Cleanup ── closeDb();