From e7b2026f7f3f2557db95e96f0f191238578117af Mon Sep 17 00:00:00 2001 From: Divyam Talwar Date: Tue, 22 Sep 2026 04:27:45 +0530 Subject: [PATCH 1/5] Preserve malformed user configuration during CLI setup CLI installers and preference migration currently interpret malformed JSON or YAML as empty configuration, then overwrite or delete the user's file. Validate before payload mutations and refuse writes when parsing fails so recovery remains user-controlled.\n\nConstraint: Existing installs must preserve unrelated settings and malformed files must never be repaired by silent replacement\nRejected: Treat malformed config as empty and continue | destroys user data and hides the recovery action\nConfidence: high\nScope-risk: moderate\nReversibility: clean\nDirective: Keep malformed-config paths fail-safe; do not broaden parsers to auto-repair unknown user files\nTested: Focused Vitest installer and user-config suites; TypeScript noEmit\nNot-tested: Native Windows filesystem behavior --- src/cli/install-codex.ts | 63 +++++++++++++------------ src/cli/install-cursor.ts | 32 +++++++++++-- src/cli/install-hermes.ts | 19 +++++--- src/user-config.ts | 23 +++++++-- tests/claude-code/user-config.test.ts | 18 +++++++ tests/cli/cli-install-codex-fs.test.ts | 18 ++++--- tests/cli/cli-install-cursor-fs.test.ts | 17 +++++++ tests/cli/cli-install-hermes.test.ts | 11 +++-- 8 files changed, 145 insertions(+), 56 deletions(-) diff --git a/src/cli/install-codex.ts b/src/cli/install-codex.ts index 64a01ad3b..97c6857a8 100644 --- a/src/cli/install-codex.ts +++ b/src/cli/install-codex.ts @@ -170,19 +170,26 @@ export function mergeHooks( } // Filesystem-bound wrapper: reads HOOKS_PATH (if present) and feeds the -// parsed result to the pure mergeHooks. Catches malformed JSON and warns. +// parsed result to the pure mergeHooks. Malformed JSON is an install blocker: +// replacing it with an empty object would destroy user hooks. // Also surfaces a warning listing any foreign-path hivemind entries // stripped (e.g. a dev clone wired in under a different directory). -function mergeHooksJson(ours: Record): Record { - let existing: Record = {}; +function readHooksJson(): Record { + if (!existsSync(HOOKS_PATH)) return {}; + let parsed: unknown; try { - if (existsSync(HOOKS_PATH)) { - const parsed = JSON.parse(readFileSync(HOOKS_PATH, "utf-8")); - if (parsed && typeof parsed === "object") existing = parsed as Record; - } + parsed = JSON.parse(readFileSync(HOOKS_PATH, "utf-8")); } catch { - warn(` Codex ${HOOKS_PATH} unparseable — ignoring prior content`); + throw new Error(`Codex hooks config at ${HOOKS_PATH} is not valid JSON; fix or remove it, then rerun.`); } + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) { + throw new Error(`Codex hooks config at ${HOOKS_PATH} must contain a JSON object; fix or remove it, then rerun.`); + } + return parsed as Record; +} + +function mergeHooksJson(ours: Record): Record { + const existing = readHooksJson(); reportForeignHivemindHooks(existing); return mergeHooks(existing, ours); } @@ -299,6 +306,9 @@ export function installCodex(): void { throw new Error(`Codex bundle missing at ${srcBundle}. Run 'npm run build' first.`); } + // Validate before copying payloads or changing Codex feature flags. + const mergedHooks = mergeHooksJson(buildHooksJson()); + ensureDir(PLUGIN_DIR); reportPruned("Codex", syncDir(srcBundle, join(PLUGIN_DIR, "bundle"))); if (existsSync(srcSkills)) reportPruned("Codex", syncDir(srcSkills, join(PLUGIN_DIR, "skills"))); @@ -309,7 +319,7 @@ export function installCodex(): void { // Codex fingerprints and re-triggers its "Hooks need review" trust prompt on // every install/update. Skipping the no-op write keeps the user from being // re-prompted each time. - if (!writeJsonIfChanged(HOOKS_PATH, mergeHooksJson(buildHooksJson()))) { + if (!writeJsonIfChanged(HOOKS_PATH, mergedHooks)) { log(` Codex hooks.json unchanged — skipped rewrite (no re-trust prompt)`); } @@ -348,28 +358,23 @@ export function uninstallCodex(): void { // defined hooks (e.g. a custom Notification handler) that lived alongside // ours. mergeHooks(existing, { hooks: {} }) preserves the user's events // and removes only the ones whose command points into PLUGIN_DIR/bundle/. - let existing: Record = {}; try { - const raw = JSON.parse(readFileSync(HOOKS_PATH, "utf-8")); - if (raw && typeof raw === "object") existing = raw as Record; - } catch { - // Malformed JSON: fall back to deleting the file rather than guess at - // intent. Same behavior as pre-fix; user can recreate cleanly. - unlinkSync(HOOKS_PATH); - log(` Codex removed unparseable ${HOOKS_PATH}`); - existing = {}; - } - if (Object.keys(existing).length > 0) { - const stripped = mergeHooks(existing, { hooks: {} }); - const survivingHooks = (stripped.hooks ?? {}) as Record; - const otherTopLevelKeys = Object.keys(stripped).filter(k => k !== "hooks"); - if (Object.keys(survivingHooks).length === 0 && otherTopLevelKeys.length === 0) { - unlinkSync(HOOKS_PATH); - log(` Codex removed ${HOOKS_PATH}`); - } else { - writeJson(HOOKS_PATH, stripped); - log(` Codex stripped hivemind hooks from ${HOOKS_PATH}`); + const existing = readHooksJson(); + if (Object.keys(existing).length > 0) { + const stripped = mergeHooks(existing, { hooks: {} }); + const survivingHooks = (stripped.hooks ?? {}) as Record; + const otherTopLevelKeys = Object.keys(stripped).filter(k => k !== "hooks"); + if (Object.keys(survivingHooks).length === 0 && otherTopLevelKeys.length === 0) { + unlinkSync(HOOKS_PATH); + log(` Codex removed ${HOOKS_PATH}`); + } else { + writeJson(HOOKS_PATH, stripped); + log(` Codex stripped hivemind hooks from ${HOOKS_PATH}`); + } } + } catch (err) { + warn(` Codex leaving malformed hooks config untouched: ${(err as Error).message}`); + return; } } if (existsSync(SKILL_LINK)) { diff --git a/src/cli/install-cursor.ts b/src/cli/install-cursor.ts index 067198e5c..61a17b6d9 100644 --- a/src/cli/install-cursor.ts +++ b/src/cli/install-cursor.ts @@ -1,6 +1,6 @@ -import { existsSync, lstatSync, rmSync, unlinkSync } from "node:fs"; +import { existsSync, lstatSync, rmSync, unlinkSync, readFileSync } from "node:fs"; import { join } from "node:path"; -import { HOME, pkgRoot, ensureDir, syncDir, readJson, writeJson, writeJsonIfChanged, symlinkForce, writeVersionStamp, log, reportPruned } from "./util.js"; +import { HOME, pkgRoot, ensureDir, syncDir, writeJson, writeJsonIfChanged, symlinkForce, writeVersionStamp, log, reportPruned } from "./util.js"; import { getVersion } from "./version.js"; // Cursor 1.7+ hooks API: https://cursor.com/docs/agent/hooks @@ -84,6 +84,20 @@ function mergeHooks(existing: Record | null): Record; } +function readHooksConfig(): Record | null { + if (!existsSync(HOOKS_PATH)) return null; + let parsed: unknown; + try { + parsed = JSON.parse(readFileSync(HOOKS_PATH, "utf-8")); + } catch { + throw new Error(`Cursor hooks config at ${HOOKS_PATH} is not valid JSON; fix or remove it, then rerun.`); + } + if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) { + throw new Error(`Cursor hooks config at ${HOOKS_PATH} must contain a JSON object; fix or remove it, then rerun.`); + } + return parsed as Record; +} + export function stripHooksFromConfig(existing: Record | null): Record | null { if (!existing) return null; const root = existing as { hooks?: Record }; @@ -104,10 +118,13 @@ export function installCursor(): void { throw new Error(`Cursor bundle missing at ${srcBundle}. Run 'npm run build' first.`); } + // Validate before copying payloads. A malformed user hooks file cannot be + // merged safely and must not be replaced with a fresh Hivemind config. + const existing = readHooksConfig(); + ensureDir(PLUGIN_DIR); reportPruned("Cursor", syncDir(srcBundle, join(PLUGIN_DIR, "bundle"))); - const existing = readJson>(HOOKS_PATH); const merged = mergeHooks(existing); // Idempotent (same rationale as codex): skip the rewrite when unchanged so // we don't perturb the hooks.json Cursor/Codex-style trust fingerprints. @@ -125,7 +142,14 @@ export function installCursor(): void { } export function uninstallCursor(): void { - const existing = readJson>(HOOKS_PATH); + let existing: Record | null; + try { + existing = readHooksConfig(); + } catch (err) { + // Never delete or rewrite a file whose structure we cannot understand. + log(` Cursor leaving malformed hooks config untouched: ${(err as Error).message}`); + return; + } if (!existing) { log(" Cursor no hooks.json to clean"); return; diff --git a/src/cli/install-hermes.ts b/src/cli/install-hermes.ts index 722d7db4f..205f3a929 100644 --- a/src/cli/install-hermes.ts +++ b/src/cli/install-hermes.ts @@ -168,11 +168,9 @@ function readConfig(): HermesConfig { if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) { return parsed as HermesConfig; } - return {}; + throw new Error("root value is not a YAML object"); } catch { - // Malformed YAML — back off to empty so we don't clobber, but the user - // will see our entry get appended fresh. - return {}; + throw new Error(`Hermes config at ${CONFIG_PATH} is not valid YAML; fix or remove it, then rerun.`); } } @@ -200,6 +198,10 @@ function packagedSkillNames(): string[] { } export function installHermes(): void { + // Validate before touching any owned payload directories. Treating a + // malformed config as empty would destroy every user setting on write. + const cfg = readConfig(); + // 1. Skills — agent context. hivemind-memory is written inline; everything // else the installer ever put in that dir (an older version's // templates/) is pruned so only the current skill body remains. @@ -238,7 +240,6 @@ export function installHermes(): void { // Update config.yaml with mcp_servers + hooks + hooks_auto_accept. // Preserves any pre-existing user configuration. - const cfg = readConfig(); if (!cfg.mcp_servers || typeof cfg.mcp_servers !== "object") cfg.mcp_servers = {}; cfg.mcp_servers[SERVER_KEY] = { command: "node", @@ -266,7 +267,13 @@ export function uninstallHermes(): void { } if (existsSync(CONFIG_PATH)) { - const cfg = readConfig(); + let cfg: HermesConfig; + try { + cfg = readConfig(); + } catch (err) { + warn(` Hermes leaving malformed config untouched: ${(err as Error).message}`); + return; + } let touched = false; if (cfg.mcp_servers && typeof cfg.mcp_servers === "object" && SERVER_KEY in cfg.mcp_servers) { delete cfg.mcp_servers[SERVER_KEY]; diff --git a/src/user-config.ts b/src/user-config.ts index d66385d56..e08e02e9d 100644 --- a/src/user-config.ts +++ b/src/user-config.ts @@ -31,6 +31,7 @@ let _configPath: () => string = () => // cache only avoids re-parsing JSON on every call. let _cache: UserConfig | null = null; let _migrated = false; +let _configMalformed = false; export function readUserConfig(): UserConfig { if (_cache !== null) return _cache; @@ -42,17 +43,31 @@ export function readUserConfig(): UserConfig { try { const raw = readFileSync(path, "utf-8"); const parsed = JSON.parse(raw) as unknown; - _cache = isPlainObject(parsed) ? (parsed as UserConfig) : {}; + if (!isPlainObject(parsed)) { + _configMalformed = true; + _cache = {}; + } else { + _configMalformed = false; + _cache = parsed as UserConfig; + } } catch { - // Corrupt or unreadable — treat as empty, but DON'T overwrite (the user - // may want to fix it by hand). A subsequent write will overwrite. + // Corrupt or unreadable — treat as empty for read-only callers, but mark + // the file so a migration or preference write cannot overwrite data the + // user may want to repair by hand. + _configMalformed = true; _cache = {}; } return _cache; } export function writeUserConfig(patch: Partial): UserConfig { + if (_configMalformed) { + throw new Error(`Hivemind user config at ${_configPath()} is not valid JSON; fix or remove it, then rerun.`); + } const current = readUserConfig(); + if (_configMalformed) { + throw new Error(`Hivemind user config at ${_configPath()} is not valid JSON; fix or remove it, then rerun.`); + } const merged = deepMerge(current, patch); const path = _configPath(); const dir = dirname(path); @@ -147,6 +162,7 @@ export function _setConfigPathForTesting(fn: () => string): void { _configPath = fn; _cache = null; _migrated = false; + _configMalformed = false; } export function _resetUserConfigForTesting(): void { @@ -154,4 +170,5 @@ export function _resetUserConfigForTesting(): void { process.env.HIVEMIND_CONFIG_PATH ?? join(homedir(), ".deeplake", "config.json"); _cache = null; _migrated = false; + _configMalformed = false; } diff --git a/tests/claude-code/user-config.test.ts b/tests/claude-code/user-config.test.ts index 5931852e2..2bc5f4742 100644 --- a/tests/claude-code/user-config.test.ts +++ b/tests/claude-code/user-config.test.ts @@ -60,6 +60,15 @@ describe("readUserConfig", () => { expect(readUserConfig()).toEqual({}); }); + it("does not overwrite malformed JSON when embeddings migration runs", () => { + const original = "{ not json"; + writeFileSync(configPath, original, "utf-8"); + _resetUserConfigForTesting(); + _setConfigPathForTesting(() => configPath); + expect(getEmbeddingsEnabled()).toBe(false); + expect(readFileSync(configPath, "utf-8")).toBe(original); + }); + it("caches the parsed config across calls (single file read per process)", () => { writeFileSync(configPath, JSON.stringify({ embeddings: { enabled: false } }), "utf-8"); _resetUserConfigForTesting(); @@ -169,4 +178,13 @@ describe("getEmbeddingsEnabled — migration from HIVEMIND_EMBEDDINGS", () => { const written = JSON.parse(readFileSync(configPath, "utf-8")); expect(written).toEqual({ embeddings: { enabled: true } }); }); + + it("refuses explicit writes over malformed config", () => { + const original = "{ not json"; + writeFileSync(configPath, original, "utf-8"); + _resetUserConfigForTesting(); + _setConfigPathForTesting(() => configPath); + expect(() => setEmbeddingsEnabled(true)).toThrow(/not valid JSON/); + expect(readFileSync(configPath, "utf-8")).toBe(original); + }); }); diff --git a/tests/cli/cli-install-codex-fs.test.ts b/tests/cli/cli-install-codex-fs.test.ts index bed6299f4..1cf9aa50c 100644 --- a/tests/cli/cli-install-codex-fs.test.ts +++ b/tests/cli/cli-install-codex-fs.test.ts @@ -266,12 +266,13 @@ describe("installCodex — happy path", () => { expect(hooks.version).toBe(7); }); - it("re-install over a malformed hooks.json does not throw and writes a fresh one", async () => { - writeFileSync(join(tmpHome, ".codex", "hooks.json"), "{ not json"); + it("refuses a malformed hooks.json without overwriting it or touching payloads", async () => { + const hooksPath = join(tmpHome, ".codex", "hooks.json"); + writeFileSync(hooksPath, "{ not json"); const { installCodex } = await importInstaller(); - expect(() => installCodex()).not.toThrow(); - const hooks = JSON.parse(readFileSync(join(tmpHome, ".codex", "hooks.json"), "utf-8")); - expect(hooks.hooks.SessionStart).toBeDefined(); + expect(() => installCodex()).toThrow(/not valid JSON/); + expect(readFileSync(hooksPath, "utf-8")).toBe("{ not json"); + expect(existsSync(join(tmpHome, ".codex", "hivemind"))).toBe(false); }); it("warns and skips the symlink (without throwing) when the skill source is missing", async () => { @@ -493,14 +494,11 @@ describe("uninstallCodex", () => { expect(after.hooks.SessionStart ?? []).toHaveLength(0); }); - it("uninstall on a malformed hooks.json deletes the file rather than crashing", async () => { - // Lock the catch path inside uninstallCodex that handles unparseable - // JSON: we'd rather drop the file than guess at intent (the user can - // re-install cleanly). + it("uninstall on a malformed hooks.json leaves the file untouched", async () => { const { uninstallCodex } = await importInstaller(); const hooksPath = join(tmpHome, ".codex", "hooks.json"); writeFileSync(hooksPath, "{ this is not valid json"); expect(() => uninstallCodex()).not.toThrow(); - expect(existsSync(hooksPath)).toBe(false); + expect(readFileSync(hooksPath, "utf-8")).toBe("{ this is not valid json"); }); }); diff --git a/tests/cli/cli-install-cursor-fs.test.ts b/tests/cli/cli-install-cursor-fs.test.ts index fda002dae..e68131ede 100644 --- a/tests/cli/cli-install-cursor-fs.test.ts +++ b/tests/cli/cli-install-cursor-fs.test.ts @@ -118,6 +118,23 @@ describe("installCursor", () => { expect(() => installCursor()).toThrow(/Cursor bundle missing/); }); + it("refuses a malformed hooks.json without overwriting it or touching payloads", async () => { + const hooksPath = join(tmpHome, ".cursor", "hooks.json"); + writeFileSync(hooksPath, "{ not json"); + const { installCursor } = await importInstaller(); + expect(() => installCursor()).toThrow(/not valid JSON/); + expect(readFileSync(hooksPath, "utf-8")).toBe("{ not json"); + expect(existsSync(join(tmpHome, ".cursor", "hivemind"))).toBe(false); + }); + + it("uninstall on a malformed hooks.json leaves the file untouched", async () => { + const hooksPath = join(tmpHome, ".cursor", "hooks.json"); + writeFileSync(hooksPath, "{ not json"); + const { uninstallCursor } = await importInstaller(); + uninstallCursor(); + expect(readFileSync(hooksPath, "utf-8")).toBe("{ not json"); + }); + it("creates embed-deps symlink when ~/.hivemind/embed-deps/node_modules exists", async () => { const embedDepsNm = join(tmpHome, ".hivemind", "embed-deps", "node_modules"); mkdirSync(embedDepsNm, { recursive: true }); diff --git a/tests/cli/cli-install-hermes.test.ts b/tests/cli/cli-install-hermes.test.ts index d2ce93ba7..09ee31b47 100644 --- a/tests/cli/cli-install-hermes.test.ts +++ b/tests/cli/cli-install-hermes.test.ts @@ -119,12 +119,15 @@ describe("installHermes — cold install", () => { expect(cfg.preserved_field).toBe("stay"); }); - it("re-install over a malformed config.yaml does not throw and lands a fresh entry", async () => { + it("refuses a malformed config.yaml without overwriting it or touching payloads", async () => { mkdirSync(join(tmpHome, ".hermes"), { recursive: true }); - writeFileSync(join(tmpHome, ".hermes", "config.yaml"), "::: not yaml :::"); + const configPath = join(tmpHome, ".hermes", "config.yaml"); + writeFileSync(configPath, "::: not yaml :::"); const { installHermes } = await importInstaller(); - expect(() => installHermes()).not.toThrow(); - expect(readConfig().mcp_servers.hivemind).toBeDefined(); + expect(() => installHermes()).toThrow(/not valid YAML/); + expect(readFileSync(configPath, "utf-8")).toBe("::: not yaml :::"); + expect(existsSync(join(tmpHome, ".hermes", "hivemind"))).toBe(false); + expect(existsSync(join(tmpHome, ".hivemind", "mcp"))).toBe(false); }); it("re-install replaces stale hivemind hooks (no duplication after N re-runs)", async () => { From d72535a2cea4df99626cc479d29aa9e111943f37 Mon Sep 17 00:00:00 2001 From: Divyam Talwar Date: Tue, 22 Sep 2026 05:25:31 +0530 Subject: [PATCH 2/5] Prevent config repair from becoming destructive setup Malformed and unreadable user configuration must remain authoritative until the user repairs it. Validate installer configuration before payload mutations, preserve passive hook fallbacks, and surface explicit uninstall failures without rewriting user bytes. Constraint: Capture and startup readers must remain non-throwing even when the shared user config is malformed or unreadable Rejected: Preflight with existsSync | lookup errors can be misclassified as missing and trigger destructive migration Confidence: high Scope-risk: moderate Directive: Keep config validation ahead of every installer payload mutation and treat only ENOENT as missing Tested: 80 focused tests, 216 adjacent tests, TypeScript typecheck, build, jscpd Not-tested: Full repository test suite, real EACCES mount behavior, live user installations --- src/cli/install-codex.ts | 50 +++++------ src/cli/install-cursor.ts | 22 ++--- src/cli/install-hermes.ts | 57 +++++++----- src/user-config.ts | 87 ++++++++++++------- .../user-config-io-failure.test.ts | 62 +++++++++++++ tests/claude-code/user-config.test.ts | 12 ++- tests/cli/cli-install-codex-fs.test.ts | 38 +++++++- tests/cli/cli-install-cursor-fs.test.ts | 4 +- tests/cli/cli-install-hermes.test.ts | 80 ++++++++++++++++- 9 files changed, 318 insertions(+), 94 deletions(-) create mode 100644 tests/claude-code/user-config-io-failure.test.ts diff --git a/src/cli/install-codex.ts b/src/cli/install-codex.ts index 97c6857a8..dd6df2487 100644 --- a/src/cli/install-codex.ts +++ b/src/cli/install-codex.ts @@ -175,10 +175,17 @@ export function mergeHooks( // Also surfaces a warning listing any foreign-path hivemind entries // stripped (e.g. a dev clone wired in under a different directory). function readHooksJson(): Record { - if (!existsSync(HOOKS_PATH)) return {}; + let raw: string; + try { + raw = readFileSync(HOOKS_PATH, "utf-8"); + } catch (err) { + if (typeof err === "object" && err !== null && "code" in err && err.code === "ENOENT") return {}; + const detail = err instanceof Error && err.message ? `: ${err.message}` : ""; + throw new Error(`Codex hooks config at ${HOOKS_PATH} could not be read${detail}`); + } let parsed: unknown; try { - parsed = JSON.parse(readFileSync(HOOKS_PATH, "utf-8")); + parsed = JSON.parse(raw); } catch { throw new Error(`Codex hooks config at ${HOOKS_PATH} is not valid JSON; fix or remove it, then rerun.`); } @@ -352,29 +359,22 @@ export function installCodex(): void { } export function uninstallCodex(): void { - if (existsSync(HOOKS_PATH)) { - // Symmetric with install: strip ONLY our hivemind entries via mergeHooks. - // The pre-fix unconditional unlinkSync(HOOKS_PATH) destroyed any user- - // defined hooks (e.g. a custom Notification handler) that lived alongside - // ours. mergeHooks(existing, { hooks: {} }) preserves the user's events - // and removes only the ones whose command points into PLUGIN_DIR/bundle/. - try { - const existing = readHooksJson(); - if (Object.keys(existing).length > 0) { - const stripped = mergeHooks(existing, { hooks: {} }); - const survivingHooks = (stripped.hooks ?? {}) as Record; - const otherTopLevelKeys = Object.keys(stripped).filter(k => k !== "hooks"); - if (Object.keys(survivingHooks).length === 0 && otherTopLevelKeys.length === 0) { - unlinkSync(HOOKS_PATH); - log(` Codex removed ${HOOKS_PATH}`); - } else { - writeJson(HOOKS_PATH, stripped); - log(` Codex stripped hivemind hooks from ${HOOKS_PATH}`); - } - } - } catch (err) { - warn(` Codex leaving malformed hooks config untouched: ${(err as Error).message}`); - return; + // Symmetric with install: strip ONLY our hivemind entries via mergeHooks. + // The pre-fix unconditional unlinkSync(HOOKS_PATH) destroyed any user- + // defined hooks (e.g. a custom Notification handler) that lived alongside + // ours. Validate first so malformed/unreadable config leaves every other + // uninstall target untouched and propagates a failure to the dispatcher. + const existing = readHooksJson(); + if (Object.keys(existing).length > 0) { + const stripped = mergeHooks(existing, { hooks: {} }); + const survivingHooks = (stripped.hooks ?? {}) as Record; + const otherTopLevelKeys = Object.keys(stripped).filter(k => k !== "hooks"); + if (Object.keys(survivingHooks).length === 0 && otherTopLevelKeys.length === 0) { + unlinkSync(HOOKS_PATH); + log(` Codex removed ${HOOKS_PATH}`); + } else { + writeJson(HOOKS_PATH, stripped); + log(` Codex stripped hivemind hooks from ${HOOKS_PATH}`); } } if (existsSync(SKILL_LINK)) { diff --git a/src/cli/install-cursor.ts b/src/cli/install-cursor.ts index 61a17b6d9..5eade3456 100644 --- a/src/cli/install-cursor.ts +++ b/src/cli/install-cursor.ts @@ -85,10 +85,17 @@ function mergeHooks(existing: Record | null): Record | null { - if (!existsSync(HOOKS_PATH)) return null; + let raw: string; + try { + raw = readFileSync(HOOKS_PATH, "utf-8"); + } catch (err) { + if (typeof err === "object" && err !== null && "code" in err && err.code === "ENOENT") return null; + const detail = err instanceof Error && err.message ? `: ${err.message}` : ""; + throw new Error(`Cursor hooks config at ${HOOKS_PATH} could not be read${detail}`); + } let parsed: unknown; try { - parsed = JSON.parse(readFileSync(HOOKS_PATH, "utf-8")); + parsed = JSON.parse(raw); } catch { throw new Error(`Cursor hooks config at ${HOOKS_PATH} is not valid JSON; fix or remove it, then rerun.`); } @@ -142,14 +149,9 @@ export function installCursor(): void { } export function uninstallCursor(): void { - let existing: Record | null; - try { - existing = readHooksConfig(); - } catch (err) { - // Never delete or rewrite a file whose structure we cannot understand. - log(` Cursor leaving malformed hooks config untouched: ${(err as Error).message}`); - return; - } + // Validation errors propagate to the dispatcher, which reports FAILED. + // Nothing is removed or rewritten before this read succeeds. + const existing = readHooksConfig(); if (!existing) { log(" Cursor no hooks.json to clean"); return; diff --git a/src/cli/install-hermes.ts b/src/cli/install-hermes.ts index 205f3a929..20b20f01d 100644 --- a/src/cli/install-hermes.ts +++ b/src/cli/install-hermes.ts @@ -160,18 +160,34 @@ function stripHivemindHooks(existing: Record | undefi return Object.keys(out).length > 0 ? out : undefined; } -function readConfig(): HermesConfig { - if (!existsSync(CONFIG_PATH)) return {}; +function readConfig(): HermesConfig | null { + let raw: string; try { - const raw = readFileSync(CONFIG_PATH, "utf-8"); - const parsed = yaml.load(raw); - if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) { - return parsed as HermesConfig; - } - throw new Error("root value is not a YAML object"); + raw = readFileSync(CONFIG_PATH, "utf-8"); + } catch (err) { + if (typeof err === "object" && err !== null && "code" in err && err.code === "ENOENT") return null; + const detail = err instanceof Error && err.message ? `: ${err.message}` : ""; + throw new Error(`Hermes config at ${CONFIG_PATH} could not be read${detail}`); + } + + let parsed: unknown; + try { + parsed = yaml.load(raw); } catch { throw new Error(`Hermes config at ${CONFIG_PATH} is not valid YAML; fix or remove it, then rerun.`); } + if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) { + return parsed as HermesConfig; + } + if ((parsed === undefined || parsed === null) && isEmptyYamlDocument(raw)) return {}; + throw new Error(`Hermes config at ${CONFIG_PATH} must contain a YAML mapping; fix or remove it, then rerun.`); +} + +function isEmptyYamlDocument(raw: string): boolean { + return raw.split(/\r?\n/).every(line => { + const trimmed = line.trim(); + return trimmed === "" || trimmed.startsWith("#"); + }); } function writeConfig(cfg: HermesConfig): void { @@ -198,9 +214,14 @@ function packagedSkillNames(): string[] { } export function installHermes(): void { + const srcBundle = join(pkgRoot(), "harnesses", "hermes", "bundle"); + if (!existsSync(srcBundle)) { + throw new Error(`Hermes bundle missing at ${srcBundle}. Run 'npm run build' first.`); + } + // Validate before touching any owned payload directories. Treating a // malformed config as empty would destroy every user setting on write. - const cfg = readConfig(); + const cfg = readConfig() ?? {}; // 1. Skills — agent context. hivemind-memory is written inline; everything // else the installer ever put in that dir (an older version's @@ -220,10 +241,6 @@ export function installHermes(): void { } // 2. Hook bundle — auto-capture via Hermes shell-hooks. - const srcBundle = join(pkgRoot(), "harnesses", "hermes", "bundle"); - if (!existsSync(srcBundle)) { - throw new Error(`Hermes bundle missing at ${srcBundle}. Run 'npm run build' first.`); - } ensureDir(HIVEMIND_DIR); reportPruned("Hermes", syncDir(srcBundle, BUNDLE_DIR)); const pluginNm = join(HIVEMIND_DIR, "node_modules"); @@ -254,6 +271,11 @@ export function installHermes(): void { } export function uninstallHermes(): void { + // Validate before removing any owned payload. If the config cannot be + // understood, uninstall must fail as one untouched operation rather than + // leave hooks pointing at a bundle that has already been deleted. + const cfg = readConfig(); + for (const dir of [SKILLS_DIR, ...packagedSkillNames().map(n => join(SKILLS_ROOT, n))]) { // A symlink at one of our names is the user's, never something we wrote. if (!existsSync(dir) || isLink(dir)) continue; @@ -266,14 +288,7 @@ export function uninstallHermes(): void { log(` Hermes removed ${HIVEMIND_DIR}`); } - if (existsSync(CONFIG_PATH)) { - let cfg: HermesConfig; - try { - cfg = readConfig(); - } catch (err) { - warn(` Hermes leaving malformed config untouched: ${(err as Error).message}`); - return; - } + if (cfg) { let touched = false; if (cfg.mcp_servers && typeof cfg.mcp_servers === "object" && SERVER_KEY in cfg.mcp_servers) { delete cfg.mcp_servers[SERVER_KEY]; diff --git a/src/user-config.ts b/src/user-config.ts index e08e02e9d..856c91eb5 100644 --- a/src/user-config.ts +++ b/src/user-config.ts @@ -26,48 +26,61 @@ export interface UserConfig { let _configPath: () => string = () => process.env.HIVEMIND_CONFIG_PATH ?? join(homedir(), ".deeplake", "config.json"); +type ConfigState = "unloaded" | "missing" | "valid" | "invalid" | "unreadable"; + // In-memory cache so the migration's env-var read and resulting write happen // at most once per process. The file on disk is the source of truth; the // cache only avoids re-parsing JSON on every call. let _cache: UserConfig | null = null; -let _migrated = false; -let _configMalformed = false; +let _configState: ConfigState = "unloaded"; +let _configReadError: Error | null = null; +let _migrationFallback: boolean | null = null; export function readUserConfig(): UserConfig { if (_cache !== null) return _cache; const path = _configPath(); - if (!existsSync(path)) { + let raw: string; + try { + raw = readFileSync(path, "utf-8"); + } catch (err) { + if (isMissingFileError(err)) { + _configState = "missing"; + _cache = {}; + return _cache; + } + // Passive consumers include capture/startup hooks, so reads remain + // non-throwing. Keep I/O failure distinct from malformed JSON so no + // migration or explicit setter can mistake it for a missing config. + _configState = "unreadable"; + _configReadError = err instanceof Error ? err : new Error(String(err)); _cache = {}; return _cache; } try { - const raw = readFileSync(path, "utf-8"); const parsed = JSON.parse(raw) as unknown; if (!isPlainObject(parsed)) { - _configMalformed = true; + _configState = "invalid"; _cache = {}; } else { - _configMalformed = false; + _configState = "valid"; _cache = parsed as UserConfig; } } catch { - // Corrupt or unreadable — treat as empty for read-only callers, but mark - // the file so a migration or preference write cannot overwrite data the - // user may want to repair by hand. - _configMalformed = true; + // Corrupt JSON remains a non-throwing empty view for passive callers, but + // mutation paths refuse to overwrite the bytes the user may want to fix. + _configState = "invalid"; _cache = {}; } return _cache; } +function isMissingFileError(err: unknown): boolean { + return typeof err === "object" && err !== null && "code" in err && err.code === "ENOENT"; +} + export function writeUserConfig(patch: Partial): UserConfig { - if (_configMalformed) { - throw new Error(`Hivemind user config at ${_configPath()} is not valid JSON; fix or remove it, then rerun.`); - } const current = readUserConfig(); - if (_configMalformed) { - throw new Error(`Hivemind user config at ${_configPath()} is not valid JSON; fix or remove it, then rerun.`); - } + assertConfigWritable(); const merged = deepMerge(current, patch); const path = _configPath(); const dir = dirname(path); @@ -94,24 +107,36 @@ export function getEmbeddingsEnabled(): boolean { if (cfg.embeddings && typeof cfg.embeddings.enabled === "boolean") { return cfg.embeddings.enabled; } - if (_migrated) { - // Migration ran this process but couldn't persist (read-only fs etc.). - // Fall back to the env var directly to avoid spinning the migration on - // every call. Cached for the lifetime of the process. - return migrationValueFromEnv(); - } - _migrated = true; + if (_migrationFallback !== null) return _migrationFallback; + const enabled = migrationValueFromEnv(); + if (_configState === "invalid" || _configState === "unreadable") { + // Hooks must keep running, but an invalid or unreadable file is not an + // empty file: do not migrate over it or infer a merge base from it. + _migrationFallback = enabled; + return enabled; + } try { writeUserConfig({ embeddings: { enabled } }); } catch { - // Persist failed (perms, full disk, etc.) — keep the in-memory cache so - // the rest of the session sees a stable value. - _cache = { ...(cfg ?? {}), embeddings: { ...(cfg?.embeddings ?? {}), enabled } }; + // Persist failed (permissions, full disk, etc.). Preserve the passive + // getter contract and cache only the fallback value, not a guessed config. + _migrationFallback = enabled; } return enabled; } +function assertConfigWritable(): void { + const path = _configPath(); + if (_configState === "invalid") { + throw new Error(`Hivemind user config at ${path} is not valid JSON; fix or remove it, then rerun.`); + } + if (_configState === "unreadable") { + const detail = _configReadError?.message ? `: ${_configReadError.message}` : ""; + throw new Error(`Hivemind user config at ${path} could not be read${detail}`); + } +} + function migrationValueFromEnv(): boolean { const raw = process.env.HIVEMIND_EMBEDDINGS; if (raw === undefined) return false; @@ -161,14 +186,16 @@ function deepMerge(base: UserConfig, patch: Partial): UserConfig { export function _setConfigPathForTesting(fn: () => string): void { _configPath = fn; _cache = null; - _migrated = false; - _configMalformed = false; + _configState = "unloaded"; + _configReadError = null; + _migrationFallback = null; } export function _resetUserConfigForTesting(): void { _configPath = () => process.env.HIVEMIND_CONFIG_PATH ?? join(homedir(), ".deeplake", "config.json"); _cache = null; - _migrated = false; - _configMalformed = false; + _configState = "unloaded"; + _configReadError = null; + _migrationFallback = null; } diff --git a/tests/claude-code/user-config-io-failure.test.ts b/tests/claude-code/user-config-io-failure.test.ts new file mode 100644 index 000000000..996042543 --- /dev/null +++ b/tests/claude-code/user-config-io-failure.test.ts @@ -0,0 +1,62 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const fsFault = vi.hoisted(() => ({ + path: "", + error: null as NodeJS.ErrnoException | null, + writeFileSync: vi.fn(), + renameSync: vi.fn(), +})); + +vi.mock("node:fs", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + readFileSync: (path: unknown, ...args: unknown[]) => { + if (path === fsFault.path && fsFault.error) throw fsFault.error; + return (actual.readFileSync as (...inner: unknown[]) => unknown)(path, ...args); + }, + writeFileSync: (...args: unknown[]) => { + fsFault.writeFileSync(...args); + return (actual.writeFileSync as (...inner: unknown[]) => unknown)(...args); + }, + renameSync: (...args: unknown[]) => { + fsFault.renameSync(...args); + return (actual.renameSync as (...inner: unknown[]) => unknown)(...args); + }, + }; +}); + +import { + _resetUserConfigForTesting, + _setConfigPathForTesting, + getEmbeddingsEnabled, + readUserConfig, + setEmbeddingsEnabled, +} from "../../src/user-config.js"; + +const originalEnv = process.env.HIVEMIND_EMBEDDINGS; + +beforeEach(() => { + fsFault.path = "/tmp/hivemind-injected-io-error/config.json"; + fsFault.error = Object.assign(new Error("injected config read failure"), { code: "EIO" }); + fsFault.writeFileSync.mockClear(); + fsFault.renameSync.mockClear(); + _setConfigPathForTesting(() => fsFault.path); + process.env.HIVEMIND_EMBEDDINGS = "true"; +}); + +afterEach(() => { + _resetUserConfigForTesting(); + if (originalEnv === undefined) delete process.env.HIVEMIND_EMBEDDINGS; + else process.env.HIVEMIND_EMBEDDINGS = originalEnv; +}); + +describe("user config I/O failures", () => { + it("keeps passive getters safe but never migrates or explicitly writes over an unreadable config", () => { + expect(readUserConfig()).toEqual({}); + expect(getEmbeddingsEnabled()).toBe(true); + expect(() => setEmbeddingsEnabled(false)).toThrow(/could not be read.*injected config read failure/); + expect(fsFault.writeFileSync).not.toHaveBeenCalled(); + expect(fsFault.renameSync).not.toHaveBeenCalled(); + }); +}); diff --git a/tests/claude-code/user-config.test.ts b/tests/claude-code/user-config.test.ts index 2bc5f4742..e49b19574 100644 --- a/tests/claude-code/user-config.test.ts +++ b/tests/claude-code/user-config.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, beforeEach, afterEach } from "vitest"; -import { mkdtempSync, rmSync, existsSync, readFileSync, writeFileSync } from "node:fs"; +import { mkdirSync, mkdtempSync, rmSync, existsSync, readFileSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { @@ -69,6 +69,16 @@ describe("readUserConfig", () => { expect(readFileSync(configPath, "utf-8")).toBe(original); }); + it("keeps passive reads non-throwing when the config cannot be read, without attempting migration", () => { + mkdirSync(configPath); + process.env.HIVEMIND_EMBEDDINGS = "true"; + + expect(readUserConfig()).toEqual({}); + expect(getEmbeddingsEnabled()).toBe(true); + expect(() => setEmbeddingsEnabled(false)).toThrow(/could not be read/); + expect(existsSync(configPath)).toBe(true); + }); + it("caches the parsed config across calls (single file read per process)", () => { writeFileSync(configPath, JSON.stringify({ embeddings: { enabled: false } }), "utf-8"); _resetUserConfigForTesting(); diff --git a/tests/cli/cli-install-codex-fs.test.ts b/tests/cli/cli-install-codex-fs.test.ts index 1cf9aa50c..041c617db 100644 --- a/tests/cli/cli-install-codex-fs.test.ts +++ b/tests/cli/cli-install-codex-fs.test.ts @@ -59,12 +59,28 @@ beforeEach(() => { afterEach(() => { rmSync(tmpRoot, { recursive: true, force: true }); clearFakeHome(); + vi.doUnmock("node:fs"); vi.restoreAllMocks(); vi.resetModules(); }); -async function importInstaller(): Promise { +async function importInstaller(configReadError?: NodeJS.ErrnoException): Promise { vi.resetModules(); + if (configReadError) { + const configPath = join(tmpHome, ".codex", "hooks.json"); + vi.doMock("node:fs", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + readFileSync: (path: unknown, ...args: unknown[]) => { + if (path === configPath) throw configReadError; + return (actual.readFileSync as (...inner: unknown[]) => unknown)(path, ...args); + }, + }; + }); + } else { + vi.doUnmock("node:fs"); + } vi.doMock("../../src/cli/util.js", async (importOriginal) => { const actual = await importOriginal(); return { ...actual, pkgRoot: () => tmpPkg }; @@ -494,11 +510,27 @@ describe("uninstallCodex", () => { expect(after.hooks.SessionStart ?? []).toHaveLength(0); }); - it("uninstall on a malformed hooks.json leaves the file untouched", async () => { + it("uninstall on a malformed hooks.json reports failure and leaves the file untouched", async () => { const { uninstallCodex } = await importInstaller(); const hooksPath = join(tmpHome, ".codex", "hooks.json"); writeFileSync(hooksPath, "{ this is not valid json"); - expect(() => uninstallCodex()).not.toThrow(); + expect(() => uninstallCodex()).toThrow(/not valid JSON/); expect(readFileSync(hooksPath, "utf-8")).toBe("{ this is not valid json"); }); + + it("validates hooks.json read errors before removing other Codex integration files", async () => { + const { installCodex } = await importInstaller(); + installCodex(); + const hooksPath = join(tmpHome, ".codex", "hooks.json"); + const agentsPath = join(tmpHome, ".codex", "AGENTS.md"); + const originalHooks = readFileSync(hooksPath, "utf-8"); + const originalAgents = readFileSync(agentsPath, "utf-8"); + const readError = Object.assign(new Error("injected hooks read failure"), { code: "EACCES" }); + const { uninstallCodex } = await importInstaller(readError); + + expect(() => uninstallCodex()).toThrow(/could not be read/); + expect(readFileSync(hooksPath, "utf-8")).toBe(originalHooks); + expect(readFileSync(agentsPath, "utf-8")).toBe(originalAgents); + expect(existsSync(join(tmpHome, ".agents", "skills", "hivemind-memory"))).toBe(true); + }); }); diff --git a/tests/cli/cli-install-cursor-fs.test.ts b/tests/cli/cli-install-cursor-fs.test.ts index e68131ede..8d90942ac 100644 --- a/tests/cli/cli-install-cursor-fs.test.ts +++ b/tests/cli/cli-install-cursor-fs.test.ts @@ -127,11 +127,11 @@ describe("installCursor", () => { expect(existsSync(join(tmpHome, ".cursor", "hivemind"))).toBe(false); }); - it("uninstall on a malformed hooks.json leaves the file untouched", async () => { + it("uninstall on a malformed hooks.json reports failure and leaves the file untouched", async () => { const hooksPath = join(tmpHome, ".cursor", "hooks.json"); writeFileSync(hooksPath, "{ not json"); const { uninstallCursor } = await importInstaller(); - uninstallCursor(); + expect(() => uninstallCursor()).toThrow(/not valid JSON/); expect(readFileSync(hooksPath, "utf-8")).toBe("{ not json"); }); diff --git a/tests/cli/cli-install-hermes.test.ts b/tests/cli/cli-install-hermes.test.ts index 09ee31b47..adbedbcac 100644 --- a/tests/cli/cli-install-hermes.test.ts +++ b/tests/cli/cli-install-hermes.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { mkdirSync, rmSync, writeFileSync, readFileSync, existsSync } from "node:fs"; +import { chmodSync, mkdirSync, rmSync, writeFileSync, readFileSync, existsSync, statSync } from "node:fs"; import { join } from "node:path"; import { tmpdir } from "node:os"; import * as yaml from "js-yaml"; @@ -44,12 +44,28 @@ beforeEach(() => { afterEach(() => { rmSync(tmpRoot, { recursive: true, force: true }); clearFakeHome(); + vi.doUnmock("node:fs"); vi.restoreAllMocks(); vi.resetModules(); }); -async function importInstaller(): Promise { +async function importInstaller(configReadError?: NodeJS.ErrnoException): Promise { vi.resetModules(); + if (configReadError) { + const configPath = join(tmpHome, ".hermes", "config.yaml"); + vi.doMock("node:fs", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + readFileSync: (path: unknown, ...args: unknown[]) => { + if (path === configPath) throw configReadError; + return (actual.readFileSync as (...inner: unknown[]) => unknown)(path, ...args); + }, + }; + }); + } else { + vi.doUnmock("node:fs"); + } vi.doMock("../../src/cli/util.js", async (importOriginal) => { const actual = await importOriginal(); return { ...actual, pkgRoot: () => tmpPkg }; @@ -119,13 +135,43 @@ describe("installHermes — cold install", () => { expect(cfg.preserved_field).toBe("stay"); }); + it.each(["", " \n\t", "# user comment only\n # another comment\n"])( + "accepts an empty YAML document without treating it as malformed (%j)", + async (raw) => { + mkdirSync(join(tmpHome, ".hermes"), { recursive: true }); + writeFileSync(join(tmpHome, ".hermes", "config.yaml"), raw); + + const { installHermes } = await importInstaller(); + expect(() => installHermes()).not.toThrow(); + expect(readConfig().mcp_servers.hivemind.command).toBe("node"); + }, + ); + + it.each(["plain scalar\n", "- array item\n"])( + "rejects a non-mapping YAML root without touching payloads (%j)", + async (raw) => { + mkdirSync(join(tmpHome, ".hermes"), { recursive: true }); + const configPath = join(tmpHome, ".hermes", "config.yaml"); + writeFileSync(configPath, raw); + + const { installHermes } = await importInstaller(); + expect(() => installHermes()).toThrow(/must contain a YAML mapping/); + expect(readFileSync(configPath, "utf-8")).toBe(raw); + expect(existsSync(join(tmpHome, ".hermes", "hivemind"))).toBe(false); + expect(existsSync(join(tmpHome, ".hivemind", "mcp"))).toBe(false); + }, + ); + it("refuses a malformed config.yaml without overwriting it or touching payloads", async () => { mkdirSync(join(tmpHome, ".hermes"), { recursive: true }); const configPath = join(tmpHome, ".hermes", "config.yaml"); writeFileSync(configPath, "::: not yaml :::"); + chmodSync(configPath, 0o640); + const originalMode = statSync(configPath).mode & 0o777; const { installHermes } = await importInstaller(); expect(() => installHermes()).toThrow(/not valid YAML/); expect(readFileSync(configPath, "utf-8")).toBe("::: not yaml :::"); + expect(statSync(configPath).mode & 0o777).toBe(originalMode); expect(existsSync(join(tmpHome, ".hermes", "hivemind"))).toBe(false); expect(existsSync(join(tmpHome, ".hivemind", "mcp"))).toBe(false); }); @@ -196,6 +242,36 @@ describe("uninstallHermes", () => { expect(cfg.mcp_servers.hivemind).toBeUndefined(); }); + it("validates malformed config before removing any payload and propagates failure", async () => { + const { installHermes, uninstallHermes } = await importInstaller(); + installHermes(); + const configPath = join(tmpHome, ".hermes", "config.yaml"); + const original = "hooks: [unterminated"; + writeFileSync(configPath, original); + chmodSync(configPath, 0o640); + const originalMode = statSync(configPath).mode & 0o777; + + expect(() => uninstallHermes()).toThrow(/not valid YAML/); + expect(readFileSync(configPath, "utf-8")).toBe(original); + expect(statSync(configPath).mode & 0o777).toBe(originalMode); + expect(existsSync(join(tmpHome, ".hermes", "skills", "hivemind-memory", "SKILL.md"))).toBe(true); + expect(existsSync(join(tmpHome, ".hermes", "hivemind", "bundle", "capture.js"))).toBe(true); + }); + + it("does not remove payload when config reading fails at the filesystem boundary", async () => { + const { installHermes } = await importInstaller(); + installHermes(); + const configPath = join(tmpHome, ".hermes", "config.yaml"); + const original = readFileSync(configPath, "utf-8"); + const readError = Object.assign(new Error("injected config read failure"), { code: "EIO" }); + const { uninstallHermes } = await importInstaller(readError); + + expect(() => uninstallHermes()).toThrow(/could not be read/); + expect(readFileSync(configPath, "utf-8")).toBe(original); + expect(existsSync(join(tmpHome, ".hermes", "skills", "hivemind-memory", "SKILL.md"))).toBe(true); + expect(existsSync(join(tmpHome, ".hermes", "hivemind", "bundle", "capture.js"))).toBe(true); + }); + it("is a no-op (no throw) when nothing has been installed", async () => { const { uninstallHermes } = await importInstaller(); expect(() => uninstallHermes()).not.toThrow(); From 3725c43fa56664b63448f7d42961685de723f981 Mon Sep 17 00:00:00 2001 From: Divyam Talwar Date: Tue, 22 Sep 2026 06:27:41 +0530 Subject: [PATCH 3/5] Validate heavy embedding operations before shared-state mutation Heavy embeddings install and uninstall now perform a read-only user-config preflight before dependency, link, prune, or daemon work. Hermes also recognizes parser-confirmed empty YAML documents only for explicit markers and supported directives, while retaining scalar, collection, and explicit-null rejection. Focused hermetic tests cover malformed and injected-read failures plus exact YAML regressions. Constraint: Preserve passive user-config getter behavior, missing-file semantics, setter contracts, and the existing cache limitation\nRejected: Make writeUserConfig transactional across provisioning | broader scope than the review correction\nConfidence: high\nScope-risk: narrow\nReversibility: clean\nDirective: Do not infer broad TOCTOU guarantees from the read-only preflight\nTested: Node 22 focused/adjacent Vitest 156 tests; OpenClaw bundle wiring 5 tests; typecheck; build; duplication scan; critical-only bundle audit\nNot-tested: Full suite and coordinator-owned Linux gate --- src/cli/embeddings.ts | 4 +- src/cli/install-hermes.ts | 30 ++++- src/user-config.ts | 10 ++ .../cli-embeddings-config-preflight.test.ts | 121 ++++++++++++++++++ tests/cli/cli-install-hermes.test.ts | 13 +- 5 files changed, 172 insertions(+), 6 deletions(-) create mode 100644 tests/cli/cli-embeddings-config-preflight.test.ts diff --git a/src/cli/embeddings.ts b/src/cli/embeddings.ts index 6dcfc16be..5d28203a9 100644 --- a/src/cli/embeddings.ts +++ b/src/cli/embeddings.ts @@ -4,7 +4,7 @@ import { userInfo } from "node:os"; import { join } from "node:path"; import { HOME, ensureDir, log, pkgRoot, symlinkForce, warn, writeJson } from "./util.js"; import { pidPathFor, socketPathFor } from "../embeddings/protocol.js"; -import { getEmbeddingsEnabled, setEmbeddingsEnabled } from "../user-config.js"; +import { assertUserConfigWritable, getEmbeddingsEnabled, setEmbeddingsEnabled } from "../user-config.js"; import { ensureGraphDeps } from "./graph-deps.js"; /** @@ -176,6 +176,7 @@ function linkAgent(install: AgentInstall): void { * prior `disable`). */ export function installEmbeddings(): void { + assertUserConfigWritable(); ensureSharedDeps(); // Provision the code-graph parsers into the same shared dir so the // graph-on-stop hook (which symlinks here) can auto-build the graph. @@ -216,6 +217,7 @@ export function enableEmbeddings(): void { * effect immediately. Counterpart to `install`. */ export function uninstallEmbeddings(opts?: { prune?: boolean }): void { + assertUserConfigWritable(); const installs = findHivemindInstalls(); for (const inst of installs) { const link = join(inst.pluginDir, "node_modules"); diff --git a/src/cli/install-hermes.ts b/src/cli/install-hermes.ts index 20b20f01d..bc1d8b6a7 100644 --- a/src/cli/install-hermes.ts +++ b/src/cli/install-hermes.ts @@ -184,10 +184,34 @@ function readConfig(): HermesConfig | null { } function isEmptyYamlDocument(raw: string): boolean { - return raw.split(/\r?\n/).every(line => { + const lines = raw.replace(/^\uFEFF/, "").split(/\r?\n/); + let sawDirective = false; + let sawDocumentMarker = false; + for (const line of lines) { const trimmed = line.trim(); - return trimmed === "" || trimmed.startsWith("#"); - }); + if (trimmed === "" || trimmed.startsWith("#")) continue; + if (isSupportedYamlDirective(trimmed)) { + if (sawDocumentMarker) return false; + sawDirective = true; + continue; + } + if (/^(?:---|\.\.\.)(?:\s+#.*)?$/.test(trimmed)) { + if (sawDocumentMarker) return false; + sawDocumentMarker = true; + continue; + } + return false; + } + // A directive must introduce a document, while a blank/comment-only stream + // is already a valid empty document. The successful yaml.load above remains + // the parser authority; this lexical check only distinguishes empty docs + // from explicit nulls and other scalar/collection roots. + return !sawDirective || sawDocumentMarker; +} + +function isSupportedYamlDirective(line: string): boolean { + return /^%YAML\s+1\.(?:1|2)(?:\s+#.*)?$/.test(line) + || /^%TAG\s+\S+\s+\S+(?:\s+#.*)?$/.test(line); } function writeConfig(cfg: HermesConfig): void { diff --git a/src/user-config.ts b/src/user-config.ts index 856c91eb5..76a0fa6c5 100644 --- a/src/user-config.ts +++ b/src/user-config.ts @@ -92,6 +92,16 @@ export function writeUserConfig(patch: Partial): UserConfig { return merged; } +/** + * Validate the current config for a mutation without writing it. Heavy CLI + * operations use this before provisioning or deleting shared resources so a + * malformed or unreadable file cannot fail only after those mutations. + */ +export function assertUserConfigWritable(): void { + readUserConfig(); + assertConfigWritable(); +} + // Reads the embeddings-enabled flag, performing the one-shot env-var // migration if no value has ever been persisted. Returns the final boolean. // diff --git a/tests/cli/cli-embeddings-config-preflight.test.ts b/tests/cli/cli-embeddings-config-preflight.test.ts new file mode 100644 index 000000000..e5a12431b --- /dev/null +++ b/tests/cli/cli-embeddings-config-preflight.test.ts @@ -0,0 +1,121 @@ +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import { execFileSync } from "node:child_process"; +import { existsSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, readlinkSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { setFakeHome, clearFakeHome } from "../shared/fake-home.js"; + +const fsFault = vi.hoisted(() => ({ + path: "", + error: null as NodeJS.ErrnoException | null, +})); +const graphDeps = vi.hoisted(() => ({ ensureGraphDeps: vi.fn() })); + +vi.mock("node:child_process", async (importOriginal) => ({ + ...(await importOriginal()), + execFileSync: vi.fn(), +})); + +vi.mock("node:fs", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + readFileSync: (path: unknown, ...args: unknown[]) => { + if (path === fsFault.path && fsFault.error) throw fsFault.error; + return (actual.readFileSync as (...inner: unknown[]) => unknown)(path, ...args); + }, + }; +}); + +vi.mock("../../src/cli/graph-deps.js", () => graphDeps); + +const HOME_DIR = mkdtempSync(join(tmpdir(), "emb-config-preflight-home-")); +let mod: typeof import("../../src/cli/embeddings.js"); +let cfg: typeof import("../../src/user-config.js"); +let processKill: ReturnType; + +beforeAll(async () => { + setFakeHome(HOME_DIR); + mod = await import("../../src/cli/embeddings.js"); + cfg = await import("../../src/user-config.js"); +}); + +beforeEach(() => { + for (const sub of [".hivemind", ".deeplake", ".codex", ".cursor", ".hermes", ".claude"]) { + rmSync(join(HOME_DIR, sub), { recursive: true, force: true }); + } + cfg._resetUserConfigForTesting(); + cfg._setConfigPathForTesting(() => join(HOME_DIR, ".deeplake", "config.json")); + mkdirSync(join(HOME_DIR, ".deeplake"), { recursive: true }); + fsFault.path = ""; + fsFault.error = null; + graphDeps.ensureGraphDeps.mockClear(); + vi.mocked(execFileSync).mockClear(); + processKill = vi.spyOn(process, "kill").mockImplementation(() => true); +}); + +afterEach(() => { + processKill.mockRestore(); +}); + +afterAll(() => { + cfg._resetUserConfigForTesting(); + clearFakeHome(); + rmSync(HOME_DIR, { recursive: true, force: true }); + vi.resetModules(); +}); + +function seedFixture(configBytes: string): { configPath: string; linkPath: string; sharedPayloadPath: string; linkInode: number } { + const configPath = join(HOME_DIR, ".deeplake", "config.json"); + writeFileSync(configPath, configBytes); + const sharedPayloadPath = join(mod.SHARED_DIR, "fixture-payload.bin"); + mkdirSync(join(mod.SHARED_NODE_MODULES, mod.TRANSFORMERS_PKG), { recursive: true }); + writeFileSync(sharedPayloadPath, "shared payload stays byte-for-byte"); + const pluginDir = join(HOME_DIR, ".codex", "hivemind"); + mkdirSync(join(pluginDir, "bundle"), { recursive: true }); + const linkPath = join(pluginDir, "node_modules"); + symlinkSync(mod.SHARED_NODE_MODULES, linkPath); + return { configPath, linkPath, sharedPayloadPath, linkInode: lstatSync(linkPath).ino }; +} + +describe("heavy embeddings config preflight", () => { + it.each([ + ["install", "{ malformed"], + ["uninstall", "{ malformed"], + ])("rejects malformed config before %s mutation", (operation, configBytes) => { + const fixture = seedFixture(configBytes); + + expect(() => operation === "install" + ? mod.installEmbeddings() + : mod.uninstallEmbeddings({ prune: true })).toThrow(/not valid JSON/); + expect(readFileSync(fixture.configPath, "utf-8")).toBe(configBytes); + expect(readFileSync(fixture.sharedPayloadPath, "utf-8")).toBe("shared payload stays byte-for-byte"); + expect(existsSync(mod.SHARED_DIR)).toBe(true); + expect(lstatSync(fixture.linkPath).isSymbolicLink()).toBe(true); + expect(readlinkSync(fixture.linkPath)).toBe(mod.SHARED_NODE_MODULES); + expect(lstatSync(fixture.linkPath).ino).toBe(fixture.linkInode); + expect(graphDeps.ensureGraphDeps).not.toHaveBeenCalled(); + expect(execFileSync).not.toHaveBeenCalled(); + expect(processKill).not.toHaveBeenCalled(); + }); + + it.each(["install", "uninstall"])("rejects injected config read errors before %s mutation", (operation) => { + const fixture = seedFixture(JSON.stringify({ embeddings: { enabled: true } })); + fsFault.path = fixture.configPath; + fsFault.error = Object.assign(new Error("injected config read failure"), { code: "EIO" }); + + expect(() => operation === "install" + ? mod.installEmbeddings() + : mod.uninstallEmbeddings({ prune: true })).toThrow(/could not be read.*injected config read failure/); + fsFault.error = null; + expect(readFileSync(fixture.configPath, "utf-8")).toBe(JSON.stringify({ embeddings: { enabled: true } })); + expect(readFileSync(fixture.sharedPayloadPath, "utf-8")).toBe("shared payload stays byte-for-byte"); + expect(existsSync(mod.SHARED_DIR)).toBe(true); + expect(lstatSync(fixture.linkPath).isSymbolicLink()).toBe(true); + expect(readlinkSync(fixture.linkPath)).toBe(mod.SHARED_NODE_MODULES); + expect(lstatSync(fixture.linkPath).ino).toBe(fixture.linkInode); + expect(graphDeps.ensureGraphDeps).not.toHaveBeenCalled(); + expect(execFileSync).not.toHaveBeenCalled(); + expect(processKill).not.toHaveBeenCalled(); + }); +}); diff --git a/tests/cli/cli-install-hermes.test.ts b/tests/cli/cli-install-hermes.test.ts index adbedbcac..20f20092e 100644 --- a/tests/cli/cli-install-hermes.test.ts +++ b/tests/cli/cli-install-hermes.test.ts @@ -135,7 +135,16 @@ describe("installHermes — cold install", () => { expect(cfg.preserved_field).toBe("stay"); }); - it.each(["", " \n\t", "# user comment only\n # another comment\n"])( + it.each([ + "", + " \n\t", + "# user comment only\n # another comment\n", + "---\n", + "--- # comment\n", + "...\n", + "\ufeff---\n", + "%YAML 1.2\n---\n", + ])( "accepts an empty YAML document without treating it as malformed (%j)", async (raw) => { mkdirSync(join(tmpHome, ".hermes"), { recursive: true }); @@ -147,7 +156,7 @@ describe("installHermes — cold install", () => { }, ); - it.each(["plain scalar\n", "- array item\n"])( + it.each(["plain scalar\n", "- array item\n", "null\n", "~\n"])( "rejects a non-mapping YAML root without touching payloads (%j)", async (raw) => { mkdirSync(join(tmpHome, ".hermes"), { recursive: true }); From d2b79139ac425763ab5aa30be28e3346b10cff2d Mon Sep 17 00:00:00 2001 From: Divyam Talwar Date: Tue, 22 Sep 2026 06:34:03 +0530 Subject: [PATCH 4/5] Distinguish empty YAML documents from tagged null roots The empty-document regression needs to accept legal start/end marker pairs and supported directives while rejecting parser-null forms such as !!null and anchored nulls. Keep the parser success check, then use a narrow lexical structure check for the empty-document branch and preserve the valid-config preflight paths in the hermetic embeddings tests. Constraint: Preserve js-yaml parser authority, exact malformed-config bytes, existing config cache/setter contracts, and missing-file behavior Rejected: Accept every parsed null or rely only on parser root kind | tagged and anchored nulls also report null-like roots Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep lexical acceptance limited to legal empty markers/directives and ordered marker pairs Tested: Node 22 Hermes 27 tests; embeddings preflight 6 tests; combined focused/adjacent 160 tests; typecheck Not-tested: Final build/audit rerun after this follow-up and full Linux suite Related: 3725c43fa56664b63448f7d42961685de723f981 --- src/cli/install-hermes.ts | 26 ++++++++------ .../cli-embeddings-config-preflight.test.ts | 34 +++++++++++++++++++ tests/cli/cli-install-hermes.test.ts | 6 ++-- 3 files changed, 54 insertions(+), 12 deletions(-) diff --git a/src/cli/install-hermes.ts b/src/cli/install-hermes.ts index bc1d8b6a7..3a083b716 100644 --- a/src/cli/install-hermes.ts +++ b/src/cli/install-hermes.ts @@ -186,27 +186,33 @@ function readConfig(): HermesConfig | null { function isEmptyYamlDocument(raw: string): boolean { const lines = raw.replace(/^\uFEFF/, "").split(/\r?\n/); let sawDirective = false; - let sawDocumentMarker = false; + let sawDocumentStart = false; + let sawDocumentEnd = false; for (const line of lines) { const trimmed = line.trim(); if (trimmed === "" || trimmed.startsWith("#")) continue; if (isSupportedYamlDirective(trimmed)) { - if (sawDocumentMarker) return false; + if (sawDocumentStart || sawDocumentEnd) return false; sawDirective = true; continue; } - if (/^(?:---|\.\.\.)(?:\s+#.*)?$/.test(trimmed)) { - if (sawDocumentMarker) return false; - sawDocumentMarker = true; + if (/^---(?:\s+#.*)?$/.test(trimmed)) { + if (sawDocumentStart || sawDocumentEnd) return false; + sawDocumentStart = true; + continue; + } + if (/^\.\.\.(?:\s+#.*)?$/.test(trimmed)) { + if (sawDocumentEnd) return false; + sawDocumentEnd = true; continue; } return false; } - // A directive must introduce a document, while a blank/comment-only stream - // is already a valid empty document. The successful yaml.load above remains - // the parser authority; this lexical check only distinguishes empty docs - // from explicit nulls and other scalar/collection roots. - return !sawDirective || sawDocumentMarker; + // A directive must introduce a document-start marker. A document-end marker + // may stand alone, while a start marker may be followed by one end marker. + // The successful yaml.load above remains the parser authority; this lexical + // check only distinguishes empty docs from explicit nulls and other roots. + return (!sawDirective || sawDocumentStart) && (!sawDocumentEnd || sawDocumentStart || !sawDirective); } function isSupportedYamlDirective(line: string): boolean { diff --git a/tests/cli/cli-embeddings-config-preflight.test.ts b/tests/cli/cli-embeddings-config-preflight.test.ts index e5a12431b..64d32af41 100644 --- a/tests/cli/cli-embeddings-config-preflight.test.ts +++ b/tests/cli/cli-embeddings-config-preflight.test.ts @@ -118,4 +118,38 @@ describe("heavy embeddings config preflight", () => { expect(execFileSync).not.toHaveBeenCalled(); expect(processKill).not.toHaveBeenCalled(); }); + + it("allows install with a valid config and existing payload/link", () => { + const config = { docs: { llmAgent: "codex" }, embeddings: { enabled: false } }; + const fixture = seedFixture(JSON.stringify(config)); + + mod.installEmbeddings(); + + expect(JSON.parse(readFileSync(fixture.configPath, "utf-8"))).toEqual({ + docs: { llmAgent: "codex" }, + embeddings: { enabled: true }, + }); + expect(readFileSync(fixture.sharedPayloadPath, "utf-8")).toBe("shared payload stays byte-for-byte"); + expect(lstatSync(fixture.linkPath).isSymbolicLink()).toBe(true); + expect(readlinkSync(fixture.linkPath)).toBe(mod.SHARED_NODE_MODULES); + expect(graphDeps.ensureGraphDeps).toHaveBeenCalledTimes(1); + expect(execFileSync).not.toHaveBeenCalled(); + }); + + it("allows prune-uninstall with a valid config and existing payload/link", () => { + const config = { docs: { llmAgent: "codex" }, embeddings: { enabled: true } }; + const fixture = seedFixture(JSON.stringify(config)); + + mod.uninstallEmbeddings({ prune: true }); + + expect(JSON.parse(readFileSync(fixture.configPath, "utf-8"))).toEqual({ + docs: { llmAgent: "codex" }, + embeddings: { enabled: false }, + }); + expect(existsSync(fixture.linkPath)).toBe(false); + expect(existsSync(fixture.sharedPayloadPath)).toBe(false); + expect(existsSync(mod.SHARED_DIR)).toBe(false); + expect(graphDeps.ensureGraphDeps).not.toHaveBeenCalled(); + expect(execFileSync).not.toHaveBeenCalled(); + }); }); diff --git a/tests/cli/cli-install-hermes.test.ts b/tests/cli/cli-install-hermes.test.ts index 20f20092e..015ae4fec 100644 --- a/tests/cli/cli-install-hermes.test.ts +++ b/tests/cli/cli-install-hermes.test.ts @@ -142,8 +142,10 @@ describe("installHermes — cold install", () => { "---\n", "--- # comment\n", "...\n", + "---\n# comment before the document end\n... # end\n", "\ufeff---\n", - "%YAML 1.2\n---\n", + "%YAML 1.2\n---\n...\n", + "%TAG !e! tag:example.com,2026:\n---\n...\n", ])( "accepts an empty YAML document without treating it as malformed (%j)", async (raw) => { @@ -156,7 +158,7 @@ describe("installHermes — cold install", () => { }, ); - it.each(["plain scalar\n", "- array item\n", "null\n", "~\n"])( + it.each(["plain scalar\n", "- array item\n", "null\n", "~\n", "!!null\n", "&empty\n"])( "rejects a non-mapping YAML root without touching payloads (%j)", async (raw) => { mkdirSync(join(tmpHome, ".hermes"), { recursive: true }); From a061e94c8f22989955e6f736145b77f6f98de502 Mon Sep 17 00:00:00 2001 From: Divyam Talwar Date: Tue, 22 Sep 2026 06:36:42 +0530 Subject: [PATCH 5/5] Reject tagged and anchored null Hermes roots The parser reports empty documents and explicit null-like roots through the same null value, but close-event metadata distinguishes an empty node from a tag or anchor. Keep the empty-document acceptance parser-driven and reject those explicit null forms without broadening the accepted syntax. Constraint: Preserve legal YAML markers/directives, BOM/comments, parser success semantics, and payload-preserving rejection Rejected: Treat every null-like parser result as an empty config | tagged and anchored null roots must remain rejected Confidence: high Scope-risk: narrow Reversibility: clean Directive: Retain the parser metadata guard whenever adding null-root compatibility cases Tested: Node 22 Hermes 29 tests; final focused matrix rerun pending Not-tested: Final build/audit rerun after this commit and full Linux suite Related: d2b79139ac425763ab5aa30be28e3346b10cff2d --- src/cli/install-hermes.ts | 51 +++++++++------------------------------ 1 file changed, 12 insertions(+), 39 deletions(-) diff --git a/src/cli/install-hermes.ts b/src/cli/install-hermes.ts index 3a083b716..cdb757006 100644 --- a/src/cli/install-hermes.ts +++ b/src/cli/install-hermes.ts @@ -171,55 +171,28 @@ function readConfig(): HermesConfig | null { } let parsed: unknown; + let rootNodeIsEmpty = true; try { - parsed = yaml.load(raw); + parsed = yaml.load(raw, { + // js-yaml represents both an empty document and an explicit null scalar + // as null. The final parser close event retains the root node metadata, + // so use it to distinguish those cases without reimplementing YAML. + listener: (event, state) => { + if (event !== "close") return; + const node = state as typeof state & { tag?: string | null; anchor?: string | null }; + rootNodeIsEmpty = node.kind == null && node.tag == null && node.anchor == null; + }, + }); } catch { throw new Error(`Hermes config at ${CONFIG_PATH} is not valid YAML; fix or remove it, then rerun.`); } if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) { return parsed as HermesConfig; } - if ((parsed === undefined || parsed === null) && isEmptyYamlDocument(raw)) return {}; + if ((parsed === undefined || parsed === null) && rootNodeIsEmpty) return {}; throw new Error(`Hermes config at ${CONFIG_PATH} must contain a YAML mapping; fix or remove it, then rerun.`); } -function isEmptyYamlDocument(raw: string): boolean { - const lines = raw.replace(/^\uFEFF/, "").split(/\r?\n/); - let sawDirective = false; - let sawDocumentStart = false; - let sawDocumentEnd = false; - for (const line of lines) { - const trimmed = line.trim(); - if (trimmed === "" || trimmed.startsWith("#")) continue; - if (isSupportedYamlDirective(trimmed)) { - if (sawDocumentStart || sawDocumentEnd) return false; - sawDirective = true; - continue; - } - if (/^---(?:\s+#.*)?$/.test(trimmed)) { - if (sawDocumentStart || sawDocumentEnd) return false; - sawDocumentStart = true; - continue; - } - if (/^\.\.\.(?:\s+#.*)?$/.test(trimmed)) { - if (sawDocumentEnd) return false; - sawDocumentEnd = true; - continue; - } - return false; - } - // A directive must introduce a document-start marker. A document-end marker - // may stand alone, while a start marker may be followed by one end marker. - // The successful yaml.load above remains the parser authority; this lexical - // check only distinguishes empty docs from explicit nulls and other roots. - return (!sawDirective || sawDocumentStart) && (!sawDocumentEnd || sawDocumentStart || !sawDirective); -} - -function isSupportedYamlDirective(line: string): boolean { - return /^%YAML\s+1\.(?:1|2)(?:\s+#.*)?$/.test(line) - || /^%TAG\s+\S+\s+\S+(?:\s+#.*)?$/.test(line); -} - function writeConfig(cfg: HermesConfig): void { ensureDir(HERMES_HOME); const dumped = yaml.dump(cfg, { lineWidth: 100, noRefs: true });