From e5878de9e413a02d4b05bd3e11fb5acd13318377 Mon Sep 17 00:00:00 2001 From: Colin McDonnell Date: Thu, 20 Nov 2025 15:37:08 -0800 Subject: [PATCH] Drop usage of execSync, switch to $ util --- mcp/files.ts | 15 +++++++---- mcp/pr.ts | 6 ++--- mcp/prInfo.ts | 8 +++--- scratch.ts | 21 +++++++-------- todo.md | 5 ++++ utils/setup.ts | 7 ++--- utils/shell.ts | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 106 insertions(+), 27 deletions(-) create mode 100644 utils/shell.ts diff --git a/mcp/files.ts b/mcp/files.ts index 8d566e7..f4f9555 100644 --- a/mcp/files.ts +++ b/mcp/files.ts @@ -1,5 +1,5 @@ -import { execSync } from "node:child_process"; import { type } from "arktype"; +import { $ } from "../utils/shell.ts"; import { contextualize, tool } from "./shared.ts"; export const ListFiles = type({ @@ -17,14 +17,19 @@ export const ListFilesTool = tool({ try { // Use git ls-files to list tracked files // This respects .gitignore and gives a clean list of source files - const command = path === "." ? "git ls-files" : `git ls-files ${path}`; - const output = execSync(command, { encoding: "utf-8" }); + const pathStr = path ?? "."; + const output = $("git", pathStr === "." ? ["ls-files"] : ["ls-files", pathStr], { + log: false, + }); const files = output.split("\n").filter((f) => f.trim() !== ""); if (files.length === 0) { // Fallback for non-git environments or untracked files - const findCmd = `find ${path} -maxdepth 3 -not -path '*/.*' -type f`; - const findOutput = execSync(findCmd, { encoding: "utf-8" }); + const findOutput = $( + "find", + [pathStr, "-maxdepth", "3", "-not", "-path", "*/.*", "-type", "f"], + { log: false } + ); return { files: findOutput.split("\n").filter((f) => f.trim() !== ""), method: "find", diff --git a/mcp/pr.ts b/mcp/pr.ts index 1e14350..abcfce7 100644 --- a/mcp/pr.ts +++ b/mcp/pr.ts @@ -1,6 +1,6 @@ -import { execSync } from "node:child_process"; import { type } from "arktype"; import { log } from "../utils/cli.ts"; +import { $ } from "../utils/shell.ts"; import { contextualize, tool } from "./shared.ts"; export const PullRequest = type({ @@ -15,9 +15,7 @@ export const PullRequestTool = tool({ parameters: PullRequest, execute: contextualize(async ({ title, body, base }, ctx) => { // Get the current branch name - const currentBranch = execSync("git rev-parse --abbrev-ref HEAD", { - encoding: "utf8", - }).trim(); + const currentBranch = $("git", ["rev-parse", "--abbrev-ref", "HEAD"], { log: false }); log.info(`Current branch: ${currentBranch}`); diff --git a/mcp/prInfo.ts b/mcp/prInfo.ts index 857bb30..7e54698 100644 --- a/mcp/prInfo.ts +++ b/mcp/prInfo.ts @@ -1,6 +1,6 @@ -import { execSync } from "node:child_process"; import { type } from "arktype"; import { log } from "../utils/cli.ts"; +import { $ } from "../utils/shell.ts"; import { contextualize, tool } from "./shared.ts"; export const PullRequestInfo = type({ @@ -30,14 +30,14 @@ export const PullRequestInfoTool = tool({ // Automatically fetch and checkout branches for review log.info(`Fetching base branch: origin/${baseBranch}`); - execSync(`git fetch origin ${baseBranch} --depth=20`, { stdio: "inherit" }); + $("git", ["fetch", "origin", baseBranch, "--depth=20"]); log.info(`Fetching PR branch: origin/${headBranch}`); - execSync(`git fetch origin ${headBranch}`, { stdio: "inherit" }); + $("git", ["fetch", "origin", headBranch]); log.info(`Checking out PR branch: origin/${headBranch}`); // check out a local branch tracking the remote branch so we can push changes - execSync(`git checkout -B ${headBranch} origin/${headBranch}`, { stdio: "inherit" }); + $("git", ["checkout", "-B", headBranch, `origin/${headBranch}`]); return { number: data.number, diff --git a/scratch.ts b/scratch.ts index ef1edc8..64ee81b 100644 --- a/scratch.ts +++ b/scratch.ts @@ -2,15 +2,14 @@ import { spawnSync } from "child_process"; import { existsSync } from "fs"; function findCliPath(name: string): string | null { - - const result = spawnSync("which", [name], { encoding: "utf-8" }); - if (result.status === 0 && result.stdout) { - const cliPath = result.stdout.trim(); - if (cliPath && existsSync(cliPath)) { - return cliPath; - } - } - return null; + const result = spawnSync("which", [name], { encoding: "utf-8" }); + if (result.status === 0 && result.stdout) { + const cliPath = result.stdout.trim(); + if (cliPath && existsSync(cliPath)) { + return cliPath; + } } - -console.log(findCliPath("codei")); \ No newline at end of file + return null; +} + +console.log(findCliPath("codei")); diff --git a/todo.md b/todo.md index a7d7984..4f13abd 100644 --- a/todo.md +++ b/todo.md @@ -8,6 +8,11 @@ [] add footer to the working comment ("executed by {agent}", link to pullfrog (homepage) w/ small logo?, feedback (create github issue), link to workflow run)- see https://github.com/colinhacks/zod/issues/5459#issuecomment-3548382991 [] avoid passing all of process.env into agents: minimum # of vars [] toon encode in prompt +[] branching: use pullfrog/ prefix +[] test modifications to existing PRs (proper branching) +[] web access settings +[] implement Learnings +[] implement Billing ## MAYBE diff --git a/utils/setup.ts b/utils/setup.ts index 087573b..e204569 100644 --- a/utils/setup.ts +++ b/utils/setup.ts @@ -2,6 +2,7 @@ import { execSync } from "node:child_process"; import { existsSync, rmSync } from "node:fs"; import { log } from "./cli.ts"; import type { RepoContext } from "./github.ts"; +import { $ } from "./shell.ts"; export interface SetupOptions { tempDir: string; @@ -25,7 +26,7 @@ export function setupTestRepo(options: SetupOptions): void { rmSync(tempDir, { recursive: true, force: true }); log.info("📦 Cloning pullfrogai/scratch into .temp..."); - execSync(`git clone ${repoUrl} ${tempDir}`, { stdio: "inherit" }); + $("git", ["clone", repoUrl, tempDir]); } else { log.info("📦 Resetting existing .temp repository..."); execSync("git reset --hard HEAD && git clean -fd", { @@ -35,7 +36,7 @@ export function setupTestRepo(options: SetupOptions): void { } } else { log.info("📦 Cloning pullfrogai/scratch into .temp..."); - execSync(`git clone ${repoUrl} ${tempDir}`, { stdio: "inherit" }); + $("git", ["clone", repoUrl, tempDir]); } } @@ -87,6 +88,6 @@ export function setupGitAuth(githubToken: string, repoContext: RepoContext): voi // Update remote URL to embed the token const remoteUrl = `https://x-access-token:${githubToken}@github.com/${repoContext.owner}/${repoContext.name}.git`; - execSync(`git remote set-url origin "${remoteUrl}"`, { stdio: "inherit" }); + $("git", ["remote", "set-url", "origin", remoteUrl]); log.info("✓ Updated remote URL with authentication token"); } diff --git a/utils/shell.ts b/utils/shell.ts new file mode 100644 index 0000000..fe85b11 --- /dev/null +++ b/utils/shell.ts @@ -0,0 +1,71 @@ +import { spawnSync } from "node:child_process"; + +interface ShellOptions { + cwd?: string; + encoding?: + | "utf-8" + | "utf8" + | "ascii" + | "base64" + | "base64url" + | "hex" + | "latin1" + | "ucs-2" + | "ucs2" + | "utf16le"; + log?: boolean; + onError?: (result: { status: number; stdout: string; stderr: string }) => void; +} + +/** + * Execute a shell command safely using spawnSync with argument arrays. + * Prevents shell injection by avoiding string interpolation in shell commands. + * + * @param cmd - The command to execute + * @param args - Array of arguments to pass to the command + * @param options - Optional configuration (cwd, encoding, onError) + * @returns The trimmed stdout output + * @throws Error if command fails and no onError handler is provided + */ +export function $(cmd: string, args: string[], options?: ShellOptions): string { + const encoding = options?.encoding ?? "utf-8"; + const result = spawnSync(cmd, args, { + stdio: ["inherit", "pipe", "pipe"], + encoding, + cwd: options?.cwd, + }); + + const stdout = result.stdout ?? ""; + const stderr = result.stderr ?? ""; + + // Write output to process streams so it behaves like stdio: "inherit" + // Only log if log option is not explicitly set to false + if (options?.log !== false) { + if (stdout) { + process.stdout.write(stdout); + } + if (stderr) { + process.stderr.write(stderr); + } + } + + // Handle errors + if (result.status !== 0) { + const errorResult = { + status: result.status ?? -1, + stdout, + stderr, + }; + + if (options?.onError) { + options.onError(errorResult); + return stdout.trim(); + } + + throw new Error( + `Command failed with exit code ${errorResult.status}: ${stderr || "Unknown error"}` + ); + } + + return stdout.trim(); +}