diff --git a/apps/desktop/package.json b/apps/desktop/package.json index d7247a74..9b78fbea 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -1,6 +1,6 @@ { "name": "desktop", - "version": "0.9.6", + "version": "0.9.7", "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 acaf14df..2c29c45b 100644 --- a/apps/desktop/src/server/operations/symphony-interactive.ts +++ b/apps/desktop/src/server/operations/symphony-interactive.ts @@ -878,6 +878,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 55b570c2..6285c30d 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 { getShellEnv, getShellPath } from "../shell-path.js"; import { @@ -8,6 +8,7 @@ import { openSync, readFileSync, readSync, + renameSync, statSync, unlinkSync, writeFileSync, @@ -67,6 +68,78 @@ export const defaultWorktreeProvider: WorktreeProvider = { getCurrentBranch: getCurrentBranchImpl, }; +// --------------------------------------------------------------------------- +// 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 // --------------------------------------------------------------------------- @@ -1005,6 +1078,7 @@ async function attemptLlmCommit( artifactSlug: string | undefined, webAppOrigin: string, committer: LoopCommitter | undefined, + getAllowedDirectories: () => string[], onTimeout?: () => void, jobStore?: JobStore, claudeWorkDir?: string, @@ -1019,7 +1093,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}`; @@ -1083,6 +1160,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 = await getShellEnv(); if (committer) { spawnEnv.GIT_AUTHOR_NAME = committer.name; @@ -1091,15 +1184,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; } @@ -1126,10 +1237,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"); } @@ -1138,6 +1253,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; @@ -1148,7 +1269,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 @@ -1233,7 +1354,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); }); @@ -1311,8 +1436,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, @@ -1344,7 +1469,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( @@ -1370,16 +1495,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", @@ -1471,6 +1604,7 @@ async function handleProcessCompletion( claudeWorkDir: string, usedTempDir: boolean, expandedRepoPath: string | null, + getAllowedDirectories: () => string[], jobStore?: JobStore, webAppOrigin?: string, telemetry?: TelemetryEmitter, @@ -1598,6 +1732,7 @@ async function handleProcessCompletion( body.artifactSlug, webAppOrigin ?? "", committer, + getAllowedDirectories, () => { warnings.push( sanitizeErrorMessage("LLM commit timed out after 90s"), @@ -2614,6 +2749,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 027fdba5..7248d31c 100644 --- a/apps/desktop/test/symphony-loop-execute.test.ts +++ b/apps/desktop/test/symphony-loop-execute.test.ts @@ -26,10 +26,12 @@ 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 type { WorktreeProvider } from "../src/server/operations/symphony-loop.js"; import { resetShellPathCache } from "../src/server/shell-path.js"; import { createFakeRunLoopScript, + initGitRepo, restoreEnv, saveEnv, startMockApiServer, @@ -934,6 +936,498 @@ 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 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`; + + 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(); + resetShellPathCache(); + 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