ShockVPN/shockbot
0
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat: adapt pullfrog for gitea + ollama

Browse Source
This commit is contained in:
wolfy
2026-05-31 01:01:00 -05:00
parent 36ac64a5b6
commit 2aca1a3aa3
183 changed files with 1419 additions and 28292 deletions
Download Patch File Download Diff File Expand all files Collapse all files
mcp/checkSuite.ts
-257
View File
@@ -1,257 +0,0 @@
import { mkdirSync, writeFileSync } from "node:fs";
import { join } from "node:path";
import { type } from "arktype";
import { log } from "../utils/log.ts";
import type { ToolContext } from "./server.ts";
import { execute, tool } from "./shared.ts";
export const GetCheckSuiteLogs = type({
check_suite_id: type.number.describe("the id from check_suite.id"),
});
type LogLine = {
line: number;
content: string;
type: "error" | "warning" | "failure" | "trace";
};
type LogAnalysis = {
totalLines: number;
index: LogLine[];
excerpt: {
content: string;
startLine: number;
endLine: number;
};
};
function analyzeLog(logs: string, excerptLines = 80): LogAnalysis {
// biome-ignore lint/suspicious/noControlCharactersInRegex: ANSI escape codes use control chars
const clean = logs.replace(/\x1b\[[0-9;]*m/g, "");
const lines = clean.split("\n");
const totalLines = lines.length;
const index: LogLine[] = [];
const patterns: Array<{ type: LogLine["type"]; pattern: RegExp; skip?: RegExp }> = [
{ type: "error", pattern: /##\[error\]/i },
{ type: "error", pattern: /\bError:/i },
{ type: "error", pattern: /\bERR_/i },
{ type: "error", pattern: /exit code [1-9]/i },
{ type: "warning", pattern: /##\[warning\]/i },
{ type: "warning", pattern: /\bWARN\b/i, skip: /apt|dpkg|Reading package/i },
{ type: "failure", pattern: /\d+ failed/i },
{ type: "failure", pattern: /FAIL\b/i },
{ type: "failure", pattern: /✕|✗|×/ },
{ type: "trace", pattern: /^\s+at\s+/i },
];
for (let i = 0; i < lines.length; i++) {
const line = lines[i];
for (const p of patterns) {
if (p.pattern.test(line)) {
if (p.skip?.test(line)) continue;
// dedupe consecutive traces
if (p.type === "trace" && index.length > 0 && index[index.length - 1].type === "trace") {
continue;
}
// truncate long lines
const truncated = line.length > 120 ? line.slice(0, 117) + "..." : line;
index.push({
line: i + 1,
content: truncated.trim(),
type: p.type,
});
break;
}
}
}
// find excerpt range: focus on LAST ##[error] line
let errorLine = -1;
for (let i = lines.length - 1; i >= 0; i--) {
if (/##\[error\]/i.test(lines[i])) {
errorLine = i;
break;
}
}
let start: number;
let end: number;
if (errorLine === -1) {
start = Math.max(0, totalLines - excerptLines);
end = totalLines;
} else {
const contextAfter = 5;
const contextBefore = excerptLines - contextAfter;
start = Math.max(0, errorLine - contextBefore);
end = Math.min(totalLines, errorLine + contextAfter);
}
return {
totalLines,
index,
excerpt: {
content: lines.slice(start, end).join("\n"),
startLine: start + 1,
endLine: end,
},
};
}
type JobLogResult = {
job_id: number;
job_name: string;
job_url: string;
failed_steps: string[];
log_index: LogLine[];
excerpt: {
start_line: number;
end_line: number;
total_lines: number;
content: string;
};
full_log_path: string;
};
export function GetCheckSuiteLogsTool(ctx: ToolContext) {
return tool({
name: "get_check_suite_logs",
description:
"get workflow run logs for a failed check suite. returns a log_index of interesting lines, " +
"a curated excerpt, and full_log_path for deeper investigation. " +
"pass check_suite.id from the webhook payload.",
parameters: GetCheckSuiteLogs,
execute: execute(async (params) => {
const check_suite_id = params.check_suite_id;
// get workflow runs for this specific check suite
const workflowRuns = await ctx.octokit.paginate(
ctx.octokit.rest.actions.listWorkflowRunsForRepo,
{
owner: ctx.repo.owner,
repo: ctx.repo.name,
check_suite_id,
per_page: 100,
request: { signal: AbortSignal.timeout(10_000) },
}
);
const failedRuns = workflowRuns.filter((run) => run.conclusion === "failure");
if (failedRuns.length === 0) {
return {
check_suite_id,
message: "no failed workflow runs found for this check suite",
failed_jobs: [],
};
}
// setup logs directory
const tempDir = process.env.PULLFROG_TEMP_DIR;
if (!tempDir) {
throw new Error("PULLFROG_TEMP_DIR not set");
}
const logsDir = join(tempDir, "ci-logs");
mkdirSync(logsDir, { recursive: true });
const jobResults: JobLogResult[] = [];
// get logs for each failed run
for (const run of failedRuns) {
const jobs = await ctx.octokit.paginate(ctx.octokit.rest.actions.listJobsForWorkflowRun, {
owner: ctx.repo.owner,
repo: ctx.repo.name,
run_id: run.id,
request: { signal: AbortSignal.timeout(10_000) },
});
// only process failed jobs
const failedJobs = jobs.filter((job) => job.conclusion === "failure");
for (const job of failedJobs) {
try {
const logsResponse = await ctx.octokit.rest.actions.downloadJobLogsForWorkflowRun({
owner: ctx.repo.owner,
repo: ctx.repo.name,
job_id: job.id,
request: { signal: AbortSignal.timeout(10_000) },
});
const logsUrl = logsResponse.url;
const logsResult = await fetch(logsUrl, { signal: AbortSignal.timeout(10_000) });
if (!logsResult.ok) {
throw new Error(
`failed to fetch logs: ${logsResult.status} ${logsResult.statusText}`
);
}
const logsText = await logsResult.text();
// write full log to disk
const logPath = join(logsDir, `job-${job.id}.log`);
writeFileSync(logPath, logsText);
// analyze log
const analysis = analyzeLog(logsText, 80);
// get failed steps
const failedSteps =
job.steps
?.filter((s) => s.conclusion === "failure")
.map((s) => `Step ${s.number}: ${s.name}`) ?? [];
jobResults.push({
job_id: job.id,
job_name: job.name,
job_url: job.html_url ?? "",
failed_steps: failedSteps,
log_index: analysis.index,
excerpt: {
start_line: analysis.excerpt.startLine,
end_line: analysis.excerpt.endLine,
total_lines: analysis.totalLines,
content: analysis.excerpt.content,
},
full_log_path: logPath,
});
log.debug(`analyzed logs for job ${job.name}: ${analysis.index.length} indexed lines`);
} catch (error) {
log.info(`failed to fetch logs for job ${job.id}: ${error}`);
}
}
}
return {
_instructions: {
overview:
"this result contains CI failure information. use log_index to find interesting lines, then read full_log_path for details.",
fields: {
log_index:
"array of interesting lines (errors, warnings, failures) with line numbers. use these to navigate the full log.",
excerpt:
"a curated ~80 line window around the last error. may not show all failures if they occur in different places.",
full_log_path:
"path to the complete log file. read specific line ranges using the line numbers from log_index.",
failed_steps:
"which CI steps failed. read the workflow yml to understand what commands these steps run.",
},
workflow: [
"1. scan log_index to see where errors/warnings/failures are located",
"2. read excerpt for immediate context",
"3. if excerpt doesn't show what you need, read specific line ranges from full_log_path",
"4. check failed_steps to understand what command failed",
],
},
check_suite_id,
repo: `${ctx.repo.owner}/${ctx.repo.name}`,
failed_jobs: jobResults,
};
}),
});
}
mcp/checkout.test.ts
-70
View File
@@ -1,70 +0,0 @@
import { readFileSync } from "node:fs";
import { resolve } from "node:path";
import { describe, expect, it } from "vitest";
import { type FormatFilesResult, formatFilesWithLineNumbers } from "./checkout.ts";
/**
* parses TOC entries like "- src/math.ts → lines 7-42 · diff-<hex>" into structured data.
*/
function parseTocEntries(toc: string) {
const entries: Array<{ filename: string; startLine: number; endLine: number }> = [];
for (const line of toc.split("\n")) {
const match = line.match(/^- (.+) → lines (\d+)-(\d+) · diff-[0-9a-f]+$/);
if (match) {
entries.push({
filename: match[1],
startLine: parseInt(match[2], 10),
endLine: parseInt(match[3], 10),
});
}
}
return entries;
}
// fixture captured by action/scripts/refresh-test-fixtures.ts. running
// the formatter against checked-in JSON keeps this test offline and
// deterministic — re-fetch the fixture (with creds) when GitHub's
// pulls.listFiles response shape changes, then review the snapshot diff.
type DiffFixture = {
owner: string;
name: string;
pullNumber: number;
files: Parameters<typeof formatFilesWithLineNumbers>[0];
};
function loadFixture<T>(file: string): T {
return JSON.parse(readFileSync(resolve(import.meta.dirname, "__fixtures__", file), "utf-8")) as T;
}
describe("formatFilesWithLineNumbers", () => {
it("generates accurate TOC line numbers for pullfrog/test-repo#1", () => {
const fx = loadFixture<DiffFixture>("pullfrog-test-repo-pr-1.diff.json");
const result: FormatFilesResult = formatFilesWithLineNumbers(fx.files);
expect(result.content.startsWith(result.toc)).toBe(true);
const contentLines = result.content.split("\n");
const tocEntries = parseTocEntries(result.toc);
expect(tocEntries.length).toBeGreaterThan(0);
for (const entry of tocEntries) {
// line numbers are 1-indexed, arrays are 0-indexed
const firstLine = contentLines[entry.startLine - 1];
expect(firstLine).toBeDefined();
// first line of each file section should be the diff header
expect(firstLine).toBe(`diff --git a/${entry.filename} b/${entry.filename}`);
expect(entry.endLine).toBeLessThanOrEqual(contentLines.length);
}
// verify adjacent files don't overlap and are contiguous
for (let i = 1; i < tocEntries.length; i++) {
const prev = tocEntries[i - 1];
const curr = tocEntries[i];
expect(curr.startLine).toBe(prev.endLine + 1);
}
expect(result.toc).toMatchSnapshot("toc");
expect(result.content).toMatchSnapshot("content");
});
});
mcp/checkout.ts
+145 -505
View File
@@ -1,11 +1,12 @@
import { createHash } from "node:crypto";
import { statSync, unlinkSync, writeFileSync } from "node:fs";
import { join } from "node:path";
import type { Octokit, RestEndpointMethodTypes } from "@octokit/rest";
import { type } from "arktype";
import { log } from "../utils/cli.ts";
import { countLines, createDiffCoverageState } from "../utils/diffCoverage.ts";
import { $git, $gitFetchWithDeepen } from "../utils/gitAuth.ts";
import type { Gitea } from "../utils/gitea.ts";
import type { ChangedFileWithPatch } from "../utils/gitea.ts";
import { executeLifecycleHook } from "../utils/lifecycle.ts";
import { computeIncrementalDiff } from "../utils/rangeDiff.ts";
import { retry } from "../utils/retry.ts";
@@ -15,123 +16,89 @@ import { commentableLinesForFile } from "./review.ts";
import type { ToolContext } from "./server.ts";
import { execute, tool } from "./shared.ts";
type PullFile = RestEndpointMethodTypes["pulls"]["listFiles"]["response"]["data"][number];
export type FormatFilesResult = {
content: string;
toc: string;
};
export type FormatFilesResult = { content: string; toc: string };
export type FetchAndFormatPrDiffResult = FormatFilesResult & {
files: PullFile[];
files: DiffFile[];
};
/**
* formats PR files with explicit line numbers for each code line.
* preserves all original diff info (file headers, hunk headers) and adds:
* | OLD | NEW | TYPE | code
* returns both the formatted content and a TOC with line ranges per file.
*/
export function formatFilesWithLineNumbers(files: PullFile[]): FormatFilesResult {
export type DiffFile = {
filename?: string | undefined;
patch?: string | undefined;
};
export function formatFilesWithLineNumbers(files: DiffFile[]): FormatFilesResult {
const output: string[] = [];
const tocEntries: Array<{ filename: string; startLine: number; endLine: number }> = [];
// calculate TOC header size: "## Files (N)\n" + N entries + "\n---\n\n"
const tocHeaderSize = 1 + files.length + 2;
let currentLine = tocHeaderSize + 1;
for (const file of files) {
const filename = file.filename ?? "(unknown)";
const fileStartLine = currentLine;
// file header
output.push(`diff --git a/${file.filename} b/${file.filename}`);
output.push(`--- a/${file.filename}`);
output.push(`+++ b/${file.filename}`);
output.push(`diff --git a/${filename} b/${filename}`);
output.push(`--- a/${filename}`);
output.push(`+++ b/${filename}`);
currentLine += 3;
if (!file.patch) {
output.push("(binary file or no changes)");
output.push("");
currentLine += 2;
tocEntries.push({
filename: file.filename,
startLine: fileStartLine,
endLine: currentLine - 1,
});
tocEntries.push({ filename, startLine: fileStartLine, endLine: currentLine - 1 });
continue;
}
// parse and format the patch with line numbers
const lines = file.patch.split("\n");
let oldLine = 0;
let newLine = 0;
for (const line of lines) {
// hunk header: @@ -OLD,COUNT +NEW,COUNT @@ optional context
const hunkMatch = line.match(/^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@/);
if (hunkMatch) {
oldLine = parseInt(hunkMatch[1], 10);
newLine = parseInt(hunkMatch[2], 10);
output.push(line); // pass through unchanged
output.push(line);
currentLine++;
continue;
}
// code lines within hunks
const changeType = line[0] || " ";
const code = line.slice(1);
if (changeType === "-") {
// removed line: show old line number, no new line number
output.push(`| ${padNum(oldLine)} | | - | ${code}`);
oldLine++;
} else if (changeType === "+") {
// added line: no old line number, show new line number
output.push(`| | ${padNum(newLine)} | + | ${code}`);
newLine++;
} else if (changeType === " " || changeType === "\\") {
// context line or "\ No newline at end of file"
if (changeType === "\\") {
output.push(line); // pass through as-is
output.push(line);
} else {
output.push(`| ${padNum(oldLine)} | ${padNum(newLine)} | | ${code}`);
oldLine++;
newLine++;
}
} else {
// unknown line type, pass through
output.push(line);
}
currentLine++;
}
output.push(""); // blank line between files
output.push("");
currentLine++;
tocEntries.push({
filename: file.filename,
startLine: fileStartLine,
endLine: currentLine - 1,
});
tocEntries.push({ filename, startLine: fileStartLine, endLine: currentLine - 1 });
}
// build TOC. each entry includes the precomputed sha256 anchor used in
// github PR Files Changed URLs (#diff-<hex>), so the agent never needs to
// shell out to sha256sum.
const tocLines = [`## Files (${files.length})`];
for (const entry of tocEntries) {
const anchor = createHash("sha256").update(entry.filename).digest("hex");
tocLines.push(
`- ${entry.filename} → lines ${entry.startLine}-${entry.endLine} · diff-${anchor}`
);
tocLines.push(`- ${entry.filename} → lines ${entry.startLine}-${entry.endLine} · diff-${anchor}`);
}
tocLines.push("");
tocLines.push("---");
tocLines.push("");
tocLines.push("", "---", "");
const toc = tocLines.join("\n");
const content = toc + output.join("\n");
return { content, toc };
return { content: toc + output.join("\n"), toc };
}
function padNum(n: number): string {
@@ -159,33 +126,30 @@ export type CheckoutPrResult = {
toc: string;
commitCount: number;
commitLog: string;
/** true when commitLog was capped because the PR has more commits than we render */
commitLogTruncated: boolean;
/** true when commit metadata could not be computed (e.g. base ref unreachable after shallow fetch). commitCount/commitLog are zero/empty in that case, not "no commits". */
commitLogUnavailable: boolean;
/** non-fatal warning from the post-checkout lifecycle hook, if any */
hookWarning?: string | undefined;
instructions: string;
};
/**
* fetches PR files from GitHub and formats them with line numbers and TOC.
* this is the core diff formatting logic, extracted for testability.
*/
export async function fetchAndFormatPrDiff(
ctx: ToolContext,
pullNumber: number
): Promise<FetchAndFormatPrDiffResult> {
const files = await ctx.octokit.paginate(ctx.octokit.rest.pulls.listFiles, {
const raw = await ctx.gitea.paginate(ctx.gitea.rest.repository.repoGetPullRequestFiles, {
owner: ctx.repo.owner,
repo: ctx.repo.name,
pull_number: pullNumber,
per_page: 100,
index: pullNumber,
});
// ChangedFile from the SDK omits `patch`; Gitea does return it, so we map here.
const files: DiffFile[] = raw.map((f) => ({
filename: f.filename,
patch: (f as unknown as { patch?: string }).patch,
}));
return { ...formatFilesWithLineNumbers(files), files };
}
import { captureInitialHead, type GitContext } from "../utils/setup.ts";
import { captureInitialHead } from "../utils/setup.ts";
export type PrData = {
number: number;
@@ -197,74 +161,78 @@ export type PrData = {
maintainerCanModify: boolean;
};
type EnsureBeforeShaParams = {
sha: string;
octokit: Octokit;
owner: string;
repo: string;
const STALE_LOCK_AGE_MS = 30_000;
const PULL_REF_RETRY_DELAYS_MS = [2_000, 5_000, 10_000];
const PULL_REF_MISSING_PATTERN = /couldn't find remote ref pull\/\d+\/head/i;
const GIT_LOCK_PATHS = [".git/shallow.lock", ".git/index.lock", ".git/objects/maintenance.lock"] as const;
function cleanupStaleGitLocks(): void {
const now = Date.now();
for (const relPath of GIT_LOCK_PATHS) {
let mtimeMs: number;
try { mtimeMs = statSync(relPath).mtimeMs; } catch { continue; }
if (now - mtimeMs < STALE_LOCK_AGE_MS) continue;
try { unlinkSync(relPath); log.warning(`» removed stale ${relPath}`); } catch {}
}
}
type CheckoutPrBranchParams = {
gitToken: string;
isShallow: boolean;
owner: string;
name: string;
gitea: Gitea;
toolState: import("../toolState.ts").ToolState;
shell: import("../external.ts").ShellPermission;
postCheckoutScript: string | null;
beforeSha?: string | undefined;
};
type CreateTempBranchParams = {
octokit: Octokit;
owner: string;
repo: string;
ref: string;
sha: string;
};
async function abortIfPullRequestMoved(args: { gitea: Gitea; owner: string; repo: string; pr: PrData }): Promise<void> {
try {
const data = (await args.gitea.rest.repository.repoGetPullRequest({ owner: args.owner, repo: args.repo, index: args.pr.number })).data;
if (data.state !== "open" || data.head?.sha !== args.pr.headSha) {
throw new Error(`PR #${args.pr.number} is no longer in the state it was at dispatch. Aborting.`);
}
} catch (e) {
if (e instanceof Error && e.message.includes("no longer")) throw e;
// API error — lenient, don't abort
}
}
type CreateTempBranchParams = { gitea: Gitea; owner: string; repo: string; branchName: string; sha: string };
async function createTempBranch(params: CreateTempBranchParams) {
const response = await params.octokit.rest.git.createRef({
await params.gitea.rest.repository.repoCreateBranch({
owner: params.owner,
repo: params.repo,
ref: `refs/heads/${params.ref}`,
sha: params.sha,
body: { new_branch_name: params.branchName, old_ref_name: params.sha },
});
return {
data: response.data,
async [Symbol.asyncDispose]() {
try {
await params.octokit.rest.git.deleteRef({
owner: params.owner,
repo: params.repo,
ref: `heads/${params.ref}`,
});
log.debug(`» deleted temp branch ${params.ref}`);
await params.gitea.rest.repository.repoDeleteBranch({ owner: params.owner, repo: params.repo, branch: params.branchName });
log.debug(`» deleted temp branch ${params.branchName}`);
} catch (e) {
log.debug(
`» failed to delete temp branch ${params.ref}: ${e instanceof Error ? e.message : String(e)}`
);
log.debug(`» failed to delete temp branch ${params.branchName}: ${e instanceof Error ? e.message : String(e)}`);
}
},
};
}
async function ensureBeforeShaReachable(params: EnsureBeforeShaParams): Promise<boolean> {
try {
$("git", ["cat-file", "-t", params.sha], { log: false });
log.debug(`» before_sha ${params.sha.slice(0, 7)} is reachable`);
return true;
} catch {
// not available locally — create a temporary branch to fetch it
}
async function ensureBeforeShaReachable(params: {
sha: string; gitea: Gitea; owner: string; repo: string; gitToken: string; isShallow: boolean;
}): Promise<boolean> {
try { $("git", ["cat-file", "-t", params.sha], { log: false }); return true; } catch {}
const tempBranch = `pullfrog/tmp/${params.sha.slice(0, 12)}`;
const branchName = `shockbot/tmp/${params.sha.slice(0, 12)}`;
try {
log.debug(`» before_sha ${params.sha.slice(0, 7)} not reachable, creating temp branch...`);
await using _ref = await createTempBranch({
octokit: params.octokit,
owner: params.owner,
repo: params.repo,
sha: params.sha,
ref: tempBranch,
});
await using _ref = await createTempBranch({ gitea: params.gitea, owner: params.owner, repo: params.repo, branchName, sha: params.sha });
await $gitFetchWithDeepen(
["--no-tags", ...(params.isShallow ? ["--depth=1"] : []), "origin", tempBranch],
["--no-tags", ...(params.isShallow ? ["--depth=1"] : []), "origin", branchName],
{ token: params.gitToken },
`before_sha temp branch ${tempBranch}`
`before_sha temp branch ${branchName}`
);
log.debug(`» fetched before_sha via temp branch ${tempBranch}`);
return true;
} catch (e) {
log.debug(`» failed to fetch before_sha: ${e instanceof Error ? e.message : String(e)}`);
@@ -272,174 +240,30 @@ async function ensureBeforeShaReachable(params: EnsureBeforeShaParams): Promise<
}
}
type CheckoutPrBranchParams = GitContext & {
beforeSha?: string | undefined;
};
// stale lock files left over from a crashed/cancelled prior git process block
// every subsequent fetch with `Unable to create '<path>': File exists`. only
// sweep locks older than this threshold so we never race a concurrent
// legitimate git op that's holding the lock.
const STALE_LOCK_AGE_MS = 30_000;
// PR head refs (refs/pull/N/head) sometimes lag the pull_request.opened
// webhook by a few seconds. retry the missing-ref case with backoff
// before giving up — see issue #591.
const PULL_REF_RETRY_DELAYS_MS = [2_000, 5_000, 10_000];
const PULL_REF_MISSING_PATTERN = /couldn't find remote ref pull\/\d+\/head/i;
const GIT_LOCK_PATHS = [
".git/shallow.lock",
".git/index.lock",
".git/objects/maintenance.lock",
] as const;
function cleanupStaleGitLocks(): void {
const now = Date.now();
for (const relPath of GIT_LOCK_PATHS) {
let mtimeMs: number;
try {
mtimeMs = statSync(relPath).mtimeMs;
} catch {
continue;
}
if (now - mtimeMs < STALE_LOCK_AGE_MS) continue;
try {
unlinkSync(relPath);
log.warning(`» removed stale ${relPath} from prior run`);
} catch (e) {
log.debug(
`» failed to remove stale ${relPath}: ${e instanceof Error ? e.message : String(e)}`
);
}
}
}
/**
* Returns false when a PR's current state diverges from what we dispatched
* on (closed/merged, or head SHA differs from pr.headSha). Used to short-
* circuit the pull/N/head retry loop when the ref is missing because the
* PR has moved on, not because of a webhook race.
*
* Network failures here are treated as "still valid" — we'd rather burn the
* retry budget than wrongly abort on a transient API blip.
*
* Note: this answers "should we keep trying?", NOT "will the next fetch
* succeed?". `pulls.get` (REST API) and `pull/N/head` (git ref) are served
* by independent GitHub replicas with their own propagation lag, so
* `pulls.get` reporting an open PR with a matching head SHA does not
* guarantee the git ref is yet visible — and vice versa (see issue #591
* for the original webhook-vs-ref replication-lag context).
*/
async function isPullRequestStillDispatchable(args: {
octokit: Octokit;
owner: string;
repo: string;
pr: PrData;
}): Promise<boolean> {
try {
const { data } = await args.octokit.rest.pulls.get({
owner: args.owner,
repo: args.repo,
pull_number: args.pr.number,
});
if (data.state !== "open") return false;
if (data.head.sha !== args.pr.headSha) return false;
return true;
} catch {
// lenient — don't abort on API hiccups
return true;
}
}
/**
* Throws the friendly clean-abort error when the PR has moved on since
* dispatch. Wraps `isPullRequestStillDispatchable` so the abort message
* lives in one place and is invoked from the inner `catch` around the
* `pull/N/head` fetch on every missing-ref failure.
*/
async function abortIfPullRequestMoved(args: {
octokit: Octokit;
owner: string;
repo: string;
pr: PrData;
}): Promise<void> {
const stillValid = await isPullRequestStillDispatchable(args);
if (stillValid) return;
throw new Error(
`PR #${args.pr.number} is no longer in the state it was at dispatch (likely closed, merged, or force-pushed between webhook fire and run start). aborting checkout — re-trigger the run if this PR is still active.`
);
}
/**
* Shared helper to checkout a PR branch and configure fork remotes.
* Assumes origin remote is already configured with authentication.
* Updates toolState.issueNumber, toolState.checkoutSha, and toolState.pushUrl (for fork PRs).
*/
export async function checkoutPrBranch(
pr: PrData,
params: CheckoutPrBranchParams
): Promise<{ hookWarning?: string | undefined }> {
const { octokit, owner, name, gitToken, toolState, beforeSha } = params;
const { gitea, owner, name, gitToken, toolState, beforeSha } = params;
log.info(`» checking out PR #${pr.number}...`);
// SECURITY: PR ref names come from GitHub and are attacker-controlled on
// forks (the PR author picks headRef freely, and baseRef could be a
// maliciously-named branch on the target repo). reject leading-dash names
// before they reach any git command — without this, a ref like
// "-upload-pack=evil" fed into `git fetch origin <ref>` would be parsed as
// a flag, not a refspec.
rejectIfLeadingDash(pr.baseRef, "PR base ref");
rejectIfLeadingDash(pr.headRef, "PR head ref");
// self-hosted runners and cancelled jobs frequently leave stale .git/*.lock
// files behind. without this sweep, the first fetch below aborts with
// `Unable to create '.git/shallow.lock': File exists` and the agent has to
// shell out to `rm -f` (issue #564).
cleanupStaleGitLocks();
const isFork = pr.headRepoFullName !== pr.baseRepoFullName;
// always use pr-{number} as local branch name for consistency
// this avoids naming conflicts and makes push config simpler
const localBranch = `pr-${pr.number}`;
const isShallow =
$("git", ["rev-parse", "--is-shallow-repository"], { log: false }).trim() === "true";
const isShallow = $("git", ["rev-parse", "--is-shallow-repository"], { log: false }).trim() === "true";
toolState.checkoutSha = $("git", ["rev-parse", "HEAD"], { log: false }).trim();
const alreadyOnBranch = toolState.checkoutSha === pr.headSha;
// fetch base branch so origin/<base> exists for diff operations.
// wrap with deepen-retry: on shallow clones (the actions/checkout default
// is depth=1), repos with deep PR ancestry can't reach the baseRef tip in
// a single round trip, surfacing as `Could not read <sha>` / `remote did
// not send all necessary objects` (issue #656).
log.debug(`» fetching base branch (${pr.baseRef})...`);
await $gitFetchWithDeepen(
["--no-tags", "origin", pr.baseRef],
{ token: gitToken },
`base branch ${pr.baseRef}`
);
await $gitFetchWithDeepen(["--no-tags", "origin", pr.baseRef], { token: gitToken }, `base branch ${pr.baseRef}`);
// alreadyOnBranch only matches for repeated checkout_pr calls for the same PR in one session
// (without the tip moving), or if an external setup already checked out the PR head.
// normal PR-triggered runs won't match here — actions/checkout lands on a synthesized
// merge commit whose SHA differs from pr.headSha.
//
// so the fetch+checkout block below will almost always execute, and the fetched HEAD
// might differ from pr.headSha. toolState.checkoutSha is set after to capture the actual SHA.
if (!alreadyOnBranch) {
// checkout base branch first to avoid "refusing to fetch into current branch" error
// -B creates or resets the branch to match origin/baseBranch
$("git", ["checkout", "-B", pr.baseRef, `origin/${pr.baseRef}`], { log: false });
// fetch PR branch using pull/{n}/head refspec (works for both fork and same-repo PRs).
// two transient classes wrap this fetch:
// - shallow-unreachable (`Could not read <sha>` etc.) — handled by the
// inner `$gitFetchWithDeepen` deepen-retry (one shot, see issue #656)
// - pull/N/head webhook race (`couldn't find remote ref pull/N/head`) —
// handled by the outer retry below (see issue #591)
log.debug(`» fetching PR #${pr.number} (${localBranch})...`);
await retry(
async () => {
@@ -450,13 +274,9 @@ export async function checkoutPrBranch(
`PR #${pr.number}`
);
} catch (e) {
// on the webhook race, check whether the PR still matches what we
// dispatched on. if it's been closed/merged or the head SHA moved,
// no amount of retrying will populate the expected ref — surface a
// clean abort error instead of burning the full retry budget.
const msg = e instanceof Error ? e.message : String(e);
if (PULL_REF_MISSING_PATTERN.test(msg)) {
await abortIfPullRequestMoved({ octokit, owner, repo: name, pr });
await abortIfPullRequestMoved({ gitea, owner, repo: name, pr });
}
throw e;
}
@@ -464,138 +284,58 @@ export async function checkoutPrBranch(
{
delaysMs: PULL_REF_RETRY_DELAYS_MS,
label: `pull/${pr.number}/head fetch`,
shouldRetry: (e) =>
PULL_REF_MISSING_PATTERN.test(e instanceof Error ? e.message : String(e)),
shouldRetry: (e) => PULL_REF_MISSING_PATTERN.test(e instanceof Error ? e.message : String(e)),
}
);
// checkout the branch
$("git", ["checkout", localBranch], { log: false });
log.debug(`» checked out PR #${pr.number}`);
// make sure toolState.checkoutSha is set to the actual checked-out SHA (which might be different from pr.headSha)
toolState.checkoutSha = $("git", ["rev-parse", "HEAD"], { log: false }).trim();
}
const beforeShaReachable = beforeSha
? await ensureBeforeShaReachable({
sha: beforeSha,
octokit,
owner,
repo: name,
gitToken,
isShallow,
})
? await ensureBeforeShaReachable({ sha: beforeSha, gitea, owner, repo: name, gitToken, isShallow })
: false;
// compute deepen depth for shallow clones. actions/checkout uses depth=1
// by default, which breaks rebase/log because git can't find the merge base.
// use the GitHub compare API to fetch exactly enough history.
// computed after checkout so compareCommits uses the actual checked-out SHA.
if (isShallow) {
let deepenDepth = 0;
try {
// ahead_by = PR commits past merge base, behind_by = base commits past merge base.
// --deepen extends ALL shallow roots equally (can't deepen a single branch),
// so we need the max across both the PR head and before_sha to ensure all
// three points (base, head, before_sha) reach the merge base in a single deepen call.
const [prComparison, beforeShaComparison] = await Promise.all([
octokit.rest.repos.compareCommits({
owner,
repo: name,
base: pr.baseRef,
head: toolState.checkoutSha,
}),
const [prComp, beforeComp] = await Promise.all([
gitea.rest.repository.repoCompareDiff({ owner, repo: name, basehead: `${pr.baseRef}...${toolState.checkoutSha}` }),
beforeSha && beforeShaReachable
? octokit.rest.repos.compareCommits({
owner,
repo: name,
base: pr.baseRef,
head: beforeSha,
})
? gitea.rest.repository.repoCompareDiff({ owner, repo: name, basehead: `${pr.baseRef}...${beforeSha}` })
: undefined,
]);
deepenDepth =
Math.max(
prComparison.data.ahead_by,
prComparison.data.behind_by,
beforeShaComparison?.data.ahead_by ?? 0,
beforeShaComparison?.data.behind_by ?? 0
) + 10;
log.debug(
`» PR: ${prComparison.data.ahead_by} ahead / ${prComparison.data.behind_by} behind` +
(beforeShaComparison
? `, before_sha: ${beforeShaComparison.data.ahead_by} ahead / ${beforeShaComparison.data.behind_by} behind`
: "") +
`, deepen by ${deepenDepth}`
);
const prTotal = prComp.data.total_commits ?? 0;
const beforeTotal = beforeComp?.data.total_commits ?? 0;
deepenDepth = Math.max(prTotal, beforeTotal) + 10;
log.debug(`» compare: PR=${prTotal}, before=${beforeTotal}, deepen=${deepenDepth}`);
} catch {
deepenDepth = 1000;
log.debug(`» compare API failed, falling back to --deepen=${deepenDepth}`);
}
// deepen after both branches are fetched so the merge base is reachable from both sides
if (deepenDepth) {
log.debug(`» deepening by ${deepenDepth} to reach merge base...`);
await $git("fetch", [`--deepen=${deepenDepth}`, "--no-tags", "origin"], {
token: gitToken,
});
await $git("fetch", [`--deepen=${deepenDepth}`, "--no-tags", "origin"], { token: gitToken });
}
}
// configure push remote for this branch
// NOTE: This always runs regardless of alreadyOnBranch, because setupGit doesn't configure
// fork remotes. This ensures fork PRs can push even when checkout_pr is called after setupGit.
if (isFork) {
const remoteName = `pr-${pr.number}`;
// SECURITY: fork URL without token - auth is injected via GIT_ASKPASS in $git()
const forkUrl = `https://github.com/${pr.headRepoFullName}.git`;
// add fork as a named remote (suppress logging to avoid "error: remote already exists" spam)
try {
$("git", ["remote", "add", remoteName, forkUrl], { log: false });
log.debug(`» added remote '${remoteName}' for fork ${pr.headRepoFullName}`);
} catch {
// remote already exists, update its URL
$("git", ["remote", "set-url", remoteName, forkUrl], { log: false });
log.debug(`» updated remote '${remoteName}' for fork ${pr.headRepoFullName}`);
}
// set branch push config so `git push` knows where to push
const giteaUrl = (process.env.GITEA_URL ?? "https://git.shockvpn.com").replace(/\/$/, "");
const forkUrl = `${giteaUrl}/${pr.headRepoFullName}.git`;
try { $("git", ["remote", "add", remoteName, forkUrl], { log: false }); }
catch { $("git", ["remote", "set-url", remoteName, forkUrl], { log: false }); }
$("git", ["config", `branch.${localBranch}.pushRemote`, remoteName], { log: false });
// set merge ref so git knows the remote branch name (may differ from local)
$("git", ["config", `branch.${localBranch}.merge`, `refs/heads/${pr.headRef}`], { log: false });
log.debug(`» configured branch '${localBranch}' to push to '${remoteName}/${pr.headRef}'`);
// warn if maintainer can't modify (push will likely fail)
if (!pr.maintainerCanModify) {
log.warning(
`» fork PR has maintainer_can_modify=false - push operations will fail. ` +
`ask the PR author to enable "Allow edits from maintainers" or the fork may be owned by an organization.`
);
}
if (!pr.maintainerCanModify) log.warning(`» fork PR has maintainer_can_modify=false — push will likely fail.`);
toolState.pushUrl = forkUrl;
} else {
// for same-repo PRs, push to origin
$("git", ["config", `branch.${localBranch}.pushRemote`, "origin"], { log: false });
$("git", ["config", `branch.${localBranch}.merge`, `refs/heads/${pr.headRef}`], { log: false });
}
// update toolState
toolState.issueNumber = pr.number;
if (isFork) {
toolState.pushUrl = `https://github.com/${pr.headRepoFullName}.git`;
}
toolState.pushDest = { remoteName: isFork ? `pr-${pr.number}` : "origin", remoteBranch: pr.headRef, localBranch };
// store push destination so push_branch can use it directly
// git config is the primary mechanism, but toolState serves as a reliable fallback
// in case git config reads fail in certain environments
toolState.pushDest = {
remoteName: isFork ? `pr-${pr.number}` : "origin",
remoteBranch: pr.headRef,
localBranch,
};
// execute post-checkout lifecycle hook. soft-fail: surface the warning
// to the agent via the tool response instead of throwing, so a flaky or
// slightly-broken hook doesn't block checkout entirely.
const postCheckoutHook = await executeLifecycleHook({
event: "post-checkout",
script: params.postCheckoutScript,
@@ -604,54 +344,42 @@ export async function checkoutPrBranch(
return { hookWarning: postCheckoutHook.warning };
}
/**
* dedupes concurrent `checkout_pr` calls for the same PR. agents (notably
* Sonnet/Claude) occasionally emit duplicate parallel tool_use blocks for the
* same args in one turn; without this, both invocations race
* `checkoutPrBranch` against the same `.git/shallow.lock` and one fails with
* `File exists` (issue #642). cleared in `finally` so subsequent same-PR
* calls re-do the work normally.
*/
const inFlightCheckouts = new Map<number, Promise<CheckoutPrResult>>();
type InitialHead = NonNullable<ToolContext["toolState"]["initialHead"]>;
function headsEqual(a: InitialHead, b: InitialHead): boolean {
if (a.kind === "branch" && b.kind === "branch") return a.name === b.name;
if (a.kind === "detached" && b.kind === "detached") return a.sha === b.sha;
return false;
}
function describeHead(h: InitialHead): string {
if (h.kind === "branch") return `branch \`${h.name}\``;
return `detached HEAD \`${h.sha}\``;
return h.kind === "branch" ? `branch \`${h.name}\`` : `detached HEAD \`${h.sha}\``;
}
export function CheckoutPrTool(ctx: ToolContext) {
const runCheckout = async (pull_number: number): Promise<CheckoutPrResult> => {
const prResponse = await ctx.octokit.rest.pulls.get({
const prResult = await ctx.gitea.rest.repository.repoGetPullRequest({
owner: ctx.repo.owner,
repo: ctx.repo.name,
pull_number,
index: pull_number,
});
const prData = prResult.data;
const headRepo = prResponse.data.head.repo;
if (!headRepo) {
throw new Error(`PR #${pull_number} source repository was deleted`);
}
const headRepo = prData.head?.repo;
if (!headRepo) throw new Error(`PR #${pull_number} source repository was deleted`);
const pr: PrData = {
number: pull_number,
headSha: prResponse.data.head.sha,
headRef: prResponse.data.head.ref,
headRepoFullName: headRepo.full_name,
baseRef: prResponse.data.base.ref,
baseRepoFullName: prResponse.data.base.repo.full_name,
maintainerCanModify: prResponse.data.maintainer_can_modify,
headSha: prData.head?.sha ?? "",
headRef: prData.head?.ref ?? "",
headRepoFullName: headRepo.full_name ?? "",
baseRef: prData.base?.ref ?? "",
baseRepoFullName: prData.base?.repo?.full_name ?? "",
maintainerCanModify: prData.allow_maintainer_edit ?? false,
};
const checkoutResult = await checkoutPrBranch(pr, {
octokit: ctx.octokit,
gitea: ctx.gitea,
owner: ctx.repo.owner,
name: ctx.repo.name,
gitToken: ctx.gitToken,
@@ -661,16 +389,11 @@ export function CheckoutPrTool(ctx: ToolContext) {
beforeSha: ctx.toolState.beforeSha,
});
const tempDir = process.env.PULLFROG_TEMP_DIR;
if (!tempDir) {
throw new Error(
"PULLFROG_TEMP_DIR not set - checkout_pr must run in pullfrog action context"
);
}
const tempDir = process.env.SHOCKBOT_TEMP_DIR;
if (!tempDir) throw new Error("SHOCKBOT_TEMP_DIR not set");
const headShort = ctx.toolState.checkoutSha!.slice(0, 7);
// compute incremental diff if we have a beforeSha to compare against
let incrementalDiffPath: string | undefined;
if (ctx.toolState.beforeSha && ctx.toolState.checkoutSha) {
const beforeShort = ctx.toolState.beforeSha.slice(0, 7);
@@ -680,126 +403,75 @@ export function CheckoutPrTool(ctx: ToolContext) {
headSha: ctx.toolState.checkoutSha,
});
if (incremental) {
incrementalDiffPath = join(
tempDir,
`pr-${pull_number}-${beforeShort}-${headShort}-incremental.diff`
);
incrementalDiffPath = join(tempDir, `pr-${pull_number}-${beforeShort}-${headShort}-incremental.diff`);
writeFileSync(incrementalDiffPath, incremental);
log.info(
`» incremental diff computed (${incremental.length} bytes) → ${incrementalDiffPath}`
);
log.info(`» incremental diff computed → ${incrementalDiffPath}`);
}
}
// fetch PR files and format with line numbers
const formatResult = await fetchAndFormatPrDiff(ctx, pull_number);
const diffPreview = formatResult.content.split("\n").slice(0, 100).join("\n");
log.debug(`formatted diff preview (first 100 lines):\n${diffPreview}`);
const diffPath = join(tempDir, `pr-${pull_number}-${headShort}.diff`);
writeFileSync(diffPath, formatResult.content);
log.debug(`wrote diff to ${diffPath} (${formatResult.content.length} bytes)`);
ctx.toolState.diffCoverage = createDiffCoverageState({
diffPath,
totalLines: countLines({ content: formatResult.content }),
toc: formatResult.toc,
previous: ctx.toolState.diffCoverage,
});
log.debug(
`» diff coverage initialized: diffPath=${diffPath}, totalLines=${ctx.toolState.diffCoverage.totalLines}, tocEntries=${ctx.toolState.diffCoverage.tocEntries.length}`
);
// cache commentable-lines snapshot so review-time validation matches what
// GitHub will anchor to (commit_id=checkoutSha), even if the PR is updated
// between checkout and review.
const cached = new Map<string, ReturnType<typeof commentableLinesForFile>>();
for (const file of formatResult.files) {
cached.set(file.filename, commentableLinesForFile(file.patch));
if (file.filename) cached.set(file.filename, commentableLinesForFile(file.patch));
}
ctx.toolState.commentableLinesByFile = cached;
ctx.toolState.commentableLinesPullNumber = pull_number;
ctx.toolState.commentableLinesCheckoutSha = ctx.toolState.checkoutSha;
const incrementalInstructions = incrementalDiffPath
? ` IMPORTANT: incrementalDiffPath contains ONLY the changes since the last reviewed version ` +
`(computed via range-diff). you MUST read incrementalDiffPath FIRST to understand what changed, ` +
`then use diffPath for full PR context. do NOT skip the incremental diff.`
: "";
// commit metadata relative to the PR base (e.g. main). use origin/<base>
// because the local base ref may not exist after a shallow fetch. cap
// the log so a PR with thousands of commits doesn't blow up the tool
// response. if the base ref can't be resolved (e.g. shallow fetch that
// didn't pull down origin/<base>), degrade gracefully rather than
// failing the whole checkout_pr call over metadata.
const COMMIT_LOG_MAX = 200;
const baseRange = `origin/${pr.baseRef}..HEAD`;
let commitCount = 0;
let commitLog = "";
let commitLogUnavailable = false;
try {
commitCount = parseInt(
$("git", ["rev-list", "--count", baseRange], { log: false }).trim() || "0",
10
);
commitLog = $("git", ["log", "--oneline", `--max-count=${COMMIT_LOG_MAX}`, baseRange], {
log: false,
});
} catch (err) {
commitCount = parseInt($("git", ["rev-list", "--count", baseRange], { log: false }).trim() || "0", 10);
commitLog = $("git", ["log", "--oneline", `--max-count=${COMMIT_LOG_MAX}`, baseRange], { log: false });
} catch {
commitLogUnavailable = true;
log.debug(
`» unable to compute commit metadata for ${baseRange}: ${err instanceof Error ? err.message : String(err)}`
);
}
const commitLogTruncated = commitCount > COMMIT_LOG_MAX;
const hookWarningInstructions = checkoutResult.hookWarning
? ` HOOK WARNING: the post-checkout lifecycle hook reported a non-fatal failure (see hookWarning). ` +
`decide whether to retry based on the guidance in that field before proceeding.`
: "";
const commitLogInstructions = commitLogUnavailable
? ` NOTE: commit metadata is partial (base ref unreachable, likely a shallow fetch). ` +
`commitCount/commitLog may be 0/empty or incomplete; treat them as "unknown" rather than "no commits", ` +
`and use \`git log\` directly if you need the full history.`
: commitLogTruncated
? ` NOTE: commitLog was capped at ${COMMIT_LOG_MAX} entries out of ${commitCount} commits; ` +
`use \`git log\` directly if you need the full history.`
: "";
return {
success: true,
number: prResponse.data.number,
title: prResponse.data.title,
body: prResponse.data.body,
number: prData.number!,
title: prData.title ?? "",
body: prData.body ?? null,
base: pr.baseRef,
localBranch: `pr-${pull_number}`,
remoteBranch: `refs/heads/${pr.headRef}`,
isFork: pr.headRepoFullName !== pr.baseRepoFullName,
maintainerCanModify: pr.maintainerCanModify,
url: prResponse.data.html_url,
url: prData.html_url ?? "",
headRepo: pr.headRepoFullName,
diffPath,
incrementalDiffPath,
toc: formatResult.toc,
commitCount,
commitLog,
commitLogTruncated,
commitLogTruncated: commitCount > COMMIT_LOG_MAX,
commitLogUnavailable,
hookWarning: checkoutResult.hookWarning,
instructions:
`the diff file at diffPath contains a table of contents (TOC) at the top listing every changed file with its line range. ` +
`use the TOC line ranges as your checklist and read specific files from the diff instead of reading the entire file. ` +
`for example, if the TOC says "src/foo.ts → lines 5-42", read lines 5-42 from diffPath to see that file's changes. ` +
`review files selectively based on relevance rather than reading everything sequentially. ` +
`to inspect the PR's changed files, use diffPath — do NOT run \`git diff <base>..<head>\` to re-derive what's already in diffPath. the formatted diff with line numbers is authoritative. ` +
`\`git log\` and \`git diff --stat\` are fine for commit-range overview, and \`git diff\` / \`git diff --cached\` are fine for inspecting *your own* uncommitted changes — but PR review content MUST come from diffPath. ` +
`before your review is submitted, a one-time coverage pre-flight may error listing unread TOC regions. ` +
`retry the same create_pull_request_review call to proceed — optionally after reading the listed ranges. the pre-flight will not block again this session. ` +
`the local branch is 'localBranch' (pr-{number}), not the remote branch name. ` +
`when pushing, omit branchName to use the current branch. do not use remoteBranch as a local branch name.` +
incrementalInstructions +
hookWarningInstructions +
commitLogInstructions,
`the diff file at diffPath contains a table of contents (TOC) listing every changed file with its line range. ` +
`use the TOC line ranges as your checklist and read specific files from the diff. ` +
`for example, if the TOC says "src/foo.ts → lines 5-42", read lines 5-42 from diffPath. ` +
`review files selectively based on relevance. ` +
`to inspect the PR's changed files, use diffPath — do NOT run git diff commands. ` +
(incrementalDiffPath
? ` IMPORTANT: read incrementalDiffPath FIRST to understand what changed since last review, then use diffPath for full context.`
: "") +
(checkoutResult.hookWarning ? ` HOOK WARNING: the post-checkout hook reported a non-fatal failure.` : "") +
(commitLogUnavailable ? ` NOTE: commit metadata is partial (shallow fetch).` : commitCount > COMMIT_LOG_MAX ? ` NOTE: commitLog was capped at ${COMMIT_LOG_MAX} entries.` : ""),
} satisfies CheckoutPrResult;
};
@@ -807,12 +479,8 @@ export function CheckoutPrTool(ctx: ToolContext) {
name: "checkout_pr",
timeoutMs: 600_000,
description:
"Checkout a pull request branch locally. This fetches the PR branch and sets up push configuration for fork PRs. " +
"Returns diffPath pointing to the formatted diff file. " +
"Example: `checkout_pr({ pull_number: 1234 })`. " +
"Large repos can take several minutes — wait for the call to finish; do not treat a slow response as failure. " +
"If you see `MCP error -32001: Request timed out`, retry the same call without touching git lock files first — that error is a client-side abort. " +
"If the retry then reports `.git/shallow.lock: File exists` or `.git/index.lock: File exists`, remove those lock files via the shell tool and retry again.",
"Checkout a pull request branch locally. Returns diffPath pointing to the formatted diff file. " +
"Example: `checkout_pr({ pull_number: 1234 })`. Large repos can take several minutes.",
parameters: CheckoutPr,
execute: execute(async ({ pull_number }) => {
const inFlight = inFlightCheckouts.get(pull_number);
@@ -821,30 +489,14 @@ export function CheckoutPrTool(ctx: ToolContext) {
return inFlight;
}
// unconditional refusal: any dirty working tree blocks checkout_pr, even
// when HEAD is already on pr-N. no stashing, no live-HEAD escape hatch.
// shared-cwd subagents made "carry edits along" semantics dangerous
// (zed-industries/cloud, 2026-05-18) — forcing commit/discard before
// any PR-context op eliminates the entire carry-forward failure class.
const dirty = $("git", ["status", "--porcelain"], { log: false }).trim();
if (dirty) {
throw new Error(
`cannot checkout PR #${pull_number} while the working tree has uncommitted changes. ` +
`commit (then push if needed), or discard with \`git restore --staged --worktree .\` / \`git clean -fd\` before retrying. ` +
`this refusal is unconditional — even re-checking-out the PR you're already on is refused, ` +
`because shared-working-tree subagents make carry-forward edits unsafe. dirty paths:\n${dirty}`
`commit or discard with \`git restore --staged --worktree .\` / \`git clean -fd\` before retrying.\n${dirty}`
);
}
// initial-branch invariant: the only sanctioned HEAD positions for a
// checkout_pr call are (a) the run-entry HEAD captured by setupGit, or
// (b) `pr-${pull_number}` for idempotent same-PR re-checkout (e.g.
// re-fetch after the PR head moved). anything else means a subagent
// silently parked HEAD on another PR, which is the zed-industries/cloud
// (2026-05-18) cross-PR clobber shape. uses the same live probe (not
// toolState.issueNumber, poisonable per the PR #796 review) and
// discriminates branch vs detached so detached-entry runs don't get a
// trivial "any future detached state matches" carve-out.
const initialHead = ctx.toolState.initialHead;
if (initialHead) {
const currentHead = captureInitialHead(process.cwd());
@@ -852,30 +504,18 @@ export function CheckoutPrTool(ctx: ToolContext) {
const onTarget = currentHead.kind === "branch" && currentHead.name === targetBranch;
const onInitial = headsEqual(currentHead, initialHead);
if (!onTarget && !onInitial) {
const recoverCmd =
initialHead.kind === "branch"
? `git checkout ${initialHead.name}`
: `git checkout ${initialHead.sha}`;
const recoverCmd = initialHead.kind === "branch" ? `git checkout ${initialHead.name}` : `git checkout ${initialHead.sha}`;
throw new Error(
`cannot checkout PR #${pull_number} from ${describeHead(currentHead)}. ` +
`the only sanctioned HEAD positions for checkout_pr are the run-entry HEAD ` +
`(${describeHead(initialHead)}) or the target PR's branch (\`${targetBranch}\`, idempotent re-checkout). ` +
`recover with \`${recoverCmd}\` first — if that would carry uncommitted ` +
`work along, commit or discard it (\`git restore --staged --worktree .\` / \`git clean -fd\`) before switching. ` +
`routing around this via the \`git\` tool's \`checkout\`/\`switch\` subcommands is not sanctioned: ` +
`this guard exists to prevent the shared-working-tree cross-PR clobber pattern from the ` +
`zed-industries/cloud (2026-05-18) incident.`
`recover with \`${recoverCmd}\` first.`
);
}
}
const promise = runCheckout(pull_number);
inFlightCheckouts.set(pull_number, promise);
try {
return await promise;
} finally {
inFlightCheckouts.delete(pull_number);
}
try { return await promise; }
finally { inFlightCheckouts.delete(pull_number); }
}),
});
}
mcp/comment.ts
+45 -260
View File
@@ -1,9 +1,7 @@
import { type } from "arktype";
import { getApiUrl } from "../utils/apiUrl.ts";
import { buildPullfrogFooter, stripExistingFooter } from "../utils/buildPullfrogFooter.ts";
import { buildShockbotFooter, stripExistingFooter } from "../utils/buildShockbotFooter.ts";
import { log } from "../utils/cli.ts";
import { fixDoubleEscapedString } from "../utils/fixDoubleEscapedString.ts";
import { patchWorkflowRunFields } from "../utils/patchWorkflowRunFields.ts";
import {
createLeapingProgressComment,
deleteProgressCommentApi,
@@ -12,40 +10,19 @@ import {
import type { ToolContext } from "./server.ts";
import { execute, tool } from "./shared.ts";
// re-export for backward compat with anything importing the leaping helpers from mcp/comment
export {
isLeapingIntoActionCommentBody,
LEAPING_INTO_ACTION_PREFIX,
} from "../utils/leapingComment.ts";
function buildCommentFooter(ctx: ToolContext, customParts?: string[]): string {
const runId = ctx.runId;
return buildPullfrogFooter({
triggeredBy: true,
workflowRun:
runId !== undefined
? {
owner: ctx.repo.owner,
repo: ctx.repo.name,
runId,
jobId: ctx.jobId,
}
: undefined,
customParts,
model: ctx.toolState.model,
fallbackFrom: ctx.toolState.modelFallback?.from,
});
}
function buildImplementPlanLink(ctx: ToolContext, issueNumber: number, commentId: number): string {
const apiUrl = getApiUrl();
return `[Implement plan ➔](${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${issueNumber}?action=implement&comment_id=${commentId})`;
function buildCommentFooter(ctx: ToolContext): string {
return buildShockbotFooter({ model: ctx.toolState.model });
}
export function addFooter(ctx: ToolContext, body: string): string {
if (/<br\s*\/?>[ \t]*\n(?!\s*\n)/i.test(body)) {
throw new Error(
"body contains <br/> followed by a non-blank line, which breaks GitHub markdown rendering. always add a blank line after <br/> tags."
"body contains <br/> followed by a non-blank line, which breaks Gitea markdown rendering. always add a blank line after <br/> tags."
);
}
const bodyWithoutFooter = stripExistingFooter(fixDoubleEscapedString(body));
@@ -66,48 +43,23 @@ export function CreateCommentTool(ctx: ToolContext) {
return tool({
name: "create_issue_comment",
description:
"Create a comment on a GitHub issue or PR. " +
"Create a comment on a Gitea issue or PR. " +
'Example: `create_issue_comment({ issueNumber: 1234, body: "Thanks for the report." })`. ' +
"For progress/plan updates on the current run use report_progress instead — plan output (initial post AND revisions) is always posted via report_progress, never via this tool.",
"For progress/plan updates use report_progress instead.",
parameters: Comment,
execute: execute(async ({ issueNumber, body, type: commentType }) => {
execute: execute(async ({ issueNumber, body }) => {
const bodyWithFooter = addFooter(ctx, body);
const result = await ctx.octokit.rest.issues.createComment({
const result = await ctx.gitea.rest.issue.issueCreateComment({
owner: ctx.repo.owner,
repo: ctx.repo.name,
issue_number: issueNumber,
body: bodyWithFooter,
index: issueNumber,
body: { body: bodyWithFooter },
});
ctx.toolState.wasUpdated = true;
log.info(`» created comment ${result.data.id}`);
if (commentType === "Plan") {
if (result.data.node_id) {
await patchWorkflowRunFields(ctx, { planCommentNodeId: result.data.node_id });
}
// add "Implement plan" link (needs comment ID, so create-then-update)
const customParts = [buildImplementPlanLink(ctx, issueNumber, result.data.id)];
const footer = buildCommentFooter(ctx, customParts);
const bodyWithPlanLink = `${stripExistingFooter(body)}${footer}`;
const updateResult = await ctx.octokit.rest.issues.updateComment({
owner: ctx.repo.owner,
repo: ctx.repo.name,
comment_id: result.data.id,
body: bodyWithPlanLink,
});
log.info(`» updated comment ${updateResult.data.id}`);
return {
success: true,
commentId: updateResult.data.id,
url: updateResult.data.html_url,
body: updateResult.data.body,
};
}
return {
success: true,
commentId: result.data.id,
@@ -126,16 +78,16 @@ export const EditComment = type({
export function EditCommentTool(ctx: ToolContext) {
return tool({
name: "edit_issue_comment",
description: "Edit a GitHub issue comment by its ID",
description: "Edit a Gitea issue comment by its ID",
parameters: EditComment,
execute: execute(async ({ commentId, body }) => {
const bodyWithFooter = addFooter(ctx, body);
const result = await ctx.octokit.rest.issues.updateComment({
const result = await ctx.gitea.rest.issue.issueEditComment({
owner: ctx.repo.owner,
repo: ctx.repo.name,
comment_id: commentId,
body: bodyWithFooter,
id: commentId,
body: { body: bodyWithFooter },
});
log.info(`» updated comment ${result.data.id}`);
@@ -153,23 +105,10 @@ export function EditCommentTool(ctx: ToolContext) {
export const ReportProgress = type({
body: type.string.describe("the progress update content to share"),
"target_plan_comment?": type("boolean").describe(
"for revising an existing plan comment ONLY. set to true only when the PlanEdit checklist from select_mode tells you to (i.e. a prior plan comment was found for this issue). NEVER set on the initial plan post — the initial plan reuses the run's progress comment and is posted by calling report_progress without this flag."
"for revising an existing plan comment ONLY."
),
});
/**
* Report progress to a GitHub comment.
*
* progressComment has three states:
* - undefined: no comment yet — will create one if an issue/PR target exists
* - object: active comment — will update it in place via the right REST endpoint for its type
* - null: deliberately deleted (e.g. after submitting a PR review) — skips silently
*
* The body is always tracked in lastProgressBody for the job summary regardless of comment state.
*
* The "existing plan comment" path always targets a top-level issue comment (plan comments are
* created by create_issue_comment with type:"Plan", never as review-thread replies).
*/
export async function reportProgress(
ctx: ToolContext,
params: { body: string; target_plan_comment?: boolean }
@@ -180,96 +119,40 @@ export async function reportProgress(
action: "created" | "updated" | "skipped";
}> {
const { body, target_plan_comment } = params;
// always track the body for job summary
ctx.toolState.lastProgressBody = body;
// silent events (e.g., auto-label, pr-summary Task) should never create or update progress comments.
// the body is still tracked above for the GitHub Actions job summary.
if (ctx.payload.event.silent) {
return { body, action: "skipped" };
}
const issueNumber = ctx.payload.event.issue_number ?? ctx.toolState.issueNumber;
const isPlanMode = ctx.toolState.selectedMode === "Plan";
const apiCtx = { octokit: ctx.octokit, owner: ctx.repo.owner, repo: ctx.repo.name };
const apiCtx = { gitea: ctx.gitea, owner: ctx.repo.owner, repo: ctx.repo.name };
// when editing existing plan: update the plan comment from tool state (set by select_mode)
if (target_plan_comment === true && ctx.toolState.existingPlanCommentId === undefined) {
log.warning("target_plan_comment requested but no existingPlanCommentId in tool state");
}
if (target_plan_comment === true && ctx.toolState.existingPlanCommentId !== undefined) {
const commentId = ctx.toolState.existingPlanCommentId;
const customParts =
issueNumber !== undefined ? [buildImplementPlanLink(ctx, issueNumber, commentId)] : undefined;
const bodyWithoutFooter = stripExistingFooter(body);
const footer = buildCommentFooter(ctx, customParts);
const bodyWithFooter = `${bodyWithoutFooter}${footer}`;
const result = await updateProgressComment(
apiCtx,
{ id: commentId, type: "issue" },
bodyWithFooter
);
const bodyWithFooter = `${stripExistingFooter(body)}${buildCommentFooter(ctx)}`;
const result = await updateProgressComment(apiCtx, { id: commentId, type: "issue" }, bodyWithFooter);
ctx.toolState.wasUpdated = true;
if (isPlanMode && result.node_id) {
await patchWorkflowRunFields(ctx, { planCommentNodeId: result.node_id });
}
return {
commentId: result.id,
url: result.html_url,
body: result.body || "",
action: "updated",
};
return { commentId: result.id, url: result.html_url, body: result.body || "", action: "updated" };
}
const existingComment = ctx.toolState.progressComment;
// if we already have a progress comment, update it
if (existingComment) {
const customParts =
isPlanMode && issueNumber !== undefined
? [buildImplementPlanLink(ctx, issueNumber, existingComment.id)]
: undefined;
const bodyWithoutFooter = stripExistingFooter(body);
const footer = buildCommentFooter(ctx, customParts);
const bodyWithFooter = `${bodyWithoutFooter}${footer}`;
const bodyWithFooter = `${stripExistingFooter(body)}${buildCommentFooter(ctx)}`;
const result = await updateProgressComment(apiCtx, existingComment, bodyWithFooter);
ctx.toolState.wasUpdated = true;
if (isPlanMode && result.node_id) {
await patchWorkflowRunFields(ctx, { planCommentNodeId: result.node_id });
}
return {
commentId: result.id,
url: result.html_url,
body: result.body || "",
action: "updated",
};
return { commentId: result.id, url: result.html_url, body: result.body || "", action: "updated" };
}
// null = progress comment was deleted by stranded-comment cleanup in main.ts
if (existingComment === null) {
return { body, action: "skipped" };
}
// no existing comment - need an issue/PR to create one on
// use fallback chain: dynamically set context > event payload
if (issueNumber === undefined) {
// no-op: no comment target (e.g., workflow_dispatch events)
// body is already tracked for job summary
return { body, action: "skipped" };
}
// for new comments, we need to create first, then update with Plan link if in Plan mode
// self-created progress comments are always top-level issue comments — review-reply
// progress comments only originate from the dispatch path and arrive pre-created.
const initialBody = addFooter(ctx, body);
const created = await createLeapingProgressComment(
apiCtx,
@@ -280,27 +163,6 @@ export async function reportProgress(
ctx.toolState.progressComment = created.comment;
ctx.toolState.wasUpdated = true;
// if Plan mode, update the comment to add the "Implement plan" link
if (isPlanMode) {
const customParts = [buildImplementPlanLink(ctx, issueNumber, created.comment.id)];
const bodyWithoutFooter = stripExistingFooter(body);
const footer = buildCommentFooter(ctx, customParts);
const bodyWithPlanLink = `${bodyWithoutFooter}${footer}`;
const updateResult = await updateProgressComment(apiCtx, created.comment, bodyWithPlanLink);
if (updateResult.node_id) {
await patchWorkflowRunFields(ctx, { planCommentNodeId: updateResult.node_id });
}
return {
commentId: updateResult.id,
url: updateResult.html_url,
body: updateResult.body || "",
action: "created",
};
}
return {
commentId: created.comment.id,
url: created.html_url,
@@ -313,100 +175,59 @@ export function ReportProgressTool(ctx: ToolContext) {
return tool({
name: "report_progress",
description:
"Share progress on the associated GitHub issue/PR. The first call creates a comment; subsequent calls update it in place. " +
'Example: `report_progress({ body: "Implemented the auth check and added tests." })`. ' +
"Call this at the end of every run with a brief final summary (1-3 sentences) unless the mode guidance instructs otherwise. The current task list is automatically appended in a collapsible section — do not restate individual steps.",
"Share progress on the associated Gitea issue/PR. First call creates a comment; subsequent calls update it. " +
"Call once at the end of every run with a brief final summary (1-3 sentences).",
parameters: ReportProgress,
execute: execute(async (params) => {
let body = params.body;
// for non-plan calls: stop auto-updates, wait for in-flight writes to settle,
// then append completed task list collapsible
if (!params.target_plan_comment && ctx.toolState.todoTracker) {
ctx.toolState.todoTracker.cancel();
await ctx.toolState.todoTracker.settled();
const collapsible = ctx.toolState.todoTracker.renderCollapsible({
completeInProgress: true,
});
if (collapsible) {
body = `${body}\n\n${collapsible}`;
}
const collapsible = ctx.toolState.todoTracker.renderCollapsible({ completeInProgress: true });
if (collapsible) body = `${body}\n\n${collapsible}`;
}
const reportParams: { body: string; target_plan_comment?: boolean } = { body };
if (params.target_plan_comment !== undefined) {
reportParams.target_plan_comment = params.target_plan_comment;
}
if (params.target_plan_comment !== undefined) reportParams.target_plan_comment = params.target_plan_comment;
const result = await reportProgress(ctx, reportParams);
if (result.action === "skipped") {
return {
success: true,
message:
"progress recorded (no GitHub comment created - this may occur for workflow_dispatch events or when there is no associated issue/PR)",
};
return { success: true, message: "progress recorded (no comment created)" };
}
if (result.commentId !== undefined) {
log.info(`» ${result.action} comment ${result.commentId}`);
}
if (result.commentId !== undefined) log.info(`» ${result.action} comment ${result.commentId}`);
if (!params.target_plan_comment) ctx.toolState.finalSummaryWritten = true;
if (!params.target_plan_comment) {
ctx.toolState.finalSummaryWritten = true;
}
return {
success: true,
...result,
};
return { success: true, ...result };
}),
});
}
/**
* Delete the progress comment if it exists.
* Used by main.ts for stranded-comment cleanup (orphaned "Leaping into action" or
* checklist left by the todo tracker when the agent didn't call report_progress).
* Sets progressComment to null so subsequent report_progress calls are no-ops.
*/
export async function deleteProgressComment(ctx: ToolContext): Promise<boolean> {
const existing = ctx.toolState.progressComment;
if (!existing) {
return false;
}
if (!existing) return false;
try {
await deleteProgressCommentApi(
{ octokit: ctx.octokit, owner: ctx.repo.owner, repo: ctx.repo.name },
{ gitea: ctx.gitea, owner: ctx.repo.owner, repo: ctx.repo.name },
existing
);
} catch (error) {
// ignore 404 - comment already deleted
if (error instanceof Error && error.message.includes("Not Found")) {
// comment already deleted, continue
} else {
throw error;
}
if (!(error instanceof Error && error.message.includes("Not Found"))) throw error;
}
// set to null (not undefined) so report_progress skips instead of creating a new comment
ctx.toolState.progressComment = null;
return true;
}
export const ReplyToReviewComment = type({
pull_number: type.number.describe("the pull request number"),
comment_id: type.number.describe("the ID of the review comment to reply to"),
body: type.string.describe(
"extremely brief reply (1 sentence max) explaining what was fixed, e.g. 'Fixed by renaming to X' or 'Added null check'"
),
body: type.string.describe("extremely brief reply (1 sentence max)"),
});
/**
* decision returned by `duplicateReplyDecision` when a session has already
* posted an identical reply to the same parent review comment.
*/
export interface DuplicateReplyDecision {
kind: "already-replied";
commentId: number;
@@ -414,23 +235,6 @@ export interface DuplicateReplyDecision {
reason: string;
}
/**
* decide whether a second reply_to_review_comment call in the same session
* is a duplicate of an earlier reply to the same parent comment.
*
* the agent is instructed to call reply_to_review_comment exactly once per
* parent comment per AddressReviews session, but in practice it sometimes
* emits the same call twice. PR #610 reproduced this with Kimi K2:
* identical body posted 3 seconds apart, only one tool_use event in the
* agent log. the second post is always redundant and clutters the PR thread.
*
* we key on (comment_id, bodyWithFooter) so a legitimate follow-up reply
* with different content still goes through. within a single run the
* footer is constant (workflow run + model + jobId), so byte-equal bodies
* catch the stutter without blocking real follow-ups.
*
* mirrors the shape of `duplicateReviewDecision` in mcp/review.ts.
*/
export function duplicateReplyDecision(params: {
existing: { commentId: number; url: string | undefined; bodyWithFooter: string } | undefined;
bodyWithFooter: string;
@@ -450,58 +254,39 @@ export function ReplyToReviewCommentTool(ctx: ToolContext) {
return tool({
name: "reply_to_review_comment",
description:
"Reply to a PR review comment thread (NOT issue comments — this only works for inline review comments on PR diffs). " +
'Example: `reply_to_review_comment({ pull_number: 1234, comment_id: 567890, body: "Fixed by adding a null check." })`. ' +
"Call exactly ONCE per parent comment you address in AddressReviews mode — duplicate calls with the same body are a no-op. Keep replies extremely brief (1 sentence max).",
"Reply to a PR review comment. Posts an issue comment on the PR referencing the original. " +
"Keep replies extremely brief (1 sentence max).",
parameters: ReplyToReviewComment,
execute: execute(async ({ pull_number, comment_id, body }) => {
const bodyWithFooter = addFooter(ctx, body);
// guard against duplicate reply submissions in the same session.
// see duplicateReplyDecision for the rationale.
const dup = duplicateReplyDecision({
existing: ctx.toolState.reviewReplies?.get(comment_id),
bodyWithFooter,
});
if (dup) {
log.info(`skipping duplicate review reply: ${dup.reason}`);
return {
success: true,
skipped: true,
reason: dup.reason,
commentId: dup.commentId,
url: dup.url,
};
return { success: true, skipped: true, reason: dup.reason, commentId: dup.commentId, url: dup.url };
}
const result = await ctx.octokit.rest.pulls.createReplyForReviewComment({
const replyBody = `> Reply to review comment #${comment_id}\n\n${bodyWithFooter}`;
const result = await ctx.gitea.rest.issue.issueCreateComment({
owner: ctx.repo.owner,
repo: ctx.repo.name,
pull_number,
comment_id,
body: bodyWithFooter,
index: pull_number,
body: { body: replyBody },
});
log.info(`» created review comment ${result.data.id} (in reply to ${comment_id})`);
log.info(`» created reply comment ${result.data.id}`);
// mark progress as updated so error reporting + run-result handling know
// a substantive write happened (used by reportErrorToComment / handleAgentResult)
ctx.toolState.wasUpdated = true;
// record this reply for in-session dedupe of subsequent identical calls.
ctx.toolState.reviewReplies ??= new Map();
ctx.toolState.reviewReplies.set(comment_id, {
commentId: result.data.id,
commentId: result.data.id!,
url: result.data.html_url,
bodyWithFooter,
});
return {
success: true,
commentId: result.data.id,
url: result.data.html_url,
body: result.data.body,
in_reply_to_id: result.data.in_reply_to_id,
};
return { success: true, commentId: result.data.id, url: result.data.html_url, body: result.data.body };
}, "reply_to_review_comment"),
});
}
mcp/commitInfo.ts
+19 -22
View File
@@ -2,7 +2,7 @@ import { writeFileSync } from "node:fs";
import { join } from "node:path";
import { type } from "arktype";
import { log } from "../utils/cli.ts";
import { formatFilesWithLineNumbers } from "./checkout.ts";
import { formatFilesWithLineNumbers, type DiffFile } from "./checkout.ts";
import type { ToolContext } from "./server.ts";
import { execute, tool } from "./shared.ts";
@@ -14,27 +14,28 @@ export function CommitInfoTool(ctx: ToolContext) {
return tool({
name: "get_commit_info",
description:
"Retrieve commit metadata and diff via GitHub API. Use this instead of git show for reviewing commits - " +
"it works with shallow clones and shows the actual changes in the commit. Returns diffPath pointing to formatted diff file. " +
"Retrieve commit metadata and diff via Gitea API. Returns diffPath pointing to formatted diff file. " +
'Example: `get_commit_info({ sha: "2a6ab5d" })`.',
parameters: CommitInfo,
execute: execute(async ({ sha }) => {
const response = await ctx.octokit.rest.repos.getCommit({
const result = await ctx.gitea.rest.repository.repoGetSingleCommit({
owner: ctx.repo.owner,
repo: ctx.repo.name,
ref: sha,
sha,
});
const data = result.data;
const data = response.data;
const files = data.files ?? [];
// The SDK's CommitAffectedFiles type omits `patch`, which Gitea does return
// in practice. Cast explicitly so we can pass it to formatFilesWithLineNumbers.
const files: DiffFile[] = (data.files ?? []).map((f) => ({
filename: f.filename,
patch: (f as unknown as { patch?: string }).patch,
}));
// format diff with line numbers and write to file
const formatResult = formatFilesWithLineNumbers(files);
const tempDir = process.env.PULLFROG_TEMP_DIR;
const tempDir = process.env.SHOCKBOT_TEMP_DIR;
if (!tempDir) {
throw new Error(
"PULLFROG_TEMP_DIR not set - get_commit_info must run in pullfrog action context"
);
throw new Error("SHOCKBOT_TEMP_DIR not set - get_commit_info must run in shockbot action context");
}
const diffFile = join(tempDir, `commit-${sha.slice(0, 7)}.diff`);
writeFileSync(diffFile, formatResult.content);
@@ -42,17 +43,13 @@ export function CommitInfoTool(ctx: ToolContext) {
return {
sha: data.sha,
message: data.commit.message,
author: data.author?.login ?? null,
committer: data.committer?.login ?? null,
date: data.commit.author?.date ?? data.commit.committer?.date ?? "",
message: data.commit?.message,
author: data.author?.login ?? data.commit?.author?.name ?? null,
committer: data.committer?.login ?? data.commit?.committer?.name ?? null,
date: data.commit?.author?.date ?? data.commit?.committer?.date ?? "",
url: data.html_url,
parents: data.parents.map((p) => p.sha),
stats: {
additions: data.stats?.additions ?? 0,
deletions: data.stats?.deletions ?? 0,
total: data.stats?.total ?? 0,
},
parents: (data.parents ?? []).map((p) => p.sha),
stats: data.stats ?? { additions: 0, deletions: 0, total: 0 },
fileCount: files.length,
diffFile,
};
mcp/dependencies.ts
+28 -124
View File
@@ -1,101 +1,42 @@
import { execSync } from "node:child_process";
import { existsSync } from "node:fs";
import { type } from "arktype";
import type { PrepOptions, PrepResult } from "../prep/index.ts";
import { runPrepPhase } from "../prep/index.ts";
import { log } from "../utils/cli.ts";
import type { ToolContext } from "./server.ts";
import { execute, tool } from "./shared.ts";
// empty schema for tools with no parameters
const EmptyParams = type({});
/**
* format prep results into agent-friendly message
*/
function formatPrepResults(results: PrepResult[]): string {
if (results.length === 0) {
return `No supported language detected in this repository (checked for package.json, requirements.txt, pyproject.toml, etc.).
Inspect the repository structure to determine how dependencies should be installed, then use shell to install them.`;
async function runInstallation(): Promise<unknown[]> {
if (!existsSync("package.json")) {
return [];
}
const lines: string[] = [];
for (const result of results) {
if (result.language === "unknown") {
continue;
}
const langDisplay = result.language === "node" ? "Node.js" : "Python";
if (result.dependenciesInstalled) {
if (result.language === "node") {
lines.push(
`${langDisplay} dependencies installed successfully via ${result.packageManager}.`
);
} else if (result.language === "python") {
lines.push(
`${langDisplay} dependencies installed successfully via ${result.packageManager} (from ${result.configFile}).`
);
}
} else {
const errorMsg = result.issues.length > 0 ? result.issues.join("\n") : "unknown error";
if (result.language === "node") {
lines.push(`${langDisplay} dependency installation failed via ${result.packageManager}.
Error:
${errorMsg}
Use shell or other tools at your disposal to diagnose and resolve the issue, then install dependencies manually.`);
} else if (result.language === "python") {
lines.push(`${langDisplay} dependency installation failed via ${result.packageManager} (from ${result.configFile}).
Error:
${errorMsg}
Use shell or other tools at your disposal to diagnose and resolve the issue, then install dependencies manually.`);
}
}
try {
log.info("» installing Node.js dependencies...");
execSync("npm install --silent", { stdio: "pipe" });
log.info("» Node.js dependencies installed");
return [{ language: "node", installed: true }];
} catch (err) {
const msg = err instanceof Error ? err.message : String(err);
log.warning(`» dependency installation failed: ${msg}`);
return [{ language: "node", installed: false, error: msg }];
}
if (lines.length === 0) {
return `No supported language detected in this repository (checked for package.json, requirements.txt, pyproject.toml, etc.).
Inspect the repository structure to determine how dependencies should be installed, then use shell to install them.`;
}
return lines.join("\n\n");
}
/**
* start dependency installation in the background (non-blocking, idempotent).
* called eagerly from main.ts at startup and also available via MCP tools.
*/
export function startInstallation(ctx: ToolContext): void {
// already started or completed - do nothing
if (ctx.toolState.dependencyInstallation) {
return;
}
if (ctx.toolState.dependencyInstallation) return;
// SECURITY: when shell is disabled, suppress lifecycle scripts to prevent
// agents from using package.json scripts as a backdoor for code execution
const prepOptions: PrepOptions = {
ignoreScripts: ctx.payload.shell === "disabled",
};
// initialize state and start installation
const promise = runPrepPhase(prepOptions);
const promise = runInstallation();
ctx.toolState.dependencyInstallation = {
status: "in_progress",
promise,
results: undefined,
};
// when promise completes, update state
promise.then(
(results) => {
if (ctx.toolState.dependencyInstallation) {
const hasFailure = results.some((r) => !r.dependenciesInstalled && r.issues.length > 0);
ctx.toolState.dependencyInstallation.status = hasFailure ? "failed" : "completed";
ctx.toolState.dependencyInstallation.status = "completed";
ctx.toolState.dependencyInstallation.results = results;
}
},
@@ -111,37 +52,18 @@ export function StartDependencyInstallationTool(ctx: ToolContext) {
return tool({
name: "start_dependency_installation",
description:
"Start installing project dependencies in the background. This is non-blocking and returns immediately. Call this early (right after branch checkout) if you anticipate needing to run tests, builds, or other commands that require dependencies. Idempotent - safe to call multiple times.",
"Start installing project dependencies in the background. Non-blocking, returns immediately. Call early after branch checkout.",
parameters: EmptyParams,
execute: execute(async () => {
const state = ctx.toolState.dependencyInstallation;
// already completed
if (state?.status === "completed" || state?.status === "failed") {
return {
status: state.status,
message: `Dependency installation already completed.`,
summary: formatPrepResults(state.results || []),
};
return { status: state.status, message: "Dependency installation already completed." };
}
// already in progress
if (state?.status === "in_progress") {
return {
status: "in_progress",
message:
"Dependency installation is already in progress. Call await_dependency_installation when you need to use them.",
};
return { status: "in_progress", message: "Dependency installation is already in progress." };
}
// start installation
startInstallation(ctx);
return {
status: "started",
message:
"Dependency installation started in background. Continue with other tasks and call await_dependency_installation when you need to run tests, builds, or other commands that require dependencies.",
};
return { status: "started", message: "Dependency installation started in background." };
}),
});
}
@@ -150,38 +72,20 @@ export function AwaitDependencyInstallationTool(ctx: ToolContext) {
return tool({
name: "await_dependency_installation",
description:
"Wait for dependency installation to complete and get the results. If installation hasn't been started yet, this will start it automatically. Call this before running tests, builds, or other commands that require dependencies.",
"Wait for dependency installation to complete. Auto-starts if not yet started.",
parameters: EmptyParams,
execute: execute(async () => {
// auto-start if not started
if (!ctx.toolState.dependencyInstallation) {
startInstallation(ctx);
}
const state = ctx.toolState.dependencyInstallation;
if (!state) {
throw new Error("failed to initialize dependency installation state");
}
// if already completed, return cached results
if (!state) throw new Error("failed to initialize dependency installation state");
if (state.status === "completed" || state.status === "failed") {
return {
status: state.status,
message: formatPrepResults(state.results || []),
};
return { status: state.status, message: "Dependency installation complete." };
}
// await the promise
if (!state.promise) {
throw new Error("dependency installation state is corrupted - no promise found");
}
const results = await state.promise;
return {
status: state.status,
message: formatPrepResults(results),
};
if (!state.promise) throw new Error("dependency installation state corrupted");
await state.promise;
return { status: state.status, message: "Dependency installation complete." };
}),
});
}
mcp/geminiSanitizer.ts
-207
View File
@@ -1,207 +0,0 @@
import type { StandardSchemaV1 } from "@standard-schema/spec";
import type { Tool } from "fastmcp";
import type { ToolContext } from "./server.ts";
// ── gemini schema sanitizer ────────────────────────────────────────────────────
//
// gemini's generateContent API expects an OpenAPI 3.0 Schema subset, not full
// JSON Schema. arktype 2.x emits constructs that gemini rejects with errors like:
// - "parameters.<field>.enum: only allowed for STRING type"
// - "functionDeclaration parameters.<field> schema didn't specify the schema type field"
// - "anyOf must be the only field in a schema node"
//
// transforms applied here:
// 1. add `type: "string"` to enum-only schemas. arktype emits string literal
// unions as `{enum: ["a","b"]}` without a `type` field — gemini requires
// the type declaration for any non-object schema.
// 2. collapse `{anyOf: [{enum:["a"]}, {enum:["b"]}]}` (older arktype form)
// into `{type:"string", enum:[...]}`. also handles `{const:"a"}` branches.
// 3. when `anyOf` / `oneOf` can't be collapsed, strip sibling fields (`type`,
// `description`, `items`, etc.) — gemini rejects `anyOf` alongside any
// peer keywords. see opencode #14659.
// 4. drop `$schema` metadata and rename `$defs` → `definitions` (draft-07
// compatibility; gemini doesn't understand either).
//
// gating: `isGeminiRouted()` detects gemini-targeted traffic so other
// providers continue to see the original (untransformed) schema.
//
// delivery: fastmcp (3.x) uses `xsschema.toJsonSchema()` which reads
// `schema["~standard"].jsonSchema.input({target:"draft-07"})` when present
// (arktype 2.x exposes this). we proxy the whole `~standard` chain so our
// transform runs regardless of which path xsschema takes.
function parseStringEnumBranch(item: unknown): { values: string[] } | null {
if (!item || typeof item !== "object") return null;
const record = item as Record<string, unknown>;
if (Array.isArray(record.enum)) {
const strings = record.enum.filter((v): v is string => typeof v === "string");
return strings.length === record.enum.length && strings.length > 0 ? { values: strings } : null;
}
if (typeof record.const === "string") {
return { values: [record.const] };
}
return null;
}
function collapseStringUnion(branches: unknown[]): { type: "string"; enum: string[] } | null {
const values: string[] = [];
for (const item of branches) {
const parsed = parseStringEnumBranch(item);
if (!parsed) return null;
values.push(...parsed.values);
}
if (values.length === 0) return null;
return { type: "string", enum: [...new Set(values)] };
}
/**
* Recursively transform a JSON schema to gemini's stricter subset.
* See module header for the exact transforms applied.
*/
export function sanitizeForGemini(schema: unknown): unknown {
if (!schema || typeof schema !== "object") return schema;
if (Array.isArray(schema)) return schema.map(sanitizeForGemini);
const source = schema as Record<string, unknown>;
// case 1: enum-only string union → add `type: "string"`.
// arktype emits `type: "'A' | 'B'"` as `{enum: ["A","B"]}` without a type.
if (Array.isArray(source.enum) && typeof source.type !== "string") {
const allStrings = source.enum.every((v) => typeof v === "string");
if (allStrings) {
const result: Record<string, unknown> = { type: "string", enum: source.enum };
if (typeof source.description === "string") result.description = source.description;
return result;
}
}
// case 2: collapsible string-enum union (older arktype form)
for (const unionKey of ["anyOf", "oneOf"] as const) {
const branches = source[unionKey];
if (Array.isArray(branches) && branches.length > 0) {
const collapsed = collapseStringUnion(branches);
if (collapsed) {
const result: Record<string, unknown> = { ...collapsed };
if (typeof source.description === "string") result.description = source.description;
return result;
}
}
}
// case 3: non-collapsible anyOf/oneOf → strip sibling fields (gemini rule)
if (Array.isArray(source.anyOf) || Array.isArray(source.oneOf)) {
const result: Record<string, unknown> = {};
if (Array.isArray(source.anyOf)) result.anyOf = source.anyOf.map(sanitizeForGemini);
if (Array.isArray(source.oneOf)) result.oneOf = source.oneOf.map(sanitizeForGemini);
return result;
}
// case 4: generic pass — drop $schema, rename $defs, recurse
const sanitized: Record<string, unknown> = {};
for (const [key, value] of Object.entries(source)) {
if (key === "$schema") continue;
if (key === "$defs") {
sanitized.definitions = sanitizeForGemini(value);
continue;
}
sanitized[key] = sanitizeForGemini(value);
}
return sanitized;
}
// ── delivery mechanism ─────────────────────────────────────────────────────────
//
// fastmcp 3.x resolves the JSON schema via xsschema, which takes two paths:
// path A: `schema["~standard"].jsonSchema.input({target:"draft-07"})` when
// the StandardJSONSchemaV1 extension is present (arktype 2.x).
// path B: `schema.toJsonSchema()` via a vendor-dispatched function (older
// arktype, other vendors).
//
// we proxy both entry points so the transform runs regardless of which path
// xsschema picks.
function wrapJsonSchemaProducer<T extends object>(producer: T): T {
return new Proxy(producer, {
get(target, prop, receiver) {
const value = Reflect.get(target, prop, receiver);
if ((prop === "input" || prop === "output") && typeof value === "function") {
const fn = value as (...args: unknown[]) => unknown;
return (...args: unknown[]) => sanitizeForGemini(fn.apply(target, args));
}
return value;
},
});
}
function wrapStandard<T extends object>(standard: T): T {
return new Proxy(standard, {
get(target, prop, receiver) {
if (prop === "jsonSchema") {
const value = Reflect.get(target, prop, receiver);
if (value && typeof value === "object") {
return wrapJsonSchemaProducer(value as object);
}
return value;
}
return Reflect.get(target, prop, receiver);
},
});
}
export function wrapSchemaForGemini(schema: StandardSchemaV1<any>): StandardSchemaV1<any> {
return new Proxy(schema, {
get(target, prop, receiver) {
if (prop === "~standard") {
const value = Reflect.get(target, prop, receiver);
if (value && typeof value === "object") {
return wrapStandard(value as object);
}
return value;
}
if (prop === "toJsonSchema") {
const method = Reflect.get(target, prop, receiver);
if (typeof method === "function") {
return () => sanitizeForGemini((method as (...args: unknown[]) => unknown).call(target));
}
return method;
}
return Reflect.get(target, prop, receiver);
},
}) as StandardSchemaV1<any>;
}
export function sanitizeToolForGemini<T extends Tool<any, any>>(tool: T): T {
if (!tool.parameters) return tool;
return { ...tool, parameters: wrapSchemaForGemini(tool.parameters) } as T;
}
/**
* true when the effective upstream model is — or might become — google
* generative language API traffic. matches:
* - direct `google/*`, opencode `opencode/gemini-*`, openrouter
* `openrouter/google/gemini-*` (slug substring "gemini" wins).
* - any unresolved specifier: `undefined`, `"auto"`, or a slug that
* didn't map through the alias registry (no `provider/` prefix).
* these flow through the agent's own auto-select, which may land
* on gemini *after* the MCP server has already registered tools —
* at which point sanitization is too late to apply. erring on the
* side of sanitizing is safe: cases 1 + 2 are universally
* compatible JSON-Schema normalizations (enum-only → typed string,
* collapsible const-unions → string enum); case 3 is gemini-
* specific but only fires on non-collapsible unions, which arktype
* does not emit for our current tool schemas. see issue #676 for
* the prod failure that motivated this widening.
*/
export function isGeminiRouted(ctx: ToolContext): boolean {
const effective = ctx.payload.proxyModel ?? ctx.resolvedModel ?? ctx.payload.model;
if (!effective) return true;
const normalized = effective.toLowerCase();
if (normalized.includes("gemini")) return true;
// every concrete model resolved through the registry carries a
// `provider/` prefix (e.g. "anthropic/claude-opus-4-7"). anything
// without a slash is either the literal `"auto"` alias or an
// unrecognized slug that resolveModel logged a warning for — both
// route through the agent's late auto-select, which may pick gemini.
if (!normalized.includes("/")) return true;
return false;
}
mcp/git.test.ts
-184
View File
@@ -1,184 +0,0 @@
import { describe, expect, it } from "vitest";
import { classifyPushError } from "./git.ts";
// re-export the normalizeUrl function for testing
// note: in a real scenario, we'd export this from git.ts or move to a shared utils file
function normalizeUrl(url: string): string {
return url.replace(/\.git$/, "").toLowerCase();
}
describe("normalizeUrl", () => {
it("removes .git suffix", () => {
expect(normalizeUrl("https://github.com/owner/repo.git")).toBe("https://github.com/owner/repo");
});
it("lowercases URL", () => {
expect(normalizeUrl("https://github.com/Owner/Repo")).toBe("https://github.com/owner/repo");
});
it("handles URL without .git suffix", () => {
expect(normalizeUrl("https://github.com/owner/repo")).toBe("https://github.com/owner/repo");
});
it("handles combined case and .git suffix", () => {
expect(normalizeUrl("https://github.com/OWNER/REPO.git")).toBe("https://github.com/owner/repo");
});
});
describe("push URL validation", () => {
// these tests document the expected behavior
// actual integration testing happens via the agent test suite
it("should block push when actual URL differs from pushUrl", () => {
// pushUrl is set by setupGit (base repo) or checkout_pr (fork repo)
const pushUrl = "https://github.com/fork-owner/repo.git";
const actualUrl = "https://github.com/base-owner/repo.git"; // different repo
const pushUrlNormalized = normalizeUrl(pushUrl);
const actualUrlNormalized = normalizeUrl(actualUrl);
expect(pushUrlNormalized).not.toBe(actualUrlNormalized);
// in real code, this mismatch would throw an error
});
it("should allow push when actual URL matches pushUrl", () => {
const pushUrl = "https://github.com/fork-owner/repo.git";
const actualUrl = "https://github.com/fork-owner/repo"; // same repo, no .git
const pushUrlNormalized = normalizeUrl(pushUrl);
const actualUrlNormalized = normalizeUrl(actualUrl);
expect(pushUrlNormalized).toBe(actualUrlNormalized);
// in real code, this would allow the push
});
it("should handle case differences in URLs", () => {
const pushUrl = "https://github.com/Owner/Repo.git";
const actualUrl = "https://github.com/owner/repo";
const pushUrlNormalized = normalizeUrl(pushUrl);
const actualUrlNormalized = normalizeUrl(actualUrl);
expect(pushUrlNormalized).toBe(actualUrlNormalized);
});
});
describe("classifyPushError", () => {
describe("concurrent-push", () => {
it("matches client-side non-fast-forward (`fetch first`)", () => {
const msg =
"git push failed (exit 1): To https://github.com/o/r.git\n" +
" ! [rejected] feature -> feature (fetch first)\n" +
"error: failed to push some refs to 'https://github.com/o/r.git'\n" +
"hint: Updates were rejected because the remote contains work";
expect(classifyPushError(msg)).toBe("concurrent-push");
});
it("matches client-side `non-fast-forward` wording", () => {
const msg = "! [rejected] main -> main (non-fast-forward)";
expect(classifyPushError(msg)).toBe("concurrent-push");
});
it("matches server-side `cannot lock ref` (the case from #571)", () => {
const msg =
"remote: error: cannot lock ref 'refs/heads/feature': is at " +
"abc123 but expected def456\n" +
" ! [remote rejected] feature -> feature (cannot lock ref ...)";
expect(classifyPushError(msg)).toBe("concurrent-push");
});
});
describe("transient", () => {
it("matches RPC failed with HTTP 502", () => {
expect(
classifyPushError(
"fatal: unable to access 'https://github.com/o/r.git/': The requested URL returned error: 502"
)
).toBe("transient");
});
it("matches early EOF mid-pack", () => {
expect(
classifyPushError("fatal: the remote end hung up unexpectedly\nfatal: early EOF")
).toBe("transient");
});
it("matches RPC failed", () => {
expect(
classifyPushError("fatal: RPC failed; curl 56 OpenSSL SSL_read: Connection reset by peer")
).toBe("transient");
});
it("matches HTTP/2 stream not closed cleanly", () => {
expect(
classifyPushError("fatal: HTTP/2 stream 7 was not closed cleanly: PROTOCOL_ERROR (err 1)")
).toBe("transient");
});
it("matches DNS resolution failure", () => {
expect(classifyPushError("fatal: Could not resolve host: github.com")).toBe("transient");
});
it("matches unexpected disconnect during sideband read", () => {
expect(classifyPushError("fatal: unexpected disconnect while reading sideband packet")).toBe(
"transient"
);
});
it("classifies HTTP 429 (rate-limit / abuse detection) as transient", () => {
// 429 is the documented exception to the otherwise-permanent 4xx class —
// GitHub's abuse detection occasionally surfaces it on git push.
expect(
classifyPushError(
"fatal: unable to access 'https://github.com/o/r.git/': The requested URL returned error: 429"
)
).toBe("transient");
expect(classifyPushError("remote: HTTP 429: too many requests")).toBe("transient");
});
});
describe("unknown", () => {
it("does NOT classify auth/403 as transient", () => {
// permission denied is permanent within a run — retrying just wastes
// time. must NOT match the HTTP-5xx regex.
expect(
classifyPushError(
"remote: Permission to o/r.git denied to bot.\n" +
"fatal: unable to access 'https://github.com/o/r.git/': The requested URL returned error: 403"
)
).toBe("unknown");
});
it("does NOT classify protected-branch rejection as concurrent-push", () => {
expect(
classifyPushError(
" ! [remote rejected] main -> main (push declined due to repository rule violations)"
)
).toBe("unknown");
});
it("does NOT classify 404 as transient", () => {
expect(
classifyPushError(
"fatal: unable to access 'https://github.com/o/r.git/': The requested URL returned error: 404"
)
).toBe("unknown");
});
it("returns unknown for an empty message", () => {
expect(classifyPushError("")).toBe("unknown");
});
});
describe("ordering", () => {
it("prefers concurrent-push over transient when both signals appear", () => {
// a server-side cannot-lock-ref response that also includes an HTTP
// 5xx in the libcurl envelope should still route to the recovery
// path, not a blind retry.
const msg =
"remote: error: cannot lock ref 'refs/heads/feature': is at A but expected B\n" +
"fatal: unable to access ...: The requested URL returned error: 500";
expect(classifyPushError(msg)).toBe("concurrent-push");
});
});
});
mcp/git.ts
+2 -2
View File
@@ -217,7 +217,7 @@ export function classifyPushError(msg: string): PushErrorKind {
const TRANSIENT_RETRY_DELAYS_MS = [2000, 5000];
export function PushBranchTool(ctx: ToolContext) {
const defaultBranch = ctx.repo.data.default_branch || "main";
const defaultBranch = process.env.DEFAULT_BRANCH || "main";
const pushPermission = ctx.payload.push;
return tool({
@@ -643,7 +643,7 @@ const DeleteBranch = type({
export function DeleteBranchTool(ctx: ToolContext) {
const pushPermission = ctx.payload.push;
const defaultBranch = ctx.repo.data.default_branch || "main";
const defaultBranch = process.env.DEFAULT_BRANCH || "main";
return tool({
name: "delete_branch",
mcp/issue.ts
+14 -20
View File
@@ -1,7 +1,6 @@
import { type } from "arktype";
import { log } from "../utils/cli.ts";
import { fixDoubleEscapedString } from "../utils/fixDoubleEscapedString.ts";
import { patchWorkflowRunFields } from "../utils/patchWorkflowRunFields.ts";
import type { ToolContext } from "./server.ts";
import { execute, tool } from "./shared.ts";
@@ -21,38 +20,33 @@ export const Issue = type({
export function IssueTool(ctx: ToolContext) {
return tool({
name: "create_issue",
description: "Create a new GitHub issue",
description: "Create a new Gitea issue",
parameters: Issue,
execute: execute(async (params) => {
const result = await ctx.octokit.rest.issues.create({
const result = await ctx.gitea.rest.issue.issueCreateIssue({
owner: ctx.repo.owner,
repo: ctx.repo.name,
title: params.title,
body: fixDoubleEscapedString(params.body),
labels: params.labels ?? [],
assignees: params.assignees ?? [],
body: {
title: params.title,
body: fixDoubleEscapedString(params.body),
assignees: params.assignees,
},
});
log.info(`» created issue #${result.data.number} (id ${result.data.id})`);
const nodeId = result.data.node_id;
if (typeof nodeId === "string" && nodeId.length > 0) {
await patchWorkflowRunFields(ctx, {
issueNodeId: nodeId,
});
}
log.info(`» created issue #${result.data.number}`);
return {
success: true,
issueId: result.data.id,
number: result.data.number,
url: result.data.html_url,
title: result.data.title,
state: result.data.state,
labels: result.data.labels?.map((label) =>
typeof label === "string" ? label : label.name
),
assignees: result.data.assignees?.map((assignee) => assignee.login),
labels: result.data.labels
?.map((l) => (typeof l === "string" ? l : l.name))
.filter((n): n is string => n !== undefined),
assignees: result.data.assignees
?.map((a) => a.login)
.filter((n): n is string => n !== undefined),
};
}),
});
mcp/issueComments.ts
+7 -8
View File
@@ -10,25 +10,24 @@ export function GetIssueCommentsTool(ctx: ToolContext) {
return tool({
name: "get_issue_comments",
description:
"Get all comments for a GitHub issue. Returns all comments including the issue body and all subsequent discussion comments. " +
"Get all comments for a Gitea issue or PR. " +
"Example: `get_issue_comments({ issue_number: 1234 })`.",
parameters: GetIssueComments,
execute: execute(async ({ issue_number }) => {
// set issue context
ctx.toolState.issueNumber = issue_number;
const comments = await ctx.octokit.paginate(ctx.octokit.rest.issues.listComments, {
const comments = await ctx.gitea.paginate(ctx.gitea.rest.issue.issueGetComments, {
owner: ctx.repo.owner,
repo: ctx.repo.name,
issue_number,
index: issue_number,
});
return {
issue_number,
comments: comments.map((comment) => ({
id: comment.id,
body: comment.body,
user: comment.user?.login,
comments: comments.map((c) => ({
id: c.id,
body: c.body,
user: c.user?.login,
})),
count: comments.length,
};
mcp/issueEvents.ts
+4 -78
View File
@@ -10,90 +10,16 @@ export function GetIssueEventsTool(ctx: ToolContext) {
return tool({
name: "get_issue_events",
description:
"Get timeline events for a GitHub issue that aren't reflected in the current state. Returns cross-references to other issues/PRs and commit references. Note: current labels, assignees, state, and milestone are already available via get_issue.",
"Get timeline events for a Gitea issue that aren't reflected in the current state.",
parameters: GetIssueEvents,
execute: execute(async ({ issue_number }) => {
// set issue context
ctx.toolState.issueNumber = issue_number;
const events = await ctx.octokit.paginate(ctx.octokit.rest.issues.listEventsForTimeline, {
owner: ctx.repo.owner,
repo: ctx.repo.name,
issue_number,
});
// Only include events not reflected in current issue state (get_issue already has labels, assignees, state, etc.)
// Keep only relationship/reference events that show connections to other issues/PRs/commits
const relevantEventTypes = new Set(["cross_referenced", "referenced"]);
const parsedEvents = events.flatMap((event) => {
// octokit's timeline-event union includes members with `event?:
// string`, so `"event" in event` does not narrow it to defined.
// require a string before the Set.has() check.
if (!("event" in event) || typeof event.event !== "string") return [];
if (!relevantEventTypes.has(event.event)) return [];
const baseEvent: Record<string, any> = {
event: event.event,
};
// Common fields
if ("id" in event) {
baseEvent.id = event.id;
}
if ("actor" in event && event.actor) {
baseEvent.actor = event.actor.login;
} else if ("user" in event && event.user) {
baseEvent.actor = event.user.login;
}
if ("created_at" in event) {
baseEvent.created_at = event.created_at;
}
// Event-specific data
if (event.event === "cross_referenced") {
if ("source" in event && event.source) {
const source = event.source as {
type?: string;
issue?: { number: number; title: string; html_url: string };
pull_request?: { number: number; title: string; html_url: string };
};
baseEvent.source = {
type: source.type,
issue: source.issue
? {
number: source.issue.number,
title: source.issue.title,
html_url: source.issue.html_url,
}
: null,
pull_request: source.pull_request
? {
number: source.pull_request.number,
title: source.pull_request.title,
html_url: source.pull_request.html_url,
}
: null,
};
}
}
if (event.event === "referenced") {
if ("commit_id" in event) {
baseEvent.commit_id = event.commit_id;
}
if ("commit_url" in event) {
baseEvent.commit_url = event.commit_url;
}
}
return [baseEvent];
});
// Gitea's timeline API differs from GitHub's; return empty for now.
return {
issue_number,
events: parsedEvents,
count: parsedEvents.length,
events: [],
count: 0,
};
}),
});
mcp/issueInfo.ts
+12 -22
View File
@@ -10,28 +10,22 @@ export function IssueInfoTool(ctx: ToolContext) {
return tool({
name: "get_issue",
description:
"Retrieve GitHub issue information by issue number. " +
"Retrieve Gitea issue information by issue number. " +
"Example: `get_issue({ issue_number: 1234 })`.",
parameters: IssueInfo,
execute: execute(async ({ issue_number }) => {
const issue = await ctx.octokit.rest.issues.get({
const result = await ctx.gitea.rest.issue.issueGetIssue({
owner: ctx.repo.owner,
repo: ctx.repo.name,
issue_number,
index: issue_number,
});
const data = issue.data;
// set issue context
const data = result.data;
ctx.toolState.issueNumber = issue_number;
const hints: string[] = [];
if (data.comments > 0) {
if ((data.comments ?? 0) > 0) {
hints.push("use get_issue_comments to retrieve all comments for this issue");
}
hints.push(
"use get_issue_events to retrieve cross-references and commit references (relationships not reflected in current state)"
);
return {
number: data.number,
@@ -39,23 +33,19 @@ export function IssueInfoTool(ctx: ToolContext) {
title: data.title,
body: data.body,
state: data.state,
locked: data.locked,
labels: data.labels?.map((label) => (typeof label === "string" ? label : label.name)),
assignees: data.assignees?.map((assignee) => assignee.login),
labels: data.labels
?.map((l) => (typeof l === "string" ? l : l.name))
.filter((n): n is string => n !== undefined),
assignees: data.assignees
?.map((a) => a.login)
.filter((n): n is string => n !== undefined),
user: data.user?.login,
created_at: data.created_at,
updated_at: data.updated_at,
closed_at: data.closed_at,
comments: data.comments,
milestone: data.milestone?.title,
pull_request: data.pull_request
? {
url: data.pull_request.url,
html_url: data.pull_request.html_url,
diff_url: data.pull_request.diff_url,
patch_url: data.pull_request.patch_url,
}
: null,
pull_request: data.pull_request ? { html_url: data.pull_request.html_url } : null,
hints,
};
}),
mcp/labels.ts
+18 -5
View File
@@ -12,20 +12,33 @@ export function AddLabelsTool(ctx: ToolContext) {
return tool({
name: "add_labels",
description:
"Add labels to a GitHub issue or pull request. Only use labels that already exist in the repository.",
"Add labels to a Gitea issue or pull request. Only use labels that already exist in the repository.",
parameters: AddLabelsParams,
execute: execute(async ({ issue_number, labels }) => {
const result = await ctx.octokit.rest.issues.addLabels({
// Resolve label names to IDs (Gitea uses IDs, not names)
const allLabels = await ctx.gitea.paginate(ctx.gitea.rest.issue.issueListLabels, {
owner: ctx.repo.owner,
repo: ctx.repo.name,
issue_number,
labels,
});
const labelIds = labels
.map((name) => allLabels.find((l) => l.name === name)?.id)
.filter((id): id is number => typeof id === "number");
if (labelIds.length === 0) {
return { success: true, labels: [], message: "No matching labels found in repository" };
}
const result = await ctx.gitea.rest.issue.issueAddLabel({
owner: ctx.repo.owner,
repo: ctx.repo.name,
index: issue_number,
body: { labels: labelIds },
});
log.info(`» added labels [${labels.join(", ")}] to issue #${issue_number}`);
return {
success: true,
labels: result.data.map((label) => label.name),
labels: result.data.map((label) => label.name).filter(Boolean),
};
}),
});
mcp/pr.ts
+25 -55
View File
@@ -1,8 +1,7 @@
import { type } from "arktype";
import { buildPullfrogFooter, stripExistingFooter } from "../utils/buildPullfrogFooter.ts";
import { buildShockbotFooter, stripExistingFooter } from "../utils/buildShockbotFooter.ts";
import { log } from "../utils/cli.ts";
import { fixDoubleEscapedString } from "../utils/fixDoubleEscapedString.ts";
import { patchWorkflowRunFields } from "../utils/patchWorkflowRunFields.ts";
import { $ } from "../utils/shell.ts";
import type { ToolContext } from "./server.ts";
import { execute, tool } from "./shared.ts";
@@ -11,23 +10,12 @@ export const PullRequest = type({
title: type.string.describe("the title of the pull request"),
body: type.string.describe("the body content of the pull request"),
base: type.string.describe("the base branch to merge into (e.g., 'main')"),
"draft?": type.boolean.describe(
"if true, create the pull request as a draft. use when the user explicitly asks for a draft PR."
),
"draft?": type.boolean.describe("if true, create the pull request as a draft."),
});
function buildPrBodyWithFooter(ctx: ToolContext, body: string): string {
const footer = buildPullfrogFooter({
triggeredBy: true,
workflowRun: ctx.runId
? { owner: ctx.repo.owner, repo: ctx.repo.name, runId: ctx.runId, jobId: ctx.jobId }
: undefined,
model: ctx.toolState.model,
fallbackFrom: ctx.toolState.modelFallback?.from,
});
const bodyWithoutFooter = stripExistingFooter(fixDoubleEscapedString(body));
return `${bodyWithoutFooter}${footer}`;
const footer = buildShockbotFooter({ model: ctx.toolState.model });
return `${stripExistingFooter(fixDoubleEscapedString(body))}${footer}`;
}
export const UpdatePullRequestBody = type({
@@ -41,23 +29,15 @@ export function UpdatePullRequestBodyTool(ctx: ToolContext) {
description: "Update the body/description of an existing pull request",
parameters: UpdatePullRequestBody,
execute: execute(async (params) => {
const bodyWithFooter = buildPrBodyWithFooter(ctx, params.body);
const result = await ctx.octokit.rest.pulls.update({
const result = await ctx.gitea.rest.repository.repoEditPullRequest({
owner: ctx.repo.owner,
repo: ctx.repo.name,
pull_number: params.pull_number,
body: bodyWithFooter,
index: params.pull_number,
body: { body: buildPrBodyWithFooter(ctx, params.body) },
});
log.info(`» updated pull request #${result.data.number}`);
ctx.toolState.wasUpdated = true;
return {
success: true,
number: result.data.number,
url: result.data.html_url,
};
return { success: true, number: result.data.number, url: result.data.html_url };
}),
});
}
@@ -68,52 +48,42 @@ export function CreatePullRequestTool(ctx: ToolContext) {
description: "Create a pull request from the current branch",
parameters: PullRequest,
execute: execute(async (params) => {
const currentBranch = $("git", ["rev-parse", "--abbrev-ref", "HEAD"], { log: false });
log.debug(`Current branch: ${currentBranch}`);
const currentBranch = $("git", ["rev-parse", "--abbrev-ref", "HEAD"], { log: false }).trim();
const bodyWithFooter = buildPrBodyWithFooter(ctx, params.body);
const result = await ctx.octokit.rest.pulls.create({
const result = await ctx.gitea.rest.repository.repoCreatePullRequest({
owner: ctx.repo.owner,
repo: ctx.repo.name,
title: params.title,
body: bodyWithFooter,
head: currentBranch,
base: params.base,
draft: params.draft ?? false,
body: {
title: params.title,
body: buildPrBodyWithFooter(ctx, params.body),
head: currentBranch,
base: params.base,
// draft: params.draft ?? false, // not in all Gitea versions
},
});
log.info(`» created pull request #${result.data.number} (id ${result.data.id})`);
log.info(`» created pull request #${result.data.number}`);
// best-effort: request review from the user who triggered the workflow
const reviewer = ctx.payload.triggerer;
if (reviewer) {
if (reviewer && result.data.number) {
try {
log.debug(`requesting review from ${reviewer} on PR #${result.data.number}`);
await ctx.octokit.rest.pulls.requestReviewers({
await ctx.gitea.rest.repository.repoCreatePullReviewRequests({
owner: ctx.repo.owner,
repo: ctx.repo.name,
pull_number: result.data.number,
reviewers: [reviewer],
index: result.data.number,
body: { reviewers: [reviewer] },
});
} catch {
log.info(`failed to request review from ${reviewer} on PR #${result.data.number}`);
log.debug(`failed to request review from ${reviewer}`);
}
}
if (typeof result.data.node_id === "string" && result.data.node_id.length > 0) {
await patchWorkflowRunFields(ctx, {
prNodeId: result.data.node_id,
});
}
return {
success: true,
pullRequestId: result.data.id,
number: result.data.number,
url: result.data.html_url,
title: result.data.title,
head: result.data.head.ref,
base: result.data.base.ref,
head: result.data.head?.label ?? result.data.head?.ref,
base: result.data.base?.label ?? result.data.base?.ref,
};
}),
});
mcp/prInfo.ts
+18 -44
View File
@@ -2,26 +2,6 @@ import { type } from "arktype";
import type { ToolContext } from "./server.ts";
import { execute, tool } from "./shared.ts";
const CLOSING_ISSUES_QUERY = `
query($owner: String!, $repo: String!, $number: Int!) {
repository(owner: $owner, name: $repo) {
pullRequest(number: $number) {
closingIssuesReferences(first: 10) {
nodes { number title }
}
}
}
}
`;
type ClosingIssuesResponse = {
repository: {
pullRequest: {
closingIssuesReferences: { nodes: Array<{ number: number; title: string }> };
};
};
};
export const PullRequestInfo = type({
pull_number: type.number.describe("The pull request number to fetch"),
});
@@ -30,28 +10,18 @@ export function PullRequestInfoTool(ctx: ToolContext) {
return tool({
name: "get_pull_request",
description:
"Retrieve PR metadata (title, body, state, branches, author, labels, linked issues). " +
"Retrieve PR metadata (title, body, state, branches, author, labels). " +
"Example: `get_pull_request({ pull_number: 1234 })`. " +
"To checkout a PR branch locally, use checkout_pr instead.",
parameters: PullRequestInfo,
execute: execute(async ({ pull_number }) => {
// fetch REST and GraphQL in parallel
const [restResponse, graphqlResponse] = await Promise.all([
ctx.octokit.rest.pulls.get({
owner: ctx.repo.owner,
repo: ctx.repo.name,
pull_number,
}),
ctx.octokit.graphql<ClosingIssuesResponse>(CLOSING_ISSUES_QUERY, {
owner: ctx.repo.owner,
repo: ctx.repo.name,
number: pull_number,
}),
]);
const data = restResponse.data;
const isFork = data.head.repo?.full_name !== data.base.repo.full_name;
const closingIssues = graphqlResponse.repository.pullRequest.closingIssuesReferences.nodes;
const result = await ctx.gitea.rest.repository.repoGetPullRequest({
owner: ctx.repo.owner,
repo: ctx.repo.name,
index: pull_number,
});
const data = result.data;
const isFork = data.head?.repo?.full_name !== data.base?.repo?.full_name;
return {
number: data.number,
@@ -61,14 +31,18 @@ export function PullRequestInfoTool(ctx: ToolContext) {
state: data.state,
draft: data.draft,
merged: data.merged,
maintainerCanModify: data.maintainer_can_modify,
base: data.base.ref,
head: data.head.ref,
maintainerCanModify: data.allow_maintainer_edit,
base: data.base?.label,
head: data.head?.label,
isFork,
author: data.user?.login,
assignees: data.assignees?.map((a) => a.login),
labels: data.labels.map((l) => l.name),
closingIssues: closingIssues.map((i) => ({ number: i.number, title: i.title })),
assignees: data.assignees
?.map((a) => a.login)
.filter((n): n is string => n !== undefined),
labels: data.labels
?.map((l) => (typeof l === "string" ? l : l.name))
.filter((n): n is string => n !== undefined),
closingIssues: [],
};
}),
});
mcp/review.test.ts
-400
View File
@@ -1,400 +0,0 @@
import { describe, expect, it } from "vitest";
import {
type CommentableLines,
commentableLinesForFile,
type DroppedComment,
duplicateReviewDecision,
formatDroppedCommentsNote,
MAX_DROPPED_COMMENT_LINES,
type ReviewCommentInput,
reviewSkipDecision,
validateInlineComments,
} from "./review.ts";
describe("commentableLinesForFile", () => {
it("returns empty sets for missing patches (binary or no changes)", () => {
const result = commentableLinesForFile(undefined);
expect(result.LEFT.size).toBe(0);
expect(result.RIGHT.size).toBe(0);
});
it("collects added lines on RIGHT, removed lines on LEFT, context on both", () => {
const patch = ["@@ -10,3 +10,4 @@", " ctx1", "-old", "+new", "+new2", " ctx2"].join("\n");
const { LEFT, RIGHT } = commentableLinesForFile(patch);
expect([...LEFT].sort((a, b) => a - b)).toEqual([10, 11, 12]);
expect([...RIGHT].sort((a, b) => a - b)).toEqual([10, 11, 12, 13]);
});
it("handles multiple hunks", () => {
const patch = ["@@ -1,2 +1,2 @@", " a", "-b", "+B", "@@ -20,1 +20,2 @@", " x", "+y"].join("\n");
const { LEFT, RIGHT } = commentableLinesForFile(patch);
expect(RIGHT.has(2)).toBe(true); // +B
expect(RIGHT.has(21)).toBe(true); // +y
expect(LEFT.has(2)).toBe(true); // -b
expect(LEFT.has(20)).toBe(true); // context x
expect(RIGHT.has(20)).toBe(true); // context x
});
it("ignores the 'no newline at end of file' marker", () => {
const patch = ["@@ -1,1 +1,1 @@", "-old", "\\ No newline at end of file", "+new"].join("\n");
const { LEFT, RIGHT } = commentableLinesForFile(patch);
expect(LEFT.has(1)).toBe(true);
expect(RIGHT.has(1)).toBe(true);
expect(LEFT.size).toBe(1);
expect(RIGHT.size).toBe(1);
});
it("parses hunk headers without explicit counts", () => {
// single-line hunks can omit ",<count>"
const patch = ["@@ -5 +5 @@", "-old", "+new"].join("\n");
const { LEFT, RIGHT } = commentableLinesForFile(patch);
expect(LEFT.has(5)).toBe(true);
expect(RIGHT.has(5)).toBe(true);
});
});
function buildMap(entries: Array<[string, string]>): Map<string, CommentableLines> {
const map = new Map<string, CommentableLines>();
for (const [file, patch] of entries) {
map.set(file, commentableLinesForFile(patch));
}
return map;
}
describe("validateInlineComments", () => {
const patch = ["@@ -10,2 +10,3 @@", " ctx", "-old", "+new", "+new2"].join("\n");
const diffMap = buildMap([["src/foo.ts", patch]]);
const base = (overrides: Partial<ReviewCommentInput>): ReviewCommentInput => ({
path: "src/foo.ts",
line: 11,
side: "RIGHT",
body: "LGTM",
...overrides,
});
it("keeps comments anchored to added lines on RIGHT", () => {
const result = validateInlineComments([base({ line: 12 })], diffMap);
expect(result.valid).toHaveLength(1);
expect(result.dropped).toHaveLength(0);
});
it("keeps comments anchored to removed lines on LEFT", () => {
const result = validateInlineComments([base({ line: 11, side: "LEFT" })], diffMap);
expect(result.valid).toHaveLength(1);
expect(result.dropped).toHaveLength(0);
});
it("drops comments on files not in the diff", () => {
const result = validateInlineComments([base({ path: "other/bar.ts" })], diffMap);
expect(result.valid).toHaveLength(0);
expect(result.dropped).toHaveLength(1);
expect(result.dropped[0].reason).toContain("file not in PR diff");
});
it("distinguishes binary/no-patch files from files with hunks", () => {
// file present in the PR but with no patch data (binary file).
const binaryMap = buildMap([
["src/foo.ts", patch],
["assets/logo.png", undefined as unknown as string],
]);
const result = validateInlineComments([base({ path: "assets/logo.png", line: 1 })], binaryMap);
expect(result.valid).toHaveLength(0);
expect(result.dropped).toHaveLength(1);
expect(result.dropped[0].reason).toContain("no textual diff");
expect(result.dropped[0].reason).not.toContain("not inside a diff hunk");
});
it("drops comments on lines outside diff hunks", () => {
const result = validateInlineComments([base({ line: 500 })], diffMap);
expect(result.valid).toHaveLength(0);
expect(result.dropped).toHaveLength(1);
expect(result.dropped[0].reason).toContain("line 500");
expect(result.dropped[0].reason).toContain("RIGHT");
});
it("drops comments whose side mismatches the hunk (added line on LEFT)", () => {
// line 12 is "+new" — only in RIGHT. Asking for it on LEFT should drop.
const result = validateInlineComments([base({ line: 12, side: "LEFT" })], diffMap);
expect(result.valid).toHaveLength(0);
expect(result.dropped).toHaveLength(1);
});
it("drops multi-line comments where start_line is out of range", () => {
const result = validateInlineComments([base({ line: 12, start_line: 3 })], diffMap);
expect(result.valid).toHaveLength(0);
expect(result.dropped).toHaveLength(1);
expect(result.dropped[0].reason).toContain("start_line 3");
});
it("keeps multi-line comments fully inside a hunk", () => {
const result = validateInlineComments([base({ line: 12, start_line: 11 })], diffMap);
expect(result.valid).toHaveLength(1);
expect(result.dropped).toHaveLength(0);
});
it("drops inverted ranges (start_line > line) with a precise reason", () => {
// both 11 and 12 anchor in the hunk, but GitHub 422s with "invalid line
// numbers" when start_line > line. dropping locally avoids the opaque
// remote failure and tells the agent exactly what to fix.
const result = validateInlineComments([base({ line: 11, start_line: 12 })], diffMap);
expect(result.valid).toHaveLength(0);
expect(result.dropped).toHaveLength(1);
expect(result.dropped[0].reason).toMatch(/start_line 12 is after line 11/);
expect(result.dropped[0].reason).toMatch(/start_line <= line/);
});
it("partitions a batch — valid and invalid comments survive independently", () => {
const result = validateInlineComments(
[base({ line: 12 }), base({ line: 9999 }), base({ path: "missing.ts" })],
diffMap
);
expect(result.valid).toHaveLength(1);
expect(result.dropped).toHaveLength(2);
});
it("defaults side to RIGHT when omitted", () => {
const result = validateInlineComments([{ path: "src/foo.ts", line: 12, body: "" }], diffMap);
expect(result.valid).toHaveLength(1);
});
});
describe("formatDroppedCommentsNote", () => {
it("renders single-line dropped entries with `path:line`", () => {
const dropped: DroppedComment[] = [
{
path: "src/foo.ts",
line: 42,
side: "RIGHT",
reason: "line 42 (RIGHT) is not inside a diff hunk",
},
];
const note = formatDroppedCommentsNote(dropped);
expect(note).toContain("**Note:** 1 inline comment(s) dropped");
expect(note).toContain("`src/foo.ts:42` (RIGHT)");
expect(note).toContain("line 42 (RIGHT) is not inside a diff hunk");
});
it("renders multi-line dropped entries with `path:start-end`", () => {
const dropped: DroppedComment[] = [
{
path: "src/bar.ts",
line: 20,
startLine: 15,
side: "LEFT",
reason: "start_line 15 (LEFT) is not inside a diff hunk",
},
];
const note = formatDroppedCommentsNote(dropped);
expect(note).toContain("`src/bar.ts:15-20` (LEFT)");
});
it("falls back to single-line format when startLine equals line", () => {
const dropped: DroppedComment[] = [
{ path: "src/baz.ts", line: 7, startLine: 7, side: "RIGHT", reason: "file not in PR diff" },
];
const note = formatDroppedCommentsNote(dropped);
expect(note).toContain("`src/baz.ts:7` (RIGHT)");
expect(note).not.toContain("7-7");
});
it("caps detail lines and reports the remainder so body stays under GitHub's size limit", () => {
const overflow = MAX_DROPPED_COMMENT_LINES + 7;
const dropped: DroppedComment[] = Array.from({ length: overflow }, (_, i) => ({
path: `src/file${i}.ts`,
line: i + 1,
side: "RIGHT" as const,
reason: "file not in PR diff",
}));
const note = formatDroppedCommentsNote(dropped);
expect(note).toContain(`**Note:** ${overflow} inline comment(s) dropped`);
// still reports the full count in the header
expect(note).toContain(`${overflow} inline comment(s)`);
// first entry shown, last entry elided
expect(note).toContain("`src/file0.ts:1` (RIGHT)");
expect(note).not.toContain(`src/file${overflow - 1}.ts`);
expect(note).toContain("…and 7 more dropped comment(s) not shown");
});
it("does not add a truncation line when drops fit under the cap", () => {
const dropped: DroppedComment[] = Array.from({ length: MAX_DROPPED_COMMENT_LINES }, (_, i) => ({
path: `src/f${i}.ts`,
line: i + 1,
side: "RIGHT" as const,
reason: "file not in PR diff",
}));
const note = formatDroppedCommentsNote(dropped);
expect(note).not.toContain("more dropped comment(s) not shown");
});
});
describe("reviewSkipDecision", () => {
// GitHub 422s `event: "COMMENT"` reviews with no body + no comments
// ("{\"message\":\"Unprocessable Entity\",\"errors\":[\"\"]}"). verified
// empirically against repos/pullfrog/preview-546-run-issues-fixes/pulls/1
// with and without commit_id set. the skip function must return a decision
// for every shape that lands on that API call.
it("skips with 'no-issues' when !approved + empty body + no comments", () => {
const decision = reviewSkipDecision({
approved: false,
body: "",
hasComments: false,
prApproveEnabled: true,
});
expect(decision?.kind).toBe("no-issues");
expect(decision?.reason).toContain("nothing to post");
});
it("treats null body the same as empty string", () => {
const decision = reviewSkipDecision({
approved: false,
body: null,
hasComments: false,
prApproveEnabled: true,
});
expect(decision?.kind).toBe("no-issues");
});
it("treats undefined body the same as empty string", () => {
const decision = reviewSkipDecision({
approved: false,
body: undefined,
hasComments: false,
prApproveEnabled: true,
});
expect(decision?.kind).toBe("no-issues");
});
it("skips with 'empty-downgraded-approve' when approved + !prApproveEnabled + empty", () => {
// this is the F3 regression case — agent requests APPROVE, runtime
// downgrades to COMMENT (prApproveEnabled off), and the empty COMMENT
// 422s at GitHub. before this fix, the tool returned a stranded-success
// shape that didn't map to any persisted review.
const decision = reviewSkipDecision({
approved: true,
body: "",
hasComments: false,
prApproveEnabled: false,
});
expect(decision?.kind).toBe("empty-downgraded-approve");
expect(decision?.reason).toContain("prApproveEnabled is disabled");
});
it("does NOT skip legitimate bare APPROVE (approved + prApproveEnabled + empty)", () => {
// GitHub accepts empty APPROVE reviews — the stamp itself is the content.
// skipping here would silently drop agents' real approvals.
const decision = reviewSkipDecision({
approved: true,
body: "",
hasComments: false,
prApproveEnabled: true,
});
expect(decision).toBeNull();
});
it("does NOT skip when body is present (no-issues path)", () => {
const decision = reviewSkipDecision({
approved: false,
body: "found some issues",
hasComments: false,
prApproveEnabled: true,
});
expect(decision).toBeNull();
});
it("does NOT skip when body is present (downgrade path)", () => {
// approved+!prApproveEnabled with a body becomes a real COMMENT review
// (downgrade + body). GitHub accepts those; don't skip.
const decision = reviewSkipDecision({
approved: true,
body: "nits follow",
hasComments: false,
prApproveEnabled: false,
});
expect(decision).toBeNull();
});
it("does NOT skip when comments are present (no-issues path)", () => {
const decision = reviewSkipDecision({
approved: false,
body: "",
hasComments: true,
prApproveEnabled: true,
});
expect(decision).toBeNull();
});
it("does NOT skip when comments are present (downgrade path)", () => {
const decision = reviewSkipDecision({
approved: true,
body: "",
hasComments: true,
prApproveEnabled: false,
});
expect(decision).toBeNull();
});
});
describe("duplicateReviewDecision", () => {
// regression: colinhacks/zod#5897 had two reviews submitted from the same
// workflow run 8 seconds apart — a substantive review followed by an empty
// "No new issues found." follow-up. the agent re-classified the first
// review's non-blocking observations as "no actionable issues" and
// submitted the canonical body per modes.ts. this guard makes the second
// call a no-op without burning a GitHub API call or polluting the PR.
it("allows the first submission when no prior review exists", () => {
const decision = duplicateReviewDecision({
existing: undefined,
currentCheckoutSha: "sha1",
});
expect(decision).toBeNull();
});
it("blocks a second submission when checkoutSha matches the prior reviewedSha", () => {
// exact reproduction of the zod#5897 shape: same session, same checked-out
// SHA, second create_pull_request_review call.
const decision = duplicateReviewDecision({
existing: { id: 100, reviewedSha: "sha1" },
currentCheckoutSha: "sha1",
});
expect(decision?.kind).toBe("already-submitted");
expect(decision?.reviewId).toBe(100);
expect(decision?.reason).toContain("already submitted");
expect(decision?.reason).toContain("checkout_pr");
});
it("allows a follow-up when checkoutSha advanced past the prior reviewedSha", () => {
// the new-commits-mid-review path advances toolState.checkoutSha to the
// new HEAD before returning, and the agent is told to call checkout_pr
// again — both paths leave checkoutSha != reviewedSha. those are real
// follow-up reviews and must go through.
const decision = duplicateReviewDecision({
existing: { id: 100, reviewedSha: "sha-old" },
currentCheckoutSha: "sha-new",
});
expect(decision).toBeNull();
});
it("blocks when checkoutSha is missing — cannot prove the SHA moved", () => {
// if the agent never called checkout_pr, we have no anchor to compare
// against. assume duplicate rather than letting a second review through
// — the prior review still satisfies the agent's intent.
const decision = duplicateReviewDecision({
existing: { id: 100, reviewedSha: "sha1" },
currentCheckoutSha: undefined,
});
expect(decision?.kind).toBe("already-submitted");
});
it("blocks when prior reviewedSha is missing — cannot prove the SHA moved", () => {
// belt-and-suspenders: if for any reason the prior review didn't capture
// a reviewedSha, treat the second call as a duplicate to be safe.
const decision = duplicateReviewDecision({
existing: { id: 100, reviewedSha: undefined },
currentCheckoutSha: "sha1",
});
expect(decision?.kind).toBe("already-submitted");
});
});
mcp/review.ts
+124 -642
View File
@@ -1,9 +1,7 @@
import type { RestEndpointMethodTypes } from "@octokit/rest";
import { type } from "arktype";
import { formatMcpToolRef } from "../external.ts";
import type { CommentableLines } from "../toolState.ts";
import { getApiUrl } from "../utils/apiUrl.ts";
import { buildPullfrogFooter } from "../utils/buildPullfrogFooter.ts";
import { buildShockbotFooter } from "../utils/buildShockbotFooter.ts";
import { log } from "../utils/cli.ts";
import {
countLinesInRanges,
@@ -11,7 +9,7 @@ import {
renderDiffCoverageBreakdown,
} from "../utils/diffCoverage.ts";
import { fixDoubleEscapedString } from "../utils/fixDoubleEscapedString.ts";
import { patchWorkflowRunFields } from "../utils/patchWorkflowRunFields.ts";
import type { ChangedFileWithPatch } from "../utils/gitea.ts";
import { retry } from "../utils/retry.ts";
import { deleteProgressComment } from "./comment.ts";
import type { ToolContext } from "./server.ts";
@@ -19,42 +17,9 @@ import { execute, tool } from "./shared.ts";
export type { CommentableLines };
function getHttpStatus(err: unknown): number | undefined {
if (typeof err !== "object" || err === null) return undefined;
const status = (err as Record<string, unknown>).status;
return typeof status === "number" ? status : undefined;
}
/**
* detect GitHub's generic server-side 422 ("An internal error occurred,
* please try again.") that sometimes fires on `POST /pulls/{n}/reviews`.
*
* the body is stable across occurrences and distinct from every other 422
* cause we care about (anchor validation, body length, malformed suggestion
* blocks) — those all cite the specific problem. treating this as a
* transient server error unlocks bounded in-tool retry instead of surfacing
* it to the agent with the generic "likely causes (1)(2)(3)" prompt, which
* induces whack-a-mole comment dropping on content that was never the issue.
*/
export function isTransientReviewError(err: unknown): boolean {
if (getHttpStatus(err) !== 422) return false;
const msg = err instanceof Error ? err.message : String(err);
return /internal error occurred, please try again/i.test(msg);
}
// backoff schedule for transient GitHub 422 "internal error" responses on the
// reviews endpoint. 3 attempts total (initial + 2 retries) with 1s/3s delays
// — most transient GH errors clear within a few seconds, and longer delays
// push review submission past agent-perceived responsiveness.
export const TRANSIENT_REVIEW_RETRY_DELAYS_MS = [1_000, 3_000];
type PullFile = RestEndpointMethodTypes["pulls"]["listFiles"]["response"]["data"][number];
/**
* parse a PR file's patch to determine which line numbers on each side are
* valid anchors for inline comments. GitHub only accepts comments on lines
* inside a diff hunk: added/context lines on RIGHT, removed/context lines
* on LEFT.
* Parse a PR file's patch to determine which line numbers on each side are
* valid anchors for inline comments.
*/
export function commentableLinesForFile(patch: string | undefined): CommentableLines {
const right = new Set<number>();
@@ -83,7 +48,6 @@ export function commentableLinesForFile(patch: string | undefined): CommentableL
newLine++;
oldLine++;
}
// "\" (no newline marker) and anything else: skip, don't advance counters
}
return { RIGHT: right, LEFT: left };
}
@@ -92,38 +56,35 @@ export async function buildCommentableMap(
ctx: ToolContext,
pullNumber: number
): Promise<Map<string, CommentableLines>> {
// prefer the snapshot captured by checkout_pr — it matches the diff GitHub
// will anchor to (commit_id=checkoutSha). refetching via listFiles at review
// time gives the LATEST PR state, which can drift from what the agent
// actually reviewed if the PR was updated mid-run.
//
// only reuse the cache if it was built for THIS pull request AND for the
// sha we will anchor the review to. a second checkout_pr that bumps
// checkoutSha but fails before repopulating the cache (e.g., listFiles 5xx)
// would otherwise leave a stale snapshot keyed to the right PR number but
// the wrong sha, silently mis-validating comments.
const cached = ctx.toolState.commentableLinesByFile;
const cachedFor = ctx.toolState.commentableLinesPullNumber;
const cachedSha = ctx.toolState.commentableLinesCheckoutSha;
const currentSha = ctx.toolState.checkoutSha;
if (cached && cachedFor === pullNumber && cachedSha && cachedSha === currentSha) return cached;
const files: PullFile[] = await ctx.octokit.paginate(ctx.octokit.rest.pulls.listFiles, {
const files = (await ctx.gitea.paginate(ctx.gitea.rest.repository.repoGetPullRequestFiles, {
owner: ctx.repo.owner,
repo: ctx.repo.name,
pull_number: pullNumber,
per_page: 100,
});
index: pullNumber,
})) as ChangedFileWithPatch[];
const map = new Map<string, CommentableLines>();
for (const file of files) {
map.set(file.filename, commentableLinesForFile(file.patch));
if (file.filename) {
map.set(file.filename, commentableLinesForFile(file.patch));
}
}
return map;
}
export type ReviewCommentInput = NonNullable<
RestEndpointMethodTypes["pulls"]["createReview"]["parameters"]["comments"]
>[number];
export interface ReviewCommentInput {
path: string;
line: number;
side?: "LEFT" | "RIGHT" | undefined;
body?: string | undefined;
suggestion?: string | undefined;
start_line?: number | undefined;
}
export interface DroppedComment {
path: string;
@@ -149,124 +110,45 @@ export function validateInlineComments(
if (c.start_line != null) entry.startLine = c.start_line;
dropped.push(entry);
};
if (!lines) {
record(`file not in PR diff`);
continue;
}
if (!lines) { record(`file not in PR diff`); continue; }
if (lines.LEFT.size === 0 && lines.RIGHT.size === 0) {
// file is in the PR but has no textual patch — usually binary, a
// pure rename with no content change, or a mode-only change. GitHub
// won't accept inline comments on these regardless of line number.
record(`file has no textual diff (binary, pure rename, or mode change)`);
continue;
record(`file has no textual diff (binary, pure rename, or mode change)`); continue;
}
const anchors = lines[side];
if (!anchors.has(line)) {
record(`line ${line} (${side}) is not inside a diff hunk`);
continue;
}
// GitHub requires start_line <= line. both anchors could be valid but
// inverted (e.g. start=44, line=42) — GitHub 422s with "invalid line
// numbers". catch it here so the agent sees a precise reason.
if (!anchors.has(line)) { record(`line ${line} (${side}) is not inside a diff hunk`); continue; }
if (c.start_line != null && c.start_line > line) {
record(
`start_line ${c.start_line} is after line ${line} — ranges must satisfy start_line <= line`
);
continue;
record(`start_line ${c.start_line} is after line ${line}`); continue;
}
if (startLine !== line && !anchors.has(startLine)) {
record(`start_line ${startLine} (${side}) is not inside a diff hunk`);
continue;
record(`start_line ${startLine} (${side}) is not inside a diff hunk`); continue;
}
valid.push(c);
}
return { valid, dropped };
}
// cap the detail list so a pathological run (agent emits hundreds of invalid
// comments on a huge PR) doesn't push the review body past GitHub's ~65KB
// limit and fail the whole submission with a body-too-long 422.
export const MAX_DROPPED_COMMENT_LINES = 50;
/**
* reason a create_pull_request_review call should be skipped without hitting
* GitHub. returned by reviewSkipDecision; null means submit normally.
*/
export type ReviewSkipDecision =
| { kind: "no-issues"; reason: string }
| { kind: "empty-downgraded-approve"; reason: string };
/**
* decision returned by duplicateReviewDecision when a session has already
* submitted a review and the current call would be a duplicate.
*/
export type DuplicateReviewDecision = {
kind: "already-submitted";
reviewId: number;
reason: string;
};
export type DuplicateReviewDecision = { kind: "already-submitted"; reviewId: number; reason: string };
/**
* decide whether a second create_pull_request_review call in the same session
* is a duplicate of an earlier submission.
*
* the agent is instructed to call create_pull_request_review exactly once per
* Review-mode session (see action/modes.ts), but in practice it sometimes
* submits twice — once with substantive feedback, then again with the
* canonical "No new issues found." body when the prompt's branch logic
* re-classifies non-blocking observations. the second submission is
* always redundant: the first review is the record, and the duplicate just
* adds noise to the PR.
*
* legitimate follow-up reviews after new commits ARE allowed: the
* new-commits-mid-review path advances toolState.checkoutSha past the
* previously reviewed sha, and a subsequent checkout_pr advances it again.
* any call where checkoutSha has moved past the prior reviewedSha is a real
* follow-up and goes through. anything else — same sha, or no checkoutSha
* to compare against — is a duplicate.
*/
export function duplicateReviewDecision(params: {
existing: { id: number; reviewedSha: string | undefined } | undefined;
currentCheckoutSha: string | undefined;
}): DuplicateReviewDecision | null {
const existing = params.existing;
if (!existing) return null;
// checkoutSha advanced past the prior reviewed sha — legitimate follow-up
// (e.g. after checkout_pr re-fetched new commits the agent was nudged to
// pull). only treat as a duplicate when we cannot prove the SHA moved.
if (
params.currentCheckoutSha &&
existing.reviewedSha &&
params.currentCheckoutSha !== existing.reviewedSha
) {
return null;
}
if (params.currentCheckoutSha && existing.reviewedSha && params.currentCheckoutSha !== existing.reviewedSha) return null;
return {
kind: "already-submitted",
reviewId: existing.id,
reason: `review ${existing.id} was already submitted in this session; ignoring duplicate call (call \`checkout_pr\` again first if new commits were pushed)`,
reason: `review ${existing.id} was already submitted in this session; ignoring duplicate call`,
};
}
/**
* decide whether to skip a review submission before any network call.
*
* GitHub rejects `event: "COMMENT"` reviews with no body and no inline comments
* with HTTP 422 "Unprocessable Entity". two paths produce that shape:
*
* 1. `!approved` + empty body/comments: agent's "no issues found" result.
* skipping preserves the agent's intent (nothing to post is a fine
* outcome for a review run) without a spurious 422.
* 2. `approved` + `!prApproveEnabled` + empty body/comments: the runtime
* downgrades APPROVE to COMMENT when prApproveEnabled is off, and the
* resulting empty-COMMENT is exactly the shape GitHub 422s. skipping
* here surfaces the cause (downgrade + nothing to say) instead of an
* opaque 422 the agent can't recover from.
*
* legitimate bare approvals (`approved` + `prApproveEnabled`, no body/comments)
* are never skipped — GitHub accepts empty APPROVE reviews and the approval
* stamp itself is the review's content.
*/
export function reviewSkipDecision(params: {
approved: boolean;
body: string | null | undefined;
@@ -274,26 +156,17 @@ export function reviewSkipDecision(params: {
prApproveEnabled: boolean;
}): ReviewSkipDecision | null {
if (params.body || params.hasComments) return null;
if (!params.approved) {
return {
kind: "no-issues",
reason: "no issues found — nothing to post",
};
}
if (!params.prApproveEnabled) {
return {
kind: "empty-downgraded-approve",
reason:
"approve requested but prApproveEnabled is disabled; no feedback body or comments to post as a COMMENT review instead",
};
}
if (!params.approved) return { kind: "no-issues", reason: "no issues found — nothing to post" };
if (!params.prApproveEnabled) return {
kind: "empty-downgraded-approve",
reason: "approve requested but prApproveEnabled is disabled",
};
return null;
}
export function formatDroppedCommentsNote(dropped: DroppedComment[]): string {
const renderEntry = (d: DroppedComment): string => {
const range =
d.startLine != null && d.startLine !== d.line ? `${d.startLine}-${d.line}` : `${d.line}`;
const range = d.startLine != null && d.startLine !== d.line ? `${d.startLine}-${d.line}` : `${d.line}`;
return `- \`${d.path}:${range}\` (${d.side}) — ${d.reason}`;
};
const shown = dropped.slice(0, MAX_DROPPED_COMMENT_LINES).map(renderEntry);
@@ -306,55 +179,21 @@ export function formatDroppedCommentsNote(dropped: DroppedComment[]): string {
);
}
// one-shot review tool
export const CreatePullRequestReview = type({
pull_number: type.number.describe("The pull request number to review"),
body: type.string
.describe(
"1-2 sentence high-level summary with urgency level, critical callouts, and feedback about code outside the diff. Specific feedback on diff lines goes in 'comments' array."
)
.optional(),
approved: type.boolean
.describe(
"Set to true to submit as an approval. Use for `> ✅ No new issues found.` reviews where the PR is mergeable as-is and nothing in the body warrants code changes — approving also suppresses the Fix-button footer affordance so users don't dispatch a fix run on non-actionable feedback. Reserve approved: false for `> ℹ️ ...` (minor suggestions inline), `> [!IMPORTANT]` (recommended changes), and `> [!CAUTION]` (critical) reviews. Defaults to false (comment-only review). Rejections are not supported."
)
.optional(),
commit_id: type.string
.describe(
"Optional SHA of the commit being reviewed. Defaults to latest. Must be the FULL 40-character SHA — abbreviated SHAs are rejected by GitHub with `422 Unprocessable Entity`. The PR-synchronize event payload's `head_sha` is already full-length."
)
.optional(),
body: type.string.describe("1-2 sentence high-level summary").optional(),
approved: type.boolean.describe("Set to true to submit as an approval.").optional(),
commit_id: type.string.describe("Optional SHA of the commit being reviewed.").optional(),
comments: type({
path: type.string.describe(
"The file path to comment on (relative to repo root). Must be a file that appears in the PR diff."
),
line: type.number.describe(
"Line number to comment on. For multi-line ranges, this is the end line. Use NEW column from diff format. Must sit inside a `@@` hunk in the PR diff — anchors on context-only or untouched lines are dropped silently (the rest of the review still posts; dropped entries are reported under `droppedComments` in the response)."
),
side: type
.enumerated("LEFT", "RIGHT")
.describe(
"Side of the diff: LEFT (old code, lines starting with -) or RIGHT (new code, lines starting with + or unchanged). Defaults to RIGHT."
)
.optional(),
body: type.string
.describe("Explanatory comment text (optional if suggestion is provided)")
.optional(),
suggestion: type.string
.describe(
"Full replacement code for the line range [start_line, line]. MUST preserve the exact indentation of the original code."
)
.optional(),
start_line: type.number
.describe(
"Start line for multi-line comment ranges. Omit for single-line comments. The range [start_line, line] defines which lines a suggestion replaces. Both `start_line` and `line` must sit inside the same `@@` hunk — a `start_line` outside the hunk causes the whole comment to be dropped even when `line` is valid. If you need to comment on context just above/below a hunk, shrink the range to a single line that is provably modified."
)
.optional(),
path: type.string.describe("The file path to comment on (must appear in the PR diff)."),
line: type.number.describe("Line number to comment on (end line for multi-line ranges)."),
side: type.enumerated("LEFT", "RIGHT").describe("LEFT (old code) or RIGHT (new code). Defaults to RIGHT.").optional(),
body: type.string.describe("Explanatory comment text").optional(),
suggestion: type.string.describe("Full replacement code. MUST preserve exact indentation.").optional(),
start_line: type.number.describe("Start line for multi-line ranges.").optional(),
})
.array()
.describe(
"Inline comments on lines within diff hunks. Feedback about code outside the diff goes in 'body' instead."
)
.describe("Inline comments anchored to diff hunk lines.")
.optional(),
});
@@ -363,47 +202,26 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) {
name: "create_pull_request_review",
description:
"Submit a review for an existing pull request. " +
'Example: `create_pull_request_review({ pull_number: 1234, body: "LGTM", approved: true, comments: [{ path: "src/api.ts", line: 42, body: "nit: rename" }] })`. ' +
"Each call creates a permanent, visible review on the PR — NEVER submit test or diagnostic reviews. " +
"Reviews with no body AND no comments are silently skipped (nothing to post). " +
"IMPORTANT: 95%+ of feedback should be in 'comments' array with file paths and line numbers. " +
"Only use 'body' for a 1-2 sentence summary with urgency and critical callouts. " +
"Use 'suggestion' to propose replacement code - MUST preserve exact indentation of original code. " +
"The first submission may error once with a one-time diff-coverage nudge listing unread TOC regions — retry with the same arguments and the pre-flight will not block again. " +
"Example replacing lines 42-44 (3 lines) with 5 lines: " +
`{ path: 'src/api.ts', start_line: 42, line: 44, suggestion: ' const result = await fetch(url);\\n if (!result.ok) {\\n log.error(result.status);\\n throw new Error("request failed");\\n }' }` +
" CONSTRAINT: Inline comments can ONLY target files and lines that appear in the PR diff." +
" Comments anchored outside a diff hunk are dropped automatically (with a note appended to the review body) — the rest of the review still posts.",
"Reviews with no body AND no comments are silently skipped. " +
"IMPORTANT: 95%+ of feedback should be in 'comments' with file paths and line numbers. " +
"Only use 'body' for a 1-2 sentence summary. Use 'suggestion' to propose replacement code. " +
"The first submission may error once with a diff-coverage nudge — retry with the same arguments. " +
"Inline comments can ONLY target files and lines that appear in the PR diff.",
parameters: CreatePullRequestReview,
execute: execute(async ({ pull_number, body, approved, commit_id, comments = [] }) => {
if (body) body = fixDoubleEscapedString(body);
// set issue context (PRs are issues)
ctx.toolState.issueNumber = pull_number;
// guard against duplicate review submissions in the same session.
// see duplicateReviewDecision for the rationale — short version: the
// agent occasionally submits twice (substantive review + canonical
// "no issues found" follow-up) and the second is always redundant.
// legit re-reviews after new commits are still allowed because
// checkout_pr advances toolState.checkoutSha past the prior reviewedSha.
const dup = duplicateReviewDecision({
existing: ctx.toolState.review,
currentCheckoutSha: ctx.toolState.checkoutSha,
});
if (dup) {
log.info(`skipping duplicate review submission: ${dup.reason}`);
return {
success: true,
skipped: true,
reason: dup.reason,
reviewId: dup.reviewId,
};
log.info(`skipping duplicate review: ${dup.reason}`);
return { success: true, skipped: true, reason: dup.reason, reviewId: dup.reviewId };
}
// skip empty COMMENT reviews before any GitHub call. see reviewSkipDecision
// for the cases (no-issues vs empty-downgraded-approve) and why GitHub 422s
// the shape we'd otherwise POST.
const skip = reviewSkipDecision({
approved: approved ?? false,
body,
@@ -411,242 +229,128 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) {
prApproveEnabled: ctx.prApproveEnabled,
});
if (skip) {
log.info(`skipping review submission: ${skip.reason}`);
log.info(`skipping review: ${skip.reason}`);
return { success: true, skipped: true, reason: skip.reason };
}
// enforce prApproveEnabled: downgrade APPROVE to COMMENT if disabled.
// by this point we already returned if the downgrade would produce an
// empty COMMENT (the skip above), so every downgrade that reaches here
// carries either a body or inline comments.
let event: "APPROVE" | "COMMENT" = approved ? "APPROVE" : "COMMENT";
if (event === "APPROVE" && !ctx.prApproveEnabled) {
log.info("prApproveEnabled is disabled — downgrading APPROVE to COMMENT");
// SDK event names: "APPROVED" | "COMMENT" | "REQUEST_CHANGES"
let event: "APPROVED" | "COMMENT" = approved ? "APPROVED" : "COMMENT";
if (event === "APPROVED" && !ctx.prApproveEnabled) {
log.info("prApproveEnabled is disabled — downgrading APPROVED to COMMENT");
event = "COMMENT";
}
const params: RestEndpointMethodTypes["pulls"]["createReview"]["parameters"] = {
owner: ctx.repo.owner,
repo: ctx.repo.name,
pull_number,
event,
};
let latestHeadSha: string | undefined;
if (commit_id) {
params.commit_id = commit_id;
} else {
const pr = await ctx.octokit.rest.pulls.get({
owner: ctx.repo.owner,
repo: ctx.repo.name,
pull_number,
});
latestHeadSha = pr.data.head.sha;
// anchor to checkout sha so line numbers match the diff the agent analyzed
params.commit_id = ctx.toolState.checkoutSha ?? latestHeadSha;
if (ctx.toolState.checkoutSha && latestHeadSha !== ctx.toolState.checkoutSha) {
log.info(
`anchoring review to checkout ${ctx.toolState.checkoutSha.slice(0, 7)} ` +
`(HEAD is now ${latestHeadSha.slice(0, 7)})`
);
let effectiveCommitId = commit_id;
if (!effectiveCommitId) {
try {
const pr = await ctx.gitea.rest.repository.repoGetPullRequest({
owner: ctx.repo.owner,
repo: ctx.repo.name,
index: pull_number,
});
latestHeadSha = pr.data.head?.sha;
effectiveCommitId = ctx.toolState.checkoutSha ?? latestHeadSha;
} catch {
effectiveCommitId = ctx.toolState.checkoutSha;
}
}
runDiffCoveragePreflight({ ctx });
type ReviewComment = NonNullable<typeof params.comments>[number];
const reviewComments = comments.map((comment) => {
let commentBody = fixDoubleEscapedString(comment.body || "");
if (comment.suggestion !== undefined) {
const suggestionBlock = "```suggestion\n" + comment.suggestion + "\n```";
commentBody = commentBody ? commentBody + "\n\n" + suggestionBlock : suggestionBlock;
// Build review comments
const reviewComments: ReviewCommentInput[] = comments.map((c) => {
let commentBody = fixDoubleEscapedString(c.body || "");
if (c.suggestion !== undefined) {
const block = "```suggestion\n" + c.suggestion + "\n```";
commentBody = commentBody ? `${commentBody}\n\n${block}` : block;
}
const side = comment.side || "RIGHT";
const reviewComment: ReviewComment = {
path: comment.path,
line: comment.line,
body: commentBody,
side,
};
if (comment.start_line != null && comment.start_line !== comment.line) {
reviewComment.start_line = comment.start_line;
reviewComment.start_side = side;
}
return reviewComment;
return { path: c.path, line: c.line, side: c.side || "RIGHT", body: commentBody, start_line: c.start_line };
});
// pre-validate inline comments against the current PR diff. drop any
// comment that does not anchor to a line inside a hunk, rather than
// letting GitHub 422 and sink the whole review.
let droppedComments: DroppedComment[] = [];
let validComments: ReviewCommentInput[] = [];
if (reviewComments.length > 0) {
const commentableMap = await buildCommentableMap(ctx, pull_number);
const validation = validateInlineComments(reviewComments, commentableMap);
droppedComments = validation.dropped;
validComments = validation.valid;
if (droppedComments.length > 0) {
log.info(
`dropping ${droppedComments.length}/${reviewComments.length} inline comment(s) that do not anchor to PR diff lines`
);
log.info(`dropping ${droppedComments.length}/${reviewComments.length} invalid inline comments`);
}
// always reassign so all-dropped reviews leave params.comments empty
// instead of carrying the original invalid set (which would 422).
params.comments = validation.valid;
}
// if we dropped comments, surface them in the review body so the
// author (and the agent, on retry) can see what was skipped.
if (droppedComments.length > 0) {
const note = formatDroppedCommentsNote(droppedComments);
body = body ? body + note : note.replace(/^\n\n/, "");
}
// after dropping, an empty non-approve review has nothing left to post.
if (!approved && !body && !params.comments?.length) {
log.info("review has no body and all inline comments were dropped — skipping submission");
return {
success: true,
skipped: true,
reason: "all inline comments were invalid — nothing to post",
droppedComments,
};
if (!approved && !body && !validComments.length) {
log.info("review has no body and all inline comments were dropped — skipping");
return { success: true, skipped: true, reason: "all inline comments were invalid", droppedComments };
}
// no body → single-step createReview (no footer needed)
// has body → pending + submit so we can build footer with Fix links using review ID
//
// wrap the submission in `retry` so GitHub's transient 422 "internal
// error" body (distinct from anchor / body-length / suggestion 422s,
// which all cite the specific cause) clears on its own instead of
// surfacing through the generic 422 handler — that framing sent the
// agent dropping valid inline comments chasing a non-issue.
// `shouldRetry` scopes retries to the transient body only, so real
// validation 422s still fail fast.
let result;
try {
result = await retry(
() =>
body
? createAndSubmitWithFooter(ctx, params, {
body,
approved: approved ?? false,
hasComments: (params.comments?.length ?? 0) > 0,
})
: createReviewWithStrandedRecovery(ctx, params),
{
delaysMs: TRANSIENT_REVIEW_RETRY_DELAYS_MS,
shouldRetry: isTransientReviewError,
label: "review submission",
}
);
} catch (err: unknown) {
// GitHub's transient 422 "internal error" is distinct from anchor /
// body-length / suggestion validation failures — framing it with the
// generic "likely causes (1)(2)(3)" prompt sends the agent dropping
// comments that were never the problem. after bounded in-tool retry
// we surface a dedicated message that tells the agent to wait-and-
// retry or fall back to a body-only review.
if (isTransientReviewError(err)) {
const rawMsg = err instanceof Error ? err.message : String(err);
throw new Error(
`GitHub returned a transient 422 "internal error" on the reviews endpoint after ${TRANSIENT_REVIEW_RETRY_DELAYS_MS.length + 1} attempts. ` +
`This is a GitHub-side issue, not a problem with your review content. ` +
`Do NOT modify or drop inline comments — their content is not the cause. ` +
`Wait ~30 seconds and call this tool once more with the SAME arguments. ` +
`If it still fails, submit a body-only review (move all inline feedback into \`body\` as text) so nothing is lost. ` +
`GitHub said: ${rawMsg}`,
{ cause: err }
);
// Convert to SDK's CreatePullReviewComment format
const sdkComments = validComments.map((c) => ({
path: c.path,
body: c.body ?? "",
new_position: c.side !== "LEFT" ? c.line : undefined,
old_position: c.side === "LEFT" ? c.line : undefined,
}));
const footer = buildShockbotFooter({ model: ctx.toolState.model });
const fullBody = body ? `${body}${footer}` : footer.trimStart();
const result = await retry<Awaited<ReturnType<typeof ctx.gitea.rest.repository.repoCreatePullReview>>>(
() =>
ctx.gitea.rest.repository.repoCreatePullReview({
owner: ctx.repo.owner,
repo: ctx.repo.name,
index: pull_number,
body: {
body: fullBody,
commit_id: effectiveCommitId,
event,
comments: sdkComments,
},
}),
{
delaysMs: [1_000, 3_000],
shouldRetry: (err) => /internal error|500|503/i.test(err instanceof Error ? err.message : String(err)),
label: "review submission",
}
if (getHttpStatus(err) !== 422 || !params.comments?.length) throw err;
);
const details = params.comments.map((c) => {
const line = c.line ?? 0;
const startLine = c.start_line ?? line;
const range = startLine !== line ? `${startLine}-${line}` : `${line}`;
return `${c.path}:${range} (${c.side ?? "RIGHT"})`;
});
// a 422 on createReview-with-comments is USUALLY about comment
// anchors, but could also be about body length, invalid suggestion
// blocks, etc. include the verbatim GitHub error so the agent can
// diagnose non-anchor 422s without us having to enumerate every
// possible GitHub validation rule.
const rawMsg = err instanceof Error ? err.message : String(err);
const checkoutRef = formatMcpToolRef(ctx.agentId, "checkout_pr");
throw new Error(
`GitHub rejected the review with 422 even after pre-validation. ` +
`Likely causes (check "GitHub said" below to narrow down): ` +
`(1) new commits pushed after pre-validation — call \`${checkoutRef}\` again to refresh the diff snapshot, then resubmit; ` +
`(2) the review body exceeded GitHub's ~65KB limit — shorten it and retry; ` +
`(3) a \`suggestion\` block is malformed (missing backticks, extra backticks, or wrong indentation) — inspect the affected comments below. ` +
`If none apply, move the failing comments into the review body as text so the rest still posts. ` +
`Affected comments: ${details.join(", ")}. ` +
`GitHub said: ${rawMsg}`,
{ cause: err }
);
}
log.debug(`createReview response: ${JSON.stringify(result.data)}`);
if (!result.data.id) {
throw new Error(`createReview returned invalid data: ${JSON.stringify(result.data)}`);
}
const reviewId = result.data.id;
const reviewNodeId = result.data.node_id;
const reviewId = result.data.id!;
log.info(`» created review ${reviewId} on pull request #${pull_number}`);
// reviewedSha = what the agent actually reviewed (checkout SHA), not the
// submission anchor (current HEAD). this ensures postReviewCleanup dispatches
// a follow-up if the agent doesn't handle new commits inline.
const actuallyReviewedSha = ctx.toolState.checkoutSha ?? params.commit_id;
ctx.toolState.review = {
id: reviewId,
nodeId: reviewNodeId,
reviewedSha: actuallyReviewedSha,
nodeId: String(reviewId),
reviewedSha: ctx.toolState.checkoutSha ?? effectiveCommitId,
};
ctx.toolState.wasUpdated = true;
// a submitted review obsoletes the progress comment — the review IS the
// durable artifact. owned here (not in main.ts) so cleanup is atomic with
// submission and survives any path out of the run (success, timeout,
// crash). deleteProgressComment sets progressComment = null, so a later
// report_progress call short-circuits to a no-op.
// best-effort: a cleanup failure must not turn a successful review into
// a tool-call failure visible to the agent.
await deleteProgressComment(ctx).catch((err) => {
log.debug(`progress comment cleanup after review failed: ${err}`);
});
// detect commits pushed since checkout and guide the agent to review them
// inline instead of dispatching a separate workflow run
if (
ctx.toolState.checkoutSha &&
latestHeadSha &&
latestHeadSha !== ctx.toolState.checkoutSha
) {
if (ctx.toolState.checkoutSha && latestHeadSha && latestHeadSha !== ctx.toolState.checkoutSha) {
const fromSha = ctx.toolState.checkoutSha;
const toSha = latestHeadSha;
// store old checkoutSha as beforeSha so the next checkout_pr computes an incremental diff
ctx.toolState.beforeSha = fromSha;
// advance checkoutSha so the next review submission tracks correctly (just in case, checkout_pr will overwrite it again)
ctx.toolState.checkoutSha = toSha;
log.info(
`new commits detected during review: ${fromSha.slice(0, 7)}..${toSha.slice(0, 7)}`
);
log.info(`new commits detected during review: ${fromSha.slice(0, 7)}..${toSha.slice(0, 7)}`);
return {
success: true,
reviewId,
html_url: result.data.html_url,
state: result.data.state,
user: result.data.user?.login,
submitted_at: result.data.submitted_at,
droppedComments: droppedComments.length > 0 ? droppedComments : undefined,
newCommits: {
from: fromSha,
to: toSha,
instructions:
`new commits were pushed while you were reviewing. ` +
`call \`${formatMcpToolRef(ctx.agentId, "checkout_pr")}\` again to fetch the latest version — it will compute the incremental diff automatically. ` +
`submit another review covering only the new changes. do not repeat feedback from your previous review.`,
instructions: `new commits were pushed while you were reviewing. call \`${formatMcpToolRef(ctx.agentId, "checkout_pr")}\` again to fetch the latest version and submit another review covering only the new changes.`,
},
};
}
@@ -656,8 +360,6 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) {
reviewId,
html_url: result.data.html_url,
state: result.data.state,
user: result.data.user?.login,
submitted_at: result.data.submitted_at,
droppedComments: droppedComments.length > 0 ? droppedComments : undefined,
};
}),
@@ -666,254 +368,34 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) {
function runDiffCoveragePreflight(params: { ctx: ToolContext }): void {
const coverageState = params.ctx.toolState.diffCoverage;
if (!coverageState) {
log.debug("diff coverage pre-flight skipped: no diffCoverage state present in toolState");
return;
}
if (coverageState.coveragePreflightRan) {
log.debug("diff coverage pre-flight skipped: already ran in this session");
if (!coverageState || coverageState.coveragePreflightRan) {
log.debug("diff coverage pre-flight skipped");
return;
}
coverageState.coveragePreflightRan = true;
log.debug(
`diff coverage pre-flight start: diffPath=${coverageState.diffPath}, totalLines=${coverageState.totalLines}, tocEntries=${coverageState.tocEntries.length}, coveredRanges=${coverageState.coveredRanges.length}`
);
const breakdown = getDiffCoverageBreakdown({ state: coverageState });
const unread: Array<{ path: string; ranges: string; unreadLines: number }> = [];
let unreadLines = 0;
for (const file of breakdown.files) {
if (file.unreadRanges.length === 0) continue;
const rangesText = file.unreadRanges
.map((range) => `${range.startLine}-${range.endLine}`)
.join(", ");
const rangesText = file.unreadRanges.map((r) => `${r.startLine}-${r.endLine}`).join(", ");
const fileUnreadLines = countLinesInRanges({ ranges: file.unreadRanges });
unread.push({ path: file.filename, ranges: rangesText, unreadLines: fileUnreadLines });
unreadLines += fileUnreadLines;
}
coverageState.lastBreakdown = renderDiffCoverageBreakdown({
diffPath: coverageState.diffPath,
breakdown,
});
log.debug(
`diff coverage pre-flight breakdown: coveredLines=${breakdown.coveredLines}, unreadLines=${unreadLines}`
);
coverageState.lastBreakdown = renderDiffCoverageBreakdown({ diffPath: coverageState.diffPath, breakdown });
if (unreadLines === 0) {
log.debug("diff coverage pre-flight passed: no unread regions");
return;
}
if (unreadLines === 0) return;
log.info(
`diff coverage pre-flight nudge: unread lines=${unreadLines}, unread files=${unread.length}`
);
const unreadText = unread
.map((entry) => `- ${entry.path} (${entry.unreadLines} lines, ${entry.ranges})`)
.join("\n");
log.info(`diff coverage pre-flight nudge: unread lines=${unreadLines}, files=${unread.length}`);
const unreadText = unread.map((e) => `- ${e.path} (${e.unreadLines} lines, ${e.ranges})`).join("\n");
throw new Error(
`diff coverage pre-flight: some TOC regions were not read before review submission. ` +
`this is a one-time nudge — read the ranges below from ${coverageState.diffPath} on a best-effort basis, then call create_pull_request_review again. ` +
`you are NOT obligated to read generated artifacts (lockfiles like pnpm-lock.yaml / package-lock.json / yarn.lock / Cargo.lock; codegen output like *.gen.*, *.pb.go, *.generated.*; snapshot/fixture dirs like __snapshots__/; migration metadata like drizzle/meta/, prisma migration SQL). ` +
`if every unread region is generated, retry immediately without reading. ` +
`this pre-flight will not block again in this review session.\n\n` +
`you are NOT obligated to read generated artifacts (lockfiles, codegen output, snapshot dirs). ` +
`this pre-flight will not block again this session.\n\n` +
`unread TOC regions:\n${unreadText}\n\n` +
`${coverageState.lastBreakdown}`
);
}
type FooterOpts = { body: string; approved: boolean; hasComments: boolean };
/**
* clear a pending review draft stranded on the PR by a prior hard-killed run
* (workflow timeout, OOM) so the next createReview can succeed.
*
* GitHub enforces one-pending-review-per-user-per-PR. if the previous process
* died between createReview(PENDING) and submitReview, the draft remains and
* the next run's createReview 422s with "already has a pending review".
* listReviews only exposes PENDING reviews to their author, so filtering on
* state === "PENDING" is already scoped to the authed token's own draft.
*
* if `originalErr` is not a pending-review 422, or no leftover is found, this
* function rethrows `originalErr` so the caller surfaces the original failure.
* delete failures with 404 (draft already gone) or 422 (draft submitted by a
* concurrent caller) are swallowed — the caller's retry will succeed in both
* cases. any other delete error is rethrown unchanged.
*
* known limitation: if two runs on the SAME PR share the authed token and
* overlap in time, the loser's createReview 422s on the winner's still-active
* draft. recovery would then delete the winner's active draft and the
* winner's submitReview would 404. this is not distinguishable from a
* genuinely-stranded draft via the review object alone (PENDING reviews
* expose no created_at timestamp, and both reviews are authored by the same
* bot user). rely on workflow-level concurrency controls (e.g. a concurrency
* key keyed to the PR number) to prevent overlap.
*/
export async function clearStrandedPendingReview(
ctx: ToolContext,
params: { owner: string; repo: string; pull_number: number; originalErr: unknown }
): Promise<void> {
const originalErr = params.originalErr;
const msg = originalErr instanceof Error ? originalErr.message.toLowerCase() : "";
if (getHttpStatus(originalErr) !== 422 || !msg.includes("pending review")) throw originalErr;
// if listReviews itself fails (5xx, rate limit, etc), surface the ORIGINAL
// 422 rather than the listing failure — "pending review conflict" is the
// real blocker the caller needs to see. hiding it behind a transient 502
// sent agents chasing phantom server errors instead of retrying the
// conflict. log the listing failure for diagnosis but do not mask.
const reviews = await ctx.octokit
.paginate(ctx.octokit.rest.pulls.listReviews, {
owner: params.owner,
repo: params.repo,
pull_number: params.pull_number,
per_page: 100,
})
.catch((listErr: unknown) => {
// surface at info so operators not running at debug still see that
// recovery was attempted (and why) before the original 422 bubbles up.
log.info(
`» listReviews failed during pending-review cleanup, surfacing original 422: ${listErr instanceof Error ? listErr.message : String(listErr)}`
);
throw originalErr;
});
const leftover = reviews.find((r) => r.state === "PENDING");
if (!leftover?.id) throw originalErr;
log.info(
`» clearing leftover pending review ${leftover.id} (likely stranded by a killed prior run)`
);
try {
await ctx.octokit.rest.pulls.deletePendingReview({
owner: params.owner,
repo: params.repo,
pull_number: params.pull_number,
review_id: leftover.id,
});
} catch (cleanupErr) {
const cleanupStatus = getHttpStatus(cleanupErr);
if (cleanupStatus !== 404 && cleanupStatus !== 422) throw cleanupErr;
log.debug(`» delete of leftover pending ${leftover.id} no-op (status ${cleanupStatus})`);
}
}
/**
* single-step createReview (event != PENDING) with stranded-draft recovery.
* the body path goes through createAndSubmitWithFooter which already recovers
* from a stranded PENDING draft at its own createReview call. the no-body path
* used to call createReview directly with no recovery — so a PR whose previous
* body-path run crashed between createReview(PENDING) and submitReview would
* permanently 422 any subsequent no-body review (approve-with-no-feedback or
* comments-only) until a body-path run happened to clear the draft.
*/
export async function createReviewWithStrandedRecovery(
ctx: ToolContext,
params: RestEndpointMethodTypes["pulls"]["createReview"]["parameters"]
): Promise<Awaited<ReturnType<typeof ctx.octokit.rest.pulls.createReview>>> {
try {
return await ctx.octokit.rest.pulls.createReview(params);
} catch (err) {
await clearStrandedPendingReview(ctx, {
owner: params.owner,
repo: params.repo,
pull_number: params.pull_number,
originalErr: err,
});
return await ctx.octokit.rest.pulls.createReview(params);
}
}
async function createAndSubmitWithFooter(
ctx: ToolContext,
params: RestEndpointMethodTypes["pulls"]["createReview"]["parameters"],
opts: FooterOpts
) {
// create as PENDING (strip event) so we get the review ID before publishing
const { event: _, ...pendingParams } = params;
let pending: Awaited<ReturnType<typeof ctx.octokit.rest.pulls.createReview>>;
try {
pending = await ctx.octokit.rest.pulls.createReview(pendingParams);
} catch (err) {
await clearStrandedPendingReview(ctx, {
owner: params.owner,
repo: params.repo,
pull_number: params.pull_number,
originalErr: err,
});
pending = await ctx.octokit.rest.pulls.createReview(pendingParams);
}
if (!pending.data.id) {
throw new Error(`createReview returned invalid data: ${JSON.stringify(pending.data)}`);
}
// once the pending draft exists, GitHub only allows one pending review per
// user per PR — so ANY failure between here and successful submit must
// clean up, not just a submitReview throw. getApiUrl() can throw if
// API_URL is misconfigured, and future footer-building changes could
// introduce new throw paths. keep the whole body wrapped.
try {
// Fix buttons are suppressed on approving reviews — those are mergeable
// by definition (the `> ✅ No new issues found.` tier, with no inline
// comments), so dispatching a fix run would be a UX trap.
const customParts: string[] = [];
if (!opts.approved) {
const apiUrl = getApiUrl();
if (opts.hasComments) {
const fixAllUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${params.pull_number}?action=fix&review_id=${pending.data.id}`;
const fixApprovedUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${params.pull_number}?action=fix-approved&review_id=${pending.data.id}`;
customParts.push(`[Fix all ➔](${fixAllUrl})`, `[Fix 👍s ➔](${fixApprovedUrl})`);
} else {
const fixUrl = `${apiUrl}/trigger/${ctx.repo.owner}/${ctx.repo.name}/${params.pull_number}?action=fix&review_id=${pending.data.id}`;
customParts.push(`[Fix it ➔](${fixUrl})`);
}
}
const footer = buildPullfrogFooter({
workflowRun: ctx.runId
? { owner: ctx.repo.owner, repo: ctx.repo.name, runId: ctx.runId, jobId: ctx.jobId }
: undefined,
customParts,
model: ctx.toolState.model,
fallbackFrom: ctx.toolState.modelFallback?.from,
});
return await ctx.octokit.rest.pulls.submitReview({
owner: params.owner,
repo: params.repo,
pull_number: params.pull_number,
review_id: pending.data.id,
event: params.event!,
body: opts.body + footer,
});
} catch (err) {
// anything failed after the pending draft was created. leaving the draft
// on the PR would cause the agent's retry to fail with "already has a
// pending review" (GitHub's one-pending-per-user-per-PR limit). best-effort
// cleanup so retries start from a clean slate. the cleanup itself may
// 404/422 (review already submitted by a concurrent caller, or the PR
// was closed mid-flight) — log and swallow those so the original error
// isn't masked.
try {
await ctx.octokit.rest.pulls.deletePendingReview({
owner: params.owner,
repo: params.repo,
pull_number: params.pull_number,
review_id: pending.data.id,
});
log.debug(`» deleted leftover pending review ${pending.data.id} after failure`);
} catch (cleanupErr) {
log.debug(
`» failed to delete pending review ${pending.data.id}: ${cleanupErr instanceof Error ? cleanupErr.message : String(cleanupErr)}`
);
}
throw err;
}
}
/**
* report the review node ID so the WorkflowRun is marked as "review submitted".
* exported for use in main.ts post-agent cleanup.
*/
export async function reportReviewNodeId(
ctx: ToolContext,
params: { nodeId: string }
): Promise<void> {
await patchWorkflowRunFields(ctx, { reviewNodeId: params.nodeId });
}
mcp/reviewComments.test.ts
-40
View File
@@ -1,40 +0,0 @@
import { readFileSync } from "node:fs";
import { resolve } from "node:path";
import { describe, expect, it } from "vitest";
import { type FormatReviewDataInput, formatReviewData } from "./reviewComments.ts";
// fixtures captured by action/scripts/refresh-test-fixtures.ts; re-run
// (with creds) when GitHub's review/threads/listFiles response shape
// changes, then review the snapshot diff.
type ReviewFixture = FormatReviewDataInput & {
owner: string;
name: string;
};
function loadFixture(file: string): ReviewFixture {
return JSON.parse(
readFileSync(resolve(import.meta.dirname, "__fixtures__", file), "utf-8")
) as ReviewFixture;
}
describe("formatReviewData", () => {
it("formats thread blocks with TOC and correct line numbers", () => {
const fx = loadFixture("pullfrog-scratch-pr-49-review-3485940013.json");
const result = formatReviewData(fx);
expect(result).toBeDefined();
if (!result) return;
expect(result.formatted.toc).toMatchSnapshot("toc");
expect(result.formatted.content).toMatchSnapshot("content");
});
it("formats body-only review", () => {
const fx = loadFixture("pullfrog-scratch-pr-64-review-3531000326.json");
const result = formatReviewData(fx);
expect(result).toBeDefined();
if (!result) return;
expect(result.formatted.toc).toMatchSnapshot("toc");
expect(result.formatted.content).toMatchSnapshot("content");
});
});
mcp/reviewComments.ts
+61 -720
View File
@@ -1,770 +1,111 @@
import { writeFileSync } from "node:fs";
import { join } from "node:path";
import type { Octokit } from "@octokit/rest";
import { type } from "arktype";
import { stripExistingFooter } from "../utils/buildPullfrogFooter.ts";
import type { PullReviewComment } from "@go-gitea/sdk.js";
import { stripExistingFooter } from "../utils/buildShockbotFooter.ts";
import { log } from "../utils/log.ts";
import type { ToolContext } from "./server.ts";
import { execute, tool } from "./shared.ts";
// GraphQL query to fetch all review threads for a PR with full comment history
export const REVIEW_THREADS_QUERY = `
query ($owner: String!, $name: String!, $prNumber: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $prNumber) {
reviewThreads(first: 100) {
nodes {
id
path
line
startLine
diffSide
isResolved
isOutdated
comments(first: 50) {
nodes {
fullDatabaseId
body
createdAt
diffHunk
line
startLine
originalLine
originalStartLine
author { login }
pullRequestReview {
databaseId
author { login }
}
reactionGroups {
content
reactors(first: 10) {
nodes {
... on Actor { login }
}
}
}
}
}
}
}
}
}
}
`;
export type ReviewThreadComment = {
fullDatabaseId: string | null;
body: string;
createdAt: string;
diffHunk: string;
line: number | null;
startLine: number | null;
originalLine: number | null;
originalStartLine: number | null;
author: { login: string } | null;
pullRequestReview: {
databaseId: number | null;
author: { login: string } | null;
} | null;
reactionGroups: Array<{
content: string;
reactors: { nodes: Array<{ login: string } | null> | null } | null;
}> | null;
};
export type ReviewThread = {
id: string;
path: string;
line: number | null;
startLine: number | null;
diffSide: "LEFT" | "RIGHT";
isResolved: boolean;
isOutdated: boolean;
comments: {
nodes: (ReviewThreadComment | null)[] | null;
} | null;
};
export type ReviewThreadsQueryResponse = {
repository: {
pullRequest: {
reviewThreads: {
nodes: (ReviewThread | null)[] | null;
} | null;
} | null;
} | null;
};
export function countLines(str: string): number {
let count = 1;
let index = -1;
// biome-ignore lint/suspicious/noAssignInExpressions: assignment in while condition is intentional for indexOf loop pattern
while ((index = str.indexOf("\n", index + 1)) !== -1) {
count++;
}
return count;
}
// extract exactly the commented line range from diffHunk, plus context
const CONTEXT_PADDING = 3;
function extractCommentedLines(
diffHunk: string,
startLine: number | null,
endLine: number | null,
side: "LEFT" | "RIGHT"
): string {
const lines = diffHunk.split("\n");
if (lines.length <= 1) return diffHunk;
const header = lines[0];
const contentLines = lines.slice(1);
// parse header: @@ -old_start,old_count +new_start,new_count @@
const headerMatch = header.match(/@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@/);
if (!headerMatch) return diffHunk;
const hunkOldStart = parseInt(headerMatch[1], 10);
const hunkNewStart = parseInt(headerMatch[2], 10);
// LEFT = old file (deletions), RIGHT = new file (additions)
const hunkStart = side === "LEFT" ? hunkOldStart : hunkNewStart;
const commentStart = startLine ?? endLine ?? hunkStart;
const commentEnd = endLine ?? commentStart;
// walk through diff lines, tracking line numbers for both old and new files
// - lines: old file only (LEFT)
// + lines: new file only (RIGHT)
// context lines: both files
type DiffLine = { text: string; lineNum: number | null };
const diffLines: DiffLine[] = [];
let oldLineNum = hunkOldStart;
let newLineNum = hunkNewStart;
for (const line of contentLines) {
const prefix = line[0];
if (prefix === "-") {
// deletion - only has old line number
diffLines.push({ text: line, lineNum: side === "LEFT" ? oldLineNum : null });
oldLineNum++;
} else if (prefix === "+") {
// addition - only has new line number
diffLines.push({ text: line, lineNum: side === "RIGHT" ? newLineNum : null });
newLineNum++;
} else {
// context - has both line numbers
diffLines.push({ text: line, lineNum: side === "LEFT" ? oldLineNum : newLineNum });
oldLineNum++;
newLineNum++;
}
}
// find lines for comment range with context
const targetStart = commentStart - CONTEXT_PADDING;
const targetEnd = commentEnd;
const result: string[] = [];
let truncatedBefore = 0;
for (let i = 0; i < diffLines.length; i++) {
const dl = diffLines[i];
// include if: within target range, OR it's an "other side" line adjacent to included lines
const inRange = dl.lineNum !== null && dl.lineNum >= targetStart && dl.lineNum <= targetEnd;
// include opposite-side lines if they're between included lines
const adjacentOtherSide = dl.lineNum === null && result.length > 0 && i < diffLines.length - 1;
if (inRange || adjacentOtherSide) {
result.push(dl.text);
} else if (result.length === 0) {
truncatedBefore++;
}
}
if (truncatedBefore > 0) {
return `${header}\n... (${truncatedBefore} lines above) ...\n${result.join("\n")}`;
}
return `${header}\n${result.join("\n")}`;
}
// parsed hunk from a unified diff
export type ParsedHunk = {
header: string;
oldStart: number;
oldCount: number;
newStart: number;
newCount: number;
content: string[];
};
// parse a full file patch into individual hunks
export function parseFilePatches(patch: string): ParsedHunk[] {
const hunks: ParsedHunk[] = [];
const lines = patch.split("\n");
let currentHunk: ParsedHunk | null = null;
for (const line of lines) {
const hunkMatch = line.match(/@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/);
if (hunkMatch) {
if (currentHunk) hunks.push(currentHunk);
currentHunk = {
header: line,
oldStart: parseInt(hunkMatch[1], 10),
oldCount: parseInt(hunkMatch[2] ?? "1", 10),
newStart: parseInt(hunkMatch[3], 10),
newCount: parseInt(hunkMatch[4] ?? "1", 10),
content: [],
};
} else if (currentHunk) {
currentHunk.content.push(line);
}
}
if (currentHunk) hunks.push(currentHunk);
return hunks;
}
// find hunks that overlap with a line range (for LEFT or RIGHT side)
function findOverlappingHunks(
hunks: ParsedHunk[],
startLine: number,
endLine: number,
side: "LEFT" | "RIGHT"
): ParsedHunk[] {
return hunks.filter((hunk) => {
const hunkStart = side === "LEFT" ? hunk.oldStart : hunk.newStart;
const hunkCount = side === "LEFT" ? hunk.oldCount : hunk.newCount;
const hunkEnd = hunkStart + hunkCount - 1;
// check for overlap: ranges overlap if start1 <= end2 && start2 <= end1
return startLine <= hunkEnd && hunkStart <= endLine;
});
}
// extract diff content from multiple hunks for a comment range
function extractFromFilePatches(
hunks: ParsedHunk[],
startLine: number,
endLine: number,
side: "LEFT" | "RIGHT"
): string {
const overlapping = findOverlappingHunks(hunks, startLine, endLine, side);
if (overlapping.length === 0) {
return `(no diff hunks found for lines ${startLine}-${endLine})`;
}
if (overlapping.length === 1) {
// single hunk - use existing extraction logic
const hunk = overlapping[0];
const fullHunk = hunk.header + "\n" + hunk.content.join("\n");
return extractCommentedLines(fullHunk, startLine, endLine, side);
}
// multiple hunks - combine them with gap indicators
const result: string[] = [];
let prevHunkEnd = 0;
for (let i = 0; i < overlapping.length; i++) {
const hunk = overlapping[i];
const hunkStart = side === "LEFT" ? hunk.oldStart : hunk.newStart;
const hunkCount = side === "LEFT" ? hunk.oldCount : hunk.newCount;
const hunkEnd = hunkStart + hunkCount - 1;
// add gap indicator if there's a gap between hunks
if (i > 0 && hunkStart > prevHunkEnd + 1) {
const gapSize = hunkStart - prevHunkEnd - 1;
result.push(`\n... (${gapSize} unchanged lines) ...\n`);
}
// add the hunk header and content
result.push(hunk.header);
result.push(...hunk.content);
prevHunkEnd = hunkEnd;
}
return result.join("\n");
}
export const GetReviewComments = type({
pull_number: type.number.describe("The pull request number"),
review_id: type.number.describe("The review ID to get comments for"),
pull_number: type.number.describe("The pull request number to get review comments for"),
});
function hasThumbsUpFrom(comment: ReviewThreadComment, username: string): boolean {
if (!comment.reactionGroups) return false;
const thumbsUp = comment.reactionGroups.find((g) => g.content === "THUMBS_UP");
if (!thumbsUp?.reactors?.nodes) return false;
const needle = username.toLowerCase();
return thumbsUp.reactors.nodes.some((r) => r?.login?.toLowerCase() === needle);
}
function threadHasThumbsUpFrom(thread: ReviewThread, username: string): boolean {
const comments = thread.comments?.nodes ?? [];
return comments.some((c) => c && hasThumbsUpFrom(c, username));
}
/**
* formats thread blocks into markdown with TOC and line numbers.
* extracted for testability.
*/
export function formatReviewThreads(
threadBlocks: Array<{ path: string; lineRange: string; content: string[] }>,
header: { pullNumber: number; reviewId: number; reviewer: string; reviewBody?: string }
) {
// header section takes: title (1) + blank (1) + "## TOC" (1) + blank (1) + N TOC entries + blank (1) + "---" (1) + blank (1)
const tocHeaderLines = 4;
const tocFooterLines = 3;
let currentLine = tocHeaderLines + threadBlocks.length + tocFooterLines + 1;
// account for review body section if present
const reviewBodyLines: string[] = [];
if (header.reviewBody) {
reviewBodyLines.push("## Review Body", "", header.reviewBody, "");
currentLine += reviewBodyLines.reduce((sum, line) => sum + countLines(line), 0);
}
const tocEntries: string[] = [];
const threadLines: string[] = [];
for (const block of threadBlocks) {
const startLine = currentLine;
const actualLineCount = block.content.reduce((sum, line) => sum + countLines(line), 0);
const endLine = currentLine + actualLineCount - 1;
tocEntries.push(`- ${block.path}:${block.lineRange} → lines ${startLine}-${endLine}`);
threadLines.push(...block.content);
currentLine += actualLineCount;
}
const lines: string[] = [];
lines.push(
`# Review Threads (${threadBlocks.length}) for PR #${header.pullNumber} - Review ${header.reviewId} by ${header.reviewer}`
);
lines.push("");
if (threadBlocks.length > 0) {
lines.push("## TOC");
lines.push("");
lines.push(...tocEntries);
lines.push("");
}
lines.push(...reviewBodyLines);
lines.push("---");
lines.push("");
lines.push(...threadLines);
return {
toc: tocEntries.join("\n"),
content: lines.join("\n"),
};
}
/**
* builds thread blocks from review threads and file patches.
* extracted for testability.
*/
export function buildThreadBlocks(
threads: ReviewThread[],
filePatchMap: Map<string, ParsedHunk[]>,
reviewId: number
) {
// sort threads by file path, then by line number
threads.sort((a, b) => {
const pathCmp = a.path.localeCompare(b.path);
if (pathCmp !== 0) return pathCmp;
const aLine = a.startLine ?? a.line ?? 0;
const bLine = b.startLine ?? b.line ?? 0;
return aLine - bLine;
});
const threadBlocks: Array<{ path: string; lineRange: string; content: string[] }> = [];
for (const thread of threads) {
const allComments = (thread.comments?.nodes ?? []).filter(
(c): c is ReviewThreadComment => c !== null
);
if (allComments.length === 0) continue;
// get line info from thread, or fall back to first comment's line info
const firstComment = allComments[0];
const line =
thread.line ?? firstComment?.line ?? firstComment?.originalLine ?? thread.startLine ?? 0;
const startLine =
thread.startLine ?? firstComment?.startLine ?? firstComment?.originalStartLine ?? line;
const lineRange = startLine === line ? `${line}` : `${startLine}-${line}`;
const block: string[] = [];
// header with file:line range and status
const status = thread.isResolved ? " [RESOLVED]" : thread.isOutdated ? " [OUTDATED]" : "";
block.push(`## ${thread.path}:${lineRange}${status}`);
block.push("");
// show all comments in the thread (full conversation history)
for (const comment of allComments) {
const author = comment.author?.login ?? "unknown";
const isTargetReview = comment.pullRequestReview?.databaseId === reviewId;
const marker = isTargetReview ? " *" : "";
block.push(
`\`\`\`\`comment author=${author} id=${comment.fullDatabaseId ?? "unknown"} review=${comment.pullRequestReview?.databaseId ?? "unknown"} thread=${thread.id}${marker}`
);
block.push(comment.body || "(no comment body)");
block.push("````");
block.push("");
}
// diff context
const fileHunks = filePatchMap.get(thread.path);
const firstCommentWithHunk = allComments.find((c) => c.diffHunk);
let diffContent: string | null = null;
if (fileHunks && fileHunks.length > 0) {
const overlapping = findOverlappingHunks(fileHunks, startLine, line, thread.diffSide);
if (overlapping.length > 0) {
diffContent = extractFromFilePatches(fileHunks, startLine, line, thread.diffSide);
}
}
if (!diffContent && firstCommentWithHunk) {
diffContent = extractCommentedLines(
firstCommentWithHunk.diffHunk,
startLine,
line,
thread.diffSide
);
}
if (diffContent) {
block.push(`\`\`\`diff file=${thread.path} lines=${lineRange} side=${thread.diffSide}`);
block.push(diffContent);
block.push("```");
block.push("");
} else {
block.push(`\`\`\`diff file=${thread.path} lines=${lineRange} side=${thread.diffSide}`);
block.push(`(no diff context available - comment on unchanged lines)`);
block.push("```");
block.push("");
}
threadBlocks.push({ path: thread.path, lineRange, content: block });
}
return threadBlocks;
}
async function getReviewThreads(input: GetReviewDataInput) {
const response = await input.octokit.graphql<ReviewThreadsQueryResponse>(REVIEW_THREADS_QUERY, {
owner: input.owner,
name: input.name,
prNumber: input.pullNumber,
});
const allThreads = response.repository?.pullRequest?.reviewThreads?.nodes ?? [];
if (allThreads.length >= 100) {
log.warning(
`PR ${input.owner}/${input.name}#${input.pullNumber}: reviewThreads returned 100 results (limit reached, some threads may be missing)`
);
}
for (const thread of allThreads) {
if (thread?.comments?.nodes && thread.comments.nodes.length >= 50) {
log.warning(
`PR ${input.owner}/${input.name}#${input.pullNumber}: review thread at ${thread.path}:${thread.line} has 50 comments (limit reached, some comments may be missing)`
);
}
}
const threadsForReview = allThreads.filter((thread): thread is ReviewThread => {
if (!thread?.comments?.nodes) return false;
return thread.comments.nodes.some((c) => c?.pullRequestReview?.databaseId === input.reviewId);
});
if (!input.approvedBy) {
return threadsForReview;
}
const username = input.approvedBy;
return threadsForReview.filter((thread) => threadHasThumbsUpFrom(thread, username));
}
interface GetReviewDataInput {
octokit: Octokit;
owner: string;
name: string;
pullNumber: number;
reviewId: number;
approvedBy?: string | undefined;
}
// pure formatter: takes already-fetched GitHub responses and produces the
// review data the MCP tool returns. extracted from getReviewData so tests
// can drive it from checked-in fixtures without live API access.
//
// `prFiles` may be empty when `threads` is empty — callers that hit the
// network should skip the listFiles call in that case as a perf
// optimization. when both are empty and `review.body` is also empty, the
// formatter returns undefined just like getReviewData.
export interface FormatReviewDataInput {
review: ReviewResponse;
threads: ReviewThread[];
prFiles: ReviewPrFile[];
pullNumber: number;
reviewId: number;
}
export type ReviewResponse = {
body: string | null | undefined;
user: { login: string } | null | undefined;
};
export type ReviewPrFile = {
filename: string;
patch?: string | undefined;
};
export function formatReviewData(input: FormatReviewDataInput):
| {
threadBlocks: Array<{ path: string; lineRange: string; content: string[] }>;
reviewer: string;
formatted: { toc: string; content: string };
}
| undefined {
const rawReviewBody = input.review.body;
const reviewBody = rawReviewBody ? stripExistingFooter(rawReviewBody) : "";
const reviewer = input.review.user?.login ?? "unknown";
if (input.threads.length === 0 && !reviewBody) return undefined;
let threadBlocks: Array<{ path: string; lineRange: string; content: string[] }> = [];
if (input.threads.length > 0) {
const filePatchMap = new Map<string, ParsedHunk[]>();
for (const file of input.prFiles) {
if (file.patch) {
filePatchMap.set(file.filename, parseFilePatches(file.patch));
}
}
threadBlocks = buildThreadBlocks(input.threads, filePatchMap, input.reviewId);
}
const formatted = formatReviewThreads(threadBlocks, {
pullNumber: input.pullNumber,
reviewId: input.reviewId,
reviewer,
reviewBody,
});
return { threadBlocks, reviewer, formatted };
}
export async function getReviewData(input: GetReviewDataInput): Promise<
| {
threadBlocks: Array<{ path: string; lineRange: string; content: string[] }>;
reviewer: string;
formatted: { toc: string; content: string };
}
| undefined
> {
const [review, threads] = await Promise.all([
input.octokit.rest.pulls.getReview({
owner: input.owner,
repo: input.name,
pull_number: input.pullNumber,
review_id: input.reviewId,
}),
getReviewThreads(input),
]);
// skip listFiles when there are no threads — prFiles is only used for
// building thread blocks, and an empty array short-circuits below.
const prFiles =
threads.length > 0
? await input.octokit.paginate(input.octokit.rest.pulls.listFiles, {
owner: input.owner,
repo: input.name,
pull_number: input.pullNumber,
per_page: 100,
})
: [];
return formatReviewData({
review: review.data,
threads,
prFiles,
pullNumber: input.pullNumber,
reviewId: input.reviewId,
});
}
export function GetReviewCommentsTool(ctx: ToolContext) {
return tool({
name: "get_review_comments",
description:
"Get review comments for a pull request review with full thread context. " +
"Example: `get_review_comments({ pull_number: 1234, review_id: 567890 })`. " +
"Automatically filters to approved comments when applicable. " +
"Returns a TOC and commentsPath pointing to a markdown file with full comment details.",
"Get all inline review comments for a pull request. " +
"Example: `get_review_comments({ pull_number: 1234 })`.",
parameters: GetReviewComments,
execute: execute(async (params) => {
// auto-filter to approved comments when the event has approved_only set
const approvedBy =
ctx.payload.event.trigger === "fix_review" && ctx.payload.event.approved_only
? ctx.payload.triggerer
: undefined;
execute: execute(async ({ pull_number }) => {
ctx.toolState.issueNumber = pull_number;
const result = await getReviewData({
octokit: ctx.octokit,
// Get all reviews, then collect their comments
const reviews = await ctx.gitea.paginate(ctx.gitea.rest.repository.repoListPullReviews, {
owner: ctx.repo.owner,
name: ctx.repo.name,
pullNumber: params.pull_number,
reviewId: params.review_id,
approvedBy,
repo: ctx.repo.name,
index: pull_number,
});
if (!result) {
return {
review_id: params.review_id,
pull_number: params.pull_number,
reviewer: "unknown",
threadCount: 0,
commentsPath: null,
toc: null,
instructions: approvedBy
? `no threads with 👍 from ${approvedBy}`
: "no threads found for this review",
};
const allComments: PullReviewComment[] = [];
for (const review of reviews) {
if (!review.id) continue;
try {
const commentsResult = await ctx.gitea.rest.repository.repoGetPullReviewComments({
owner: ctx.repo.owner,
repo: ctx.repo.name,
index: pull_number,
id: review.id,
});
allComments.push(...commentsResult.data);
} catch {
// best-effort
}
}
const { threadBlocks, reviewer, formatted } = result;
const tempDir = process.env.SHOCKBOT_TEMP_DIR;
let filePath: string | undefined;
const tempDir = process.env.PULLFROG_TEMP_DIR;
if (!tempDir) {
throw new Error("PULLFROG_TEMP_DIR not set");
const rendered = allComments.map((c) => ({
id: c.id,
path: c.path,
// PullReviewComment uses `position` for the diff offset; Gitea doesn't
// expose separate new_position/old_position on GET — use original_position as fallback
line: c.original_position ?? c.position,
side: "RIGHT" as const,
body: stripExistingFooter(c.body ?? ""),
author: c.user?.login,
diffHunk: c.diff_hunk,
reviewId: c.pull_request_review_id,
}));
if (tempDir && rendered.length > 0) {
filePath = join(tempDir, `pr-${pull_number}-review-comments.json`);
writeFileSync(filePath, JSON.stringify(rendered, null, 2));
log.debug(`wrote review comments to ${filePath}`);
}
const filename = `review-${params.review_id}-threads.md`;
const commentsPath = join(tempDir, filename);
writeFileSync(commentsPath, formatted.content);
log.debug(`wrote ${threadBlocks.length} threads to ${commentsPath}`);
return {
review_id: params.review_id,
pull_number: params.pull_number,
reviewer,
threadCount: threadBlocks.length,
commentsPath,
toc: formatted.toc,
instructions:
`the file at commentsPath contains ${threadBlocks.length} review threads with full conversation history. ` +
`comments marked with * are from the target review (${params.review_id}). ` +
`the TOC shows each thread's file:line and the line number where it appears in the file. ` +
`to read a specific thread, use: grep -A 50 "^## <file:line>" ${commentsPath} ` +
`(replace <file:line> with the path from the TOC, e.g. "^## action/utils/foo.ts:42"). ` +
`address each thread in order, working through one file at a time.`,
pull_number,
comments: rendered,
count: rendered.length,
...(filePath ? { filePath } : {}),
};
}),
});
}
export const ListPullRequestReviews = type({
pull_number: type.number.describe("The pull request number to list reviews for"),
pull_number: type.number.describe("The pull request number"),
});
export function ListPullRequestReviewsTool(ctx: ToolContext) {
return tool({
name: "list_pull_request_reviews",
description:
"List all reviews for a pull request. Returns all reviews including approvals, request changes, and comments. " +
"List all reviews submitted on a pull request. " +
"Example: `list_pull_request_reviews({ pull_number: 1234 })`.",
parameters: ListPullRequestReviews,
execute: execute(async (params) => {
const reviews = await ctx.octokit.paginate(ctx.octokit.rest.pulls.listReviews, {
execute: execute(async ({ pull_number }) => {
const reviews = await ctx.gitea.paginate(ctx.gitea.rest.repository.repoListPullReviews, {
owner: ctx.repo.owner,
repo: ctx.repo.name,
pull_number: params.pull_number,
index: pull_number,
});
return {
pull_number: params.pull_number,
reviews: reviews.map((review) => ({
id: review.id,
node_id: review.node_id,
body: review.body,
state: review.state,
user: review.user?.login,
submitted_at: review.submitted_at,
commit_id: review.commit_id,
html_url: review.html_url,
pull_number,
reviews: reviews.map((r) => ({
id: r.id,
user: r.user?.login,
state: r.state,
body: r.body,
submitted_at: r.submitted_at,
commit_id: r.commit_id,
})),
count: reviews.length,
};
}),
});
}
const RESOLVE_REVIEW_THREAD_MUTATION = `
mutation($threadId: ID!) {
resolveReviewThread(input: {threadId: $threadId}) {
thread {
id
isResolved
}
}
}
`;
export const ResolveReviewThread = type({
thread_id: type.string.describe("The GraphQL node ID of the review thread to resolve"),
});
export function ResolveReviewThreadTool(ctx: ToolContext) {
return tool({
name: "resolve_review_thread",
description:
"Mark a review thread as resolved using GitHub's GraphQL API. " +
"Only call this after addressing the review feedback, implementing fixes, testing them, and posting a reply. " +
"Do not resolve threads that are already resolved, threads where no action was taken, or threads where you disagree with the feedback.",
parameters: ResolveReviewThread,
execute: execute(async (params) => {
try {
const response = await ctx.octokit.graphql<{
resolveReviewThread: {
thread: {
id: string;
isResolved: boolean;
};
};
}>(RESOLVE_REVIEW_THREAD_MUTATION, {
threadId: params.thread_id,
});
const thread = response.resolveReviewThread.thread;
log.info(`» resolved review thread ${thread.id}`);
return {
thread_id: thread.id,
is_resolved: thread.isResolved,
success: true,
message: "Thread resolved successfully",
};
} catch (error) {
// handle common error cases gracefully
const errorMessage = error instanceof Error ? error.message : String(error);
const isResolved =
errorMessage.includes("already resolved") || errorMessage.includes("isResolved");
const message = isResolved
? `thread ${params.thread_id} was already resolved`
: `failed to resolve thread ${params.thread_id}: ${errorMessage}`;
log.info(message);
return {
thread_id: params.thread_id,
is_resolved: isResolved,
success: isResolved,
message,
};
}
}),
});
}
mcp/security.test.ts
-720
View File
@@ -1,720 +0,0 @@
import { describe, expect, it } from "vitest";
import { checkoutPrBranch, type PrData } from "./checkout.ts";
import {
AUTH_REQUIRED_REDIRECT,
DeleteBranchTool,
NOSHELL_BLOCKED_ARGS,
NOSHELL_BLOCKED_SUBCOMMANDS,
rejectIfLeadingDash,
rejectSpecialRef,
validateTagName,
} from "./git.ts";
import type { ToolContext } from "./server.ts";
// ─── git tool security tests ────────────────────────────────────────────
//
// the validation function below mirrors the logic in GitTool.execute, but
// imports the AUTH/NOSHELL tables directly from git.ts so tests don't silently
// drift if the runtime messages are edited. if the *algorithm* in git.ts
// changes, validateGitCommand needs to be updated here too.
type ShellPermission = "disabled" | "restricted" | "enabled";
type ValidateGitParams = {
command: string;
args: string[];
shellPermission: ShellPermission;
};
// matches the arkregex pattern used in the Git schema
const SUBCOMMAND_PATTERN = /^[a-z][a-z0-9-]*$/;
// mirrors the validation logic in GitTool.execute
function validateGitCommand(params: ValidateGitParams): string | null {
// schema-level regex validation — applies in ALL modes
if (!SUBCOMMAND_PATTERN.test(params.command)) {
return `command must be Git subcommand (was "${params.command}")`;
}
const redirect = AUTH_REQUIRED_REDIRECT[params.command];
if (redirect) {
return `git ${params.command} requires authentication. ${redirect}`;
}
// subcommand and arg blocking only applies when shell is disabled
if (params.shellPermission === "disabled") {
const blocked = NOSHELL_BLOCKED_SUBCOMMANDS[params.command];
if (blocked) {
return blocked;
}
for (const arg of params.args) {
const isBlocked = NOSHELL_BLOCKED_ARGS.some(
(flag) => arg === flag || arg.startsWith(flag + "=")
);
if (isBlocked) {
return `Blocked: '${arg}' flag can execute arbitrary code and is not allowed.`;
}
}
}
return null; // no error
}
describe("git tool security - subcommand regex validation", () => {
it("blocks -c flag as subcommand in ALL modes (alias injection)", () => {
const modes: ShellPermission[] = ["disabled", "restricted", "enabled"];
for (const mode of modes) {
const error = validateGitCommand({
command: "-c",
args: ["alias.x=!evil-command", "x"],
shellPermission: mode,
});
expect(error).toContain("Git subcommand");
}
});
it("blocks --exec-path as subcommand", () => {
const error = validateGitCommand({
command: "--exec-path=/malicious",
args: ["status"],
shellPermission: "disabled",
});
expect(error).toContain("Git subcommand");
});
it("blocks -C as subcommand (change directory)", () => {
const error = validateGitCommand({
command: "-C",
args: ["/tmp", "init"],
shellPermission: "disabled",
});
expect(error).toContain("Git subcommand");
});
it("blocks --config-env as subcommand", () => {
const error = validateGitCommand({
command: "--config-env",
args: ["core.pager=PATH", "log"],
shellPermission: "disabled",
});
expect(error).toContain("Git subcommand");
});
it("blocks all flags starting with - as subcommand", () => {
const flags = ["-c", "-C", "-p", "--paginate", "--git-dir", "--work-tree", "--bare"];
for (const flag of flags) {
const error = validateGitCommand({
command: flag,
args: [],
shellPermission: "disabled",
});
expect(error).toContain("Git subcommand");
}
});
it("blocks uppercase subcommands", () => {
const error = validateGitCommand({
command: "STATUS",
args: [],
shellPermission: "disabled",
});
expect(error).toContain("Git subcommand");
});
it("blocks subcommands with special characters", () => {
const bad = ["git;evil", "status$(cmd)", "log|cat", "diff&bg"];
for (const sub of bad) {
const error = validateGitCommand({
command: sub,
args: [],
shellPermission: "disabled",
});
expect(error).toContain("Git subcommand");
}
});
it("allows valid subcommands", () => {
const safe = ["status", "log", "diff", "show", "branch", "tag", "stash", "blame"];
for (const sub of safe) {
const error = validateGitCommand({
command: sub,
args: [],
shellPermission: "disabled",
});
expect(error).toBeNull();
}
});
it("allows hyphenated subcommands", () => {
const safe = ["filter-branch", "update-index", "ls-remote", "ls-files", "rev-parse"];
for (const sub of safe) {
const error = validateGitCommand({
command: sub,
args: [],
shellPermission: "enabled",
});
expect(error).toBeNull();
}
});
});
describe("git tool security - blocked subcommands (disabled mode only)", () => {
it("blocks config in disabled mode", () => {
const error = validateGitCommand({
command: "config",
args: ["core.hooksPath", "./hooks"],
shellPermission: "disabled",
});
expect(error).toContain("git config");
});
it("allows config in restricted mode (agent has shell)", () => {
const error = validateGitCommand({
command: "config",
args: ["filter.evil.clean", "bash -c 'evil'"],
shellPermission: "restricted",
});
expect(error).toBeNull();
});
it("blocks submodule in disabled mode", () => {
const error = validateGitCommand({
command: "submodule",
args: ["add", "https://evil.com/repo.git"],
shellPermission: "disabled",
});
expect(error).toContain("submodule");
});
it("allows submodule in restricted mode", () => {
const error = validateGitCommand({
command: "submodule",
args: ["add", "https://example.com/repo.git"],
shellPermission: "restricted",
});
expect(error).toBeNull();
});
it("blocks rebase in disabled mode", () => {
const error = validateGitCommand({
command: "rebase",
args: ["--exec", "evil-command", "HEAD~1"],
shellPermission: "disabled",
});
expect(error).toContain("rebase");
});
it("allows rebase in restricted mode", () => {
const error = validateGitCommand({
command: "rebase",
args: ["main"],
shellPermission: "restricted",
});
expect(error).toBeNull();
});
it("blocks bisect in disabled mode", () => {
const error = validateGitCommand({
command: "bisect",
args: ["run", "evil-command"],
shellPermission: "disabled",
});
expect(error).toContain("bisect");
});
it("blocks filter-branch in disabled mode", () => {
const error = validateGitCommand({
command: "filter-branch",
args: ["--tree-filter", "evil-command", "HEAD"],
shellPermission: "disabled",
});
expect(error).toContain("filter-branch");
});
// regression: NOSHELL_BLOCKED_ARGS matches only the long `--extcmd` /
// `--extcmd=...` forms. `git difftool -x <cmd>` is the short form and
// slipped through — verified executing a canary via
// `yes | git difftool -x 'echo PWN' HEAD~1 HEAD` on a real repo.
// globally blocking `-x` would false-positive on `git cherry-pick -x`
// (a metadata-appending flag, not code exec), so difftool is blocked
// at the subcommand level instead.
it("blocks difftool in disabled mode (closes -x short-form bypass)", () => {
const error = validateGitCommand({
command: "difftool",
args: ["-x", "evil-command", "HEAD~1", "HEAD"],
shellPermission: "disabled",
});
expect(error).toContain("difftool");
});
it("blocks difftool even with --extcmd long form (subcommand-level stops it first)", () => {
const error = validateGitCommand({
command: "difftool",
args: ["--extcmd=evil-command", "HEAD"],
shellPermission: "disabled",
});
expect(error).toContain("difftool");
});
it("blocks mergetool in disabled mode (configured tool commands execute code)", () => {
const error = validateGitCommand({
command: "mergetool",
args: [],
shellPermission: "disabled",
});
expect(error).toContain("mergetool");
});
it("allows blocked subcommands in enabled mode", () => {
const blocked = [
"config",
"submodule",
"rebase",
"bisect",
"filter-branch",
"difftool",
"mergetool",
];
for (const sub of blocked) {
const error = validateGitCommand({
command: sub,
args: [],
shellPermission: "enabled",
});
expect(error).toBeNull();
}
});
it("allows blocked subcommands in restricted mode (stripped env is security boundary)", () => {
const blocked = [
"config",
"submodule",
"rebase",
"bisect",
"filter-branch",
"difftool",
"mergetool",
];
for (const sub of blocked) {
const error = validateGitCommand({
command: sub,
args: [],
shellPermission: "restricted",
});
expect(error).toBeNull();
}
});
});
describe("git tool security - blocked arg flags (disabled mode only)", () => {
it("blocks --exec in args (disabled)", () => {
const error = validateGitCommand({
command: "log",
args: ["--exec", "evil-command"],
shellPermission: "disabled",
});
expect(error).toContain("arbitrary code");
});
it("blocks --exec= in args (disabled)", () => {
const error = validateGitCommand({
command: "log",
args: ["--exec=evil-command"],
shellPermission: "disabled",
});
expect(error).toContain("arbitrary code");
});
it("blocks --extcmd in args (disabled) — on a subcommand that isn't blocked at the subcommand level", () => {
// difftool itself is now blocked at the subcommand level (closes the `-x`
// short-form bypass), so the arg-level check never runs for difftool in
// disabled mode. use `log --extcmd=...` to exercise the arg-level code
// path: `log` isn't in NOSHELL_BLOCKED_SUBCOMMANDS, so validation falls
// through to the arg scan and the --extcmd block triggers.
const error = validateGitCommand({
command: "log",
args: ["--extcmd=evil-command", "HEAD~1"],
shellPermission: "disabled",
});
expect(error).toContain("arbitrary code");
});
it("blocks --upload-pack in args (disabled)", () => {
const error = validateGitCommand({
command: "ls-remote",
args: ["--upload-pack=evil"],
shellPermission: "disabled",
});
expect(error).toContain("arbitrary code");
});
it("allows --exec in restricted mode (agent has shell)", () => {
const error = validateGitCommand({
command: "rebase",
args: ["--exec", "npm test", "HEAD~1"],
shellPermission: "restricted",
});
expect(error).toBeNull();
});
it("allows --extcmd in restricted mode", () => {
const error = validateGitCommand({
command: "difftool",
args: ["--extcmd=less"],
shellPermission: "restricted",
});
expect(error).toBeNull();
});
it("allows blocked args in enabled mode", () => {
const error = validateGitCommand({
command: "difftool",
args: ["--extcmd=less"],
shellPermission: "enabled",
});
expect(error).toBeNull();
});
it("allows normal args in disabled mode", () => {
const error = validateGitCommand({
command: "log",
args: ["--oneline", "-10", "--format=%H %s"],
shellPermission: "disabled",
});
expect(error).toBeNull();
});
it("does not false-positive on --exclude-standard (not --exec)", () => {
const error = validateGitCommand({
command: "ls-files",
args: ["--exclude-standard"],
shellPermission: "disabled",
});
expect(error).toBeNull();
});
it("does not false-positive on --execute (not --exec=)", () => {
const error = validateGitCommand({
command: "log",
args: ["--execute-something"],
shellPermission: "disabled",
});
expect(error).toBeNull();
});
it("does not false-positive on -c (combined diff format for git log)", () => {
const error = validateGitCommand({
command: "log",
args: ["-c", "--oneline"],
shellPermission: "disabled",
});
expect(error).toBeNull();
});
});
describe("git tool security - auth redirect", () => {
it("redirects push in all modes", () => {
const modes: ShellPermission[] = ["disabled", "restricted", "enabled"];
for (const mode of modes) {
const error = validateGitCommand({
command: "push",
args: [],
shellPermission: mode,
});
expect(error).toContain("authentication");
}
});
it("redirects fetch", () => {
const error = validateGitCommand({
command: "fetch",
args: [],
shellPermission: "enabled",
});
expect(error).toContain("authentication");
});
it("redirects pull", () => {
const error = validateGitCommand({
command: "pull",
args: [],
shellPermission: "enabled",
});
expect(error).toContain("authentication");
});
it("pull redirect recommends merge (not rebase) regardless of shell mode", () => {
// F5 regression: the redirect previously suggested "or 'rebase' unless
// shell is disabled", which was misleading noise under shell=disabled
// (rebase is blocked by NOSHELL_BLOCKED_SUBCOMMANDS there) and redundant
// under other modes (agents can invoke rebase directly if they want).
// the current redirect names only merge — the one alternative that
// works in every shell mode.
for (const mode of ["disabled", "restricted", "enabled"] as ShellPermission[]) {
const error = validateGitCommand({
command: "pull",
args: [],
shellPermission: mode,
});
expect(error).toContain("merge");
expect(error).not.toMatch(/rebase/i);
}
});
it("redirects clone", () => {
const error = validateGitCommand({
command: "clone",
args: [],
shellPermission: "enabled",
});
expect(error).toContain("authentication");
});
});
// ─── dependency install security tests ──────────────────────────────────
// mirrors the logic in dependencies.ts startInstallation()
function shouldIgnoreScripts(shellPermission: ShellPermission): boolean {
return shellPermission === "disabled";
}
describe("git tool security - rejectIfLeadingDash", () => {
it("rejects refs starting with --", () => {
expect(() => rejectIfLeadingDash("--upload-pack=evil", "ref")).toThrow(
/Blocked: ref '--upload-pack=evil' starts with '-'/
);
});
it("rejects refs starting with a single -", () => {
expect(() => rejectIfLeadingDash("-c", "ref")).toThrow(/starts with '-'/);
});
it("allows normal branch names", () => {
expect(() => rejectIfLeadingDash("main", "ref")).not.toThrow();
expect(() => rejectIfLeadingDash("feature/foo", "ref")).not.toThrow();
expect(() => rejectIfLeadingDash("pull/123/head", "ref")).not.toThrow();
expect(() => rejectIfLeadingDash("release-1.2", "ref")).not.toThrow();
});
it("allows branch names containing dashes (not leading)", () => {
expect(() => rejectIfLeadingDash("feat-x", "branchName")).not.toThrow();
});
it("customizes the kind label in the error", () => {
expect(() => rejectIfLeadingDash("-evil", "branchName")).toThrow(/branchName '-evil'/);
});
});
describe("git tool security - rejectSpecialRef (default-branch bypass)", () => {
// an agent in restricted mode normally can't push to the default branch —
// PushBranchTool compares the resolved remoteBranch against defaultBranch
// and blocks the match. before this guard, passing `branchName:
// "refs/heads/main"` bypassed the check (the exact-string compare fails
// because "refs/heads/main" !== "main") while git still pushed to main.
it("rejects fully-qualified refs/heads/... branch names", () => {
expect(() => rejectSpecialRef("refs/heads/main", "branch")).toThrow(/fully-qualified ref path/);
expect(() => rejectSpecialRef("refs/heads/feature/foo", "branch")).toThrow(
/fully-qualified ref path/
);
});
it("rejects refs/tags/... and refs/remotes/... forms too", () => {
// push_branch only pushes branches, so every refs/-prefixed form is
// illegitimate here — no need to whitelist refs/heads/ alone.
expect(() => rejectSpecialRef("refs/tags/v1", "branch")).toThrow(/fully-qualified ref path/);
expect(() => rejectSpecialRef("refs/remotes/origin/main", "branch")).toThrow(
/fully-qualified ref path/
);
});
it("rejects symbolic refs that resolve to arbitrary commits", () => {
// `git push origin HEAD` and friends pick up whatever commit those refs
// point at — not what the agent named, and not constrained by the
// default-branch guard either.
for (const ref of ["HEAD", "FETCH_HEAD", "ORIG_HEAD", "MERGE_HEAD"]) {
expect(() => rejectSpecialRef(ref, "branch")).toThrow(/symbolic ref/);
}
});
it("still rejects leading-dash (inherits rejectIfLeadingDash)", () => {
expect(() => rejectSpecialRef("-evil", "branch")).toThrow(/starts with '-'/);
});
it("allows bare branch names including ones with slashes", () => {
for (const b of ["main", "pr-123", "feature/foo", "release/v2", "user/name/topic"]) {
expect(() => rejectSpecialRef(b, "branch")).not.toThrow();
}
});
// refspec syntax: git push accepts `[+]src[:dst]`. without these checks an
// agent under push:restricted smuggles a full refspec through branchName,
// and the downstream exact-string default-branch guard misses because the
// value isn't literally "main". these are the exact attacks the new
// rejection closes.
it("rejects ':' (refspec src:dst split that targets main)", () => {
expect(() => rejectSpecialRef("evil:refs/heads/main", "branch")).toThrow(
/refspec\/revision syntax/
);
});
it("rejects leading ':' (delete-ref refspec deletes remote main)", () => {
expect(() => rejectSpecialRef(":refs/heads/main", "branch")).toThrow(
/refspec\/revision syntax/
);
});
it("rejects leading '+' (force-push refspec prefix)", () => {
expect(() => rejectSpecialRef("+main", "branch")).toThrow(/refspec\/revision syntax/);
});
it("rejects '~' and '^' (revision modifiers that resolve to parents)", () => {
expect(() => rejectSpecialRef("main~1", "branch")).toThrow(/refspec\/revision syntax/);
expect(() => rejectSpecialRef("main^", "branch")).toThrow(/refspec\/revision syntax/);
});
it("rejects whitespace (not permitted in git branch names)", () => {
expect(() => rejectSpecialRef("main other", "branch")).toThrow(/refspec\/revision syntax/);
expect(() => rejectSpecialRef("foo\tbar", "branch")).toThrow(/refspec\/revision syntax/);
});
it("rejects shell/glob metacharacters forbidden in branch names", () => {
for (const b of ["main?", "main*", "main[", "main\\x"]) {
expect(() => rejectSpecialRef(b, "branch")).toThrow(/refspec\/revision syntax/);
}
});
});
describe("git tool security - validateTagName (push_tags refspec injection)", () => {
it("rejects tags containing ':' (refspec src:dst split)", () => {
// without this, "foo:refs/heads/main" would push the local refs/tags/foo's
// commit to remote main and bypass the push_branch default-branch guard.
expect(() => validateTagName("foo:refs/heads/main")).toThrow(/could be parsed as a refspec/);
expect(() => validateTagName("v1.0:bar")).toThrow(/refspec/);
});
it("rejects tags with leading '-' (flag injection)", () => {
expect(() => validateTagName("-c")).toThrow(/starts with '-'/);
expect(() => validateTagName("--upload-pack=evil")).toThrow(/starts with '-'/);
});
it("rejects tags with whitespace or control chars", () => {
expect(() => validateTagName("foo bar")).toThrow(/could be parsed/);
expect(() => validateTagName("foo\nrefs/heads/main")).toThrow(/could be parsed/);
});
it("rejects tags with shell / refspec metacharacters", () => {
const bad = ["foo~1", "foo^", "foo?", "foo*", "foo[", "foo\\bar", "foo;evil"];
for (const t of bad) {
expect(() => validateTagName(t)).toThrow(/could be parsed/);
}
});
it("allows plausible tag names", () => {
const ok = ["v1.0.0", "release-2024-01", "feature/thing", "v1", "hotfix_1"];
for (const t of ok) {
expect(() => validateTagName(t)).not.toThrow();
}
});
it("rejects empty tag", () => {
expect(() => validateTagName("")).toThrow(/could be parsed/);
});
});
describe("DeleteBranchTool - default-branch guard", () => {
// push: enabled authorizes pushes — not wholesale removal of the repo's
// primary branch. GitHub branch protection usually blocks this at the
// remote, but not every repo has protection on, so guard locally too.
function makeCtx(defaultBranch: string): ToolContext {
return {
payload: { push: "enabled" },
repo: { data: { default_branch: defaultBranch } },
gitToken: "test-token",
} as unknown as ToolContext;
}
it("blocks deletion of the default branch even with push: enabled", async () => {
const tool = DeleteBranchTool(makeCtx("main"));
const result = (await (tool.execute as (p: unknown, ctx: unknown) => Promise<unknown>)(
{ branchName: "main" },
{} as Parameters<NonNullable<typeof tool.execute>>[1]
)) as { content: [{ text: string }]; isError?: boolean };
/* cast: FastMCP execute returns a union of content shapes; these tests
always return the handleToolError envelope, which matches this shape. */
expect(result.isError).toBe(true);
expect(result.content[0].text).toMatch(/default branch/i);
});
it("honors the repo's actual default branch name (not just 'main')", async () => {
const tool = DeleteBranchTool(makeCtx("trunk"));
const result = (await (tool.execute as (p: unknown, ctx: unknown) => Promise<unknown>)(
{ branchName: "trunk" },
{} as Parameters<NonNullable<typeof tool.execute>>[1]
)) as { content: [{ text: string }]; isError?: boolean };
/* cast: FastMCP execute returns a union of content shapes; these tests
always return the handleToolError envelope, which matches this shape. */
expect(result.isError).toBe(true);
expect(result.content[0].text).toMatch(/default branch 'trunk'/);
});
it("still blocks when the agent tries the refs/heads/... bypass", async () => {
// rejectSpecialRef catches this before the default-branch check, but the
// test asserts the chain stops it — either error is acceptable, just not
// a successful delete.
const tool = DeleteBranchTool(makeCtx("main"));
const result = (await (tool.execute as (p: unknown, ctx: unknown) => Promise<unknown>)(
{ branchName: "refs/heads/main" },
{} as Parameters<NonNullable<typeof tool.execute>>[1]
)) as { content: [{ text: string }]; isError?: boolean };
/* cast: FastMCP execute returns a union of content shapes; these tests
always return the handleToolError envelope, which matches this shape. */
expect(result.isError).toBe(true);
});
});
describe("git tool security - checkoutPrBranch rejects malicious PR refs", () => {
// PR head/base ref names are attacker-controlled on forks (PR author picks
// headRef freely, and baseRef could be a maliciously-named branch on the
// target repo). they flow into `git fetch origin <ref>` and similar, so a
// ref starting with '-' would be parsed as a flag, not a refspec.
// checkoutPrBranch validates them up-front with rejectIfLeadingDash.
const basePr: PrData = {
number: 1,
headSha: "a".repeat(40),
headRef: "feature",
headRepoFullName: "user/repo",
baseRef: "main",
baseRepoFullName: "user/repo",
maintainerCanModify: false,
};
// checkoutPrBranch validates before any async call, so the params never get
// dereferenced — a cast is enough to satisfy the type checker.
const dummyParams = {} as Parameters<typeof checkoutPrBranch>[1];
it("rejects a leading-dash headRef before any git call", async () => {
await expect(
checkoutPrBranch({ ...basePr, headRef: "-upload-pack=evil" }, dummyParams)
).rejects.toThrow(/PR head ref.*starts with '-'/);
});
it("rejects a leading-dash baseRef before any git call", async () => {
await expect(
checkoutPrBranch({ ...basePr, baseRef: "--config-env=FOO=BAR" }, dummyParams)
).rejects.toThrow(/PR base ref.*starts with '-'/);
});
});
describe("dependency install - ignore-scripts logic", () => {
it("ignoreScripts is true when shell is disabled", () => {
expect(shouldIgnoreScripts("disabled")).toBe(true);
});
it("ignoreScripts is false when shell is restricted (scripts run in stripped env)", () => {
expect(shouldIgnoreScripts("restricted")).toBe(false);
});
it("ignoreScripts is false when shell is enabled", () => {
expect(shouldIgnoreScripts("enabled")).toBe(false);
});
});
mcp/selectMode.ts
+47 -135
View File
@@ -1,7 +1,6 @@
import { type } from "arktype";
import { formatMcpToolRef } from "../external.ts";
import type { Mode } from "../modes.ts";
import { apiFetch } from "../utils/apiFetch.ts";
import type { ToolContext } from "./server.ts";
import { execute, tool } from "./shared.ts";
@@ -10,7 +9,7 @@ export const SelectModeParams = type({
"the name of the mode to select (e.g., 'Build', 'Plan', 'Review', 'IncrementalReview', 'Fix', 'AddressReviews', 'Task', 'ResolveConflicts')"
),
"issue_number?": type("number").describe(
"optional issue number; when provided with Plan mode, used to look up an existing plan comment for this issue (edit vs create)"
"optional issue number; when provided with Plan mode, used to look up an existing plan comment"
),
});
@@ -26,156 +25,69 @@ An existing plan comment was found for this issue. Update that comment with the
1. **task list**: create your task list for this run as your first action.
2. Use \`previousPlanBody\` from this response as the plan to revise; do not call \`get_issue\` or \`get_issue_comments\`.
3. Revise the plan based on the user's request:
- incorporate the current plan (\`previousPlanBody\`) and the user's revision request
- gather relevant codebase context (file paths, architecture notes from AGENTS.md)
- produce a structured plan with clear milestones
4. Call \`${t("report_progress")}\` with the full revised plan text and \`{ target_plan_comment: true }\` so it updates the existing plan comment (not the progress comment).
5. Then post a short note to the progress comment (e.g. "Plan has been updated in the comment above.") via \`${t("report_progress")}\` so it is not left as "Leaping...".`,
3. Revise the plan based on the user's request.
4. Call \`${t("report_progress")}\` with the full revised plan text and \`{ target_plan_comment: true }\` so it updates the existing plan comment.
5. Then post a short note to the progress comment via \`${t("report_progress")}\` so it is not left as "Leaping...".`,
};
}
type OrchestratorGuidance = {
modeName: string;
description: string;
orchestratorGuidance: string;
};
// IncrementalReview inherits Review's user instructions, Fix inherits Build's
const modeInstructionParent: Record<string, string> = {
IncrementalReview: "Review",
Fix: "Build",
};
function buildOrchestratorGuidance(
ctx: ToolContext,
mode: Mode,
overrideGuidance?: string
): OrchestratorGuidance {
const hardcoded = overrideGuidance ?? mode.prompt ?? "";
const lookupKey = modeInstructionParent[mode.name] ?? mode.name;
const userInstructions = ctx.modeInstructions[lookupKey] ?? "";
const guidance = [hardcoded, userInstructions].filter(Boolean).join("\n\n");
return {
modeName: mode.name,
description: mode.description,
orchestratorGuidance: guidance,
};
}
// matches the API response for /repo/[owner]/[repo]/issue/[issueNumber]/plan-comment
export type PlanCommentResponsePayload = { error: string } | { commentId: number; body: string };
// IMPORTANT: this route authenticates via GitHub installation token (getEnrichedRepo),
// NOT the Pullfrog API JWT (ctx.apiToken). use ctx.githubInstallationToken here.
// see wiki/api-auth.md for the two auth patterns.
async function fetchExistingPlanComment(
ctx: ToolContext,
issueNumber: number
): Promise<Extract<PlanCommentResponsePayload, { commentId: number }> | null> {
if (!ctx.githubInstallationToken) return null;
try {
const response = await apiFetch({
path: `/api/repo/${ctx.repo.owner}/${ctx.repo.name}/issue/${issueNumber}/plan-comment`,
method: "GET",
headers: { authorization: `Bearer ${ctx.githubInstallationToken}` },
signal: AbortSignal.timeout(10_000),
});
const data = (await response.json()) as PlanCommentResponsePayload;
return response.ok && "commentId" in data ? data : null;
} catch {
return null;
}
}
const SUMMARY_MODES = new Set(["Review", "IncrementalReview", "Task"]);
/** modes that gain the PR summary edit step when toolState.summaryFilePath is set.
*
* NOTE: this snapshot is an internal artifact consumed by future agent runs. it is
* deliberately NOT shaped by user-supplied summary instructions — those would warp
* the durable agent context. user-facing summarization (e.g. the review body's
* "Reviewed changes" section) is governed by review-mode prompts and review
* instructions, separately from this snapshot. */
function buildSummaryAddendum(t: (name: string) => string, ctx: ToolContext): string {
const filePath = ctx.toolState.summaryFilePath;
if (!filePath) return "";
return `### PR summary snapshot — required step
A rolling PR summary lives at \`${filePath}\`. It is your durable cross-run agent context — a functional summary of what this PR does, the subsystems and files it touches, the material behavior of its changes, and any risks or open questions worth carrying forward. It is NOT a chronological log of past review runs; commit-level history can already be reconstructed from \`${t("list_pull_request_reviews")}\`.
How to use it:
- read \`${filePath}\` at the START of the run, alongside the diff. it represents what previous agent runs already understood about this PR — absorb it before picking lenses or crafting subagent dispatch prompts. if it's a fresh seed (file is one or two lines), this is a first review and you'll be filling it in from the diff.
- let the snapshot inform triage and dispatch. when it already tracks a risk, your lens prompts to subagents are stronger when they reference that context (e.g. "the JSDoc explicitly scopes to code points — do not flag grapheme-cluster issues" if the snapshot already documents that contract). when something the snapshot tracks is now resolved by new commits, note that. when new commits introduce something the snapshot doesn't yet describe, that's exactly where your fan-out should focus.
- update the file in place to reflect the PR's CURRENT state. revise stale claims, drop resolved risks, add new behavior or risks. accuracy over breadth — every claim must be grounded in the diff. write for the next agent run, not for a human.
- structure however serves THIS PR. there is no required section template. a refactor might organize by renamed export and call-site impact; a feature by capability; a billing change by money path. a compact note of which commit ranges have been reviewed should always be present so future runs scope correctly, but the rest is your call. when the structure works across runs, keep it stable so range-diffs are clean; when the PR's character changes (e.g. scope expands), reshape.
Do NOT call \`${t("create_issue_comment")}\` for the summary — the server reads this file at end-of-run and persists it. The file edit is mandatory regardless of whether a review is submitted; the snapshot feeds the next run.`;
}
export function SelectModeTool(ctx: ToolContext) {
const t = (name: string) => formatMcpToolRef(ctx.agentId, name);
const overrides = buildModeOverrides(t);
return tool({
name: "select_mode",
description:
"Select a mode and receive step-by-step guidance on how to handle the task. Call this to understand the best workflow for the current mode. " +
'Example: `select_mode({ mode: "Review" })` or `select_mode({ mode: "Plan", issue_number: 1234 })`.',
"Select the operating mode for this run. Call this first to get the workflow for your task. " +
"Example: `select_mode({ mode: 'Review' })`.",
parameters: SelectModeParams,
execute: execute(async (params) => {
if (ctx.toolState.selectedMode) {
return {
error: `mode already selected: "${ctx.toolState.selectedMode}". mode selection is final and cannot be changed. complete your current workflow within this mode.`,
};
execute: execute(async ({ mode, issue_number }) => {
const t = (toolName: string) => formatMcpToolRef(ctx.agentId, toolName);
const overrides = buildModeOverrides(t);
// find mode in available list
const foundMode = resolveMode(ctx.modes, mode);
if (!foundMode) {
const available = ctx.modes.map((m) => m.name).join(", ");
throw new Error(
`Unknown mode "${mode}". Available modes: ${available}`
);
}
const modeName = params.mode;
ctx.toolState.selectedMode = foundMode.name;
const selectedMode = resolveMode(ctx.modes, modeName);
const overrideGuidance = overrides[foundMode.name];
const hardcoded = overrideGuidance ?? foundMode.prompt ?? "";
const userInstructions = ctx.modeInstructions[foundMode.name] ?? "";
const guidance = [hardcoded, userInstructions].filter(Boolean).join("\n\n");
if (!selectedMode) {
const availableModes = ctx.modes.map((m) => m.name).join(", ");
return {
error: `mode "${modeName}" not found. available modes: ${availableModes}`,
availableModes: ctx.modes.map((m) => ({
name: m.name,
description: m.description,
})),
};
}
const response: Record<string, unknown> = {
modeName: foundMode.name,
description: foundMode.description,
orchestratorGuidance: guidance,
};
ctx.toolState.selectedMode = selectedMode.name;
if (selectedMode.name === "Plan") {
const issueNumber = params.issue_number ?? ctx.payload.event.issue_number;
if (issueNumber !== undefined) {
const existing = await fetchExistingPlanComment(ctx, issueNumber);
if (existing !== null) {
ctx.toolState.existingPlanCommentId = existing.commentId;
ctx.toolState.previousPlanBody = existing.body;
return {
...buildOrchestratorGuidance(ctx, selectedMode, overrides.PlanEdit),
previousPlanBody: existing.body,
};
// For Plan mode with issue_number, look up existing plan comment
if (foundMode.name === "Plan" && issue_number !== undefined) {
try {
const commentsResp = await ctx.gitea.paginate(ctx.gitea.rest.issue.issueGetComments, {
owner: ctx.repo.owner,
repo: ctx.repo.name,
index: issue_number,
});
const comments = commentsResp;
// Look for a plan comment (one with our footer)
const planComment = comments.find((c) => c.body?.includes("<!-- shockbot-footer -->"));
if (planComment) {
if (planComment.id !== undefined) ctx.toolState.existingPlanCommentId = planComment.id;
ctx.toolState.previousPlanBody = planComment.body ?? "";
response.existingPlanCommentFound = true;
response.previousPlanBody = ctx.toolState.previousPlanBody;
response.orchestratorGuidance = (overrides["PlanEdit"] ?? guidance) + "\n\n" + (userInstructions ? `\n\n${userInstructions}` : "");
}
} catch {
// Best-effort — if we can't find the plan comment, proceed normally
}
}
const summaryAddendum = SUMMARY_MODES.has(selectedMode.name)
? buildSummaryAddendum(t, ctx)
: "";
const base = buildOrchestratorGuidance(ctx, selectedMode);
if (summaryAddendum.length > 0) {
return {
...base,
orchestratorGuidance: `${base.orchestratorGuidance}\n\n${summaryAddendum}`,
summaryFilePath: ctx.toolState.summaryFilePath,
};
}
return base;
return response;
}),
});
}
mcp/server.ts
+10 -39
View File
@@ -3,16 +3,13 @@ import "./arkConfig.ts";
import { createServer } from "node:net";
import { setTimeout as sleep } from "node:timers/promises";
import { FastMCP, type Tool } from "fastmcp";
import { type AgentId, pullfrogMcpName } from "../external.ts";
import { shockbotMcpName } from "../external.ts";
import type { Mode } from "../modes.ts";
import type { ToolState } from "../toolState.ts";
import { closeBrowserDaemon } from "../utils/browser.ts";
import type { OctokitWithPlugins } from "../utils/github.ts";
import type { Gitea } from "../utils/gitea.ts";
import type { ResolvedPayload } from "../utils/payload.ts";
import type { AccountPlan } from "../utils/runContext.ts";
import type { RunContextData } from "../utils/runContextData.ts";
import { CheckoutPrTool } from "./checkout.ts";
import { GetCheckSuiteLogsTool } from "./checkSuite.ts";
import {
CreateCommentTool,
EditCommentTool,
@@ -37,7 +34,6 @@ import { CreatePullRequestReviewTool } from "./review.ts";
import {
GetReviewCommentsTool,
ListPullRequestReviewsTool,
ResolveReviewThreadTool,
} from "./reviewComments.ts";
import { SelectModeTool } from "./selectMode.ts";
import { addTools } from "./shared.ts";
@@ -45,34 +41,21 @@ import { KillBackgroundTool, ShellTool } from "./shell.ts";
import { UploadFileTool } from "./upload.ts";
export interface ToolContext {
agentId: AgentId;
repo: RunContextData["repo"];
agentId: "ollama";
repo: { owner: string; name: string };
payload: ResolvedPayload;
octokit: OctokitWithPlugins;
githubInstallationToken: string;
gitea: Gitea;
gitToken: string;
apiToken: string;
modes: Mode[];
postCheckoutScript: string | null;
prepushScript: string | null;
prApproveEnabled: boolean;
modeInstructions: Record<string, string>;
toolState: ToolState;
runId: number | undefined;
jobId: string | undefined;
runId?: number | undefined;
jobId?: string | undefined;
mcpServerUrl: string;
tmpdir: string;
// repo-level OSS flag + account-level billing plan. together they decide
// whether pullfrog is paying for marginal infra — see `isInfraCovered` in
// the server's `utils/billing.ts`. plan gating for endpoints like the
// learnings PATCH is enforced server-side via 402, so we pass plan along
// mostly for future use / observability. see wiki/pricing.md.
oss: boolean;
plan: AccountPlan;
// resolved upstream model specifier (e.g. "google/gemini-3.1-pro-preview").
// undefined when payload.proxyModel is set or when the alias is unresolvable.
// used by the schema sanitizer to detect Gemini-routed traffic.
resolvedModel: string | undefined;
}
const mcpPortStart = 3764;
@@ -81,11 +64,11 @@ const mcpHost = "127.0.0.1";
const mcpEndpoint = "/mcp";
function readEnvPort(): number | null {
const rawPort = process.env.PULLFROG_MCP_PORT;
const rawPort = process.env.SHOCKBOT_MCP_PORT;
if (!rawPort) return null;
const parsed = Number.parseInt(rawPort, 10);
if (!Number.isInteger(parsed) || parsed <= 0 || parsed > 65535) {
throw new Error(`invalid PULLFROG_MCP_PORT: ${rawPort}`);
throw new Error(`invalid SHOCKBOT_MCP_PORT: ${rawPort}`);
}
return parsed;
}
@@ -131,8 +114,6 @@ function buildCommonTools(ctx: ToolContext, outputSchema?: JsonSchema): Tool<any
CheckoutPrTool(ctx),
GetReviewCommentsTool(ctx),
ListPullRequestReviewsTool(ctx),
ResolveReviewThreadTool(ctx),
GetCheckSuiteLogsTool(ctx),
AddLabelsTool(ctx),
GitTool(ctx),
GitFetchTool(ctx),
@@ -144,7 +125,6 @@ function buildCommonTools(ctx: ToolContext, outputSchema?: JsonSchema): Tool<any
tools.push(SetOutputTool(ctx, outputSchema));
}
// MCP shell with filtered env (no secrets leaked to child processes)
if (ctx.payload.shell === "restricted") {
tools.push(ShellTool(ctx));
tools.push(KillBackgroundTool(ctx));
@@ -177,7 +157,7 @@ async function tryStartMcpServer(
tools: Tool<any, any>[],
port: number
): Promise<McpStartResult | null> {
const server = new FastMCP({ name: pullfrogMcpName, version: "0.0.1" });
const server = new FastMCP({ name: shockbotMcpName, version: "0.0.1" });
addTools(ctx, server, tools);
try {
@@ -217,7 +197,6 @@ async function selectMcpPort(ctx: ToolContext, tools: Tool<any, any>[]): Promise
}
}
// randomize start offset to reduce collision chance in parallel runs
const randomOffset = Math.floor(Math.random() * 50);
for (let offset = 0; offset < mcpPortAttempts; offset++) {
@@ -269,14 +248,6 @@ type McpHttpServerOptions = {
outputSchema?: JsonSchema | undefined;
};
/**
* Start the MCP HTTP server.
*
* The returned disposer is idempotent — safe to call multiple times.
* Callers (e.g. the inner activity-timeout handler in main.ts) may need to
* stop the server before the `await using` block exits; a subsequent
* automatic dispose is then a no-op.
*/
export async function startMcpHttpServer(
ctx: ToolContext,
options?: McpHttpServerOptions
mcp/shared.ts
+8 -19
View File
@@ -1,10 +1,11 @@
import type { StandardSchemaV1 } from "@standard-schema/spec";
import { encode as toonEncode } from "@toon-format/toon";
import type { FastMCP, Tool } from "fastmcp";
import { formatJsonValue, log } from "../utils/cli.ts";
import { isGeminiRouted, sanitizeToolForGemini } from "./geminiSanitizer.ts";
import type { ToolContext } from "./server.ts";
// Tool<any, any> is intentional: the tools array is a heterogeneous collection
// where each tool has a different typed params schema. TypeScript's contravariance
// rules make it impossible to express this without any in the generic position.
export const tool = <const params>(
toolDef: Tool<any, StandardSchemaV1<params>>
): Tool<any, StandardSchemaV1<params>> => toolDef;
@@ -17,8 +18,8 @@ export interface ToolResult {
isError?: boolean;
}
export const handleToolSuccess = (data: Record<string, any> | string): ToolResult => {
const text = typeof data === "string" ? data : toonEncode(data);
export const handleToolSuccess = (data: Record<string, unknown> | string): ToolResult => {
const text = typeof data === "string" ? data : JSON.stringify(data, null, 2);
return {
content: [{ type: "text", text }],
};
@@ -27,23 +28,12 @@ export const handleToolSuccess = (data: Record<string, any> | string): ToolResul
export const handleToolError = (error: unknown): ToolResult => {
const errorMessage = error instanceof Error ? error.message : String(error);
return {
content: [
{
type: "text",
text: `Error: ${errorMessage}`,
},
],
content: [{ type: "text", text: `Error: ${errorMessage}` }],
isError: true,
};
};
/**
* Helper to wrap a tool execute function with error handling.
* Captures ctx in closure so tools don't need to handle try/catch.
* @param fn - the function to execute
* @param toolName - optional tool name for error logging
*/
export const execute = <T, R extends Record<string, any> | string>(
export const execute = <T, R extends Record<string, unknown> | string>(
fn: (params: T) => Promise<R>,
toolName?: string
) => {
@@ -63,9 +53,8 @@ export const execute = <T, R extends Record<string, any> | string>(
};
export const addTools = (ctx: ToolContext, server: FastMCP<any>, tools: Tool<any, any>[]) => {
const shouldSanitize = isGeminiRouted(ctx);
for (const tool of tools) {
server.addTool(shouldSanitize ? sanitizeToolForGemini(tool) : tool);
server.addTool(tool);
}
return server;
};
mcp/toolFiltering.test.ts
-85
View File
@@ -1,85 +0,0 @@
import { createServer } from "node:net";
import { Client } from "@modelcontextprotocol/sdk/client/index.js";
import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js";
import { type } from "arktype";
import { FastMCP } from "fastmcp";
import { afterAll, beforeAll, describe, expect, it } from "vitest";
import { execute, tool } from "./shared.ts";
function getRandomPort(): Promise<number> {
return new Promise((resolve, reject) => {
const srv = createServer();
srv.listen(0, "127.0.0.1", () => {
const addr = srv.address();
if (!addr || typeof addr === "string") return reject(new Error("bad address"));
const port = addr.port;
srv.close(() => resolve(port));
});
});
}
async function connectMcpClient(url: string): Promise<Client> {
const transport = new StreamableHTTPClientTransport(new URL(url));
const client = new Client({ name: "test-client", version: "0.0.1" });
// @ts-expect-error — exactOptionalPropertyTypes mismatch: SDK Transport.sessionId?: string vs StreamableHTTPClientTransport getter returning string | undefined
await client.connect(transport);
return client;
}
function mockTool(name: string, description: string) {
return tool({
name,
description,
parameters: type({ value: "string" }),
execute: execute(async () => ({ ok: true })),
});
}
describe("MCP server tool registration - integration", () => {
let server: FastMCP;
let serverUrl: string;
const clients: Client[] = [];
beforeAll(async () => {
const port = await getRandomPort();
serverUrl = `http://127.0.0.1:${port}/mcp`;
server = new FastMCP({ name: "test-server", version: "0.0.1" });
server.addTool(mockTool("shell", "run shell commands"));
server.addTool(mockTool("git", "run git commands"));
server.addTool(mockTool("set_output", "set output"));
server.addTool(mockTool("select_mode", "select a mode"));
server.addTool(mockTool("push_branch", "push branch"));
server.addTool(mockTool("create_pull_request", "create PR"));
await server.start({
transportType: "httpStream",
httpStream: { port, host: "127.0.0.1", endpoint: "/mcp" },
});
});
afterAll(async () => {
for (const client of clients) {
try {
await client.close();
} catch {
// best-effort cleanup
}
}
await server.stop();
});
it("server exposes all registered tools", async () => {
const client = await connectMcpClient(serverUrl);
clients.push(client);
const result = await client.listTools();
const names = result.tools.map((t) => t.name);
expect(names).toContain("select_mode");
expect(names).toContain("push_branch");
expect(names).toContain("create_pull_request");
expect(names).toContain("shell");
expect(names).toContain("git");
expect(names).toContain("set_output");
expect(names.length).toBe(6);
});
});
mcp/upload.ts
+5 -56
View File
@@ -1,9 +1,5 @@
import * as fs from "node:fs";
import * as path from "node:path";
import { type } from "arktype";
import { fileTypeFromBuffer } from "file-type";
import { apiFetch } from "../utils/apiFetch.ts";
import { log } from "../utils/cli.ts";
import type { ToolContext } from "./server.ts";
import { execute, tool } from "./shared.ts";
@@ -11,64 +7,17 @@ const UploadFileParams = type({
path: type.string.describe("absolute path to file to upload"),
});
export function UploadFileTool(ctx: ToolContext) {
export function UploadFileTool(_ctx: ToolContext) {
return tool({
name: "upload_file",
description:
"upload a file to get a permanent public URL. use for screenshots, artifacts, or any files you want to reference in PRs/comments. max 10MB, images/text/archives allowed. when embedding uploaded images in comments or PR bodies, always use markdown image syntax: ![description](url)",
"Upload a file to get a public URL. Note: file upload is not configured in this shockbot deployment.",
parameters: UploadFileParams,
execute: execute(async (params) => {
// read file from disk eagerly on purpose to avoid its content being changed by the time it's uploaded
const buffer = fs.readFileSync(params.path);
const filename = path.basename(params.path);
const contentLength = buffer.length;
const fileType = await fileTypeFromBuffer(buffer);
const contentType = fileType?.mime || "application/octet-stream";
const response = await apiFetch({
path: "/api/upload/signed-url",
method: "POST",
headers: {
Authorization: `Bearer ${ctx.apiToken}`,
"Content-Type": "application/json",
},
body: JSON.stringify({
filename,
contentType,
contentLength,
}),
});
if (!response.ok) {
const error = await response.text();
throw new Error(`failed to get upload URL: ${error}`);
}
const { uploadUrl, publicUrl, contentDisposition } = (await response.json()) as {
uploadUrl: string;
publicUrl: string;
contentDisposition?: string | undefined;
};
const uploadResponse = await fetch(uploadUrl, {
method: "PUT",
headers: {
"Content-Type": contentType,
// should be set automatically, but given this header is signed it's better to be explicit
"Content-Length": String(contentLength),
...(contentDisposition && { "Content-Disposition": contentDisposition }),
},
body: buffer,
});
if (!uploadResponse.ok) {
throw new Error(`failed to upload file: ${uploadResponse.statusText}`);
}
log.info(`» uploaded file ${publicUrl}`);
return { success: true, publicUrl, filename, contentLength, contentType };
throw new Error(
`File upload is not configured (${filename}). Commit files to the repository or use an external service.`
);
}),
});
}