diff --git a/apps/desktop/package.json b/apps/desktop/package.json index 45636ab5..d71f2b83 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -1,6 +1,6 @@ { "name": "desktop", - "version": "0.7.2", + "version": "0.8.1", "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 0a598fb0..c30b9dde 100644 --- a/apps/desktop/src/server/operations/symphony-interactive.ts +++ b/apps/desktop/src/server/operations/symphony-interactive.ts @@ -886,7 +886,7 @@ function resolveWorktreeForComment( return expandedRepoPath; } -function sanitizeCommitMessage(text: string): string { +export function sanitizeCommitMessage(text: string): string { return text .replaceAll(/claude\s*code/gi, "") .replaceAll(/\bopus\b/gi, "") diff --git a/apps/desktop/src/server/operations/symphony-loop.ts b/apps/desktop/src/server/operations/symphony-loop.ts index 8efdd94b..15af888e 100644 --- a/apps/desktop/src/server/operations/symphony-loop.ts +++ b/apps/desktop/src/server/operations/symphony-loop.ts @@ -1,7 +1,7 @@ import { execSync, spawn } from "node:child_process"; import { gatewayLog } from "../../main/gateway-logger.js"; import crypto from "node:crypto"; -import { closeSync, existsSync, openSync, readFileSync } from "node:fs"; +import { closeSync, existsSync, mkdirSync, openSync, readFileSync, unlinkSync, writeFileSync } from "node:fs"; import fs from "node:fs/promises"; import os from "node:os"; import path from "node:path"; @@ -17,6 +17,7 @@ import { readEvaluatePrdOutputs, writePrdArtifact, } from "./symphony-prd-artifacts.js"; +import { sanitizeCommitMessage } from "./symphony-interactive.js"; import { expandHome, resolveWorktreeParentDir, @@ -88,6 +89,31 @@ interface LoopRequestBody { localRepoPath?: string; } +interface ExecutionResult { + prUrl: string; + prNumber: number; + branchName: string; + commitSha: string; +} + +function isExecutionResult(value: unknown): value is ExecutionResult { + if (typeof value !== "object" || value === null) return false; + const v = value as Record; + if ( + typeof v.prUrl !== "string" || + typeof v.prNumber !== "number" || + typeof v.branchName !== "string" || + typeof v.commitSha !== "string" + ) { + return false; + } + // Sanity-check field shapes to reject garbage values from the LLM + if (!/^https?:\/\//.test(v.prUrl)) return false; + if (!/^[a-f0-9]{7,}$/i.test(v.commitSha)) return false; + if (!v.branchName.trim()) return false; + return true; +} + /** Track running loop processes for cancellation and to prevent GC of ChildProcess. */ interface RunningLoop { pid: number; @@ -196,6 +222,7 @@ function buildClaudePipeline( } return { cmd: "claude", args: claudeArgs }; } + /** Find the local repo path for a given fullName (e.g. "org/repo"). */ function findLocalRepo( fullName: string, @@ -640,6 +667,205 @@ function parseTokenUsage(claudeWorkDir: string): { input: number; output: number return totals; } +// --------------------------------------------------------------------------- +// LLM-assisted commit (EXECUTE only) +// --------------------------------------------------------------------------- + +async function attemptLlmCommit( + worktreeDir: string, + baseBranch: string, + loopId: string, + command: string, + artifactSlug: string | undefined, + webAppOrigin: string, + committer: LoopCommitter | undefined +): Promise { + // Build metadata footer for PR body + // Strip newlines from user-controlled fields to prevent prompt injection + const safeBranch = baseBranch.replace(/[\r\n]/g, ''); + const safeLoopId = sanitizeCommitMessage(loopId).replace(/[\r\n]/g, ''); + let footer: string; + if (artifactSlug) { + const safeSlug = sanitizeCommitMessage(artifactSlug).replace(/[\r\n]/g, ''); + const artifactLink = `${webAppOrigin}/artifact/by-slug/${safeSlug}`; + footer = `---\nLoop ID: ${safeLoopId}\nArtifact: ${artifactLink}`; + } else { + footer = `---\nLoop ID: ${safeLoopId}`; + } + + // Build slug instruction for the prompt + const slugInstruction = artifactSlug + ? `The artifact slug is ${sanitizeCommitMessage(artifactSlug).replace(/[\r\n]/g, '')}. ` + + `You MUST prefix the PR title with "${sanitizeCommitMessage(artifactSlug).replace(/[\r\n]/g, '')}: " ` + + `(e.g., "${sanitizeCommitMessage(artifactSlug).replace(/[\r\n]/g, '')}: Add feature X"). ` + + `Also prefix the commit message the same way.` + : "No artifact slug is available — use a descriptive title without a prefix."; + + const prompt = [ + `You are a commit assistant finalizing work from a Symphony ${command} loop.`, + "", + slugInstruction, + "", + "Review all uncommitted changes in this repository and create a proper commit, push it, and create a pull request.", + "", + "STEPS:", + "1. Run `git status` and `git diff --stat` to understand what changed", + "2. Stage all changed/new files EXCEPT the .claude/ directory:", + " git add -- . ':!.claude/'", + "3. Write a clear, descriptive commit message based on the actual code changes", + " - Summarize WHAT changed and WHY (not just 'Symphony loop output')", + " - Use conventional commit style if the changes have a clear category", + " - If an artifact slug is provided, prefix the commit message with it", + "4. Run `git commit` (do NOT use --no-verify). If pre-commit hooks fail, attempt to fix", + " the issue (e.g., run the linter/formatter if the error message tells you how).", + " If you cannot quickly fix it, the commit fails — do not bypass hooks.", + "5. Push to origin with: git push -u origin HEAD", + "6. Check if a PR already exists for this branch: gh pr list --head ", + " - If NO PR exists:", + " a. Write a file called pr-body.md with:", + " - A summary section describing what changed and why (2-4 sentences)", + " - Then the following metadata footer on its own lines:", + ` ${footer}`, + ` b. Create the PR: gh pr create --label symphony --base ${shellEscape(safeBranch)} --title '' --body-file pr-body.md`, + " - If a PR already exists, get its URL with: gh pr view --json url,number", + ` Then ensure the metadata footer is present: write pr-body.md with the footer above and run gh pr edit --body-file pr-body.md`, + "7. ONLY after a successful commit AND push, write this EXACT JSON file:", + " File path: execution-result.json", + " ```json", + " {", + ' "prUrl": "",', + ' "prNumber": ,', + ' "branchName": "",', + ' "commitSha": ""', + " }", + " ```", + " Run `git rev-parse HEAD` to get the commit SHA.", + "", + "RULES:", + "- NEVER stage or commit the .claude/ directory", + "- Do NOT use --no-verify on git commit", + "- Do NOT modify any source code except to fix pre-commit hook failures (formatting, lint)", + "- Do NOT write execution-result.json unless you successfully committed AND pushed", + "- Keep it quick — commit, push, PR, write result file, done", + ].join("\n"); + + loopLog(loopId, "Attempting LLM-assisted commit..."); + + const spawnEnv: Record = { ...process.env } as Record; + if (committer) { + spawnEnv.GIT_AUTHOR_NAME = committer.name; + spawnEnv.GIT_AUTHOR_EMAIL = committer.email; + spawnEnv.GIT_COMMITTER_NAME = committer.name; + spawnEnv.GIT_COMMITTER_EMAIL = committer.email; + } + + let child: ReturnType; + try { + child = spawn( + "claude", + ["-p", prompt, "--allowedTools", "Bash,Read,Write,Glob,Grep"], + { cwd: worktreeDir, detached: true, stdio: "pipe", env: spawnEnv } + ); + } catch (err) { + loopError(loopId, "LLM commit spawn failed:", err); + return null; + } + + const pid = child.pid ?? null; + if (!pid) { + loopError(loopId, "LLM commit: spawn returned no PID"); + return null; + } + + return new Promise((resolve) => { + let killed = false; + + const killTimer = setTimeout(() => { + if (!killed) { + killed = true; + loopError(loopId, "LLM commit timed out after 90s — sending SIGTERM"); + try { + process.kill(-pid, "SIGTERM"); + } catch (killErr) { + loopError(loopId, "Failed to kill LLM commit process:", killErr); + } + // Escalate to SIGKILL after 5s if process survives SIGTERM + setTimeout(() => { + try { + process.kill(pid, 0); // check alive + process.kill(-pid, "SIGKILL"); + } catch { + // Already gone + } + }, 5_000); + } + }, 90_000); + + const stdoutChunks: Buffer[] = []; + const stderrChunks: Buffer[] = []; + + child.stdout?.on("data", (chunk: Buffer) => { + stdoutChunks.push(chunk); + }); + + child.stderr?.on("data", (chunk: Buffer) => { + stderrChunks.push(chunk); + }); + + child.on("close", (code: number | null) => { + clearTimeout(killTimer); + + const stdout = Buffer.concat(stdoutChunks).toString("utf-8"); + const stderr = Buffer.concat(stderrChunks).toString("utf-8"); + if (stdout) { + loopLog(loopId, `LLM commit stdout (tail): ${stdout.slice(-2000)}`); + } + if (stderr) { + loopLog(loopId, `LLM commit stderr (tail): ${stderr.slice(-1000)}`); + } + + // code is null when the process was killed by a signal + if (killed || code == null || code !== 0) { + loopError(loopId, `LLM commit exited with code ${code ?? "killed"}`); + resolve(null); + return; + } + + // Read execution-result.json written by the LLM, then clean up scratch + // files unconditionally so they never leak into subsequent worktree runs. + const resultFilePath = path.join(worktreeDir, "execution-result.json"); + const prBodyFilePath = path.join(worktreeDir, "pr-body.md"); + let result: ExecutionResult | null = null; + try { + const raw = readFileSync(resultFilePath, "utf-8"); + const parsed: unknown = JSON.parse(raw); + if (isExecutionResult(parsed)) { + loopLog(loopId, `LLM commit wrote execution-result.json, pr=${parsed.prUrl}`); + result = parsed; + } else { + loopError(loopId, "LLM execution-result.json failed type guard, returning null"); + } + } catch (err) { + loopError(loopId, "LLM commit: failed to read execution-result.json:", err); + } + // Always remove LLM scratch files from the worktree + try { unlinkSync(resultFilePath); } catch { /* may not exist */ } + try { unlinkSync(prBodyFilePath); } catch { /* may not exist */ } + resolve(result); + }); + + child.on("error", (err: Error) => { + clearTimeout(killTimer); + loopError(loopId, "LLM commit process error:", err); + resolve(null); + }); + + // unref AFTER event listeners are attached so the ChildProcess handle + // is not garbage-collected before exit/error events fire. + child.unref(); + }); +} + // --------------------------------------------------------------------------- // Git operations (EXECUTE only) // --------------------------------------------------------------------------- @@ -647,8 +873,13 @@ function parseTokenUsage(claudeWorkDir: string): { input: number; output: number function executeGitOperations( worktreeDir: string, committer: LoopCommitter | undefined, - baseBranch: string + baseBranch: string, + loopId: string, + command: string, + artifactSlug?: string, + webAppOrigin?: string ): { prUrl: string; prNumber: number; branchName: string; commitSha: string } | null { + const shortId = loopId.slice(0, 8); const env: Record = { ...process.env } as Record; if (committer) { env.GIT_AUTHOR_NAME = committer.name; @@ -675,14 +906,16 @@ function executeGitOperations( // Stage, commit, push try { - execSync("git add -A", { + execSync("git add -- . ':!.claude/'", { cwd: worktreeDir, stdio: "pipe", env, timeout: 10_000, }); - execSync('git commit -m "Symphony: implement plan"', { + const commitPrefix = artifactSlug ? `${artifactSlug}: ` : ""; + const commitMessage = `${commitPrefix}Symphony: ${command} -- loop ${shortId}`; + execSync(`git commit -m ${shellEscape(commitMessage)}`, { cwd: worktreeDir, stdio: "pipe", env, @@ -710,6 +943,16 @@ function executeGitOperations( timeout: 10_000, }).trim(); + // Build PR body with metadata footer, written to a temp file to avoid + // shell escaping issues with special characters (--body-file approach). + const artifactLine = artifactSlug && webAppOrigin + ? `\nArtifact: ${webAppOrigin}/artifact/by-slug/${artifactSlug}` + : ""; + const prBody = `Loop ID: ${loopId}\nCommand: ${command}${artifactLine}`; + const bodyFile = path.join(worktreeDir, ".claude", "work", "pr-body.md"); + mkdirSync(path.dirname(bodyFile), { recursive: true }); + writeFileSync(bodyFile, prBody); + // Check for existing PR before creating (handles retries gracefully) let prUrl: string; let prNumber: number; @@ -728,9 +971,12 @@ function executeGitOperations( prUrl = parsed.url; prNumber = parsed.number; } catch { - // No existing PR — create one + // 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 "Symphony: implement plan" --body "Automated PR from Symphony loop" --base ${shellEscape(baseBranch)}`, + `gh pr create --title ${shellEscape(prTitle)} --body-file ${shellEscape(bodyFile)} --base ${shellEscape(baseBranch)}`, { cwd: worktreeDir, encoding: "utf-8", @@ -742,6 +988,42 @@ function executeGitOperations( prUrl = prOutput; const prNumberMatch = /\/pull\/(\d+)/.exec(prUrl); prNumber = prNumberMatch ? Number.parseInt(prNumberMatch[1], 10) : 0; + + // Best-effort label attachment — non-fatal if the label doesn't exist + if (prNumber) { + try { + execSync(`gh pr edit ${prNumber} --add-label symphony`, { + cwd: worktreeDir, + stdio: "pipe", + env, + timeout: 15_000, + }); + } catch { + // Label may not exist on this repo — not critical + } + } + } + + // Ensure the metadata footer is present on the PR body. For existing PRs, + // fetch the current body and append the metadata instead of replacing it. + try { + const currentBody = execSync( + `gh pr view ${prNumber} --json body --jq .body`, + { cwd: worktreeDir, encoding: "utf-8", stdio: "pipe", env, timeout: 15_000 } + ).trim(); + // Only update if the footer isn't already present + if (!currentBody.includes(`Loop ID: ${loopId}`)) { + const updatedBody = currentBody + ? `${currentBody}\n\n---\n${prBody}` + : prBody; + writeFileSync(bodyFile, updatedBody); + execSync( + `gh pr edit ${prNumber} --body-file ${shellEscape(bodyFile)}`, + { cwd: worktreeDir, stdio: "pipe", env, timeout: 15_000 } + ); + } + } catch { + // Non-critical — PR exists, metadata is best-effort } return { prUrl, prNumber, branchName, commitSha }; @@ -763,7 +1045,8 @@ async function handleProcessCompletion( claudeWorkDir: string, usedTempDir: boolean, expandedRepoPath: string | null, - jobStore?: JobStore + jobStore?: JobStore, + webAppOrigin?: string ): Promise { const { loopId, command, closedLoopAuthToken, committer } = body; @@ -814,11 +1097,31 @@ async function handleProcessCompletion( // Git operations for EXECUTE if (worktreeDir) { const baseBranch = body.repo?.branch ?? "main"; - const gitResult = executeGitOperations( + + // Try LLM-assisted commit first; fall back to executeGitOperations if it + // returns null. Never call both. + const llmResult = await attemptLlmCommit( worktreeDir, - committer, - baseBranch + baseBranch, + loopId, + command, + body.artifactSlug, + webAppOrigin ?? "", + committer ); + + // Clean up any remaining LLM scratch files before fallback to prevent + // them from being committed by executeGitOperations. attemptLlmCommit + // already cleans up on success, but these guards cover edge cases where + // the process was killed before the cleanup ran. + if (!llmResult) { + try { unlinkSync(path.join(worktreeDir, 'execution-result.json')); } catch { /* may not exist */ } + try { unlinkSync(path.join(worktreeDir, 'pr-body.md')); } catch { /* may not exist */ } + } + + const gitResult: { prUrl: string; prNumber: number; branchName: string; commitSha: string } | null = + llmResult ?? executeGitOperations(worktreeDir, committer, baseBranch, loopId, command, body.artifactSlug, webAppOrigin ?? ""); + if (gitResult) { // Merge git info into execution result const execResult = @@ -939,7 +1242,8 @@ async function handleLoopRequest( context: OperationRequestContext, getAllowedDirectories: () => string[], getApiOrigin?: () => string, - jobStore?: JobStore + jobStore?: JobStore, + getWebAppOrigin?: () => string ): Promise { // Derive the callback URL from the gateway's trusted configuration. // body.apiBaseUrl is ignored -- the caller does not control where @@ -949,6 +1253,7 @@ async function handleLoopRequest( json(context, 503, { error: "API origin not configured" }); return; } + const webAppOrigin = getWebAppOrigin?.() ?? ''; const rawBody = parseJsonBody(context); if (!rawBody) { @@ -1067,24 +1372,14 @@ async function handleLoopRequest( let claudeWorkDir: string; let usedTempDir = false; - if (body.command === "DECOMPOSE") { - // DECOMPOSE: use temp dir, no worktree needed - usedTempDir = true; - const tmpDir = path.join( - os.tmpdir(), - `symphony-decompose-${body.loopId.slice(0, 8)}` - ); - await fs.rm(tmpDir, { recursive: true, force: true }); - await fs.mkdir(tmpDir, { recursive: true }); - claudeWorkDir = tmpDir; - await writePrdArtifact(claudeWorkDir, body.artifacts, body.prompt); - } else if (body.command === "EVALUATE_PRD") { - // EVALUATE_PRD: use temp dir, no worktree needed. + if (body.command === "DECOMPOSE" || body.command === "EVALUATE_PRD") { + // DECOMPOSE and EVALUATE_PRD: use temp dir, no worktree needed. // Temp dir is intentionally exempt from assertPathAllowed. usedTempDir = true; + const label = body.command === "DECOMPOSE" ? "decompose" : "evaluate-prd"; const tmpDir = path.join( os.tmpdir(), - `symphony-evaluate-prd-${body.loopId.slice(0, 8)}` + `symphony-${label}-${body.loopId.slice(0, 8)}` ); await fs.rm(tmpDir, { recursive: true, force: true }); await fs.mkdir(tmpDir, { recursive: true }); @@ -1096,13 +1391,9 @@ async function handleLoopRequest( }); return; } else if (body.command === "PLAN" || body.command === "EXECUTE" || body.command === "REQUEST_CHANGES") { - const repoPath = expandedRepoPath; - if (!repoPath) { - json(context, 400, { - error: "Repository required for PLAN, EXECUTE, and REQUEST_CHANGES commands", - }); - return; - } + // expandedRepoPath is guaranteed non-null here: the repoRequirement === "REQUIRED" + // guard above already returned 400 when it was missing. + const repoPath = expandedRepoPath!; // Worktree keyed by artifact slug (e.g., symphony/PLAN-5). // PLAN always creates fresh; EXECUTE/REQUEST_CHANGES reuse. @@ -1202,13 +1493,9 @@ async function handleLoopRequest( ); } } else if (body.command === "GENERATE_PRD") { - const repoPath = expandedRepoPath; - if (!repoPath) { - json(context, 400, { - error: "Repository required for GENERATE_PRD command", - }); - return; - } + // expandedRepoPath is guaranteed non-null here: the repoRequirement === "REQUIRED" + // guard above already returned 400 when it was missing. + const repoPath = expandedRepoPath!; // Use a dedicated branch namespace to avoid collisions with PLAN/EXECUTE worktrees. // GENERATE_PRD always starts fresh -- it must not inherit a prior PLAN worktree. @@ -1259,8 +1546,14 @@ async function handleLoopRequest( return; } - const cleanupTempClaudeWorkDir = (): void => { - fs.rm(claudeWorkDir, { recursive: true, force: true }).catch(() => {}); + /** Clean up temporary resources on early-return error paths. */ + const cleanupOnError = async (): Promise => { + if (usedTempDir) { + await fs.rm(claudeWorkDir, { recursive: true, force: true }).catch(() => {}); + } + if (body.command === "GENERATE_PRD" && worktreeDir && expandedRepoPath) { + await cleanupGeneratePrdWorktree(worktreeDir, expandedRepoPath, body.loopId); + } }; // Pre-flight: verify required binary exists BEFORE posting 'started' event. @@ -1287,12 +1580,7 @@ async function handleLoopRequest( message: "claude CLI not found in PATH", } ); - if (usedTempDir) { - cleanupTempClaudeWorkDir(); - } - if (body.command === "GENERATE_PRD" && worktreeDir && expandedRepoPath) { - await cleanupGeneratePrdWorktree(worktreeDir, expandedRepoPath, body.loopId); - } + await cleanupOnError(); json(context, 500, { error: "claude CLI not found in PATH" }); return; } @@ -1335,12 +1623,7 @@ async function handleLoopRequest( code: "SPAWN_FAILED", message: `Cannot open log file: ${msg}`, }); - if (usedTempDir) { - cleanupTempClaudeWorkDir(); - } - if (body.command === "GENERATE_PRD" && worktreeDir && expandedRepoPath) { - await cleanupGeneratePrdWorktree(worktreeDir, expandedRepoPath, body.loopId); - } + await cleanupOnError(); json(context, 500, { error: `Cannot open log file: ${msg}` }); return; } @@ -1353,6 +1636,18 @@ async function handleLoopRequest( PATH: `${process.env.PATH}:/opt/homebrew/bin:/usr/local/bin`, }; + // Shared claude CLI args for commands that run claude directly. + // REQUEST_CHANGES omits "-" (stdin) because it passes the prompt as a CLI argument. + const baseClaudeArgs: string[] = [ + "-p", + "--output-format", "stream-json", + "--verbose", + "--allowedTools", + "Bash,Glob,Grep,Read,Write,Edit,Task,Skill,SlashCommand,TodoWrite", + "--max-turns", "200", + ]; + const stdinClaudeArgs = ["-p", "-", ...baseClaudeArgs.slice(1)]; + if (body.command === "DECOMPOSE") { // DECOMPOSE: write prompt to file and pass via stdin to avoid E2BIG const prdContent = readTextFile(path.join(claudeWorkDir, "prd.md")) ?? ""; @@ -1360,15 +1655,7 @@ async function handleLoopRequest( const promptFile = path.join(claudeWorkDir, "decompose-prompt.txt"); await fs.writeFile(promptFile, decomposePrompt); - const claudeArgs = [ - "-p", "-", - "--output-format", "stream-json", - "--verbose", - "--allowedTools", - "Bash,Glob,Grep,Read,Write,Edit,Task,Skill,SlashCommand,TodoWrite", - "--max-turns", "200", - ]; - const pipeline = buildClaudePipeline(claudeArgs, claudeWorkDir, promptFile); + const pipeline = buildClaudePipeline(stdinClaudeArgs, claudeWorkDir, promptFile); child = spawn(pipeline.cmd, pipeline.args, { cwd: claudeWorkDir, detached: true, @@ -1385,15 +1672,8 @@ async function handleLoopRequest( } const promptFile = path.join(claudeWorkDir, "evaluate-prd-prompt.txt"); await fs.writeFile(promptFile, evaluatePrdPrompt); - const claudeArgs = [ - "-p", "-", - "--output-format", "stream-json", - "--verbose", - "--allowedTools", - "Bash,Glob,Grep,Read,Write,Edit,Task,Skill,SlashCommand,TodoWrite", - "--max-turns", "200", - ]; - const pipeline = buildClaudePipeline(claudeArgs, claudeWorkDir, promptFile); + + const pipeline = buildClaudePipeline(stdinClaudeArgs, claudeWorkDir, promptFile); child = spawn(pipeline.cmd, pipeline.args, { cwd: claudeWorkDir, detached: true, @@ -1405,14 +1685,7 @@ async function handleLoopRequest( // REQUEST_CHANGES: use claude directly with /code:amend-plan. // Must use -p (headless mode) so --allowedTools grants full permission // without prompting. Pipes through stream_formatter.py for readable logs. - const claudeArgs: string[] = [ - "-p", - "--output-format", "stream-json", - "--verbose", - "--allowedTools", - "Bash,Glob,Grep,Read,Write,Edit,Task,Skill,SlashCommand,TodoWrite", - "--max-turns", "200", - ]; + const claudeArgs = [...baseClaudeArgs]; // Resume from parent session if available (matches harness --resume) if (body.parentSessionId) { @@ -1446,15 +1719,7 @@ async function handleLoopRequest( const promptFile = path.join(claudeWorkDir, "generate-prd-prompt.txt"); await fs.writeFile(promptFile, body.prompt!); - const claudeArgs = [ - "-p", "-", - "--output-format", "stream-json", - "--verbose", - "--allowedTools", - "Bash,Glob,Grep,Read,Write,Edit,Task,Skill,SlashCommand,TodoWrite", - "--max-turns", "200", - ]; - const pipeline = buildClaudePipeline(claudeArgs, claudeWorkDir, promptFile); + const pipeline = buildClaudePipeline(stdinClaudeArgs, claudeWorkDir, promptFile); child = spawn(pipeline.cmd, pipeline.args, { cwd: worktreeDir!, detached: true, @@ -1494,12 +1759,7 @@ async function handleLoopRequest( code: "SPAWN_FAILED", message: msg, }); - if (usedTempDir) { - cleanupTempClaudeWorkDir(); - } - if (body.command === "GENERATE_PRD" && worktreeDir && expandedRepoPath) { - await cleanupGeneratePrdWorktree(worktreeDir, expandedRepoPath, body.loopId); - } + await cleanupOnError(); json(context, 500, { error: `Failed to spawn process: ${msg}` }); return; } @@ -1521,7 +1781,8 @@ async function handleLoopRequest( claudeWorkDir, usedTempDir, expandedRepoPath, - jobStore + jobStore, + webAppOrigin ).catch((err) => { loopError(body.loopId, "Completion handler error:", err); gatewayLog.error("loop-harness", `Completion handler error for loopId=${body.loopId}: ${err instanceof Error ? err.message : err}`); @@ -1660,13 +1921,14 @@ export function registerSymphonyLoopRoutes( dispatcher: OperationDispatcher, getAllowedDirectories: () => string[], getApiOrigin?: () => string, - jobStore?: JobStore + jobStore?: JobStore, + getWebAppOrigin?: () => string ): void { dispatcher.register( "POST", "/api/engineer/symphony/loop", async (context) => { - await handleLoopRequest(context, getAllowedDirectories, getApiOrigin, jobStore); + await handleLoopRequest(context, getAllowedDirectories, getApiOrigin, jobStore, getWebAppOrigin); } ); diff --git a/apps/desktop/src/server/router.ts b/apps/desktop/src/server/router.ts index 5b619f05..93faae73 100644 --- a/apps/desktop/src/server/router.ts +++ b/apps/desktop/src/server/router.ts @@ -155,7 +155,8 @@ export class GatewayRouter { this.operationDispatcher, this.options.getAllowedDirectories, this.options.getApiOrigin, - this.options.jobStore + this.options.jobStore, + this.options.getWebAppOrigin ?? (() => this.options.webAppOrigin) ); registerSymphonyLogsRoutes(this.operationDispatcher, this.options.getAllowedDirectories); registerSymphonyPlanRoutes(this.operationDispatcher, this.options.getAllowedDirectories); diff --git a/apps/desktop/test/symphony-loop-execute.test.ts b/apps/desktop/test/symphony-loop-execute.test.ts new file mode 100644 index 00000000..5133febd --- /dev/null +++ b/apps/desktop/test/symphony-loop-execute.test.ts @@ -0,0 +1,554 @@ +/** + * Integration tests for the EXECUTE loop command, specifically: + * + * T-5.1: No-changes paths + * - executeGitOperations returns null when git status --porcelain is empty + * - attemptLlmCommit returns null when claude exits 0 without writing execution-result.json + * + * T-5.2: Existing-PR paths + * - executeGitOperations returns existing PR URL when gh pr view succeeds (no gh pr create) + * - handleProcessCompletion returns PR URL from pre-written execution-result.json + * without calling executeGitOperations + * + * Tests go through the HTTP gateway, not direct function calls. + * Fake binaries (run-loop.sh, claude, git, gh) are placed in a temp fake-bin/ dir + * prepended to PATH. CLOSEDLOOP_SYMPHONY_TEST_RAW_CLAUDE_PIPELINE=1 disables the + * stream_formatter pipeline so the fake claude can emit simple output. + */ + +import assert from "node:assert/strict"; +import { execFile, execSync } from "node:child_process"; +import fs from "node:fs/promises"; +import http from "node:http"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, test } from "node:test"; +import { promisify } from "node:util"; +import { DesktopGatewayServer } from "../src/server/server.js"; +import { EMPTY_CAPABILITIES, PORT_PROBE_ORDER } from "../src/shared/contracts.js"; + +const execFileAsync = promisify(execFile); + +// --------------------------------------------------------------------------- +// Shared state and cleanup +// --------------------------------------------------------------------------- + +const serversToClose: DesktopGatewayServer[] = []; +const mockServersToClose: http.Server[] = []; +const tempPathsToClean: string[] = []; + +const originalSymphonyWorktreeParentDir = process.env.SYMPHONY_WORKTREE_PARENT_DIR; +const originalPath = process.env.PATH; +const originalHome = process.env.HOME; +const originalRawPipeline = process.env.CLOSEDLOOP_SYMPHONY_TEST_RAW_CLAUDE_PIPELINE; + +afterEach(async () => { + if (originalSymphonyWorktreeParentDir === undefined) { + delete process.env.SYMPHONY_WORKTREE_PARENT_DIR; + } else { + process.env.SYMPHONY_WORKTREE_PARENT_DIR = originalSymphonyWorktreeParentDir; + } + + if (originalPath === undefined) { + delete process.env.PATH; + } else { + process.env.PATH = originalPath; + } + + if (originalHome === undefined) { + delete process.env.HOME; + } else { + process.env.HOME = originalHome; + } + + if (originalRawPipeline === undefined) { + delete process.env.CLOSEDLOOP_SYMPHONY_TEST_RAW_CLAUDE_PIPELINE; + } else { + process.env.CLOSEDLOOP_SYMPHONY_TEST_RAW_CLAUDE_PIPELINE = originalRawPipeline; + } + + for (const server of serversToClose.splice(0)) { + await server.stop(); + } + + for (const ms of mockServersToClose.splice(0)) { + await new Promise((resolve, reject) => { + ms.close((err) => (err ? reject(err) : resolve())); + }); + } + + for (const tempPath of tempPathsToClean.splice(0)) { + await fs.rm(tempPath, { recursive: true, force: true }); + } +}); + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +async function initGitRepo(repoPath: string): Promise { + await execFileAsync("git", ["init", "-b", "main", repoPath]); + await execFileAsync("git", ["-C", repoPath, "config", "user.email", "test@test.com"]); + await execFileAsync("git", ["-C", repoPath, "config", "user.name", "Test"]); + await fs.writeFile(path.join(repoPath, "README.md"), "# initial\n"); + await execFileAsync("git", ["-C", repoPath, "add", "."]); + await execFileAsync("git", ["-C", repoPath, "commit", "-m", "initial"]); +} + +type RecordedRequest = { method: string; url: string; body: string }; + +async function startMockApiServer(): Promise<{ + server: http.Server; + port: number; + requests: RecordedRequest[]; + waitForRequest: (urlSubstring: string, timeoutMs?: number) => Promise; +}> { + const requests: RecordedRequest[] = []; + const waiters: Array<{ urlSubstring: string; resolve: (r: RecordedRequest) => void }> = []; + + const server = http.createServer((req, res) => { + void (async () => { + const chunks: Buffer[] = []; + for await (const chunk of req) { + chunks.push(typeof chunk === "string" ? Buffer.from(chunk) : chunk); + } + const recorded: RecordedRequest = { + method: req.method ?? "", + url: req.url ?? "", + body: Buffer.concat(chunks).toString("utf-8"), + }; + requests.push(recorded); + + for (let i = waiters.length - 1; i >= 0; i--) { + if (recorded.url.includes(waiters[i].urlSubstring)) { + waiters[i].resolve(recorded); + waiters.splice(i, 1); + } + } + + res.statusCode = 200; + res.setHeader("content-type", "application/json"); + res.end(JSON.stringify({ success: true })); + })(); + }); + + await new Promise((resolve, reject) => { + server.listen(0, "127.0.0.1", () => resolve()); + server.once("error", reject); + }); + + const address = server.address(); + if (!address || typeof address === "string") { + throw new Error("failed to bind mock API server"); + } + + function waitForRequest(urlSubstring: string, timeoutMs = 20_000): Promise { + const existing = requests.find((r) => r.url.includes(urlSubstring)); + if (existing) { + return Promise.resolve(existing); + } + return new Promise((resolve, reject) => { + const timer = setTimeout(() => { + reject( + new Error( + `Timed out waiting for request matching "${urlSubstring}" after ${timeoutMs}ms` + ) + ); + }, timeoutMs); + waiters.push({ + urlSubstring, + resolve: (r) => { + clearTimeout(timer); + resolve(r); + }, + }); + }); + } + + return { server, port: address.port, requests, waitForRequest }; +} + +/** + * Create the fake plugin cache structure so findPluginScript("code", "run-loop.sh") + * finds the provided script content. + */ +async function createFakeRunLoopScript(homeDir: string, scriptContent: string): Promise { + const scriptDir = path.join( + homeDir, + ".claude", + "plugins", + "cache", + "closedloop-ai", + "code", + "1.0.0", + "scripts" + ); + await fs.mkdir(scriptDir, { recursive: true }); + const scriptPath = path.join(scriptDir, "run-loop.sh"); + await fs.writeFile(scriptPath, scriptContent, { mode: 0o755 }); + return scriptPath; +} + +// --------------------------------------------------------------------------- +// Test 1: No-changes → executeGitOperations returns null (no PR URL in upload) +// --------------------------------------------------------------------------- + +test("EXECUTE: no PR URL in upload when worktree has no changes (git status empty)", async () => { + const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "execute-nochange-")); + tempPathsToClean.push(tmpDir); + + // Use real git to initialise repo before we point HOME at tmpDir + const repoPath = path.join(tmpDir, "repo-nochange"); + await initGitRepo(repoPath); + + const worktreeParent = path.join(tmpDir, "worktrees"); + await fs.mkdir(worktreeParent, { recursive: true }); + + // Redirect HOME so getPluginCacheRoot() returns a path we control + process.env.HOME = tmpDir; + + // fake run-loop.sh: exits 0 without making any changes + await createFakeRunLoopScript(tmpDir, "#!/bin/sh\nexit 0\n"); + + // fake-bin: claude that exits 0 without writing execution-result.json + // (simulates attemptLlmCommit finding no result file → returns null) + const fakeBin = path.join(tmpDir, "fake-bin"); + await fs.mkdir(fakeBin, { recursive: true }); + await fs.writeFile( + path.join(fakeBin, "claude"), + "#!/bin/sh\nexit 0\n", + { mode: 0o755 } + ); + + // Disable stream_formatter pipeline — fake claude output is not a real stream + 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: PORT_PROBE_ORDER[0], + fallbackPorts: PORT_PROBE_ORDER.slice(1), + webAppOrigin: "https://app.symphony.com", + getAllowedDirectories: () => [tmpDir], + machineName: "execute-nochange-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-000000000100"; + 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: `nochange/${path.basename(repoPath)}`, branch: "main" }, + }), + } + ); + + assert.equal( + response.status, + 200, + `Expected 200 but got ${response.status}: ${await response.text().catch(() => "")}` + ); + + // Wait for the upload call that signals process completion + const uploadReq = await mock.waitForRequest("upload-artifacts"); + const uploadBody = JSON.parse(uploadReq.body) as { + artifacts: { + executionResult?: { + pr_url?: string; + has_changes?: boolean; + }; + }; + metadata: Record; + }; + + // No changes → no PR URL in execution result + assert.equal( + uploadBody.artifacts.executionResult?.pr_url, + undefined, + `Expected no pr_url when there are no changes, got: ${uploadBody.artifacts.executionResult?.pr_url}` + ); + assert.equal( + uploadBody.artifacts.executionResult?.has_changes, + undefined, + "Expected has_changes to be absent when there are no changes" + ); +}); + +// --------------------------------------------------------------------------- +// Test 2: Pre-written execution-result.json (LLM path) → PR URL without +// calling executeGitOperations +// --------------------------------------------------------------------------- + +test("EXECUTE: handleProcessCompletion reads pre-written execution-result.json and returns PR URL", async () => { + const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "execute-llmresult-")); + tempPathsToClean.push(tmpDir); + + const repoPath = path.join(tmpDir, "repo-llmresult"); + 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 without making any changes + // (attemptLlmCommit is called after this exits) + await createFakeRunLoopScript(tmpDir, "#!/bin/sh\nexit 0\n"); + + const fakeBin = path.join(tmpDir, "fake-bin"); + await fs.mkdir(fakeBin, { recursive: true }); + + // fake claude for attemptLlmCommit: writes a valid execution-result.json to $CLOSEDLOOP_WORKDIR + // Then exits 0. attemptLlmCommit reads the file and returns the result. + // Because execution-result.json is present and valid, executeGitOperations is never called. + // + // The worktree dir is the cwd when attemptLlmCommit spawns claude. + // execution-result.json is expected at path.join(worktreeDir, "execution-result.json"). + const expectedPrUrl = "https://github.com/org/repo-llmresult/pull/77"; + const executionResultContent = JSON.stringify({ + prUrl: expectedPrUrl, + prNumber: 77, + branchName: "symphony/loop-test-branch", + commitSha: "aabbccdd1122334455667788990011223344556677", + }); + const claudeScript = [ + "#!/bin/sh", + // Write execution-result.json relative to cwd (which is worktreeDir for attemptLlmCommit) + `printf '%s' ${JSON.stringify(executionResultContent).replace(/'/g, String.raw`'\''`)} > execution-result.json`, + "exit 0", + ].join("\n"); + await fs.writeFile(path.join(fakeBin, "claude"), claudeScript, { mode: 0o755 }); + + // fake git that stubs push (so executeGitOperations wouldn't fail if accidentally called) + // We verify via upload payload that git ops were NOT needed. + 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 }); + + 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: PORT_PROBE_ORDER[0], + fallbackPorts: PORT_PROBE_ORDER.slice(1), + webAppOrigin: "https://app.symphony.com", + getAllowedDirectories: () => [tmpDir], + machineName: "execute-llmresult-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-000000000200"; + 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: `llmresult/${path.basename(repoPath)}`, branch: "main" }, + }), + } + ); + + assert.equal( + response.status, + 200, + `Expected 200 but got ${response.status}: ${await response.text().catch(() => "")}` + ); + + // Wait for upload — signals process completion including attemptLlmCommit + const uploadReq = await mock.waitForRequest("upload-artifacts"); + const uploadBody = JSON.parse(uploadReq.body) as { + artifacts: { + executionResult?: Record; + }; + metadata: Record; + }; + + // The LLM wrote execution-result.json, so the PR URL should appear in the upload + assert.equal( + uploadBody.artifacts.executionResult?.pr_url, + expectedPrUrl, + `Expected pr_url=${expectedPrUrl} from pre-written execution-result.json, got: ${String(uploadBody.artifacts.executionResult?.pr_url)}` + ); + assert.equal( + uploadBody.artifacts.executionResult?.pr_number, + 77, + "Expected pr_number=77 from pre-written execution-result.json" + ); + assert.equal( + uploadBody.artifacts.executionResult?.has_changes, + true, + "Expected has_changes=true when execution-result.json was written" + ); +}); + +// --------------------------------------------------------------------------- +// Test 3: Existing PR via gh pr view → no gh pr create called +// --------------------------------------------------------------------------- + +test("EXECUTE: uses existing PR URL from gh pr view without calling gh pr create", async () => { + const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "execute-existingpr-")); + tempPathsToClean.push(tmpDir); + + const repoPath = path.join(tmpDir, "repo-existingpr"); + 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 so the worktree has changes for git status + await createFakeRunLoopScript( + tmpDir, + [ + "#!/bin/sh", + // Write a file to create an uncommitted change + "echo 'implement feature' > feature-output.txt", + "exit 0", + ].join("\n") + ); + + const fakeBin = path.join(tmpDir, "fake-bin"); + await fs.mkdir(fakeBin, { recursive: true }); + + // fake claude for attemptLlmCommit: exits 0 without writing execution-result.json + // → attemptLlmCommit returns null → falls through to executeGitOperations + await fs.writeFile( + path.join(fakeBin, "claude"), + "#!/bin/sh\nexit 0\n", + { mode: 0o755 } + ); + + // Capture file to record whether gh pr create was called + const captureFile = path.join(tmpDir, "gh-calls.txt"); + + // fake gh: pr view returns existing PR JSON; pr create records a call and exits 1 + const fakeGhScript = [ + "#!/bin/sh", + "if [ \"$1\" = pr ] && [ \"$2\" = view ]; then", + " printf '{\"url\":\"https://github.com/org/repo-existingpr/pull/42\",\"number\":42}\\n'", + " exit 0", + "fi", + "if [ \"$1\" = pr ] && [ \"$2\" = create ]; then", + ` echo "gh pr create was called (should not happen)" >> ${JSON.stringify(captureFile)}`, + " exit 1", + "fi", + `exec /usr/bin/gh "$@" 2>/dev/null`, + ].join("\n"); + await fs.writeFile(path.join(fakeBin, "gh"), fakeGhScript, { mode: 0o755 }); + + // fake git: pass through all commands except push (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 }); + + 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: PORT_PROBE_ORDER[0], + fallbackPorts: PORT_PROBE_ORDER.slice(1), + webAppOrigin: "https://app.symphony.com", + getAllowedDirectories: () => [tmpDir], + machineName: "execute-existingpr-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-000000000300"; + 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: `existingpr/${path.basename(repoPath)}`, branch: "main" }, + }), + } + ); + + assert.equal( + response.status, + 200, + `Expected 200 but got ${response.status}: ${await response.text().catch(() => "")}` + ); + + // Wait for upload — signals that git ops + PR lookup completed + const uploadReq = await mock.waitForRequest("upload-artifacts"); + const uploadBody = JSON.parse(uploadReq.body) as { + artifacts: { + executionResult?: Record; + }; + metadata: Record; + }; + + // Existing PR URL should appear in the execution result + assert.equal( + uploadBody.artifacts.executionResult?.pr_url, + "https://github.com/org/repo-existingpr/pull/42", + `Expected existing PR URL in pr_url, got: ${String(uploadBody.artifacts.executionResult?.pr_url)}` + ); + assert.equal( + uploadBody.artifacts.executionResult?.pr_number, + 42, + "Expected pr_number=42 from gh pr view" + ); + + // gh pr create must NOT have been called + const ghCalls = await fs.readFile(captureFile, "utf-8").catch(() => ""); + assert.equal( + ghCalls.trim(), + "", + `gh pr create should not have been called, but capture file contains: ${ghCalls}` + ); +});