From 6fc632f9a162abcfaa8ed1bdc659ecce526f26ed Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Thu, 12 Feb 2026 11:15:11 -0800 Subject: [PATCH] Harden git and terminal flows and surface open PR status - include open PR metadata in git status with graceful `gh` failure handling - centralize git command execution in `GitCoreService` via `runProcess` with truncation support - add PTY spawn-helper permission fixes, shell fallback retries, and tests for new behavior --- apps/server/src/git.test.ts | 19 ++ apps/server/src/git.ts | 427 +++++++++++------------- apps/server/src/gitManager.test.ts | 59 ++++ apps/server/src/gitManager.ts | 76 +++-- apps/server/src/processRunner.test.ts | 31 ++ apps/server/src/processRunner.ts | 66 +++- apps/server/src/ptyAdapter.test.ts | 54 +++ apps/server/src/ptyAdapter.ts | 61 +++- apps/server/src/terminalManager.test.ts | 43 ++- apps/server/src/terminalManager.ts | 106 +++++- apps/server/src/wsServer.test.ts | 2 + apps/web/src/components/ChatView.tsx | 158 +++++---- packages/contracts/src/git.test.ts | 2 +- packages/contracts/src/git.ts | 10 +- 14 files changed, 780 insertions(+), 334 deletions(-) create mode 100644 apps/server/src/processRunner.test.ts create mode 100644 apps/server/src/ptyAdapter.test.ts diff --git a/apps/server/src/git.test.ts b/apps/server/src/git.test.ts index fd1e4b92e3c8eba40ee3b70a8fedb1a57f2a86d3..5ceddd7db93fa5f85ad60b5e8d59fb45cf3c78ca 100644 --- a/apps/server/src/git.test.ts +++ b/apps/server/src/git.test.ts @@ -438,6 +438,25 @@ describe("git integration", () => { }); describe("GitCoreService", () => { + it("supports branch lifecycle operations through the service API", async () => { + await using tmp = await makeTmpDir(); + const core = new GitCoreService(); + + await core.initRepo({ cwd: tmp.path }); + await git(tmp.path, "config user.email 'test@test.com'"); + await git(tmp.path, "config user.name 'Test'"); + await writeFile(path.join(tmp.path, "README.md"), "# test\n"); + await git(tmp.path, "add ."); + await git(tmp.path, "commit -m 'initial commit'"); + + await core.createBranch({ cwd: tmp.path, branch: "feature/service-api" }); + await core.checkoutBranch({ cwd: tmp.path, branch: "feature/service-api" }); + const branches = await core.listBranches({ cwd: tmp.path }); + + expect(branches.isRepo).toBe(true); + expect(branches.branches.find((branch) => branch.current)?.name).toBe("feature/service-api"); + }); + it("reports status details and dirty state", async () => { await using tmp = await makeTmpDir(); await initRepoWithCommit(tmp.path); diff --git a/apps/server/src/git.ts b/apps/server/src/git.ts index 43482ebf5110102680409a562df68a3e289c73fa..6c55ef9cdd1d35e5391f78dec1d59995d8c9e55d 100644 --- a/apps/server/src/git.ts +++ b/apps/server/src/git.ts @@ -1,4 +1,3 @@ -import { spawn } from "node:child_process"; import fs from "node:fs"; import os from "node:os"; import path from "node:path"; @@ -17,6 +16,7 @@ import { type GitStatusInput, type GitStatusResult, } from "@t3tools/contracts"; +import { type ProcessRunResult, runProcess } from "./processRunner"; export interface TerminalCommandInput { command: string; @@ -25,15 +25,9 @@ export interface TerminalCommandInput { maxOutputBytes?: number; } -export interface TerminalCommandResult { - stdout: string; - stderr: string; - code: number | null; - signal: NodeJS.Signals | null; - timedOut: boolean; -} +export type TerminalCommandResult = ProcessRunResult; -export interface GitStatusDetails extends GitStatusResult { +export interface GitStatusDetails extends Omit { upstreamRef: string | null; } @@ -60,84 +54,26 @@ interface RunGitOptions { allowNonZeroExit?: boolean | undefined; } +interface ExecuteGitOptions { + timeoutMs?: number | undefined; + allowNonZeroExit?: boolean | undefined; + fallbackErrorMessage?: string | undefined; +} + const DEFAULT_MAX_OUTPUT_BYTES = 1_000_000; -function appendChunkWithinLimit( - target: string, - currentBytes: number, - chunk: Buffer, - maxBytes: number, -): { - next: string; - nextBytes: number; - truncated: boolean; -} { - const remaining = maxBytes - currentBytes; - if (remaining <= 0) { - return { next: target, nextBytes: currentBytes, truncated: true }; - } - if (chunk.length <= remaining) { - return { - next: `${target}${chunk.toString()}`, - nextBytes: currentBytes + chunk.length, - truncated: false, - }; - } - return { - next: `${target}${chunk.subarray(0, remaining).toString()}`, - nextBytes: currentBytes + remaining, - truncated: true, - }; -} - /** Spawn git directly with an argv array — no shell, no quoting needed. */ -function runGit(args: readonly string[], cwd: string, timeoutMs = 30_000): Promise { - return new Promise((resolve, reject) => { - const child = spawn("git", args, { - cwd, - env: process.env, - stdio: ["ignore", "pipe", "pipe"], - }); - - const maxOutputBytes = DEFAULT_MAX_OUTPUT_BYTES; - let stdout = ""; - let stderr = ""; - let stdoutBytes = 0; - let stderrBytes = 0; - let timedOut = false; - let outputTruncated = false; - - const timeout = setTimeout(() => { - timedOut = true; - child.kill("SIGTERM"); - setTimeout(() => { - if (!child.killed) child.kill("SIGKILL"); - }, 1_000).unref(); - }, timeoutMs); - - child.stdout?.on("data", (chunk: Buffer) => { - const appended = appendChunkWithinLimit(stdout, stdoutBytes, chunk, maxOutputBytes); - stdout = appended.next; - stdoutBytes = appended.nextBytes; - outputTruncated = outputTruncated || appended.truncated; - }); - child.stderr?.on("data", (chunk: Buffer) => { - const appended = appendChunkWithinLimit(stderr, stderrBytes, chunk, maxOutputBytes); - stderr = appended.next; - stderrBytes = appended.nextBytes; - outputTruncated = outputTruncated || appended.truncated; - }); - child.on("error", (error) => { - clearTimeout(timeout); - reject(error); - }); - child.on("close", (code, signal) => { - clearTimeout(timeout); - if (outputTruncated) { - stderr = `${stderr}\n[output truncated at ${maxOutputBytes} bytes]`; - } - resolve({ stdout, stderr, code: code ?? null, signal: signal ?? null, timedOut }); - }); +function runGit( + args: readonly string[], + cwd: string, + timeoutMs = 30_000, +): Promise { + return runProcess("git", args, { + cwd, + timeoutMs, + allowNonZeroExit: true, + maxBufferBytes: DEFAULT_MAX_OUTPUT_BYTES, + outputMode: "truncate", }); } @@ -162,11 +98,10 @@ function normalizeGitSpawnError(cwd: string, error: unknown): Error { if (!(error instanceof Error)) { return new Error("Failed to run git command."); } - const code = (error as NodeJS.ErrnoException).code; - if (code === "ENOENT") { - if (!fs.existsSync(cwd)) { - return new Error(`Working directory does not exist: ${cwd}`); - } + if (!fs.existsSync(cwd)) { + return new Error(`Working directory does not exist: ${cwd}`); + } + if (error.message.includes("Command not found: git")) { return new Error("Git is required but not available on PATH."); } return new Error(`Failed to run git command: ${error.message}`); @@ -206,6 +141,30 @@ async function runGitOrThrow( return result; } +async function executeGit( + cwd: string, + args: readonly string[], + options: ExecuteGitOptions = {}, +): Promise { + const result = await runGitOrThrow(cwd, args, { + timeoutMs: options.timeoutMs, + allowNonZeroExit: true, + }); + + if (options.allowNonZeroExit || result.code === 0) { + return result; + } + + const stderr = result.stderr.trim(); + if (stderr.length > 0) { + throw new Error(stderr); + } + if (options.fallbackErrorMessage) { + throw new Error(options.fallbackErrorMessage); + } + throw normalizeGitExecutionError(args, result); +} + export class GitCoreService { async status(raw: GitStatusInput): Promise { const input = gitStatusInputSchema.parse(raw); @@ -216,6 +175,7 @@ export class GitCoreService { hasUpstream: details.hasUpstream, aheadCount: details.aheadCount, behindCount: details.behindCount, + openPr: null, }); } @@ -352,16 +312,132 @@ export class GitCoreService { return value.length > 0 ? value : null; } + async listBranches(input: GitListBranchesInput): Promise { + const result = await executeGit(input.cwd, ["branch", "--no-color"], { + timeoutMs: 10_000, + allowNonZeroExit: true, + }); + + if (result.code !== 0) { + const stderr = result.stderr.trim(); + if (stderr.toLowerCase().includes("not a git repository")) { + return { branches: [], isRepo: false }; + } + throw new Error(stderr || "git branch failed"); + } + + const [defaultRef, worktreeList] = await Promise.all([ + executeGit(input.cwd, ["symbolic-ref", "refs/remotes/origin/HEAD"], { + timeoutMs: 5_000, + allowNonZeroExit: true, + }), + executeGit(input.cwd, ["worktree", "list", "--porcelain"], { + timeoutMs: 5_000, + allowNonZeroExit: true, + }), + ]); + const defaultBranch = + defaultRef.code === 0 + ? defaultRef.stdout.trim().replace(/^refs\/remotes\/origin\//, "") + : null; + + const worktreeMap = new Map(); + if (worktreeList.code === 0) { + let currentPath: string | null = null; + for (const line of worktreeList.stdout.split("\n")) { + if (line.startsWith("worktree ")) { + const candidatePath = line.slice("worktree ".length); + currentPath = fs.existsSync(candidatePath) ? candidatePath : null; + } else if (line.startsWith("branch refs/heads/") && currentPath) { + worktreeMap.set(line.slice("branch refs/heads/".length), currentPath); + } else if (line === "") { + currentPath = null; + } + } + } + + const branches = result.stdout + .split("\n") + .map((line) => line.trim()) + .filter((line) => line.length > 0) + .map((line) => { + const name = line.replace(/^[*+]\s+/, ""); + return { + name, + current: line.startsWith("* "), + isDefault: name === defaultBranch, + worktreePath: worktreeMap.get(name) ?? null, + }; + }) + .toSorted((a, b) => { + if (a.current !== b.current) return a.current ? -1 : 1; + if (a.isDefault !== b.isDefault) return a.isDefault ? -1 : 1; + return a.name.localeCompare(b.name); + }); + + return { branches, isRepo: true }; + } + + async createWorktree(input: GitCreateWorktreeInput): Promise { + const sanitizedBranch = input.newBranch.replace(/\//g, "-"); + const repoName = path.basename(input.cwd); + const worktreePath = + input.path ?? path.join(os.homedir(), ".t3", "worktrees", repoName, sanitizedBranch); + + await executeGit( + input.cwd, + ["worktree", "add", "-b", input.newBranch, worktreePath, input.branch], + { fallbackErrorMessage: "git worktree add failed" }, + ); + + return { + worktree: { + path: worktreePath, + branch: input.newBranch, + }, + }; + } + + async removeWorktree(input: GitRemoveWorktreeInput): Promise { + await executeGit(input.cwd, ["worktree", "remove", input.path], { + timeoutMs: 15_000, + fallbackErrorMessage: "git worktree remove failed", + }); + } + + async createBranch(input: GitCreateBranchInput): Promise { + await executeGit(input.cwd, ["branch", input.branch], { + timeoutMs: 10_000, + fallbackErrorMessage: "git branch create failed", + }); + } + + async checkoutBranch(input: GitCheckoutInput): Promise { + await executeGit(input.cwd, ["checkout", input.branch], { + timeoutMs: 10_000, + fallbackErrorMessage: "git checkout failed", + }); + } + + async initRepo(input: GitInitInput): Promise { + await executeGit(input.cwd, ["init"], { + timeoutMs: 10_000, + fallbackErrorMessage: "git init failed", + }); + } + async git(cwd: string, args: readonly string[], allowNonZeroExit = false): Promise { - await runGitOrThrow(cwd, args, { allowNonZeroExit }); + await executeGit(cwd, args, { allowNonZeroExit }); } async gitStdout(cwd: string, args: readonly string[], allowNonZeroExit = false): Promise { - const result = await runGitOrThrow(cwd, args, { allowNonZeroExit }); + const result = await executeGit(cwd, args, { allowNonZeroExit }); return result.stdout; } } +const defaultGitCoreService = new GitCoreService(); + export async function runTerminalCommand( input: TerminalCommandInput, ): Promise { @@ -374,173 +450,46 @@ export async function runTerminalCommand( const args = process.platform === "win32" ? ["/d", "/s", "/c", input.command] : ["-lc", input.command]; - return new Promise((resolve, reject) => { - const child = spawn(shellPath, args, { - cwd: input.cwd, - env: process.env, - stdio: ["ignore", "pipe", "pipe"], - }); - - let stdout = ""; - let stderr = ""; - let stdoutBytes = 0; - let stderrBytes = 0; - let timedOut = false; - let outputTruncated = false; - - const timeout = setTimeout(() => { - timedOut = true; - child.kill("SIGTERM"); - setTimeout(() => { - if (!child.killed) { - child.kill("SIGKILL"); - } - }, 1_000).unref(); - }, input.timeoutMs ?? 30_000); - - child.stdout?.on("data", (chunk: Buffer) => { - const appended = appendChunkWithinLimit(stdout, stdoutBytes, chunk, maxOutputBytes); - stdout = appended.next; - stdoutBytes = appended.nextBytes; - outputTruncated = outputTruncated || appended.truncated; - }); - - child.stderr?.on("data", (chunk: Buffer) => { - const appended = appendChunkWithinLimit(stderr, stderrBytes, chunk, maxOutputBytes); - stderr = appended.next; - stderrBytes = appended.nextBytes; - outputTruncated = outputTruncated || appended.truncated; - }); - - child.on("error", (error) => { - clearTimeout(timeout); - reject(error); - }); - - child.on("close", (code, signal) => { - clearTimeout(timeout); - if (outputTruncated) { - stderr = `${stderr}\n[output truncated at ${maxOutputBytes} bytes]`; - } - resolve({ - stdout, - stderr, - code: code ?? null, - signal: signal ?? null, - timedOut, - }); - }); + const result = await runProcess(shellPath, args, { + cwd: input.cwd, + timeoutMs: input.timeoutMs ?? 30_000, + allowNonZeroExit: true, + maxBufferBytes: maxOutputBytes, + outputMode: "truncate", }); + + if (!result.stdoutTruncated && !result.stderrTruncated) { + return result; + } + + return { + ...result, + stderr: `${result.stderr}\n[output truncated at ${maxOutputBytes} bytes]`, + }; } export async function listGitBranches(input: GitListBranchesInput): Promise { - const result = await runGit(["branch", "--no-color"], input.cwd, 10_000); - - if (result.code !== 0) { - const stderr = result.stderr.trim(); - if (stderr.includes("not a git repository")) { - return { branches: [], isRepo: false }; - } - throw new Error(stderr || "git branch failed"); - } - - const [defaultRef, worktreeList] = await Promise.all([ - runGit(["symbolic-ref", "refs/remotes/origin/HEAD"], input.cwd, 5_000), - runGit(["worktree", "list", "--porcelain"], input.cwd, 5_000), - ]); - const defaultBranch = - defaultRef.code === 0 ? defaultRef.stdout.trim().replace(/^refs\/remotes\/origin\//, "") : null; - - const worktreeMap = new Map(); - if (worktreeList.code === 0) { - let currentPath: string | null = null; - for (const line of worktreeList.stdout.split("\n")) { - if (line.startsWith("worktree ")) { - const candidatePath = line.slice("worktree ".length); - currentPath = fs.existsSync(candidatePath) ? candidatePath : null; - } else if (line.startsWith("branch refs/heads/") && currentPath) { - worktreeMap.set(line.slice("branch refs/heads/".length), currentPath); - } else if (line === "") { - currentPath = null; - } - } - } - - const branches = result.stdout - .split("\n") - .map((line) => line.trim()) - .filter((line) => line.length > 0) - .map((line) => { - const name = line.replace(/^[*+]\s+/, ""); - return { - name, - current: line.startsWith("* "), - isDefault: name === defaultBranch, - worktreePath: worktreeMap.get(name) ?? null, - }; - }) - .toSorted((a, b) => { - if (a.current !== b.current) return a.current ? -1 : 1; - if (a.isDefault !== b.isDefault) return a.isDefault ? -1 : 1; - return a.name.localeCompare(b.name); - }); - - return { branches, isRepo: true }; + return defaultGitCoreService.listBranches(input); } export async function createGitWorktree( input: GitCreateWorktreeInput, ): Promise { - const sanitizedBranch = input.newBranch.replace(/\//g, "-"); - const repoName = path.basename(input.cwd); - const worktreePath = - input.path ?? path.join(os.homedir(), ".t3", "worktrees", repoName, sanitizedBranch); - - const result = await runGit( - ["worktree", "add", "-b", input.newBranch, worktreePath, input.branch], - input.cwd, - ); - - if (result.code !== 0) { - throw new Error(result.stderr.trim() || "git worktree add failed"); - } - - return { - worktree: { - path: worktreePath, - branch: input.newBranch, - }, - }; + return defaultGitCoreService.createWorktree(input); } export async function removeGitWorktree(input: GitRemoveWorktreeInput): Promise { - const result = await runGit(["worktree", "remove", input.path], input.cwd, 15_000); - - if (result.code !== 0) { - throw new Error(result.stderr.trim() || "git worktree remove failed"); - } + await defaultGitCoreService.removeWorktree(input); } export async function createGitBranch(input: GitCreateBranchInput): Promise { - const result = await runGit(["branch", input.branch], input.cwd, 10_000); - - if (result.code !== 0) { - throw new Error(result.stderr.trim() || "git branch create failed"); - } + await defaultGitCoreService.createBranch(input); } export async function checkoutGitBranch(input: GitCheckoutInput): Promise { - const result = await runGit(["checkout", input.branch], input.cwd, 10_000); - - if (result.code !== 0) { - throw new Error(result.stderr.trim() || "git checkout failed"); - } + await defaultGitCoreService.checkoutBranch(input); } export async function initGitRepo(input: GitInitInput): Promise { - const result = await runGit(["init"], input.cwd, 10_000); - - if (result.code !== 0) { - throw new Error(result.stderr.trim() || "git init failed"); - } + await defaultGitCoreService.initRepo(input); } diff --git a/apps/server/src/gitManager.test.ts b/apps/server/src/gitManager.test.ts index 57a3c7cc8ef8236c8bfeb4b42df548838e0030ec..092473936b66cb6174eccd352579a6812b3de489 100644 --- a/apps/server/src/gitManager.test.ts +++ b/apps/server/src/gitManager.test.ts @@ -168,6 +168,65 @@ afterEach(() => { }); describe("GitManager", () => { + it("status includes open PR metadata when branch already has an open PR", async () => { + const repoDir = makeTempDir("t3code-git-manager-"); + await initRepo(repoDir); + await runGit(repoDir, ["checkout", "-b", "feature/status-open-pr"]); + const remoteDir = await createBareRemote(); + await runGit(repoDir, ["remote", "add", "origin", remoteDir]); + await runGit(repoDir, ["push", "-u", "origin", "feature/status-open-pr"]); + + const { runner } = createRunnerWithFakeGh({ + prListSequence: [ + JSON.stringify([ + { + number: 13, + title: "Existing PR", + url: "https://github.com/pingdotgg/codething-mvp/pull/13", + baseRefName: "main", + headRefName: "feature/status-open-pr", + }, + ]), + ], + }); + + const manager = new GitManager({ + runProcess: runner, + textGenerator: createTextGenerator(), + }); + + const status = await manager.status({ cwd: repoDir }); + expect(status.branch).toBe("feature/status-open-pr"); + expect(status.openPr).toEqual({ + number: 13, + title: "Existing PR", + url: "https://github.com/pingdotgg/codething-mvp/pull/13", + baseBranch: "main", + headBranch: "feature/status-open-pr", + }); + }); + + it("status is resilient to gh lookup failures and returns openPr null", async () => { + const repoDir = makeTempDir("t3code-git-manager-"); + await initRepo(repoDir); + await runGit(repoDir, ["checkout", "-b", "feature/status-no-gh"]); + const remoteDir = await createBareRemote(); + await runGit(repoDir, ["remote", "add", "origin", remoteDir]); + await runGit(repoDir, ["push", "-u", "origin", "feature/status-no-gh"]); + + const { runner } = createRunnerWithFakeGh({ + failWith: new Error("Command not found: gh"), + }); + const manager = new GitManager({ + runProcess: runner, + textGenerator: createTextGenerator(), + }); + + const status = await manager.status({ cwd: repoDir }); + expect(status.branch).toBe("feature/status-no-gh"); + expect(status.openPr).toBeNull(); + }); + it("creates a commit when working tree is dirty", async () => { const repoDir = makeTempDir("t3code-git-manager-"); await initRepo(repoDir); diff --git a/apps/server/src/gitManager.ts b/apps/server/src/gitManager.ts index 9c16f1761cbdee771521a3da2b17d89e84bc5775..682d56a0d2b9ae6f0809d53fb634ca086864c231 100644 --- a/apps/server/src/gitManager.ts +++ b/apps/server/src/gitManager.ts @@ -6,6 +6,7 @@ import path from "node:path"; import { gitRunStackedActionInputSchema, gitRunStackedActionResultSchema, + gitStatusInputSchema, type GitRunStackedActionInput, type GitRunStackedActionResult, type GitStatusInput, @@ -19,7 +20,11 @@ import type { } from "./coreServices"; import { CodexTextGenerator } from "./codexTextGenerator"; import { GitCoreService } from "./git"; -import { type ProcessRunOptions, runProcess } from "./processRunner"; +import { + type ProcessRunOptions, + type ProcessRunResult, + runProcess, +} from "./processRunner"; const GH_CLI_TIMEOUT_MS = 30_000; @@ -28,13 +33,7 @@ interface GitManagerDeps { command: string, args: readonly string[], options?: ProcessRunOptions, - ) => Promise<{ - stdout: string; - stderr: string; - code: number | null; - signal: NodeJS.Signals | null; - timedOut: boolean; - }>; + ) => Promise; textGenerator?: TextGenerationService; gitCore?: GitCoreService; } @@ -155,6 +154,22 @@ function normalizeGitHubAuthError(error: unknown): Error | undefined { return undefined; } +function toStatusOpenPr(pr: OpenPrInfo): { + number: number; + title: string; + url: string; + baseBranch: string; + headBranch: string; +} { + return { + number: pr.number, + title: pr.title, + url: pr.url, + baseBranch: pr.baseRefName, + headBranch: pr.headRefName, + }; +} + export class GitManager { private readonly run: NonNullable; private readonly textGenerator: TextGenerationService; @@ -167,7 +182,29 @@ export class GitManager { } async status(raw: GitStatusInput): Promise { - return this.gitCore.status(raw); + const input = gitStatusInputSchema.parse(raw); + const details = await this.gitCore.statusDetails(input.cwd); + + let openPr: ReturnType | null = null; + if (details.branch && details.hasUpstream) { + try { + const existing = await this.findOpenPr(input.cwd, details.branch); + if (existing) { + openPr = toStatusOpenPr(existing); + } + } catch { + // PR lookup is best-effort for status rendering. + } + } + + return { + branch: details.branch, + hasWorkingTreeChanges: details.hasWorkingTreeChanges, + hasUpstream: details.hasUpstream, + aheadCount: details.aheadCount, + behindCount: details.behindCount, + openPr, + }; } async runStackedAction( @@ -413,24 +450,23 @@ export class GitManager { } private async runGh(cwd: string, args: readonly string[]): Promise { - try { - await this.run("gh", args, { cwd, timeoutMs: GH_CLI_TIMEOUT_MS }); - } catch (error) { - throw ( - asCommandNotFound("gh", error) ?? - normalizeGitHubAuthError(error) ?? - (error instanceof Error ? error : new Error("GitHub CLI command failed.")) - ); - } + await this.runGhCommand(cwd, args); } private async runGhStdout(cwd: string, args: readonly string[]): Promise { + const result = await this.runGhCommand(cwd, args); + return result.stdout; + } + + private async runGhCommand( + cwd: string, + args: readonly string[], + ): Promise { try { - const result = await this.run("gh", args, { + return await this.run("gh", args, { cwd, timeoutMs: GH_CLI_TIMEOUT_MS, }); - return result.stdout; } catch (error) { throw ( asCommandNotFound("gh", error) ?? diff --git a/apps/server/src/processRunner.test.ts b/apps/server/src/processRunner.test.ts new file mode 100644 index 0000000000000000000000000000000000000000..f7756ac95ea71ea08ab09884bb025f3a3fdf58ca --- /dev/null +++ b/apps/server/src/processRunner.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from "vitest"; + +import { runProcess } from "./processRunner"; + +describe("runProcess", () => { + it("fails when output exceeds max buffer in default mode", async () => { + await expect( + runProcess( + "node", + ["-e", "process.stdout.write('x'.repeat(2048))"], + { maxBufferBytes: 128 }, + ), + ).rejects.toThrow("exceeded stdout buffer limit"); + }); + + it("truncates output when outputMode is truncate", async () => { + const result = await runProcess( + "node", + ["-e", "process.stdout.write('x'.repeat(2048))"], + { + maxBufferBytes: 128, + outputMode: "truncate", + }, + ); + + expect(result.code).toBe(0); + expect(result.stdout.length).toBeLessThanOrEqual(128); + expect(result.stdoutTruncated).toBe(true); + expect(result.stderrTruncated).toBe(false); + }); +}); diff --git a/apps/server/src/processRunner.ts b/apps/server/src/processRunner.ts index 3d690ed6b88bfdbe6f3e1e9c0c02d61a3afc4c38..cb6e9f89e2da9e5362ed87a6ea16ce45e60876c8 100644 --- a/apps/server/src/processRunner.ts +++ b/apps/server/src/processRunner.ts @@ -7,6 +7,7 @@ export interface ProcessRunOptions { stdin?: string | undefined; allowNonZeroExit?: boolean | undefined; maxBufferBytes?: number | undefined; + outputMode?: "error" | "truncate" | undefined; } export interface ProcessRunResult { @@ -15,6 +16,8 @@ export interface ProcessRunResult { code: number | null; signal: NodeJS.Signals | null; timedOut: boolean; + stdoutTruncated?: boolean | undefined; + stderrTruncated?: boolean | undefined; } function commandLabel(command: string, args: readonly string[]): string { @@ -79,6 +82,34 @@ function normalizeBufferError( const DEFAULT_MAX_BUFFER_BYTES = 8 * 1024 * 1024; +function appendChunkWithinLimit( + target: string, + currentBytes: number, + chunk: Buffer, + maxBytes: number, +): { + next: string; + nextBytes: number; + truncated: boolean; +} { + const remaining = maxBytes - currentBytes; + if (remaining <= 0) { + return { next: target, nextBytes: currentBytes, truncated: true }; + } + if (chunk.length <= remaining) { + return { + next: `${target}${chunk.toString()}`, + nextBytes: currentBytes + chunk.length, + truncated: false, + }; + } + return { + next: `${target}${chunk.subarray(0, remaining).toString()}`, + nextBytes: currentBytes + remaining, + truncated: true, + }; +} + export async function runProcess( command: string, args: readonly string[], @@ -86,6 +117,7 @@ export async function runProcess( ): Promise { const timeoutMs = options.timeoutMs ?? 60_000; const maxBufferBytes = options.maxBufferBytes ?? DEFAULT_MAX_BUFFER_BYTES; + const outputMode = options.outputMode ?? "error"; return new Promise((resolve, reject) => { const child = spawn(command, args, { @@ -98,6 +130,8 @@ export async function runProcess( let stderr = ""; let stdoutBytes = 0; let stderrBytes = 0; + let stdoutTruncated = false; + let stderrTruncated = false; let timedOut = false; let settled = false; let forceKillTimer: ReturnType | null = null; @@ -131,15 +165,41 @@ export async function runProcess( stream: "stdout" | "stderr", chunk: Buffer | string, ): Error | null => { - const text = chunk.toString(); - const byteLength = Buffer.byteLength(text); + const chunkBuffer = + typeof chunk === "string" ? Buffer.from(chunk) : chunk; + const text = chunkBuffer.toString(); + const byteLength = chunkBuffer.length; if (stream === "stdout") { + if (outputMode === "truncate") { + const appended = appendChunkWithinLimit( + stdout, + stdoutBytes, + chunkBuffer, + maxBufferBytes, + ); + stdout = appended.next; + stdoutBytes = appended.nextBytes; + stdoutTruncated = stdoutTruncated || appended.truncated; + return null; + } stdout += text; stdoutBytes += byteLength; if (stdoutBytes > maxBufferBytes) { return normalizeBufferError(command, args, "stdout", maxBufferBytes); } } else { + if (outputMode === "truncate") { + const appended = appendChunkWithinLimit( + stderr, + stderrBytes, + chunkBuffer, + maxBufferBytes, + ); + stderr = appended.next; + stderrBytes = appended.nextBytes; + stderrTruncated = stderrTruncated || appended.truncated; + return null; + } stderr += text; stderrBytes += byteLength; if (stderrBytes > maxBufferBytes) { @@ -176,6 +236,8 @@ export async function runProcess( code, signal, timedOut, + stdoutTruncated, + stderrTruncated, }; finalize(() => { diff --git a/apps/server/src/ptyAdapter.test.ts b/apps/server/src/ptyAdapter.test.ts new file mode 100644 index 0000000000000000000000000000000000000000..5fd37292fe2fadcea31f1ed4dfa70842dd423aab --- /dev/null +++ b/apps/server/src/ptyAdapter.test.ts @@ -0,0 +1,54 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +import { afterEach, describe, expect, it } from "vitest"; + +import { ensureNodePtySpawnHelperExecutable } from "./ptyAdapter"; + +function fileMode(filePath: string): number { + return fs.statSync(filePath).mode & 0o777; +} + +describe("ensureNodePtySpawnHelperExecutable", () => { + const tempDirs: string[] = []; + + afterEach(() => { + for (const dir of tempDirs.splice(0, tempDirs.length)) { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it("adds executable bits when helper exists but is not executable", () => { + if (process.platform === "win32") return; + + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "pty-helper-test-")); + tempDirs.push(dir); + const helperPath = path.join(dir, "spawn-helper"); + fs.writeFileSync(helperPath, "#!/bin/sh\nexit 0\n", "utf8"); + fs.chmodSync(helperPath, 0o644); + + ensureNodePtySpawnHelperExecutable(helperPath); + + expect(fileMode(helperPath) & 0o111).toBe(0o111); + }); + + it("keeps executable helper as executable", () => { + if (process.platform === "win32") return; + + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "pty-helper-test-")); + tempDirs.push(dir); + const helperPath = path.join(dir, "spawn-helper"); + fs.writeFileSync(helperPath, "#!/bin/sh\nexit 0\n", "utf8"); + fs.chmodSync(helperPath, 0o755); + + ensureNodePtySpawnHelperExecutable(helperPath); + + expect(fileMode(helperPath) & 0o111).toBe(0o111); + }); + + it("does not throw when helper path is missing", () => { + const missing = path.join(os.tmpdir(), `does-not-exist-${Date.now()}`); + expect(() => ensureNodePtySpawnHelperExecutable(missing)).not.toThrow(); + }); +}); diff --git a/apps/server/src/ptyAdapter.ts b/apps/server/src/ptyAdapter.ts index e00724b83a3ed2668e33e3ae7fbde0e96673ad51..104a0648b92fd9ab2acf2cb91324851d83374677 100644 --- a/apps/server/src/ptyAdapter.ts +++ b/apps/server/src/ptyAdapter.ts @@ -1,3 +1,7 @@ +import fs from "node:fs"; +import { createRequire } from "node:module"; +import path from "node:path"; + import * as nodePty from "node-pty"; export interface PtyExitEvent { @@ -26,6 +30,57 @@ export interface PtyAdapter { spawn(input: PtySpawnInput): PtyProcess; } +const requireForNodePty = createRequire(import.meta.url); +let didEnsureSpawnHelperExecutable = false; + +function resolveNodePtySpawnHelperPath(): string | null { + try { + const packageJsonPath = requireForNodePty.resolve("node-pty/package.json"); + const packageDir = path.dirname(packageJsonPath); + const candidates = [ + path.join(packageDir, "build", "Release", "spawn-helper"), + path.join(packageDir, "build", "Debug", "spawn-helper"), + path.join( + packageDir, + "prebuilds", + `${process.platform}-${process.arch}`, + "spawn-helper", + ), + ]; + + for (const candidate of candidates) { + if (fs.existsSync(candidate)) { + return candidate; + } + } + + return null; + } catch { + return null; + } +} + +export function ensureNodePtySpawnHelperExecutable(explicitPath?: string): void { + if (process.platform === "win32") return; + if (!explicitPath && didEnsureSpawnHelperExecutable) return; + + const helperPath = explicitPath ?? resolveNodePtySpawnHelperPath(); + if (!helperPath) return; + if (!explicitPath) { + didEnsureSpawnHelperExecutable = true; + } + + try { + const stat = fs.statSync(helperPath); + const mode = stat.mode & 0o777; + if ((mode & 0o111) === 0) { + fs.chmodSync(helperPath, mode | 0o111); + } + } catch { + // Best effort only. If chmod fails, node-pty spawn will surface the real error. + } +} + class NodePtyProcess implements PtyProcess { constructor(private readonly process: nodePty.IPty) {} @@ -67,12 +122,16 @@ class NodePtyProcess implements PtyProcess { export class NodePtyAdapter implements PtyAdapter { spawn(input: PtySpawnInput): PtyProcess { + ensureNodePtySpawnHelperExecutable(); const ptyProcess = nodePty.spawn(input.shell, [], { cwd: input.cwd, cols: input.cols, rows: input.rows, env: input.env, - name: globalThis.process.platform === "win32" ? "xterm-color" : "xterm-256color", + name: + globalThis.process.platform === "win32" + ? "xterm-color" + : "xterm-256color", }); return new NodePtyProcess(ptyProcess); } diff --git a/apps/server/src/terminalManager.test.ts b/apps/server/src/terminalManager.test.ts index 281a716ae79f790c6e384f0dd656140de50c3c70..906c3d0fe48cdb5a7e9f4d02094983c3f8d7a72a 100644 --- a/apps/server/src/terminalManager.test.ts +++ b/apps/server/src/terminalManager.test.ts @@ -62,10 +62,15 @@ class FakePtyProcess implements PtyProcess { class FakePtyAdapter implements PtyAdapter { readonly spawnInputs: PtySpawnInput[] = []; readonly processes: FakePtyProcess[] = []; + readonly spawnFailures: Error[] = []; private nextPid = 9000; spawn(input: PtySpawnInput): PtyProcess { this.spawnInputs.push(input); + const failure = this.spawnFailures.shift(); + if (failure) { + throw failure; + } const process = new FakePtyProcess(this.nextPid++); this.processes.push(process); return process; @@ -117,7 +122,10 @@ describe("TerminalManager", () => { } }); - function makeManager(historyLineLimit = 5) { + function makeManager( + historyLineLimit = 5, + options: { shellResolver?: () => string } = {}, + ) { const logsDir = fs.mkdtempSync(path.join(os.tmpdir(), "t3code-terminal-")); tempDirs.push(logsDir); const ptyAdapter = new FakePtyAdapter(); @@ -125,7 +133,7 @@ describe("TerminalManager", () => { logsDir, ptyAdapter, historyLineLimit, - shellResolver: () => "/bin/bash", + shellResolver: options.shellResolver ?? (() => "/bin/bash"), }); return { logsDir, ptyAdapter, manager }; } @@ -266,4 +274,35 @@ describe("TerminalManager", () => { manager.dispose(); }); + + it("retries with fallback shells when preferred shell spawn fails", async () => { + const { manager, ptyAdapter } = makeManager(5, { + shellResolver: () => "/definitely/missing-shell -l", + }); + ptyAdapter.spawnFailures.push(new Error("posix_spawnp failed.")); + + const snapshot = await manager.open(openInput()); + + expect(snapshot.status).toBe("running"); + expect(ptyAdapter.spawnInputs.length).toBeGreaterThanOrEqual(2); + expect(ptyAdapter.spawnInputs[0]?.shell).toBe("/definitely/missing-shell"); + + if (process.platform === "win32") { + expect( + ptyAdapter.spawnInputs.some( + (input) => input.shell === "cmd.exe" || input.shell === "powershell.exe", + ), + ).toBe(true); + } else { + expect( + ptyAdapter.spawnInputs.some((input) => + ["/bin/zsh", "/bin/bash", "/bin/sh", "zsh", "bash", "sh"].includes( + input.shell, + ), + ), + ).toBe(true); + } + + manager.dispose(); + }); }); diff --git a/apps/server/src/terminalManager.ts b/apps/server/src/terminalManager.ts index f13b02280a89a514aa4446cdf17ddbed85dbd71b..0e4e130e02a180cd09e8c82aa3c859790135083c 100644 --- a/apps/server/src/terminalManager.ts +++ b/apps/server/src/terminalManager.ts @@ -57,6 +57,68 @@ function defaultShellResolver(): string { return process.env.SHELL ?? "bash"; } +function normalizeShellCommand(value: string | undefined): string | null { + if (!value) return null; + const trimmed = value.trim(); + if (trimmed.length === 0) return null; + + if (process.platform === "win32") { + return trimmed; + } + + const firstToken = trimmed.split(/\s+/g)[0]?.trim(); + if (!firstToken) return null; + return firstToken.replace(/^['"]|['"]$/g, ""); +} + +function uniqueShells(shells: Array): string[] { + const seen = new Set(); + const ordered: string[] = []; + for (const shell of shells) { + if (!shell || shell.length === 0) continue; + if (seen.has(shell)) continue; + seen.add(shell); + ordered.push(shell); + } + return ordered; +} + +function resolveShellCandidates(shellResolver: () => string): string[] { + const requested = normalizeShellCommand(shellResolver()); + + if (process.platform === "win32") { + return uniqueShells([ + requested, + process.env.ComSpec ?? null, + "powershell.exe", + "cmd.exe", + ]); + } + + return uniqueShells([ + requested, + normalizeShellCommand(process.env.SHELL), + "/bin/zsh", + "/bin/bash", + "/bin/sh", + "zsh", + "bash", + "sh", + ]); +} + +function isRetryableShellSpawnError(error: unknown): boolean { + if (!(error instanceof Error)) return false; + const message = error.message.toLowerCase(); + return ( + message.includes("posix_spawnp failed") || + message.includes("enoent") || + message.includes("not found") || + message.includes("file not found") || + message.includes("no such file") + ); +} + function capHistory(history: string, maxLines: number): string { if (history.length === 0) return history; const hasTrailingNewline = history.endsWith("\n"); @@ -270,16 +332,43 @@ export class TerminalManager extends EventEmitter { session.exitSignal = null; session.updatedAt = new Date().toISOString(); - const shell = this.shellResolver(); let ptyProcess: PtyProcess | null = null; + let startedShell: string | null = null; try { - ptyProcess = this.ptyAdapter.spawn({ - shell, - cwd: session.cwd, - cols: session.cols, - rows: session.rows, - env: process.env, - }); + const shellCandidates = resolveShellCandidates(this.shellResolver); + let lastSpawnError: unknown = null; + + for (const shell of shellCandidates) { + try { + ptyProcess = this.ptyAdapter.spawn({ + shell, + cwd: session.cwd, + cols: session.cols, + rows: session.rows, + env: process.env, + }); + startedShell = shell; + break; + } catch (error) { + lastSpawnError = error; + if (!isRetryableShellSpawnError(error)) { + throw error; + } + } + } + + if (!ptyProcess) { + const detail = + lastSpawnError instanceof Error + ? lastSpawnError.message + : "Terminal start failed"; + const tried = + shellCandidates.length > 0 + ? ` Tried shells: ${shellCandidates.join(", ")}.` + : ""; + throw new Error(`${detail}.${tried}`.trim()); + } + session.process = ptyProcess; session.pid = ptyProcess.pid; session.status = "running"; @@ -318,6 +407,7 @@ export class TerminalManager extends EventEmitter { this.logger.error("failed to start terminal", { threadId: session.threadId, error: message, + ...(startedShell ? { shell: startedShell } : {}), }); } } diff --git a/apps/server/src/wsServer.test.ts b/apps/server/src/wsServer.test.ts index 9d256604f5a8729d75d04a5976800e41ba1ccbed..e0a73a943d64b06b9675f6b77de572b53671d7ae 100644 --- a/apps/server/src/wsServer.test.ts +++ b/apps/server/src/wsServer.test.ts @@ -524,6 +524,7 @@ describe("WebSocket Server", () => { hasUpstream: false, aheadCount: 0, behindCount: 0, + openPr: null, }), runStackedAction: vi.fn(), }; @@ -547,6 +548,7 @@ describe("WebSocket Server", () => { hasUpstream: false, aheadCount: 0, behindCount: 0, + openPr: null, }); expect(gitManager.status).toHaveBeenCalledWith({ cwd: "/test" }); }); diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index 9435f2c9622469449ce6cf33a76317bfea9cacf7..7770ebf0397c5f62c61937992e62a4f657885fe5 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -63,6 +63,69 @@ interface GitActionMenuItem { id: GitStackedAction; label: string; disabled: boolean; + icon: "commit" | "push" | "pr"; +} + +function GitActionIcon(props: { icon: GitActionMenuItem["icon"]; disabled: boolean }) { + const toneClass = props.disabled ? "text-muted-foreground/45" : "text-foreground/85"; + + if (props.icon === "commit") { + return ( + + ); + } + + if (props.icon === "push") { + return ( + + ); + } + + return ( + + ); } function workToneClass(tone: "thinking" | "tool" | "info" | "error"): string { @@ -277,65 +340,34 @@ export default function ChatView() { if (!gitStatus) return []; const hasBranch = gitStatus.branch !== null; + const hasOpenPr = gitStatus.openPr !== null; const canCommit = !gitBaseDisabled && gitStatus.hasWorkingTreeChanges; - const canPushOrPr = !gitBaseDisabled && hasBranch; + const canPush = + !gitBaseDisabled && hasBranch && (gitStatus.hasWorkingTreeChanges || gitStatus.aheadCount > 0); + const canViewPr = + !gitBaseDisabled && + hasBranch && + gitStatus.behindCount === 0 && + (gitStatus.hasWorkingTreeChanges || gitStatus.aheadCount > 0 || hasOpenPr); - if (!hasBranch) { - return [ - { - id: "commit", - label: "Commit", - disabled: !canCommit, - }, - ]; - } - - if (gitStatus.hasWorkingTreeChanges) { - return [ - { - id: "commit", - label: "Commit", - disabled: !canCommit, - }, - { - id: "commit_push", - label: "Commit & Push", - disabled: !canPushOrPr, - }, - { - id: "commit_push_pr", - label: "Commit, Push & Create PR", - disabled: !canPushOrPr, - }, - ]; - } - - if (gitStatus.aheadCount > 0) { - return [ - { - id: "commit", - label: "Commit", - disabled: true, - }, - { - id: "commit_push", - label: "Commit & Push", - disabled: !canPushOrPr, - }, - { - id: "commit_push_pr", - label: "Commit, Push & Create PR", - disabled: !canPushOrPr, - }, - ]; - } - - const openPrDisabled = !canPushOrPr || gitStatus.behindCount > 0; return [ + { + id: "commit", + label: "Commit", + disabled: !canCommit, + icon: "commit", + }, + { + id: "commit_push", + label: "Push", + disabled: !canPush, + icon: "push", + }, { id: "commit_push_pr", - label: "Open PR", - disabled: openPrDisabled, + label: "View PR", + disabled: !canViewPr, + icon: "pr", }, ]; }, [gitBaseDisabled, gitStatus]); @@ -882,15 +914,20 @@ export default function ChatView() { {isGitMenuOpen && ( -
-

+

+

Git actions

{gitActionMenuItems.map((item) => { @@ -898,13 +935,14 @@ export default function ChatView() { ); })} @@ -918,7 +956,7 @@ export default function ChatView() { !gitStatus.hasWorkingTreeChanges && gitStatus.aheadCount === 0 && gitStatus.behindCount > 0 && ( -

+

Branch is behind upstream. Pull/rebase before opening a PR.

)} diff --git a/packages/contracts/src/git.test.ts b/packages/contracts/src/git.test.ts index de13eed5d4944367dc68b4cc548bc2f6346cfc33..cecf63650ee720ef4d35be66df428c2c78d4d427 100644 --- a/packages/contracts/src/git.test.ts +++ b/packages/contracts/src/git.test.ts @@ -21,6 +21,7 @@ describe("git contracts", () => { hasUpstream: false, aheadCount: 0, behindCount: 0, + openPr: null, }); expect(parsed.branch).toBe("feature/git-actions"); expect(parsed.hasWorkingTreeChanges).toBe(true); @@ -70,4 +71,3 @@ describe("git contracts", () => { expect(parsed.pr.number).toBe(123); }); }); - diff --git a/packages/contracts/src/git.ts b/packages/contracts/src/git.ts index 1dba6024940b16b99882a1b34a28d18d0ac87cb6..d8e4542d37cb011f69a06725b3715367720dc659 100644 --- a/packages/contracts/src/git.ts +++ b/packages/contracts/src/git.ts @@ -74,6 +74,15 @@ export const gitStatusResultSchema = z.object({ hasUpstream: z.boolean(), aheadCount: z.number().int().nonnegative(), behindCount: z.number().int().nonnegative(), + openPr: z + .object({ + number: z.number().int().positive(), + title: z.string().min(1), + url: z.string().url(), + baseBranch: z.string().min(1), + headBranch: z.string().min(1), + }) + .nullable(), }); export const gitStackedActionSchema = z.enum([ @@ -127,4 +136,3 @@ export type GitStatusResult = z.infer; export type GitStackedAction = z.infer; export type GitRunStackedActionInput = z.input; export type GitRunStackedActionResult = z.infer; - -- 2.54.0