From 9132a597580dfb0dd2a063bf32ec6da028695558 Mon Sep 17 00:00:00 2001 From: Colin McDonnell Date: Tue, 16 Dec 2025 22:14:41 -0800 Subject: [PATCH] Fix create_review and various opencode things --- agents/opencode.ts | 48 ++++++++++---- fixtures/basic.txt | 2 +- main.ts | 17 +++-- mcp/prInfo.ts | 4 +- mcp/review.ts | 152 ++++++++++++++++++++++----------------------- utils/setup.ts | 8 +++ 6 files changed, 129 insertions(+), 102 deletions(-) diff --git a/agents/opencode.ts b/agents/opencode.ts index 5c52195..1bf3de0 100644 --- a/agents/opencode.ts +++ b/agents/opencode.ts @@ -117,6 +117,19 @@ interface OpenCodeResultEvent { [key: string]: unknown; } +interface OpenCodeErrorEvent { + type: "error"; + timestamp?: string; + sessionID?: string; + error?: { + name?: string; + message?: string; + data?: unknown; + [key: string]: unknown; + }; + [key: string]: unknown; +} + type OpenCodeEvent = | OpenCodeInitEvent | OpenCodeMessageEvent @@ -125,7 +138,8 @@ type OpenCodeEvent = | OpenCodeStepFinishEvent | OpenCodeToolUseEvent | OpenCodeToolResultEvent - | OpenCodeResultEvent; + | OpenCodeResultEvent + | OpenCodeErrorEvent; let finalOutput = ""; let accumulatedTokens: { input: number; output: number } = { input: 0, output: 0 }; @@ -315,7 +329,9 @@ export const opencode = agent({ const prompt = addInstructions({ payload, prepResults, repo }); log.group("Full prompt", () => log.info(prompt)); - const args = ["run", "--format", "json", prompt]; + + // message positional must come right after "run", before flags + const args = ["run", prompt, "--format", "json"]; if (payload.sandbox) { log.info("🔒 sandbox mode enabled: restricting to read-only operations"); @@ -359,6 +375,7 @@ export const opencode = agent({ timeout: 600000, // 10 minutes timeout to prevent infinite hangs stdio: ["ignore", "pipe", "pipe"], onStdout: async (chunk) => { + log.debug(`[opencode stdout] ${chunk}`); const text = chunk.toString(); output += text; @@ -401,6 +418,7 @@ export const opencode = agent({ "tool_use", "tool_result", "result", + "error", ].includes(event.type) ) { log.debug(`📋 OpenCode event (unhandled): type=${event.type}`); @@ -506,6 +524,7 @@ function configureOpenCodeMcpServers({ /** * Configure OpenCode sandbox mode via opencode.json. * When sandbox is enabled, restricts tools to read-only operations. + * See https://opencode.ai/docs/permissions/ for config format. */ function configureOpenCodeSandbox({ sandbox }: { sandbox: boolean }): void { const tempHome = process.env.PULLFROG_TEMP_DIR!; @@ -525,18 +544,23 @@ function configureOpenCodeSandbox({ sandbox }: { sandbox: boolean }): void { } if (sandbox) { - // sandbox mode: disable write, bash, and webfetch tools - config.tools = { - write: false, - bash: false, - webfetch: false, + // sandbox mode: deny write, bash, and webfetch tools + config.permission = { + edit: "deny", + bash: "deny", + webfetch: "deny", + doom_loop: "allow", + external_directory: "allow", }; } else { - // normal mode: enable all tools (or don't set tools config to use defaults) - config.tools = { - write: true, - bash: true, - webfetch: true, + // normal mode: allow all tools without prompts + // external_directory: "allow" is critical to avoid permission prompts for temp dirs + config.permission = { + edit: "allow", + bash: "allow", + webfetch: "allow", + doom_loop: "allow", + external_directory: "allow", }; } diff --git a/fixtures/basic.txt b/fixtures/basic.txt index c6aeeca..dd6429e 100644 --- a/fixtures/basic.txt +++ b/fixtures/basic.txt @@ -1 +1 @@ -Implement .omit() in a new PR \ No newline at end of file +review this https://github.com/pullfrog/colinhacks/pull/5 \ No newline at end of file diff --git a/main.ts b/main.ts index 30e0c1a..93416ae 100644 --- a/main.ts +++ b/main.ts @@ -424,23 +424,22 @@ function parsePayload(inputs: Inputs): Payload { } async function startMcpServer(ctx: Context): Promise { - const runId = process.env.GITHUB_RUN_ID; - if (!runId) { - throw new Error("GITHUB_RUN_ID environment variable is required"); - } + const runId = process.env.GITHUB_RUN_ID || ""; ctx.runId = runId; // fetch the pre-created progress comment ID from the database // this must be set BEFORE starting the MCP server so comment.ts can read it - const workflowRunInfo = await fetchWorkflowRunInfo(ctx.runId); - if (workflowRunInfo.progressCommentId) { - process.env.PULLFROG_PROGRESS_COMMENT_ID = workflowRunInfo.progressCommentId; - log.info(`📝 Using pre-created progress comment: ${workflowRunInfo.progressCommentId}`); + if (runId) { + const workflowRunInfo = await fetchWorkflowRunInfo(ctx.runId); + if (workflowRunInfo.progressCommentId) { + process.env.PULLFROG_PROGRESS_COMMENT_ID = workflowRunInfo.progressCommentId; + log.info(`📝 Using pre-created progress comment: ${workflowRunInfo.progressCommentId}`); + } } // fetch job ID by matching GITHUB_JOB name const jobName = process.env.GITHUB_JOB; - if (jobName) { + if (jobName && runId) { const jobs = await ctx.octokit.rest.actions.listJobsForWorkflowRun({ owner: ctx.owner, repo: ctx.name, diff --git a/mcp/prInfo.ts b/mcp/prInfo.ts index 62a5852..432b0b4 100644 --- a/mcp/prInfo.ts +++ b/mcp/prInfo.ts @@ -21,8 +21,8 @@ export function PullRequestInfoTool(ctx: Context) { const data = pr.data; - // detect fork PRs - head repo differs from base repo - const isFork = data.head.repo.full_name !== data.base.repo.full_name; + // detect fork PRs - head repo differs from base repo (head.repo can be null if fork was deleted) + const isFork = data.head.repo?.full_name !== data.base.repo.full_name; return { number: data.number, diff --git a/mcp/review.ts b/mcp/review.ts index e5d213b..6ec7d07 100644 --- a/mcp/review.ts +++ b/mcp/review.ts @@ -5,22 +5,12 @@ import type { RestEndpointMethodTypes } from "@octokit/rest"; import { type } from "arktype"; import type { Context } from "../main.ts"; import { buildPullfrogFooter } from "../utils/buildPullfrogFooter.ts"; +import { log } from "../utils/cli.ts"; import { deleteProgressComment } from "./comment.ts"; import { execute, tool } from "./shared.ts"; -// graphql mutation to create a pending review -const ADD_PULL_REQUEST_REVIEW = ` -mutation AddPullRequestReview($pullRequestId: ID!) { - addPullRequestReview(input: { pullRequestId: $pullRequestId, event: PENDING }) { - pullRequestReview { - id - databaseId - } - } -} -`; - // graphql mutation to add a comment thread to a pending review +// note: REST API doesn't support adding comments to an existing pending review const ADD_PULL_REQUEST_REVIEW_THREAD = ` mutation AddPullRequestReviewThread($pullRequestReviewId: ID!, $path: String!, $line: Int!, $body: String!, $side: DiffSide) { addPullRequestReviewThread(input: { @@ -37,34 +27,6 @@ mutation AddPullRequestReviewThread($pullRequestReviewId: ID!, $path: String!, $ } `; -// graphql mutation to submit a pending review -const SUBMIT_PULL_REQUEST_REVIEW = ` -mutation SubmitPullRequestReview($pullRequestReviewId: ID!, $body: String, $event: PullRequestReviewEvent!) { - submitPullRequestReview(input: { - pullRequestReviewId: $pullRequestReviewId, - body: $body, - event: $event - }) { - pullRequestReview { - id - databaseId - state - url - } - } -} -`; - -// graphql response types -type AddPullRequestReviewResponse = { - addPullRequestReview: { - pullRequestReview: { - id: string; - databaseId: number; - }; - }; -}; - type AddPullRequestReviewThreadResponse = { addPullRequestReviewThread: { thread: { @@ -73,16 +35,26 @@ type AddPullRequestReviewThreadResponse = { }; }; -type SubmitPullRequestReviewResponse = { - submitPullRequestReview: { - pullRequestReview: { - id: string; - databaseId: number; - state: string; - url: string; - }; - }; -}; +// helper to find existing pending review for the authenticated user +async function findPendingReview( + ctx: Context, + pull_number: number +): Promise<{ id: number; node_id: string } | null> { + const reviews = await ctx.octokit.rest.pulls.listReviews({ + owner: ctx.owner, + repo: ctx.name, + pull_number, + per_page: 100, + }); + + // find a PENDING review from our bot + // note: authenticated user is the GitHub App, reviews show as "pullfrog[bot]" + const pendingReview = reviews.data.find((r) => r.state === "PENDING"); + if (pendingReview) { + return { id: pendingReview.id, node_id: pendingReview.node_id }; + } + return null; +} // start_review tool export const StartReview = type({ @@ -96,30 +68,56 @@ export function StartReviewTool(ctx: Context) { "Start a new review session for a pull request. Creates a scratchpad file for gathering thoughts and a pending review on GitHub. Must be called before add_review_comment.", parameters: StartReview, execute: execute(ctx, async ({ pull_number }) => { - // check if review already started + // check if review already started in this session if (ctx.toolState.review) { throw new Error( `Review session already in progress. Call submit_review first to finish it.` ); } - // get the PR to get its node_id for GraphQL + // get the PR to get head commit SHA const pr = await ctx.octokit.rest.pulls.get({ owner: ctx.owner, repo: ctx.name, pull_number, }); - // create pending review via GraphQL - const response = await ctx.octokit.graphql( - ADD_PULL_REQUEST_REVIEW, - { - pullRequestId: pr.data.node_id, - } - ); + let reviewId: number; + let reviewNodeId: string; - const reviewId = response.addPullRequestReview.pullRequestReview.id; - const reviewDatabaseId = response.addPullRequestReview.pullRequestReview.databaseId; + // try to create a new pending review (omitting 'event' creates PENDING state) + log.debug(`creating pending review for PR #${pull_number}...`); + try { + const result = await ctx.octokit.rest.pulls.createReview({ + owner: ctx.owner, + repo: ctx.name, + pull_number, + commit_id: pr.data.head.sha, + // no 'event' = PENDING review + }); + reviewId = result.data.id; + reviewNodeId = result.data.node_id; + log.debug(`created new pending review: id=${reviewId}`); + } catch (error) { + // check for "already has pending review" error + const errorMessage = error instanceof Error ? error.message : String(error); + log.debug(`createReview failed: ${errorMessage}`); + if (errorMessage.includes("pending review")) { + // find the existing pending review + log.debug(`pending review already exists, fetching existing review...`); + const existing = await findPendingReview(ctx, pull_number); + if (!existing) { + throw new Error( + "GitHub says a pending review exists but we couldn't find it. Try again or check the PR reviews." + ); + } + reviewId = existing.id; + reviewNodeId = existing.node_id; + log.debug(`reusing existing pending review: id=${reviewId}`); + } else { + throw error; + } + } // create scratchpad file const scratchpadId = randomBytes(4).toString("hex"); @@ -130,8 +128,8 @@ export function StartReviewTool(ctx: Context) { // set PR context and review state ctx.toolState.prNumber = pull_number; ctx.toolState.review = { - nodeId: reviewId, - id: reviewDatabaseId, + nodeId: reviewNodeId, + id: reviewId, }; return { @@ -168,7 +166,7 @@ export function AddReviewCommentTool(ctx: Context) { throw new Error("No review session started. Call start_review first."); } - // add comment thread via GraphQL + // add comment thread via GraphQL (REST doesn't support adding to existing pending review) await ctx.octokit.graphql( ADD_PULL_REQUEST_REVIEW_THREAD, { @@ -226,17 +224,15 @@ export function SubmitReviewTool(ctx: Context) { const bodyWithFooter = (body || "") + footer; - // submit the review via GraphQL - const response = await ctx.octokit.graphql( - SUBMIT_PULL_REQUEST_REVIEW, - { - pullRequestReviewId: ctx.toolState.review.nodeId, - body: bodyWithFooter, - event: "COMMENT", - } - ); - - const result = response.submitPullRequestReview.pullRequestReview; + // submit the pending review via REST + const result = await ctx.octokit.rest.pulls.submitReview({ + owner: ctx.owner, + repo: ctx.name, + pull_number: ctx.toolState.prNumber, + review_id: reviewId, + event: "COMMENT", + body: bodyWithFooter, + }); // clear review state delete ctx.toolState.review; @@ -246,9 +242,9 @@ export function SubmitReviewTool(ctx: Context) { return { success: true, - reviewId: result.databaseId, - html_url: result.url, - state: result.state, + reviewId: result.data.id, + html_url: result.data.html_url, + state: result.data.state, }; }), }); diff --git a/utils/setup.ts b/utils/setup.ts index 8693756..4f6ab48 100644 --- a/utils/setup.ts +++ b/utils/setup.ts @@ -52,6 +52,14 @@ export function setupGitConfig(): void { cwd: repoDir, stdio: "pipe", }); + // disable credential helper to prevent macOS keychain prompts when using x-access-token + // only needed locally - GitHub Actions doesn't have this issue + if (!process.env.GITHUB_ACTIONS) { + execSync('git config --local credential.helper ""', { + cwd: repoDir, + stdio: "pipe", + }); + } log.debug("setupGitConfig: ✓ Git configuration set successfully (scoped to repo)"); } catch (error) { // If git config fails, log warning but don't fail the action