From ed9325bd2f26717b3a5eaaca07d48abaa94ba41e Mon Sep 17 00:00:00 2001 From: Thadeus Burgess Date: Thu, 26 Mar 2026 18:06:44 -0500 Subject: [PATCH 1/3] PLAN-108: Fix loop runner PR title format and artifact links MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace /artifact/by-slug/ with /implementation-plans/ in both LLM commit prompt footer and SAFETY commit PR body - Fix SAFETY commit PR title to use "PLAN-##: Automated changes from loop {shortId}" format via shared fallbackTitle constant - Resolve claude binary path at first use via which/login-shell to handle Electron's restricted PATH - Add assertPathAllowed sandbox gate before LLM commit spawn - Write PID file atomically via .pid.tmp rename - Replace unsafe JSON.parse cast with proper isExecutionResult type guard using unknown intermediate and shape checks - Add sanitizeCommitMessage doc comment re: backtick safety - Add 3 new integration tests (artifact links, PR title format, LLM spawn correctness) Testing: All 12 EXECUTE loop tests pass Risks: Low — changes isolated to symphony-loop.ts post-processing --- apps/desktop/package.json | 2 +- .../server/operations/symphony-interactive.ts | 6 + .../src/server/operations/symphony-loop.ts | 166 +++++- .../test/symphony-loop-execute.test.ts | 491 ++++++++++++++++++ 4 files changed, 649 insertions(+), 16 deletions(-) diff --git a/apps/desktop/package.json b/apps/desktop/package.json index f186e00f..54d82d15 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -1,6 +1,6 @@ { "name": "desktop", - "version": "0.9.2", + "version": "0.9.3", "description": "ClosedLoop Desktop", "author": "ClosedLoop AI ", "private": true, diff --git a/apps/desktop/src/server/operations/symphony-interactive.ts b/apps/desktop/src/server/operations/symphony-interactive.ts index afc61671..585186cb 100644 --- a/apps/desktop/src/server/operations/symphony-interactive.ts +++ b/apps/desktop/src/server/operations/symphony-interactive.ts @@ -949,6 +949,12 @@ function resolveWorktreeForComment( return expandedRepoPath; } +// Strips AI-vendor branding from commit text and normalises whitespace. +// Backticks are intentionally NOT stripped here — slugs used in URL paths +// (e.g. /implementation-plans/) never contain backticks because slug +// values arrive as alphanumeric-hyphen strings; the caller additionally strips +// newlines via .replace(/[\r\n]/g, ''), so the result is safe for shell +// heredocs and template-literal URL construction. export function sanitizeCommitMessage(text: string): string { return text .replaceAll(/claude\s*code/gi, "") diff --git a/apps/desktop/src/server/operations/symphony-loop.ts b/apps/desktop/src/server/operations/symphony-loop.ts index 06b470fb..2e1acc01 100644 --- a/apps/desktop/src/server/operations/symphony-loop.ts +++ b/apps/desktop/src/server/operations/symphony-loop.ts @@ -1,4 +1,4 @@ -import { execSync, spawn } from "node:child_process"; +import { execFileSync, execSync, spawn } from "node:child_process"; import crypto from "node:crypto"; import { closeSync, @@ -7,6 +7,7 @@ import { openSync, readFileSync, readSync, + renameSync, statSync, unlinkSync, writeFileSync, @@ -106,6 +107,78 @@ async function killLegacyAndMigrate(worktreeDir: string): Promise { migrateWorkDirIfNeeded(worktreeDir); } +// --------------------------------------------------------------------------- +// Claude binary resolution +// --------------------------------------------------------------------------- + +/** + * Cached absolute path to the `claude` binary, resolved once at first use. + * + * Resolution strategy (tried in order): + * 1. `which claude` using the current process.env.PATH (fast; works in + * tests where PATH is set to a fake-bin directory, and in dev shells). + * 2. `bash -lc 'which claude'` in a login shell so that nvm/homebrew/local + * bin directories are found even when Electron strips PATH at launch via + * the .app bundle, launchd (macOS), or systemd (Linux). + * 3. Falls back to the bare string "claude" so that the caller can still + * attempt spawn and receive a descriptive ENOENT error. + */ +let resolvedClaudePath: string | null = null; + +/** + * Reset the cached claude binary path. Intended for use in tests where PATH + * changes between test cases — production code should not call this. + */ +export function resetResolvedClaudePath(): void { + resolvedClaudePath = null; +} + +export function getResolvedClaudePath(): string { + // If we have a cached path, return it only if the binary still exists on + // disk. This handles test scenarios where a fake binary directory is cleaned + // up between test cases, causing the cached path to become stale. + if (resolvedClaudePath !== null && existsSync(resolvedClaudePath)) { + return resolvedClaudePath; + } + // Invalidate stale cache entry before re-resolving + resolvedClaudePath = null; + + // Strategy 1: which via current process PATH (works in tests and dev shells) + try { + const result = execFileSync("which", ["claude"], { + encoding: "utf-8", + stdio: "pipe", + timeout: 5_000, + }).trim(); + if (result) { + resolvedClaudePath = result; + return resolvedClaudePath; + } + } catch { + // Not found in current PATH — try login shell + } + + // Strategy 2: login shell which — sources ~/.nvm/nvm.sh and similar to + // populate the full user PATH that Electron strips on launch. + try { + const result = execFileSync("bash", ["-lc", "which claude"], { + encoding: "utf-8", + stdio: "pipe", + timeout: 5_000, + }).trim(); + if (result) { + resolvedClaudePath = result; + return resolvedClaudePath; + } + } catch { + // Login shell which also failed — fall through to bare name fallback + } + + // Fall back to bare name; spawn will throw ENOENT with a descriptive message + resolvedClaudePath = "claude"; + return resolvedClaudePath; +} + // --------------------------------------------------------------------------- // Types // --------------------------------------------------------------------------- @@ -1014,6 +1087,7 @@ async function attemptLlmCommit( artifactSlug: string | undefined, webAppOrigin: string, committer: LoopCommitter | undefined, + getAllowedDirectories: () => string[], onTimeout?: () => void, jobStore?: JobStore, claudeWorkDir?: string, @@ -1028,7 +1102,10 @@ async function attemptLlmCommit( let footer: string; if (safeSlug) { - const artifactLink = `${webAppOrigin}/artifact/by-slug/${safeSlug}`; + // safeSlug contains only alphanumerics, hyphens, and underscores after + // sanitizeCommitMessage() + newline stripping — no backticks that would + // break shell heredocs or prompt injection via template literals. + const artifactLink = `${webAppOrigin}/implementation-plans/${safeSlug}`; footer = `---\nLoop ID: ${safeLoopId}\nArtifact: ${artifactLink}`; } else { footer = `---\nLoop ID: ${safeLoopId}`; @@ -1092,6 +1169,22 @@ async function attemptLlmCommit( loopLog(loopId, "Attempting LLM-assisted commit..."); + // Sandbox gate: verify the worktree directory is within an allowed path + // before spawning any child process on it. This mirrors the assertPathAllowed + // check performed in handleLoopRequest before the main loop spawn. + try { + assertPathAllowed(worktreeDir, getAllowedDirectories()); + } catch (sandboxErr) { + if (sandboxErr instanceof DirectoryNotAllowedError) { + loopError( + loopId, + `LLM commit aborted: worktreeDir not in allowed sandbox: ${worktreeDir}`, + ); + return null; + } + throw sandboxErr; + } + const spawnEnv: Record = { ...process.env } as Record< string, string @@ -1103,15 +1196,33 @@ async function attemptLlmCommit( spawnEnv.GIT_COMMITTER_EMAIL = committer.email; } + // Resolve the absolute path to the `claude` binary once at first use. + // Electron strips PATH to a minimal system set when launching via the .app + // bundle or launchd (macOS) / systemd (Linux), so the bare name "claude" + // typically resolves to ENOENT even though it works in a terminal. Running + // `which claude` in a login shell picks up the full user PATH including + // nvm/homebrew/local bin directories. getResolvedClaudePath() caches the + // result for the process lifetime. + const claudeBinary = getResolvedClaudePath(); + const spawnArgs = ["-p", prompt, "--allowedTools", "Bash,Read,Write,Glob,Grep"]; + loopLog( + loopId, + `LLM commit spawn: binary=${claudeBinary} args=["-p", "", "--allowedTools", "Bash,Read,Write,Glob,Grep"] cwd=${worktreeDir} PATH=${spawnEnv.PATH ?? "(unset)"}` + ); + let child: ReturnType; try { child = spawn( - "claude", - ["-p", prompt, "--allowedTools", "Bash,Read,Write,Glob,Grep"], + claudeBinary, + spawnArgs, { cwd: worktreeDir, detached: true, stdio: "pipe", env: spawnEnv }, ); } catch (err) { - loopError(loopId, "LLM commit spawn failed:", err); + const code = (err as NodeJS.ErrnoException).code ?? "unknown"; + const enoentDetail = code === "ENOENT" + ? ` — '${claudeBinary}' binary not found; PATH=${spawnEnv.PATH ?? "(unset)"}` + : ""; + loopError(loopId, `LLM commit spawn failed [code=${code}${enoentDetail}]`, err); return null; } @@ -1138,10 +1249,14 @@ async function attemptLlmCommit( } // Update on-disk PID file so readProcessPidSync (used by plan-loop cancel and // status endpoint liveness checks) sees the LLM commit child, not the dead - // main-loop PID. + // main-loop PID. Write atomically via a .pid.tmp temp file renamed into + // place to prevent a concurrent reader from observing a partial write. if (claudeWorkDir) { try { - writeFileSync(path.join(claudeWorkDir, "process.pid"), String(pid)); + const pidFilePath = path.join(claudeWorkDir, "process.pid"); + const pidTmpPath = path.join(claudeWorkDir, "process.pid.tmp"); + writeFileSync(pidTmpPath, String(pid)); + renameSync(pidTmpPath, pidFilePath); } catch { loopLog(loopId, "Failed to update process.pid for LLM commit child"); } @@ -1150,6 +1265,12 @@ async function attemptLlmCommit( return new Promise((resolve) => { let killed = false; + // Process group kill behavior: + // The child is spawned with `detached: true`, which places it in its own + // process group (pgid === child.pid on POSIX). Sending SIGTERM/SIGKILL to + // -pid (negative PID) targets the entire process group, ensuring that any + // subprocesses spawned by claude (git, gh, etc.) are also terminated and + // do not become orphans when the timeout fires or cancel is requested. const killTimer = setTimeout(() => { if (!killed) { killed = true; @@ -1160,7 +1281,7 @@ async function attemptLlmCommit( } catch (killErr) { loopError(loopId, "Failed to kill LLM commit process:", killErr); } - // Escalate to SIGKILL after 5s if process survives SIGTERM + // Escalate to SIGKILL after 5s if the process group survives SIGTERM setTimeout(() => { try { process.kill(pid, 0); // check alive @@ -1245,7 +1366,11 @@ async function attemptLlmCommit( child.on("error", (err: Error) => { clearTimeout(killTimer); - loopError(loopId, "LLM commit process error:", err); + const code = (err as NodeJS.ErrnoException).code ?? "unknown"; + const enoentDetail = code === "ENOENT" + ? ` — '${claudeBinary}' binary not found; PATH=${spawnEnv.PATH ?? "(unset)"}` + : ""; + loopError(loopId, `LLM commit process error [code=${code}${enoentDetail}]:`, err); resolve(null); }); @@ -1322,8 +1447,8 @@ function executeGitOperations( }); const commitPrefix = artifactSlug ? `${artifactSlug}: ` : ""; - const commitMessage = `${commitPrefix}Symphony: ${command} -- loop ${shortId}`; - execSync(`git commit -m ${shellEscape(commitMessage)}`, { + const fallbackTitle = `${commitPrefix}Automated changes from loop ${shortId}`; + execSync(`git commit -m ${shellEscape(fallbackTitle)}`, { cwd: worktreeDir, stdio: "pipe", env, @@ -1355,7 +1480,7 @@ function executeGitOperations( // shell escaping issues with special characters (--body-file approach). const artifactLine = artifactSlug && webAppOrigin - ? `\nArtifact: ${webAppOrigin}/artifact/by-slug/${artifactSlug}` + ? `\nArtifact: ${webAppOrigin}/implementation-plans/${artifactSlug}` : ""; const prBody = `Loop ID: ${loopId}\nCommand: ${command}${artifactLine}`; const bodyFile = path.join( @@ -1381,16 +1506,24 @@ function executeGitOperations( timeout: 15_000, }, ).trim(); - const parsed = JSON.parse(existingPr) as { url: string; number: number }; + const parsedUnknown: unknown = JSON.parse(existingPr); + if ( + typeof parsedUnknown !== "object" || + parsedUnknown === null || + typeof (parsedUnknown as Record).url !== "string" || + typeof (parsedUnknown as Record).number !== "number" + ) { + throw new Error("Unexpected shape from gh pr view JSON"); + } + const parsed = parsedUnknown as { url: string; number: number }; prUrl = parsed.url; prNumber = parsed.number; } catch { // No existing PR — create one using --body-file to avoid shell escaping. // Create without --label first so the PR still succeeds on repos where the // 'symphony' label doesn't exist yet, then attach the label best-effort. - const prTitle = `${commitPrefix}Symphony: ${command} -- loop ${shortId}`; const prOutput = execSync( - `gh pr create --title ${shellEscape(prTitle)} --body-file ${shellEscape(bodyFile)} --base ${shellEscape(baseBranch)}`, + `gh pr create --title ${shellEscape(fallbackTitle)} --body-file ${shellEscape(bodyFile)} --base ${shellEscape(baseBranch)}`, { cwd: worktreeDir, encoding: "utf-8", @@ -1482,6 +1615,7 @@ async function handleProcessCompletion( claudeWorkDir: string, usedTempDir: boolean, expandedRepoPath: string | null, + getAllowedDirectories: () => string[], jobStore?: JobStore, webAppOrigin?: string, telemetry?: TelemetryEmitter, @@ -1608,6 +1742,7 @@ async function handleProcessCompletion( body.artifactSlug, webAppOrigin ?? "", committer, + getAllowedDirectories, () => { warnings.push( sanitizeErrorMessage("LLM commit timed out after 90s"), @@ -2667,6 +2802,7 @@ async function handleLoopRequest( claudeWorkDir, usedTempDir, expandedRepoPath, + getAllowedDirectories, jobStore, webAppOrigin, telemetry, diff --git a/apps/desktop/test/symphony-loop-execute.test.ts b/apps/desktop/test/symphony-loop-execute.test.ts index 9125088c..fe546be8 100644 --- a/apps/desktop/test/symphony-loop-execute.test.ts +++ b/apps/desktop/test/symphony-loop-execute.test.ts @@ -25,6 +25,7 @@ import { afterEach, test } from "node:test"; import { JobStore } from "../src/main/job-store.js"; import { DesktopGatewayServer } from "../src/server/server.js"; import { EMPTY_CAPABILITIES } from "../src/shared/contracts.js"; +import { resetResolvedClaudePath } from "../src/server/operations/symphony-loop.js"; import { createFakeRunLoopScript, initGitRepo, @@ -867,6 +868,496 @@ test("EXECUTE: cancel during attemptLlmCommit ends job as CANCELLED with no comp ); }); +// --------------------------------------------------------------------------- +// Test 8 (T-1.3): Artifact links use /implementation-plans/ path in both the +// SAFETY commit PR body and the LLM commit prompt footer. +// +// The fake gh binary captures --body-file content. +// The fake claude binary captures its -p argument to a file (then exits without +// writing execution-result.json so the code falls through to executeGitOperations). +// Both captures are asserted to contain /implementation-plans/ and not to +// contain /artifact/by-slug/. +// --------------------------------------------------------------------------- + +test("EXECUTE: artifact links use /implementation-plans/ in PR body and LLM prompt footer", async () => { + const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "execute-artifactlink-")); + tempPathsToClean.push(tmpDir); + + const repoPath = path.join(tmpDir, "repo-artifactlink"); + await initGitRepo(repoPath); + + const worktreeParent = path.join(tmpDir, "worktrees"); + await fs.mkdir(worktreeParent, { recursive: true }); + + process.env.HOME = tmpDir; + + // fake run-loop.sh: writes a file to create an uncommitted change so that + // executeGitOperations finds something to commit after attemptLlmCommit falls through. + await createFakeRunLoopScript( + tmpDir, + [ + "#!/bin/sh", + "echo 'feature output' > feature-output.txt", + "exit 0", + ].join("\n") + ); + + const fakeBin = path.join(tmpDir, "fake-bin"); + await fs.mkdir(fakeBin, { recursive: true }); + + // Capture paths + const claudePromptCapture = path.join(tmpDir, "claude-prompt-capture.txt"); + const ghBodyCapture = path.join(tmpDir, "gh-body-capture.txt"); + + // fake claude for attemptLlmCommit: captures the -p argument (the LLM prompt) + // to a file, then exits 0 without writing execution-result.json so the code + // falls through to the SAFETY executeGitOperations path. + const claudeScript = [ + "#!/bin/sh", + "# Capture the argument following -p", + "prev=''", + 'for arg in "$@"; do', + ' if [ "$prev" = "-p" ]; then', + ` printf '%s' "$arg" > ${JSON.stringify(claudePromptCapture)}`, + " fi", + ' prev="$arg"', + "done", + "exit 0", + ].join("\n"); + await fs.writeFile(path.join(fakeBin, "claude"), claudeScript, { mode: 0o755 }); + + // fake git: pass through all real git operations; stub push to avoid remote requirement + const fakeGitScript = [ + "#!/bin/sh", + "if [ \"$1\" = push ]; then exit 0; fi", + `exec /usr/bin/git "$@"`, + ].join("\n"); + await fs.writeFile(path.join(fakeBin, "git"), fakeGitScript, { mode: 0o755 }); + + // fake gh: capture --body-file content to ghBodyCapture. + // pr view (existing-PR check) exits non-zero so code proceeds to gh pr create. + // pr view --json body returns empty body so the metadata-footer update is a no-op. + const fakeGhScript = [ + "#!/bin/sh", + "if [ \"$1\" = pr ] && [ \"$2\" = view ] && [ \"$3\" != \"--json\" ]; then", + " exit 1", + "fi", + "if [ \"$1\" = pr ] && [ \"$2\" = view ] && [ \"$3\" = \"--json\" ]; then", + " printf '{\"body\":\"\"}\\n'", + " exit 0", + "fi", + "if [ \"$1\" = pr ] && [ \"$2\" = create ]; then", + " prev=''", + " for arg in \"$@\"; do", + " if [ \"$prev\" = \"--body-file\" ] && [ -f \"$arg\" ]; then", + ` cp "$arg" ${JSON.stringify(ghBodyCapture)}`, + " fi", + " prev=\"$arg\"", + " done", + " printf 'https://github.com/org/repo-artifactlink/pull/99\\n'", + " exit 0", + "fi", + "if [ \"$1\" = pr ] && [ \"$2\" = edit ]; then", + " exit 0", + "fi", + `exec /usr/bin/gh "$@" 2>/dev/null || true`, + ].join("\n"); + await fs.writeFile(path.join(fakeBin, "gh"), fakeGhScript, { mode: 0o755 }); + + // Reset cached claude path so this test's fake-bin is used + resetResolvedClaudePath(); + process.env.CLOSEDLOOP_SYMPHONY_TEST_RAW_CLAUDE_PIPELINE = "1"; + process.env.SYMPHONY_WORKTREE_PARENT_DIR = worktreeParent; + process.env.PATH = `${fakeBin}:/usr/bin:/bin`; + + const mock = await startMockApiServer(); + mockServersToClose.push(mock.server); + + const server = new DesktopGatewayServer({ + host: "127.0.0.1", + preferredPort: 0, + fallbackPorts: [0], + webAppOrigin: "https://app.symphony.com", + getAllowedDirectories: () => [tmpDir], + machineName: "execute-artifactlink-machine", + version: "0.1.0-test", + capabilities: EMPTY_CAPABILITIES, + discoveryFilePath: path.join(tmpDir, "electron-port"), + getApiOrigin: () => `http://127.0.0.1:${mock.port}`, + }); + serversToClose.push(server); + await server.start(); + + const loopId = "00000000-0000-0000-0000-000000001000"; + const artifactSlug = "PLAN-42"; + const response = await fetch( + `http://127.0.0.1:${server.getActivePort()}/api/engineer/symphony/loop`, + { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + loopId, + command: "EXECUTE", + closedLoopAuthToken: "tok", + artifacts: [], + artifactSlug, + repo: { fullName: `artifactlink/${path.basename(repoPath)}`, branch: "main" }, + }), + } + ); + + assert.equal( + response.status, + 200, + `Expected 200 but got ${response.status}: ${await response.text().catch(() => "")}` + ); + + // Wait for upload to confirm the flow completed + await mock.waitForRequest("upload-artifacts"); + + // Assert the LLM prompt footer contains /implementation-plans/ and not /artifact/by-slug/ + const capturedPrompt = await fs.readFile(claudePromptCapture, "utf-8").catch(() => ""); + assert.ok( + capturedPrompt.includes("/implementation-plans/"), + `Expected LLM prompt footer to contain /implementation-plans/, got prompt (tail): ${capturedPrompt.slice(-500)}` + ); + assert.ok( + !capturedPrompt.includes("/artifact/by-slug/"), + `Expected LLM prompt to NOT contain /artifact/by-slug/, but it does. Prompt (tail): ${capturedPrompt.slice(-500)}` + ); + + // Assert the SAFETY commit PR body also contains /implementation-plans/ and not /artifact/by-slug/ + const capturedBody = await fs.readFile(ghBodyCapture, "utf-8").catch(() => ""); + assert.ok( + capturedBody.includes("/implementation-plans/"), + `Expected SAFETY PR body to contain /implementation-plans/, got body: ${capturedBody}` + ); + assert.ok( + !capturedBody.includes("/artifact/by-slug/"), + `Expected SAFETY PR body to NOT contain /artifact/by-slug/, but it does. Body: ${capturedBody}` + ); +}); + +// --------------------------------------------------------------------------- +// Test 9 (T-2.3): SAFETY commit PR title format is +// ": Automated changes from loop " +// and does NOT contain the old 'Symphony: EXECUTE' substring. +// --------------------------------------------------------------------------- + +test("EXECUTE: SAFETY commit PR title uses ': Automated changes from loop ' format", async () => { + const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "execute-prtitle-")); + tempPathsToClean.push(tmpDir); + + const repoPath = path.join(tmpDir, "repo-prtitle"); + await initGitRepo(repoPath); + + const worktreeParent = path.join(tmpDir, "worktrees"); + await fs.mkdir(worktreeParent, { recursive: true }); + + process.env.HOME = tmpDir; + + // fake run-loop.sh: write a file so there are changes to commit + await createFakeRunLoopScript( + tmpDir, + [ + "#!/bin/sh", + "echo 'implementation output' > impl.txt", + "exit 0", + ].join("\n") + ); + + const fakeBin = path.join(tmpDir, "fake-bin"); + await fs.mkdir(fakeBin, { recursive: true }); + + // fake claude: exits 0 without execution-result.json so code falls through to + // executeGitOperations (the SAFETY commit path) + await fs.writeFile( + path.join(fakeBin, "claude"), + "#!/bin/sh\nexit 0\n", + { mode: 0o755 } + ); + + // Capture file for the gh pr create --title argument + const ghTitleCapture = path.join(tmpDir, "gh-title-capture.txt"); + + // fake git: stub push; delegate everything else to real git + const fakeGitScript = [ + "#!/bin/sh", + "if [ \"$1\" = push ]; then exit 0; fi", + `exec /usr/bin/git "$@"`, + ].join("\n"); + await fs.writeFile(path.join(fakeBin, "git"), fakeGitScript, { mode: 0o755 }); + + // fake gh: capture --title argument; return a fake PR URL from pr create; + // return non-zero for pr view (no existing PR) so pr create is called; + // return empty body for pr view --json body to skip the footer-update step. + const fakeGhScript = [ + "#!/bin/sh", + "if [ \"$1\" = pr ] && [ \"$2\" = view ] && [ \"$3\" != \"--json\" ]; then", + " exit 1", + "fi", + "if [ \"$1\" = pr ] && [ \"$2\" = view ] && [ \"$3\" = \"--json\" ]; then", + " printf '{\"body\":\"\"}\\n'", + " exit 0", + "fi", + "if [ \"$1\" = pr ] && [ \"$2\" = create ]; then", + " prev=''", + " for arg in \"$@\"; do", + " if [ \"$prev\" = \"--title\" ]; then", + ` printf '%s' "$arg" > ${JSON.stringify(ghTitleCapture)}`, + " fi", + " prev=\"$arg\"", + " done", + " printf 'https://github.com/org/repo-prtitle/pull/55\\n'", + " exit 0", + "fi", + "if [ \"$1\" = pr ] && [ \"$2\" = edit ]; then", + " exit 0", + "fi", + `exec /usr/bin/gh "$@" 2>/dev/null || true`, + ].join("\n"); + await fs.writeFile(path.join(fakeBin, "gh"), fakeGhScript, { mode: 0o755 }); + + resetResolvedClaudePath(); + process.env.CLOSEDLOOP_SYMPHONY_TEST_RAW_CLAUDE_PIPELINE = "1"; + process.env.SYMPHONY_WORKTREE_PARENT_DIR = worktreeParent; + process.env.PATH = `${fakeBin}:/usr/bin:/bin`; + + const mock = await startMockApiServer(); + mockServersToClose.push(mock.server); + + const server = new DesktopGatewayServer({ + host: "127.0.0.1", + preferredPort: 0, + fallbackPorts: [0], + webAppOrigin: "https://app.symphony.com", + getAllowedDirectories: () => [tmpDir], + machineName: "execute-prtitle-machine", + version: "0.1.0-test", + capabilities: EMPTY_CAPABILITIES, + discoveryFilePath: path.join(tmpDir, "electron-port"), + getApiOrigin: () => `http://127.0.0.1:${mock.port}`, + }); + serversToClose.push(server); + await server.start(); + + const loopId = "00000000-0000-0000-0000-000000001100"; + const artifactSlug = "PLAN-55"; + const shortId = loopId.slice(0, 8); // "00000000" + + const response = await fetch( + `http://127.0.0.1:${server.getActivePort()}/api/engineer/symphony/loop`, + { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + loopId, + command: "EXECUTE", + closedLoopAuthToken: "tok", + artifacts: [], + artifactSlug, + repo: { fullName: `prtitle/${path.basename(repoPath)}`, branch: "main" }, + }), + } + ); + + assert.equal( + response.status, + 200, + `Expected 200 but got ${response.status}: ${await response.text().catch(() => "")}` + ); + + // Wait for the upload to confirm git operations completed + await mock.waitForRequest("upload-artifacts"); + + const capturedTitle = await fs.readFile(ghTitleCapture, "utf-8").catch(() => ""); + + // Assert the title matches the expected format: + // ": Automated changes from loop " + const expectedTitle = `${artifactSlug}: Automated changes from loop ${shortId}`; + assert.equal( + capturedTitle, + expectedTitle, + `Expected PR title "${expectedTitle}", got "${capturedTitle}"` + ); + + // Assert the old 'Symphony: EXECUTE' format is NOT used + assert.ok( + !capturedTitle.includes("Symphony: EXECUTE"), + `PR title must not contain 'Symphony: EXECUTE', got: "${capturedTitle}"` + ); +}); + +// --------------------------------------------------------------------------- +// Test 10 (T-3.3): LLM commit spawn correctness +// - spawn uses the resolved absolute binary path (not bare 'claude' string) +// - assertPathAllowed is called before spawn (evidenced by the spawn succeeding +// when worktreeDir is within allowed directories) +// - PID is written atomically (process.pid exists and .pid.tmp is cleaned up) +// --------------------------------------------------------------------------- + +test("EXECUTE: LLM commit spawns claude via resolved absolute path and writes PID atomically", async () => { + const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "execute-llmspawn-")); + tempPathsToClean.push(tmpDir); + + const repoPath = path.join(tmpDir, "repo-llmspawn"); + await initGitRepo(repoPath); + + const worktreeParent = path.join(tmpDir, "worktrees"); + await fs.mkdir(worktreeParent, { recursive: true }); + + process.env.HOME = tmpDir; + + // fake run-loop.sh: exits 0 immediately so attemptLlmCommit is reached + await createFakeRunLoopScript(tmpDir, "#!/bin/sh\nexit 0\n"); + + const fakeBin = path.join(tmpDir, "fake-bin"); + await fs.mkdir(fakeBin, { recursive: true }); + + // Capture paths + const claudeArgvCapture = path.join(tmpDir, "claude-argv-capture.txt"); + const claudeBinaryCapture = path.join(tmpDir, "claude-binary-capture.txt"); + + // fake claude for attemptLlmCommit: + // 1. Writes its own invocation path ($0) to claudeBinaryCapture — this is the + // path that the OS resolved when spawning the binary. If spawn used the + // absolute path it will start with '/'; if it used bare 'claude' it will + // just be 'claude'. + // 2. Writes all args to claudeArgvCapture for inspection. + // 3. Exits 0 without writing execution-result.json (falls through to SAFETY path, + // which is fine — we only care about proving the spawn happened). + const claudeScript = [ + "#!/bin/sh", + `printf '%s' "$0" > ${JSON.stringify(claudeBinaryCapture)}`, + `printf '%s\\n' "$@" > ${JSON.stringify(claudeArgvCapture)}`, + "exit 0", + ].join("\n"); + await fs.writeFile(path.join(fakeBin, "claude"), claudeScript, { mode: 0o755 }); + + // fake git: stub push; pass everything else to real git + const fakeGitScript = [ + "#!/bin/sh", + "if [ \"$1\" = push ]; then exit 0; fi", + `exec /usr/bin/git "$@"`, + ].join("\n"); + await fs.writeFile(path.join(fakeBin, "git"), fakeGitScript, { mode: 0o755 }); + + // fake gh: return non-zero for pr view so SAFETY path tries to create + // but return non-zero for create too — we don't need a real PR since the test + // only asserts on the LLM spawn behaviour (claude exits without result file, + // executeGitOperations runs, git status returns empty because run-loop.sh + // made no changes, so no-changes path is taken — no gh calls needed). + await fs.writeFile( + path.join(fakeBin, "gh"), + "#!/bin/sh\nexit 1\n", + { mode: 0o755 } + ); + + resetResolvedClaudePath(); + process.env.CLOSEDLOOP_SYMPHONY_TEST_RAW_CLAUDE_PIPELINE = "1"; + process.env.SYMPHONY_WORKTREE_PARENT_DIR = worktreeParent; + process.env.PATH = `${fakeBin}:/usr/bin:/bin`; + + const mock = await startMockApiServer(); + mockServersToClose.push(mock.server); + + const jobStore = new JobStore({ cwd: tmpDir, name: "test-jobs-llmspawn" }); + + const server = new DesktopGatewayServer({ + host: "127.0.0.1", + preferredPort: 0, + fallbackPorts: [0], + webAppOrigin: "https://app.symphony.com", + getAllowedDirectories: () => [tmpDir], + machineName: "execute-llmspawn-machine", + version: "0.1.0-test", + capabilities: EMPTY_CAPABILITIES, + discoveryFilePath: path.join(tmpDir, "electron-port"), + getApiOrigin: () => `http://127.0.0.1:${mock.port}`, + jobStore, + }); + serversToClose.push(server); + await server.start(); + + const loopId = "00000000-0000-0000-0000-000000001200"; + const response = await fetch( + `http://127.0.0.1:${server.getActivePort()}/api/engineer/symphony/loop`, + { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + loopId, + command: "EXECUTE", + closedLoopAuthToken: "tok", + artifacts: [], + repo: { fullName: `llmspawn/${path.basename(repoPath)}`, branch: "main" }, + }), + } + ); + + assert.equal( + response.status, + 200, + `Expected 200 but got ${response.status}: ${await response.text().catch(() => "")}` + ); + + // Wait for the upload to confirm the full post-processing pipeline ran + await mock.waitForRequest("upload-artifacts"); + + // --- Assert 1: claude was spawned with the resolved absolute binary path --- + // The fake claude writes $0 (its own path as seen by the OS) to claudeBinaryCapture. + // When spawned via the absolute path the value will be the full path under fakeBin. + // If the code fell back to bare 'claude' it would just be 'claude'. + const capturedBinary = await fs.readFile(claudeBinaryCapture, "utf-8").catch(() => ""); + assert.ok( + capturedBinary.startsWith("/"), + `Expected claude binary path to be absolute (starts with '/'), got: "${capturedBinary}"` + ); + assert.ok( + capturedBinary.includes(fakeBin), + `Expected claude binary path to be under fakeBin (${fakeBin}), got: "${capturedBinary}"` + ); + + // --- Assert 2: spawn received -p as first argument (correct arg format) --- + const capturedArgv = await fs.readFile(claudeArgvCapture, "utf-8").catch(() => ""); + assert.ok( + capturedArgv.startsWith("-p\n"), + `Expected first captured arg to be '-p', got argv (head): "${capturedArgv.slice(0, 100)}"` + ); + + // --- Assert 3: PID written atomically (process.pid exists, .pid.tmp cleaned up) --- + // The PID file is written inside claudeWorkDir = worktreeDir/.claude/work + // We don't know the exact worktreeDir, but we can get it from the job store. + const job = jobStore.getByLoopId(loopId); + assert.ok(job, "Expected job to exist in store after completion"); + + const claudeWorkDir = job!.claudeWorkDir; + assert.ok(claudeWorkDir, "Expected claudeWorkDir to be set on job"); + + const pidFilePath = path.join(claudeWorkDir!, "process.pid"); + const pidTmpPath = path.join(claudeWorkDir!, "process.pid.tmp"); + + // process.pid should exist and contain a numeric PID + const pidContent = await fs.readFile(pidFilePath, "utf-8").catch(() => ""); + assert.ok( + /^\d+$/.test(pidContent.trim()), + `Expected process.pid to contain a numeric PID, got: "${pidContent}"` + ); + + // process.pid.tmp should NOT exist — the atomic rename should have moved it + let tmpExists = false; + try { + await fs.access(pidTmpPath); + tmpExists = true; + } catch { + // Expected: file does not exist + } + assert.ok( + !tmpExists, + `Expected process.pid.tmp to be cleaned up after atomic rename, but it still exists` + ); +}); + // --------------------------------------------------------------------------- // Test 7: Non-zero exit with CANCEL_PENDING — PROCESS_FAILED event skipped // run-loop.sh sleeps then exits with code 1. CANCEL_PENDING is set From 991f5a3ed197fcf77e8a4eba2780aff269a44d5f Mon Sep 17 00:00:00 2001 From: Thadeus Burgess Date: Fri, 27 Mar 2026 16:39:02 -0500 Subject: [PATCH 2/3] PLAN-108: Fix missing initGitRepo import after merge conflict resolution --- apps/desktop/test/symphony-loop-execute.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/apps/desktop/test/symphony-loop-execute.test.ts b/apps/desktop/test/symphony-loop-execute.test.ts index 560bee8f..64fde4ab 100644 --- a/apps/desktop/test/symphony-loop-execute.test.ts +++ b/apps/desktop/test/symphony-loop-execute.test.ts @@ -31,6 +31,7 @@ import type { WorktreeProvider } from "../src/server/operations/symphony-loop.js import { resetShellPathCache } from "../src/server/shell-path.js"; import { createFakeRunLoopScript, + initGitRepo, restoreEnv, saveEnv, startMockApiServer, From 320dfa45121d79fa665235f619f71774b36c1475 Mon Sep 17 00:00:00 2001 From: Thadeus Burgess Date: Fri, 27 Mar 2026 16:41:10 -0500 Subject: [PATCH 3/3] PLAN-108: Add resetShellPathCache() to artifact-link and PR-title tests --- apps/desktop/test/symphony-loop-execute.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/apps/desktop/test/symphony-loop-execute.test.ts b/apps/desktop/test/symphony-loop-execute.test.ts index 64fde4ab..7248d31c 100644 --- a/apps/desktop/test/symphony-loop-execute.test.ts +++ b/apps/desktop/test/symphony-loop-execute.test.ts @@ -1032,8 +1032,9 @@ test("EXECUTE: artifact links use /implementation-plans/ in PR body and LLM prom ].join("\n"); await fs.writeFile(path.join(fakeBin, "gh"), fakeGhScript, { mode: 0o755 }); - // Reset cached claude path so this test's fake-bin is used + // Reset cached claude path and shell PATH so this test's fake-bin is used resetResolvedClaudePath(); + resetShellPathCache(); process.env.CLOSEDLOOP_SYMPHONY_TEST_RAW_CLAUDE_PIPELINE = "1"; process.env.SYMPHONY_WORKTREE_PARENT_DIR = worktreeParent; process.env.PATH = `${fakeBin}:/usr/bin:/bin`; @@ -1187,6 +1188,7 @@ test("EXECUTE: SAFETY commit PR title uses ': Automated changes from loop await fs.writeFile(path.join(fakeBin, "gh"), fakeGhScript, { mode: 0o755 }); resetResolvedClaudePath(); + resetShellPathCache(); process.env.CLOSEDLOOP_SYMPHONY_TEST_RAW_CLAUDE_PIPELINE = "1"; process.env.SYMPHONY_WORKTREE_PARENT_DIR = worktreeParent; process.env.PATH = `${fakeBin}:/usr/bin:/bin`;