8e36f76cfa
* postrun: thread AgentRunContext through the retry loop instead of repackaging
drop the per-gate plumbing in `runPostRunRetryLoop`: the loop now receives
`ctx: AgentRunContext` whole and reads `ctx.stopScript` + `ctx.toolState.*`
directly. `getUnsubmittedReview` becomes a pure utility in postRun.ts
instead of a closure shipped over `AgentRunContext`. `AgentRunContext`
loses 4 fields that duplicated `toolState` (`summaryFilePath`,
`summarySeed`, `learningsFilePath`, `getUnsubmittedReview`) and gains
`toolState: ToolState`. both harness call sites collapse from 11 lines to
7; main.ts deletes the inline closure.
`ToolState` and friends move from `action/mcp/server.ts` to
`action/toolState.ts` so non-MCP code (agents, post-run loop) stops
importing run-state types from the MCP server module.
no behavior change. 503/503 tests green.
* toolState: relocate `CommentableLines` to break dep cycle with mcp/review
`action/toolState.ts` was importing `CommentableLines` from
`mcp/review.ts`, which pulled the entire MCP server compile graph (24
files) into any consumer of `ToolState` — including `cf-worker-indexing`
via the `pullfrog/internal` re-export chain through `utils/log.ts` →
`agents/shared.ts` → `toolState.ts`. that exposed a pre-existing TS
error in `mcp/issueEvents.ts` (octokit types resolve differently under
cf-worker's `moduleResolution: bundler`).
move `CommentableLines` (a small `{ RIGHT: Set<number>; LEFT: Set<number> }`
state-shape type) to `toolState.ts` where it's used; re-export from
`mcp/review.ts` for back-compat with test and call-site imports. cuts
cf-worker's mcp/ compile inclusion from 24 files back to 0.
* postRun: drop mock-heavy retry-loop tests; keep pure gate predicate
`runPostRunRetryLoop` and `executeStopHook` were covered by ~560 lines
of mock-heavy regression-gate tests that stubbed `spawn` / `getGitStatus`
and fabricated `AgentRunContext` to drive orchestration paths. per
AGENTS.md ("prefer no test over a mock-heavy test that only catches the
most obvious form of regression") and the empirical track record — the
one real production failure of this code path (#646) was a missing npm
release, not a logic bug a unit test could catch — the value-to-ceremony
ratio is poor. delete them.
keep only the pure predicate: `getUnsubmittedReview(toolState)` is a
decision function whose four input conditions have user-visible
consequences when wrong. 5 assertions, no mocks, no ctx fabrication.
488 tests still pass.
* toolState: import PrepResult from prep/types.ts, not the barrel
same dep-cycle class as the previous CommentableLines fix. importing
PrepResult from prep/index.ts pulled prep/installNodeDependencies.ts
into the Next.js production build's typecheck graph (via
pullfrog/internal → utils/log.ts → agents/shared.ts → toolState.ts →
prep/index.ts → installNodeDependencies.ts), and Next.js's stricter
NODE_ENV-required ProcessEnv shape rejected an existing
`env: { PATH: ... }` literal.
prep/types.ts is a leaf module with zero imports — re-routing the type
import severs the chain. Vercel preview deploy goes from Error → Ready;
preview-sync stops racing the deploy.
226 lines
8.3 KiB
TypeScript
226 lines
8.3 KiB
TypeScript
import { execFileSync, execSync } from "node:child_process";
|
|
import { mkdtempSync } from "node:fs";
|
|
import { tmpdir } from "node:os";
|
|
import { join } from "node:path";
|
|
import type { ShellPermission } from "../external.ts";
|
|
import type { ToolState } from "../toolState.ts";
|
|
import { log } from "./cli.ts";
|
|
import type { OctokitWithPlugins } from "./github.ts";
|
|
import { isInsideDocker } from "./globals.ts";
|
|
import { $ } from "./shell.ts";
|
|
|
|
export interface SetupOptions {
|
|
tempDir: string;
|
|
}
|
|
|
|
/**
|
|
* Create a shared temp directory for the action
|
|
*/
|
|
export function createTempDirectory(): string {
|
|
const sharedTempDir = mkdtempSync(join(tmpdir(), "pullfrog-"));
|
|
process.env.PULLFROG_TEMP_DIR = sharedTempDir;
|
|
log.info(`» created temp dir at ${sharedTempDir}`);
|
|
return sharedTempDir;
|
|
}
|
|
|
|
/**
|
|
* Setup the test repository for running actions
|
|
*/
|
|
export function setupTestRepo(options: SetupOptions): void {
|
|
const tempDir = options.tempDir;
|
|
const repo = process.env.GITHUB_REPOSITORY;
|
|
if (!repo) throw new Error("GITHUB_REPOSITORY is required");
|
|
log.info(`» cloning ${repo} into ${tempDir}...`);
|
|
|
|
// use https with token in ci or when running inside docker
|
|
if (process.env.CI || isInsideDocker) {
|
|
const token = process.env.GITHUB_TOKEN ?? process.env.GH_TOKEN;
|
|
if (!token) {
|
|
throw new Error("GITHUB_TOKEN or GH_TOKEN is required for https clone in ci or docker");
|
|
}
|
|
$("git", ["clone", `https://x-access-token:${token}@github.com/${repo}.git`, tempDir]);
|
|
} else {
|
|
$("git", ["clone", `git@github.com:${repo}.git`, tempDir]);
|
|
}
|
|
}
|
|
|
|
/**
|
|
* build an env suitable for targeting a specific git repo via `cwd`.
|
|
*
|
|
* inherited GIT_DIR / GIT_WORK_TREE / GIT_INDEX_FILE override cwd resolution,
|
|
* which matters when this code runs as a child of `git push` (pre-push hook)
|
|
* or inside another git subcommand. if we don't strip them, a call that
|
|
* names `repoDir` in cwd silently operates on the outer repo instead.
|
|
*/
|
|
function envScopedToRepo(): NodeJS.ProcessEnv {
|
|
const scoped = { ...process.env };
|
|
for (const key of Object.keys(scoped)) {
|
|
if (key.startsWith("GIT_")) delete scoped[key];
|
|
}
|
|
return scoped;
|
|
}
|
|
|
|
/**
|
|
* remove any `[includeIf ...]` entries from the local git config so that
|
|
* actions/checkout-persisted credentials don't ride alongside ASKPASS-provided
|
|
* auth for subsequent git operations.
|
|
*
|
|
* SECURITY: git config subsection values can contain arbitrary characters
|
|
* including `$(...)` command substitutions, and `${IFS}` spacing tricks defeat
|
|
* naive split-on-space filtering. we read keys via the `-z` (null-terminated)
|
|
* output format and feed them to a spawn-array `git config --unset-all` so
|
|
* the shell never interpolates key contents — closing the RCE path that a
|
|
* string-interpolated `execSync(...)` would expose.
|
|
*/
|
|
export function removeIncludeIfEntries(repoDir: string): void {
|
|
const env = envScopedToRepo();
|
|
let configOutput: string;
|
|
try {
|
|
configOutput = execSync("git config --local --get-regexp -z ^includeif\\.", {
|
|
cwd: repoDir,
|
|
encoding: "utf-8",
|
|
stdio: "pipe",
|
|
env,
|
|
});
|
|
} catch {
|
|
log.debug("» no includeIf credential entries to remove");
|
|
return;
|
|
}
|
|
const seen = new Set<string>();
|
|
for (const entry of configOutput.split("\0")) {
|
|
if (!entry) continue;
|
|
// -z format: each entry is "<key>\n<value>". the key is up to the first newline.
|
|
const nl = entry.indexOf("\n");
|
|
const key = nl === -1 ? entry : entry.slice(0, nl);
|
|
if (!key || seen.has(key)) continue;
|
|
seen.add(key);
|
|
try {
|
|
// execFileSync (not execSync) so the key — which can contain arbitrary
|
|
// characters including shell metacharacters and $() command substitutions
|
|
// — is passed as an argv element and never interpolated by a shell.
|
|
// this is the load-bearing side of a9aa3b2b's injection fix.
|
|
execFileSync("git", ["config", "--local", "--unset-all", key], {
|
|
cwd: repoDir,
|
|
stdio: "pipe",
|
|
env,
|
|
});
|
|
} catch (error) {
|
|
log.debug(
|
|
`» failed to unset ${key}: ${error instanceof Error ? error.message : String(error)}`
|
|
);
|
|
}
|
|
}
|
|
if (seen.size > 0)
|
|
log.info(
|
|
`» removed ${seen.size} includeIf credential ${seen.size === 1 ? "entry" : "entries"}`
|
|
);
|
|
}
|
|
|
|
export interface GitContext {
|
|
gitToken: string;
|
|
owner: string;
|
|
name: string;
|
|
octokit: OctokitWithPlugins;
|
|
toolState: ToolState;
|
|
// shell permission level — controls hook and security behavior:
|
|
// enabled: full shell, hooks run, no restrictions
|
|
// restricted: MCP shell in stripped env, hooks run, token protection on auth ops
|
|
// disabled: no shell, hooks disabled globally, all code execution paths blocked
|
|
shell: ShellPermission;
|
|
postCheckoutScript: string | null;
|
|
}
|
|
|
|
export type SetupGitParams = GitContext;
|
|
|
|
/**
|
|
* setup git configuration and authentication for the repository.
|
|
* - configures git identity (user.email, user.name)
|
|
* - sets up authentication via gitToken (minimal contents:write)
|
|
*
|
|
* gitToken is a minimal-permission token (contents + workflows) used for git operations.
|
|
* it is assumed to be potentially exfiltratable, so it has limited scope.
|
|
*/
|
|
export async function setupGit(params: SetupGitParams): Promise<void> {
|
|
const repoDir = process.cwd();
|
|
|
|
// 1. configure git identity
|
|
log.info("» setting up git configuration...");
|
|
try {
|
|
// check current config - only set defaults if not configured or using generic bot
|
|
let currentEmail = "";
|
|
try {
|
|
currentEmail = execSync("git config user.email", {
|
|
cwd: repoDir,
|
|
stdio: "pipe",
|
|
encoding: "utf-8",
|
|
}).trim();
|
|
} catch {
|
|
// not configured
|
|
}
|
|
|
|
const shouldSetDefaults =
|
|
!currentEmail || currentEmail === "github-actions[bot]@users.noreply.github.com";
|
|
|
|
if (shouldSetDefaults) {
|
|
execSync('git config --local user.email "226033991+pullfrog[bot]@users.noreply.github.com"', {
|
|
cwd: repoDir,
|
|
stdio: "pipe",
|
|
});
|
|
execSync('git config --local user.name "pullfrog[bot]"', {
|
|
cwd: repoDir,
|
|
stdio: "pipe",
|
|
});
|
|
log.debug("» git user configured (using defaults)");
|
|
} else {
|
|
log.debug(`» git user already configured (${currentEmail}), skipping`);
|
|
}
|
|
|
|
// SECURITY: disable git hooks when shell is disabled to prevent code execution.
|
|
// in restricted mode, hooks run in the stripped sandbox — that's fine.
|
|
// in enabled mode, the agent has full shell anyway.
|
|
// in disabled mode, hooks are the primary code-execution escape vector.
|
|
if (params.shell === "disabled") {
|
|
execSync("git config --local core.hooksPath /dev/null", {
|
|
cwd: repoDir,
|
|
stdio: "pipe",
|
|
});
|
|
log.debug("» git hooks disabled (shell=disabled)");
|
|
}
|
|
} catch (error) {
|
|
// If git config fails, log warning but don't fail the action
|
|
// This can happen if we're not in a git repo or git isn't available
|
|
log.info(`Failed to set git config: ${error instanceof Error ? error.message : String(error)}`);
|
|
}
|
|
|
|
// 2. setup authentication
|
|
// remove existing git auth headers that actions/checkout might have set
|
|
try {
|
|
execSync("git config --local --unset-all http.https://github.com/.extraheader", {
|
|
cwd: repoDir,
|
|
stdio: "pipe",
|
|
});
|
|
log.info("» removed existing authentication headers");
|
|
} catch {
|
|
log.debug("» no existing authentication headers to remove");
|
|
}
|
|
|
|
// remove includeIf entries that actions/checkout@v6 uses for credential persistence.
|
|
// v6 stores credentials in an external file loaded via includeIf.gitdir, which our
|
|
// --unset-all above doesn't catch. without this, stale credentials from actions/checkout
|
|
// would be sent alongside ASKPASS-provided credentials.
|
|
removeIncludeIfEntries(repoDir);
|
|
|
|
// SECURITY: set origin URL without token - auth is injected via GIT_ASKPASS
|
|
// in $git() calls. this prevents token leakage to git hooks and subprocesses.
|
|
const originUrl = `https://github.com/${params.owner}/${params.name}.git`;
|
|
$("git", ["remote", "set-url", "origin", originUrl], { cwd: repoDir });
|
|
|
|
// initialize pushUrl to base repo - may be updated by checkout_pr for fork PRs
|
|
params.toolState.pushUrl = originUrl;
|
|
|
|
// disable credential helpers to prevent prompts and ensure clean auth state
|
|
$("git", ["config", "--local", "credential.helper", ""], { cwd: repoDir });
|
|
|
|
log.info("» git authentication configured");
|
|
}
|