From 2e34fd6944ce28ee85ad78b3b7b05bcbb6816252 Mon Sep 17 00:00:00 2001 From: Will Hampson Date: Mon, 17 Aug 2026 13:46:01 -0700 Subject: [PATCH 1/2] fix: isolate committer child and guarantee SIGKILL escalation The committer child pi now runs with --no-extensions so a trusted project cannot reload pi-brain and append to the log.md being distilled. The abort path no longer guards SIGKILL behind proc.killed (true as soon as kill() is called), so a child that ignores SIGTERM is killed after 3s instead of hanging the commit. Locks the model/thinking argv contract (config override, session inherit, fallbacks) with fake-pi spawn tests. --- src/subagent.test.ts | 228 +++++++++++++++++++++++++++++++++++++++++++ src/subagent.ts | 23 ++++- 2 files changed, 248 insertions(+), 3 deletions(-) diff --git a/src/subagent.test.ts b/src/subagent.test.ts index d2c1992..2748616 100644 --- a/src/subagent.test.ts +++ b/src/subagent.test.ts @@ -1,10 +1,16 @@ +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; + import fc from "fast-check"; import { buildCommitterTask, extractCommitBlocks, extractFinalText, + spawnCommitter, } from "./subagent.js"; +import type { CommitterOptions } from "./types.js"; // Helpers @@ -364,3 +370,225 @@ describe("buildCommitterTask property-based tests", () => { ); }); }); + +// --- spawnCommitter argv resolution --- + +/** Read the value that follows a flag in captured argv, or undefined when absent */ +function argAfter(argv: string[], flag: string): string | undefined { + const index = argv.indexOf(flag); + return index === -1 ? undefined : argv[index + 1]; +} + +function sleep(ms: number): Promise { + return new Promise((resolve) => { + setTimeout(resolve, ms); + }); +} + +describe("spawnCommitter model and thinking resolution", () => { + const originalPath = process.env.PATH; + let binDir: string; + let capturePath: string; + let tmpCwd: string; + + beforeEach(() => { + tmpCwd = fs.mkdtempSync(path.join(os.tmpdir(), "committer-cwd-")); + binDir = fs.mkdtempSync(path.join(os.tmpdir(), "committer-bin-")); + capturePath = path.join(binDir, "argv.txt"); + // Fake pi: capture argv and exit 0 (a successful, empty commit run) + fs.writeFileSync( + path.join(binDir, "pi"), + `#!/bin/sh\nprintf '%s\\n' "$@" > "${capturePath}"\nexit 0\n`, + { mode: 0o755 } + ); + process.env.PATH = `${binDir}:${originalPath}`; + }); + + afterEach(() => { + process.env.PATH = originalPath; + fs.rmSync(tmpCwd, { recursive: true, force: true }); + fs.rmSync(binDir, { recursive: true, force: true }); + }); + + async function runSpawn(current: CommitterOptions = {}): Promise { + const result = await spawnCommitter( + tmpCwd, + "distill the log", + undefined, + current + ); + + expect(result.exitCode).toBe(0); + expect(result.error).toBeUndefined(); + return fs.readFileSync(capturePath, "utf8").split("\n").filter(Boolean); + } + + function writeConfig(content: string): void { + fs.mkdirSync(path.join(tmpCwd, ".memory"), { recursive: true }); + fs.writeFileSync(path.join(tmpCwd, ".memory", "config.yaml"), content); + } + + it("omits --model and --thinking when no config and no session values exist", async () => { + const argv = await runSpawn(); + + expect(argv).not.toContain("--model"); + expect(argv).not.toContain("--thinking"); + // Preservation: core committer flags unchanged + expect(argAfter(argv, "--mode")).toBe("json"); + expect(argv).toContain("--no-session"); + expect(argAfter(argv, "-p")).toBe("Task: distill the log"); + }); + + it("passes session model and thinking when no config exists", async () => { + const argv = await runSpawn({ + model: "openai/gpt-5.6-luna", + thinking: "high", + }); + + expect(argAfter(argv, "--model")).toBe("openai/gpt-5.6-luna"); + expect(argAfter(argv, "--thinking")).toBe("high"); + }); + + it("lets config override session values", async () => { + writeConfig( + "committer:\n model: google/gemini-3.6-flash\n thinking: low\n" + ); + + const argv = await runSpawn({ + model: "openai/gpt-5.6-luna", + thinking: "high", + }); + + expect(argAfter(argv, "--model")).toBe("google/gemini-3.6-flash"); + expect(argAfter(argv, "--thinking")).toBe("low"); + }); + + it("merges config and session values per field", async () => { + writeConfig("committer:\n model: google/gemini-3.6-flash\n"); + + const argv = await runSpawn({ + model: "openai/gpt-5.6-luna", + thinking: "high", + }); + + expect(argAfter(argv, "--model")).toBe("google/gemini-3.6-flash"); + expect(argAfter(argv, "--thinking")).toBe("high"); + }); + + it("inherits session values from a comment-only config file", async () => { + writeConfig( + "# Optional Brain configuration.\n# committer:\n# model: x\n" + ); + + const argv = await runSpawn({ + model: "openai/gpt-5.6-luna", + thinking: "high", + }); + + expect(argAfter(argv, "--model")).toBe("openai/gpt-5.6-luna"); + expect(argAfter(argv, "--thinking")).toBe("high"); + }); + + it("falls back to session values when the config file is unreadable", async () => { + fs.mkdirSync(path.join(tmpCwd, ".memory", "config.yaml"), { + recursive: true, + }); + + const argv = await runSpawn({ + model: "openai/gpt-5.6-luna", + thinking: "high", + }); + + expect(argAfter(argv, "--model")).toBe("openai/gpt-5.6-luna"); + expect(argAfter(argv, "--thinking")).toBe("high"); + }); + + it("falls back to session values when committer is not a map", async () => { + writeConfig("committer: not-a-map\n"); + + const argv = await runSpawn({ + model: "openai/gpt-5.6-luna", + thinking: "high", + }); + + expect(argAfter(argv, "--model")).toBe("openai/gpt-5.6-luna"); + expect(argAfter(argv, "--thinking")).toBe("high"); + }); + + it("treats empty config strings as unset and inherits session values", async () => { + writeConfig('committer:\n model: ""\n'); + + const argv = await runSpawn({ + model: "openai/gpt-5.6-luna", + thinking: "high", + }); + + expect(argAfter(argv, "--model")).toBe("openai/gpt-5.6-luna"); + expect(argAfter(argv, "--thinking")).toBe("high"); + }); + + it("runs the committer child with extension discovery disabled", async () => { + const argv = await runSpawn(); + + expect(argv).toContain("--no-extensions"); + }); +}); + +describe("spawnCommitter abort escalation", () => { + const originalPath = process.env.PATH; + let binDir: string; + let tmpCwd: string; + + beforeEach(() => { + tmpCwd = fs.mkdtempSync(path.join(os.tmpdir(), "committer-cwd-")); + binDir = fs.mkdtempSync(path.join(os.tmpdir(), "committer-bin-")); + // Fake pi that ignores SIGTERM and self-exits after 20s as a safety net + fs.writeFileSync( + path.join(binDir, "pi"), + [ + "#!/usr/bin/env node", + 'process.on("SIGTERM", () => {});', + "setTimeout(() => process.exit(0), 20_000);", + "", + ].join("\n"), + { mode: 0o755 } + ); + process.env.PATH = `${binDir}:${originalPath}`; + }); + + afterEach(() => { + process.env.PATH = originalPath; + fs.rmSync(tmpCwd, { recursive: true, force: true }); + fs.rmSync(binDir, { recursive: true, force: true }); + }); + + it( + "kills a child that ignores SIGTERM and resolves with an error", + { timeout: 15_000 }, + async () => { + const controller = new AbortController(); + const pending = spawnCommitter( + tmpCwd, + "distill the log", + controller.signal, + { model: "openai/gpt-5.6-luna" } + ); + + // Give the child time to install its SIGTERM handler + await sleep(600); + controller.abort(); + + const result = await Promise.race([ + pending, + sleep(9000).then(() => { + throw new Error( + "spawnCommitter never resolved: SIGKILL escalation did not fire" + ); + }), + ]); + + expect(result.exitCode).not.toBe(0); + expect(result.error).toBeTruthy(); + } + ); +}); diff --git a/src/subagent.ts b/src/subagent.ts index aacfaee..c1f38fc 100644 --- a/src/subagent.ts +++ b/src/subagent.ts @@ -7,6 +7,7 @@ import type { CommitterOptions, SubagentResult } from "./types.js"; import { parseYaml } from "./yaml.js"; const COMMITTER_TOOLS = "read,grep,find,ls"; +const KILL_ESCALATION_MS = 3000; interface AgentDefinition { prompt: string; @@ -193,7 +194,6 @@ export function extractCommitBlocks(text: string): string | null { return null; } - // Extract from "### Branch Purpose" onward const fromStart = text.slice(branchPurposeIndex); const lines = fromStart.split("\n"); @@ -280,7 +280,17 @@ export function spawnCommitter( const committerOptions = resolveCommitterOptions(cwd, current); - const args = ["--mode", "json", "--no-session", "--tools", agentDef.tools]; + const args = [ + "--mode", + "json", + "--no-session", + // Isolate distillation from project extensions: a trusted project would + // reload pi-brain in the child, whose turn_end hook appends to the very + // log.md being distilled. + "--no-extensions", + "--tools", + agentDef.tools, + ]; if (committerOptions.model) { args.push("--model", committerOptions.model); @@ -379,7 +389,14 @@ export function spawnCommitter( if (signal) { const kill = () => { proc.kill("SIGTERM"); - setTimeout(() => !proc.killed && proc.kill("SIGKILL"), 3000); + // proc.killed turns true as soon as kill() is called, even when the + // child ignores SIGTERM — escalate unconditionally so a stuck + // committer child cannot hang the commit forever. + const escalation = setTimeout( + () => proc.kill("SIGKILL"), + KILL_ESCALATION_MS + ); + proc.on("close", () => clearTimeout(escalation)); }; if (signal.aborted) { kill(); From 6eb9685321b21b37d2fd2d6a1268143a23eec28a Mon Sep 17 00:00:00 2001 From: Will Hampson Date: Mon, 17 Aug 2026 13:46:09 -0700 Subject: [PATCH 2/2] fix: support YAML comments in parser and config parseYaml now strips inline comments (unquoted ' #' only), skips comment lines anywhere in nested maps and lists, and no longer turns '# comment:' lines into bogus keys or drops a nested map separated from its key by a comment. serializeYaml quotes values containing ' #' so they round-trip. Also covers the init-script config skeleton: comment-only file parses to an empty object and re-init never overwrites a user config. Fixes a stale 'all five' mutation-checklist count. --- src/cache-safety.test.ts | 2 +- src/init-script.test.ts | 32 +++++++++++++++++++++ src/yaml.test.ts | 62 ++++++++++++++++++++++++++++++++++++++++ src/yaml.ts | 45 +++++++++++++++++++++++++---- 4 files changed, 134 insertions(+), 7 deletions(-) diff --git a/src/cache-safety.test.ts b/src/cache-safety.test.ts index 12d625d..f3895f8 100644 --- a/src/cache-safety.test.ts +++ b/src/cache-safety.test.ts @@ -224,7 +224,7 @@ describe("cache safety invariants", () => { // 2) Do not clear frozen snapshot on session_compact. // 3) Recompute snapshot on every before_agent_start (no freeze). // 4) Allow mid-epoch main.md edits to leak into frozen snapshot. - // This test should fail for all five. + // This test should fail for all four. it("should freeze roadmap content within an epoch and refresh only after reset events", async () => { const opArb = fc.array( fc.constantFrom( diff --git a/src/init-script.test.ts b/src/init-script.test.ts index 963bb89..9c26d39 100644 --- a/src/init-script.test.ts +++ b/src/init-script.test.ts @@ -4,6 +4,8 @@ import * as os from "node:os"; import * as path from "node:path"; import { fileURLToPath } from "node:url"; +import { parseYaml } from "./yaml.js"; + describe("brain-init.sh", () => { let tmpDir: string; const testDir = path.dirname(fileURLToPath(import.meta.url)); @@ -136,4 +138,34 @@ describe("brain-init.sh", () => { expect(memoryAgents).toContain("## When to Commit"); expect(memoryAgents).toContain("end the session"); }); + + it("should create a comment-only .memory/config.yaml skeleton", () => { + // Act + execFileSync("bash", [scriptPath], { cwd: tmpDir }); + + // Assert + const configPath = path.join(tmpDir, ".memory", "config.yaml"); + expect(fs.existsSync(configPath)).toBeTruthy(); + + const content = fs.readFileSync(configPath, "utf8"); + // Skeleton must mention the committer keys so users can discover them + expect(content).toContain("committer"); + // Comment-only file parses to an empty object: inherit the active session + expect(parseYaml(content)).toStrictEqual({}); + }); + + it("should preserve an existing .memory/config.yaml on re-init", () => { + // Arrange + fs.mkdirSync(path.join(tmpDir, ".memory"), { recursive: true }); + const custom = "committer:\n model: openai/gpt-5.6-luna\n"; + fs.writeFileSync(path.join(tmpDir, ".memory", "config.yaml"), custom); + + // Act + execFileSync("bash", [scriptPath], { cwd: tmpDir }); + + // Assert + expect( + fs.readFileSync(path.join(tmpDir, ".memory", "config.yaml"), "utf8") + ).toBe(custom); + }); }); diff --git a/src/yaml.test.ts b/src/yaml.test.ts index 2ac0e31..a99990e 100644 --- a/src/yaml.test.ts +++ b/src/yaml.test.ts @@ -239,3 +239,65 @@ describe("yaml property-based tests", () => { ); }); }); + +// --- comment handling --- + +describe("parseYaml comment handling", () => { + it("strips an inline comment after an unquoted scalar", () => { + expect(parseYaml("model: openai/gpt-4o # note")).toStrictEqual({ + model: "openai/gpt-4o", + }); + }); + + it("keeps # inside double-quoted values", () => { + expect(parseYaml('model: "openai/x # y"')).toStrictEqual({ + model: "openai/x # y", + }); + }); + + it("keeps # inside single-quoted values", () => { + expect(parseYaml("model: 'a # b'")).toStrictEqual({ model: "a # b" }); + }); + + it("keeps # that is not preceded by whitespace", () => { + expect(parseYaml("model: a#b")).toStrictEqual({ model: "a#b" }); + }); + + it("treats a value that is only a comment like an empty value", () => { + // `key:` with no value parses as an empty nested map; a comment-only + // value must behave identically after stripping + expect(parseYaml("model: #fff")).toStrictEqual({ model: {} }); + }); + + it("ignores top-level comment lines, even with colons", () => { + expect(parseYaml("# a comment: with colon\nkey: v")).toStrictEqual({ + key: "v", + }); + }); + + it("preserves a nested map across a comment line after the key", () => { + expect(parseYaml("committer:\n# note\n model: x\n")).toStrictEqual({ + committer: { model: "x" }, + }); + }); + + it("preserves indented comment lines inside a nested map", () => { + expect(parseYaml("committer:\n # note\n model: x\n")).toStrictEqual({ + committer: { model: "x" }, + }); + }); + + it("preserves comment lines inside a list", () => { + expect(parseYaml("items:\n # note\n - name: a\n")).toStrictEqual({ + items: [{ name: "a" }], + }); + }); + + it("serializes values containing ' #' quoted so they round-trip", () => { + const original = { key: "x #y" }; + const serialized = serializeYaml(original); + + expect(serialized).toBe('key: "x #y"'); + expect(parseYaml(serialized)).toStrictEqual(original); + }); +}); diff --git a/src/yaml.ts b/src/yaml.ts index 648c1b2..8f5c37d 100644 --- a/src/yaml.ts +++ b/src/yaml.ts @@ -7,7 +7,40 @@ type YamlItem = Record; type YamlValue = string | Record | YamlItem[]; type YamlObject = Record; -const NEEDS_QUOTING = /[-:{}[\],&*?|>!%@`]|^\d{4}-\d{2}/; +const NEEDS_QUOTING = /[-:{}[\],&*?|>!%@#`]|^\d{4}-\d{2}/; + +/** True for lines that are blank or start with a YAML comment */ +function isCommentOrBlank(line: string): boolean { + const trimmed = line.trim(); + return trimmed === "" || trimmed.startsWith("#"); +} + +/** + * Strip a trailing YAML comment from a scalar value: a `#` starts a comment + * only at the start of the value or when preceded by whitespace, and only + * outside single- or double-quoted regions. + */ +function stripComment(value: string): string { + let inSingle = false; + let inDouble = false; + + for (let i = 0; i < value.length; i++) { + const char = value[i]; + + if (char === "'" && !inDouble) { + inSingle = !inSingle; + } else if (char === '"' && !inSingle) { + inDouble = !inDouble; + } else if (char === "#" && !inSingle && !inDouble) { + const prev = i === 0 ? "" : value[i - 1]; + if (i === 0 || prev === " " || prev === "\t") { + return value.slice(0, i).trimEnd(); + } + } + } + + return value; +} function unquote(value: string): string { if ( @@ -42,7 +75,7 @@ function parseKeyValue(text: string): { key: string; value: string } | null { return { key: text.slice(0, colonIdx).trim(), - value: text.slice(colonIdx + 1).trim(), + value: stripComment(text.slice(colonIdx + 1).trim()), }; } @@ -58,7 +91,7 @@ function parseNestedObject( while (i < lines.length) { const line = lines[i]; - if (line.trim() === "") { + if (isCommentOrBlank(line)) { i++; continue; } @@ -91,7 +124,7 @@ function parseList( while (i < lines.length) { const line = lines[i]; - if (line.trim() === "") { + if (isCommentOrBlank(line)) { i++; continue; } @@ -135,7 +168,7 @@ export function parseYaml(input: string): YamlObject { while (i < lines.length) { const line = lines[i]; - if (line.trim() === "") { + if (isCommentOrBlank(line)) { i++; continue; } @@ -159,7 +192,7 @@ export function parseYaml(input: string): YamlObject { } i++; - while (i < lines.length && lines[i].trim() === "") { + while (i < lines.length && isCommentOrBlank(lines[i])) { i++; }