diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 3bb8a9ea5..1e48a7ac4 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -28,6 +28,9 @@ - Fixed the `apply_patch` tool silently overwriting pre-existing destinations for `*** Add File:` (create) and `*** Move to:` (rename), which could clobber unrelated content and, in the rename case, also delete the source. Both operations now reject upfront with an `ApplyPatchError` and leave the source and destination byte-identical ([#4074](https://github.com/can1357/oh-my-pi/issues/4074)). - Fixed multi-file `apply_patch` swallowing per-file failures: the aggregate result now stops at the first failing file, surfaces the applied vs. skipped file paths, and sets top-level `isError` so the agent loop and renderer take the error branch instead of treating a partial application as success ([#4074](https://github.com/can1357/oh-my-pi/issues/4074)). +### Fixed + +- Fixed git and GitHub CLI subprocesses hanging on interactive credential prompts or buffering unbounded output by forcing non-interactive env defaults, adding a timeout, and capping captured stdout/stderr. ([#4072](https://github.com/can1357/oh-my-pi/issues/4072)) ## [16.2.12] - 2026-07-01 diff --git a/packages/coding-agent/src/utils/git.ts b/packages/coding-agent/src/utils/git.ts index c9817b72a..870a3fd28 100644 --- a/packages/coding-agent/src/utils/git.ts +++ b/packages/coding-agent/src/utils/git.ts @@ -2,6 +2,7 @@ import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; import { $which, hasFsCode, isEisdir, isEnoent, isEnotdir, Snowflake } from "@oh-my-pi/pi-utils"; +import type { Subprocess } from "bun"; import { parseDiffHunks as parseCommitDiffHunks, parseFileDiffs, @@ -184,11 +185,144 @@ const AMBIENT_GIT_ENV = { GIT_ALTERNATE_OBJECT_DIRECTORIES: undefined, } satisfies Record; +const GIT_NON_INTERACTIVE_ENV = { + GIT_ASKPASS: "true", + GIT_EDITOR: "true", + GIT_TERMINAL_PROMPT: "0", + GPG_TTY: "not a tty", + SSH_ASKPASS: "/usr/bin/false", +} satisfies Record; +const GH_NON_INTERACTIVE_ENV = { + ...GIT_NON_INTERACTIVE_ENV, + GH_PROMPT_DISABLED: "1", +} satisfies Record; + +/** Default deadline for git and gh subprocesses spawned by the coding agent. */ +export const GIT_COMMAND_TIMEOUT_MS = 5 * 60 * 1000; +/** Maximum captured stdout or stderr bytes retained from git and gh subprocesses. */ +export const GIT_COMMAND_OUTPUT_LIMIT_BYTES = 8 * 1024 * 1024; + +const GIT_COMMAND_TIMEOUT_EXIT_CODE = 124; +const GIT_OUTPUT_TRUNCATED_MARKER = "\n[git subprocess output truncated after 8 MiB]\n"; + +type CommandName = "git" | "gh"; + +function resolveTimeoutMs(timeoutMs: number | undefined): number { + if (timeoutMs === undefined) return GIT_COMMAND_TIMEOUT_MS; + if (!Number.isFinite(timeoutMs) || timeoutMs < 0) return GIT_COMMAND_TIMEOUT_MS; + return Math.trunc(timeoutMs); +} + +function resolveOutputLimit(maxOutputBytes: number | undefined): number { + if (maxOutputBytes === undefined) return GIT_COMMAND_OUTPUT_LIMIT_BYTES; + if (!Number.isFinite(maxOutputBytes) || maxOutputBytes < 0) return GIT_COMMAND_OUTPUT_LIMIT_BYTES; + return Math.trunc(maxOutputBytes); +} + +function formatCommandLabel(command: CommandName, args: readonly string[]): string { + return `${command} ${args.join(" ")}`.trim(); +} + +async function waitForExitWithTimeout( + child: Subprocess, + commandLabel: string, + timeoutMs: number, +): Promise<{ exitCode: number | null; timedOut: false } | { timedOut: true; stderr: string }> { + if (timeoutMs === 0) { + child.kill("SIGTERM"); + return { timedOut: true, stderr: `${commandLabel} timed out after 0ms` }; + } + const timeout = Promise.withResolvers<"timeout">(); + const timer = setTimeout(() => timeout.resolve("timeout"), timeoutMs); + timer.unref?.(); + try { + const result = await Promise.race([ + child.exited.then(exitCode => ({ kind: "exit" as const, exitCode })), + timeout.promise.then(() => ({ kind: "timeout" as const })), + ]); + if (result.kind === "exit") { + return { timedOut: false, exitCode: result.exitCode }; + } + child.kill("SIGTERM"); + return { timedOut: true, stderr: `${commandLabel} timed out after ${timeoutMs}ms` }; + } finally { + clearTimeout(timer); + } +} + +async function readCappedText(stream: ReadableStream, maxBytes: number): Promise { + const reader = stream.getReader(); + const decoder = new TextDecoder(); + const chunks: string[] = []; + let remaining = maxBytes; + let truncated = false; + try { + while (true) { + const { done, value } = await reader.read(); + if (done) break; + if (!truncated && value.length <= remaining) { + chunks.push(decoder.decode(value, { stream: true })); + remaining -= value.length; + continue; + } + if (!truncated && remaining > 0) { + chunks.push(decoder.decode(value.subarray(0, remaining), { stream: true })); + remaining = 0; + } + truncated = true; + } + chunks.push(decoder.decode()); + if (truncated) chunks.push(GIT_OUTPUT_TRUNCATED_MARKER); + return chunks.join(""); + } finally { + reader.releaseLock(); + } +} + +async function cancelOutput(stream: ReadableStream): Promise { + try { + await stream.cancel(); + } catch { + // Best-effort cleanup after a timeout; the subprocess has already been signaled. + } +} + +async function collectSubprocessResult( + command: CommandName, + args: readonly string[], + child: Subprocess, + options: Pick = {}, +): Promise { + const stdoutStream = child.stdout; + const stderrStream = child.stderr; + if (!(stdoutStream instanceof ReadableStream) || !(stderrStream instanceof ReadableStream)) { + throw new Error(`Failed to capture ${command} command output.`); + } + const maxOutputBytes = resolveOutputLimit(options.maxOutputBytes); + const stdoutPromise = readCappedText(stdoutStream, maxOutputBytes); + const stderrPromise = readCappedText(stderrStream, maxOutputBytes); + const exit = await waitForExitWithTimeout( + child, + formatCommandLabel(command, args), + resolveTimeoutMs(options.timeoutMs), + ); + if (exit.timedOut) { + void stdoutPromise.catch(() => undefined); + void stderrPromise.catch(() => undefined); + await Promise.all([cancelOutput(stdoutStream), cancelOutput(stderrStream)]); + return { exitCode: GIT_COMMAND_TIMEOUT_EXIT_CODE, stdout: "", stderr: exit.stderr }; + } + const [stdout, stderr] = await Promise.all([stdoutPromise, stderrPromise]); + return { exitCode: exit.exitCode ?? 0, stdout, stderr }; +} + interface CommandOptions { readonly env?: Record; + readonly maxOutputBytes?: number; readonly readOnly?: boolean; readonly signal?: AbortSignal; readonly stdin?: string | Uint8Array | ArrayBuffer | SharedArrayBuffer; + readonly timeoutMs?: number; } function normalizeStdin(input: CommandOptions["stdin"]): "ignore" | Uint8Array { @@ -204,6 +338,7 @@ function buildGitEnv(overrides?: Record): Record; + +function createTextStream(text: string): ReadableStream { + const body = new Response(text).body; + if (!body) throw new Error("Failed to create response stream."); + return body; +} + +function createFakeProcess(stdout = "", stderr = "", exitCode = 0, exited?: Promise): Subprocess { + return { + pid: 12345, + stdout: createTextStream(stdout), + stderr: createTextStream(stderr), + exited: exited ?? Promise.resolve(exitCode), + kill: vi.fn(), + } as unknown as Subprocess; +} + +function createSpawnMock(factory: () => Subprocess, calls?: SpawnOptions[]) { + function mockSpawn(options: SpawnOptions & { cmd: string[] }): Subprocess; + function mockSpawn(cmd: string[], options?: SpawnOptions): Subprocess; + function mockSpawn(first: string[] | (SpawnOptions & { cmd: string[] }), second?: SpawnOptions): Subprocess { + if (calls) { + if (Array.isArray(first)) { + calls.push(second ?? {}); + } else { + const { cmd, ...options } = first; + void cmd; + calls.push(options); + } + } + return factory(); + } + + return mockSpawn; +} + +async function flushMicrotasks(): Promise { + await Promise.resolve(); + await Promise.resolve(); +} + +afterEach(() => { + vi.restoreAllMocks(); + vi.useRealTimers(); +}); + +describe("git subprocess safety", () => { + it("passes non-interactive credential env to git", async () => { + const calls: SpawnOptions[] = []; + vi.spyOn(Bun, "spawn").mockImplementation(createSpawnMock(() => createFakeProcess(), calls)); + + await git.push("/work/pi"); + + expect(calls[0]?.env?.GIT_TERMINAL_PROMPT).toBe("0"); + expect(calls[0]?.env?.GIT_ASKPASS).toBeDefined(); + expect(calls[0]?.env?.SSH_ASKPASS).toBeDefined(); + expect(calls[0]?.env?.GPG_TTY).toBe("not a tty"); + }); + + it("bounds captured stdout", async () => { + const tooLarge = "x".repeat(git.GIT_COMMAND_OUTPUT_LIMIT_BYTES + 1); + vi.spyOn(Bun, "spawn").mockImplementation(createSpawnMock(() => createFakeProcess(tooLarge))); + + const output = await git.show("/work/pi", "HEAD"); + + expect(output.length).toBeLessThanOrEqual(git.GIT_COMMAND_OUTPUT_LIMIT_BYTES + 200); + expect(output).toContain("truncated"); + }); + + it("kills git commands that exceed the subprocess timeout", async () => { + vi.useFakeTimers(); + const child = createFakeProcess("", "", 0, new Promise(() => {})); + vi.spyOn(Bun, "spawn").mockImplementation(createSpawnMock(() => child)); + + const failure = git.push("/work/pi").then( + () => undefined, + error => error, + ); + vi.advanceTimersByTime(git.GIT_COMMAND_TIMEOUT_MS); + await flushMicrotasks(); + const error = await failure; + + expect(child.kill).toHaveBeenCalledWith("SIGTERM"); + expect(error).toBeInstanceOf(git.GitCommandError); + expect(String(error.message)).toContain("timed out"); + }); +});