Merge pull request #8 from pullfrog/thinking-reviews

Improve review thinking

main.ts

@@ -63,6 +63,7 @@ export async function main(inputs: Inputs): Promise<MainResult> {
 
     const partialCtx = await initializeContext(inputs, payload);
     const ctx = partialCtx as Context;
+    ctx.toolState = {};
     timer.checkpoint("initializeContext");
 
     await setupGit(ctx);
@@ -242,6 +243,18 @@ export interface Context {
   // workflow run info
   runId: string;
   jobId: string | undefined;
+
+  // tool state - mutable scratchpad for tools
+  toolState: ToolState;
+}
+
+export interface ToolState {
+  prNumber?: number;
+  issueNumber?: number;
+  review?: {
+    id: number; // REST API database ID (for fix URLs)
+    nodeId: string; // GraphQL node ID (for mutations)
+  };
 }
 
 async function initializeContext(
@@ -259,6 +272,7 @@ async function initializeContext(
     | "prepResults"
     | "runId"
     | "jobId"
+    | "toolState"
   >
 > {
   log.info(`🐸 Running pullfrog/action@${packageJson.version}...`);

mcp/checkout.ts

@@ -88,6 +88,9 @@ export function CheckoutPrTool(ctx: Context) {
         $("git", ["config", `branch.${headBranch}.pushRemote`, "origin"]);
       }
 
+      // set PR context
+      ctx.toolState.prNumber = pull_number;
+
       return {
         success: true,
         number: pr.data.number,
@@ -102,4 +105,3 @@ export function CheckoutPrTool(ctx: Context) {
     }),
   });
 }
-

mcp/comment.ts

@@ -204,7 +204,9 @@ export async function reportProgress(
   }
 
   // no existing comment - create one
-  const issueNumber = ctx.payload.event.issue_number;
+  // use fallback chain: dynamically set context > event payload
+  const issueNumber =
+    ctx.toolState.prNumber ?? ctx.toolState.issueNumber ?? ctx.payload.event.issue_number;
   if (issueNumber === undefined) {
     // cannot create comment without issue_number (e.g., workflow_dispatch events)
     return undefined;

mcp/issueComments.ts

@@ -13,6 +13,9 @@ export function GetIssueCommentsTool(ctx: Context) {
       "Get all comments for a GitHub issue. Returns all comments including the issue body and all subsequent discussion comments.",
     parameters: GetIssueComments,
     execute: execute(ctx, async ({ issue_number }) => {
+      // set issue context
+      ctx.toolState.issueNumber = issue_number;
+
       const comments = await ctx.octokit.paginate(ctx.octokit.rest.issues.listComments, {
         owner: ctx.owner,
         repo: ctx.name,

mcp/issueEvents.ts

@@ -13,6 +13,9 @@ export function GetIssueEventsTool(ctx: Context) {
       "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.",
     parameters: GetIssueEvents,
     execute: execute(ctx, async ({ issue_number }) => {
+      // set issue context
+      ctx.toolState.issueNumber = issue_number;
+
       const events = await ctx.octokit.paginate(ctx.octokit.rest.issues.listEventsForTimeline, {
         owner: ctx.owner,
         repo: ctx.name,

mcp/issueInfo.ts

@@ -20,6 +20,9 @@ export function IssueInfoTool(ctx: Context) {
 
       const data = issue.data;
 
+      // set issue context
+      ctx.toolState.issueNumber = issue_number;
+
       const hints: string[] = [];
       if (data.comments > 0) {
         hints.push("use get_issue_comments to retrieve all comments for this issue");

mcp/review.ts

@@ -1,3 +1,6 @@
+import { randomBytes } from "node:crypto";
+import { writeFileSync } from "node:fs";
+import { join } from "node:path";
 import type { RestEndpointMethodTypes } from "@octokit/rest";
 import { type } from "arktype";
 import type { Context } from "../main.ts";
@@ -5,6 +8,253 @@ import { buildPullfrogFooter } from "../utils/buildPullfrogFooter.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
+const ADD_PULL_REQUEST_REVIEW_THREAD = `
+mutation AddPullRequestReviewThread($pullRequestReviewId: ID!, $path: String!, $line: Int!, $body: String!, $side: DiffSide) {
+  addPullRequestReviewThread(input: {
+    pullRequestReviewId: $pullRequestReviewId,
+    path: $path,
+    line: $line,
+    body: $body,
+    side: $side
+  }) {
+    thread {
+      id
+    }
+  }
+}
+`;
+
+// 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: {
+      id: string;
+    };
+  };
+};
+
+type SubmitPullRequestReviewResponse = {
+  submitPullRequestReview: {
+    pullRequestReview: {
+      id: string;
+      databaseId: number;
+      state: string;
+      url: string;
+    };
+  };
+};
+
+// start_review tool
+export const StartReview = type({
+  pull_number: type.number.describe("The pull request number to review"),
+});
+
+export function StartReviewTool(ctx: Context) {
+  return tool({
+    name: "start_review",
+    description:
+      "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
+      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
+      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<AddPullRequestReviewResponse>(
+        ADD_PULL_REQUEST_REVIEW,
+        {
+          pullRequestId: pr.data.node_id,
+        }
+      );
+
+      const reviewId = response.addPullRequestReview.pullRequestReview.id;
+      const reviewDatabaseId = response.addPullRequestReview.pullRequestReview.databaseId;
+
+      // create scratchpad file
+      const scratchpadId = randomBytes(4).toString("hex");
+      const scratchpadPath = join(ctx.sharedTempDir, `pullfrog-review-${scratchpadId}.md`);
+      const scratchpadContent = `# Review ${scratchpadId}\n\n`;
+      writeFileSync(scratchpadPath, scratchpadContent);
+
+      // set PR context and review state
+      ctx.toolState.prNumber = pull_number;
+      ctx.toolState.review = {
+        nodeId: reviewId,
+        id: reviewDatabaseId,
+      };
+
+      return {
+        reviewId: scratchpadId,
+        scratchpadPath,
+        message: `Review session started. Use the scratchpad file to gather your thoughts, then call add_review_comment for each comment.`,
+      };
+    }),
+  });
+}
+
+// add_review_comment tool
+export const AddReviewComment = type({
+  path: type.string.describe("The file path to comment on (relative to repo root)"),
+  line: type.number.describe(
+    "The line number in the file (use line numbers from the diff - the NEW file line number)"
+  ),
+  body: type.string.describe("The comment text for this specific line"),
+  side: type
+    .enumerated("LEFT", "RIGHT")
+    .describe("Side of the diff: LEFT (old code) or RIGHT (new code). Defaults to RIGHT.")
+    .optional(),
+});
+
+export function AddReviewCommentTool(ctx: Context) {
+  return tool({
+    name: "add_review_comment",
+    description:
+      "Add a comment to the current review session. Must call start_review first. Comments are stored in draft state until submit_review is called.",
+    parameters: AddReviewComment,
+    execute: execute(ctx, async ({ path, line, body, side }) => {
+      // check if review started
+      if (!ctx.toolState.review) {
+        throw new Error("No review session started. Call start_review first.");
+      }
+
+      // add comment thread via GraphQL
+      await ctx.octokit.graphql<AddPullRequestReviewThreadResponse>(
+        ADD_PULL_REQUEST_REVIEW_THREAD,
+        {
+          pullRequestReviewId: ctx.toolState.review.nodeId,
+          path,
+          line,
+          body,
+          side: side || "RIGHT",
+        }
+      );
+
+      return {
+        success: true,
+        message: `Comment added to ${path}:${line}`,
+      };
+    }),
+  });
+}
+
+// submit_review tool
+export const SubmitReview = type({
+  body: type.string
+    .describe(
+      "Review body text. Typically 1-3 sentences with high-level overview and urgency level. Action links are auto-appended."
+    )
+    .optional(),
+});
+
+export function SubmitReviewTool(ctx: Context) {
+  return tool({
+    name: "submit_review",
+    description:
+      "Submit the current review session. All comments added via add_review_comment will be published. Must call start_review first.",
+    parameters: SubmitReview,
+    execute: execute(ctx, async ({ body }) => {
+      // check if review started
+      if (!ctx.toolState.review) {
+        throw new Error("No review session started. Call start_review first.");
+      }
+      if (ctx.toolState.prNumber === undefined) {
+        throw new Error("No PR context. Call checkout_pr or start_review first.");
+      }
+
+      const reviewId = ctx.toolState.review.id;
+
+      // build quick links footer
+      const apiUrl = process.env.API_URL || "https://pullfrog.com";
+      const fixAllUrl = `${apiUrl}/trigger/${ctx.owner}/${ctx.name}/${ctx.toolState.prNumber}?action=fix&review_id=${reviewId}`;
+      const fixApprovedUrl = `${apiUrl}/trigger/${ctx.owner}/${ctx.name}/${ctx.toolState.prNumber}?action=fix-approved&review_id=${reviewId}`;
+
+      const footer = buildPullfrogFooter({
+        workflowRun: { owner: ctx.owner, repo: ctx.name, runId: ctx.runId, jobId: ctx.jobId },
+        customParts: [`[Fix all ➔](${fixAllUrl})`, `[Fix 👍s ➔](${fixApprovedUrl})`],
+      });
+
+      const bodyWithFooter = (body || "") + footer;
+
+      // submit the review via GraphQL
+      const response = await ctx.octokit.graphql<SubmitPullRequestReviewResponse>(
+        SUBMIT_PULL_REQUEST_REVIEW,
+        {
+          pullRequestReviewId: ctx.toolState.review.nodeId,
+          body: bodyWithFooter,
+          event: "COMMENT",
+        }
+      );
+
+      const result = response.submitPullRequestReview.pullRequestReview;
+
+      // clear review state
+      delete ctx.toolState.review;
+
+      // delete progress comment
+      await deleteProgressComment(ctx);
+
+      return {
+        success: true,
+        reviewId: result.databaseId,
+        html_url: result.url,
+        state: result.state,
+      };
+    }),
+  });
+}
+
+// legacy tool - kept for backwards compatibility
 export const Review = type({
   pull_number: type.number.describe("The pull request number to review"),
   body: type.string
@@ -46,11 +296,15 @@ export function ReviewTool(ctx: Context) {
   return tool({
     name: "submit_pull_request_review",
     description:
+      "DEPRECATED: Use start_review, add_review_comment, and submit_review instead for iterative review workflow. " +
       "Submit a review for an existing pull request. " +
       "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.",
     parameters: Review,
     execute: execute(ctx, async ({ pull_number, body, commit_id, comments = [] }) => {
+      // set PR context
+      ctx.toolState.prNumber = pull_number;
+
       // get the PR to determine the head commit if commit_id not provided
       const pr = await ctx.octokit.rest.pulls.get({
         owner: ctx.owner,

mcp/server.ts

@@ -21,7 +21,12 @@ import { IssueInfoTool } from "./issueInfo.ts";
 import { AddLabelsTool } from "./labels.ts";
 import { PullRequestTool } from "./pr.ts";
 import { PullRequestInfoTool } from "./prInfo.ts";
-import { ReviewTool } from "./review.ts";
+import {
+  AddReviewCommentTool,
+  ReviewTool,
+  StartReviewTool,
+  SubmitReviewTool,
+} from "./review.ts";
 import { GetReviewCommentsTool, ListPullRequestReviewsTool } from "./reviewComments.ts";
 import { SelectModeTool } from "./selectMode.ts";
 import { addTools, isProgressCommentDisabled } from "./shared.ts";
@@ -78,6 +83,9 @@ export async function startMcpHttpServer(
     GetIssueEventsTool(ctx),
     PullRequestTool(ctx),
     ReviewTool(ctx),
+    StartReviewTool(ctx),
+    AddReviewCommentTool(ctx),
+    SubmitReviewTool(ctx),
     PullRequestInfoTool(ctx),
     CheckoutPrTool(ctx),
     GetReviewCommentsTool(ctx),

modes.ts

@@ -95,30 +95,36 @@ ${
       prompt: `Follow these steps:
 1. Checkout the PR using ${ghPullfrogMcpName}/checkout_pr with the PR number. This fetches the PR branch and base branch, preparing the repo for review.
 
-2. **IMPORTANT**: After calling checkout_pr, the PR branch is checked out locally. View diff using: \`git diff origin/<base>..HEAD\` (replace <base> with 'base' from checkout_pr result, e.g., \`git diff origin/main..HEAD\`). Use two dots (..) not three dots (...) for reliable diffs. Do NOT use \`origin/<head>\` - the branch is checked out locally, not as a remote tracking branch. This works for both same-repo and fork PRs.
+2. **IMPORTANT**: After calling ${ghPullfrogMcpName}/checkout_pr, the PR branch is checked out locally. View diff using: \`git diff origin/<base>..HEAD\` (replace <base> with 'base' from checkout_pr result, e.g., \`git diff origin/main..HEAD\`). Use two dots (..) not three dots (...) for reliable diffs. Do NOT use \`origin/<head>\` - the branch is checked out locally, not as a remote tracking branch. This works for both same-repo and fork PRs.
 
-3. Read files from the checked-out PR branch to understand the implementation. Always use **relative paths** from repo root (e.g., \`src/index.ts\`), never absolute paths.
+3. Start review session using ${ghPullfrogMcpName}/start_review. This creates a scratchpad file at a temp path (e.g., \`/tmp/pullfrog-review-abc123.md\`) and returns a session ID. The scratchpad file header contains the session ID for reference. Use this file as free-form space to gather your thoughts before adding comments.
 
-4. Submit review using ${ghPullfrogMcpName}/submit_pull_request_review
+4. **ANALYZE** - Use the scratchpad to gather your thoughts:
+   - Summarize what changes this PR makes
+   - Evaluate the approach - is it sound? If not, **stop here** and leave feedback on the approach. Don't waste time on implementation details if the approach is wrong.
+   - If approach is sound, analyze implementation - consider potential issues per file
+   - Identify bugs, security issues, edge cases
+
+5. **SELF-CRITIQUE** - Before adding comments, review your scratchpad:
+   - Remove nitpicks unless explicitly requested. Think documentation, JSDoc/docstrings, useless comments (compliments)
+   - Your level of nitpickiness should be proportional to the current state of the codebase. Try to guess how much the user will care about a specific critique.
+
+6. Add inline review comments one0-by-one using ${ghPullfrogMcpName}/add_review_comment
+   - Use **relative paths** from repo root (e.g., \`packages/core/src/utils.ts\`)
+   - Use the NEW file line number from the diff (shown after \`+\` in hunk headers like \`@@ -10,5 +12,8 @@\` means new file starts at line 12)
+   - Only comment on lines that appear in the diff. GitHub will reject comments on unchanged lines.
+   - For issues appearing in multiple places, comment on the FIRST occurrence and reference others (e.g., "also at lines X, Y")
+
+7. Submit the review using ${ghPullfrogMcpName}/submit_review
+   - The "body" field is ONLY for: (1) a 1-3 sentence high-level overview, (2) urgency level (e.g., "minor suggestions" vs "blocking issues"), (3) critical security callouts (e.g., API key exposure)
 
 **GENERAL GUIDANCE**
 
 - Do not leave any comments that are not potentially actionable. Do not leave complimentary comments just to be nice.
-- *CRITICAL* — Use **relative paths** from repo root (e.g., \`packages/core/src/utils.ts\`)
-  - For line numbers, use the NEW file line number from the diff (shown after \`+\` in hunk headers like \`@@ -10,5 +12,8 @@\` means new file starts at line 12)
-  - Only comment on lines that appear in the diff. GitHub will reject comments on unchanged lines.
+- Do not nitpick unless instructed explicitly to do so by the user's additional instructions. This includes: requesting documentation/docstrings/JSDoc.
 - **CRITICAL: Prioritize per-line feedback over summary text.**
-  - ALL specific feedback MUST go in the 'comments' array with file paths and line numbers from the diff
-  - For issues appearing in multiple places, comment on the FIRST occurrence and reference others (e.g., "also at lines X, Y" or "similar issue in otherFile.ts:42")
-  - The "body" field is ONLY for: (1) a 1-2 sentence high-level overview, (2) urgency level (e.g., "minor suggestions" vs "blocking issues"), (3) critical security callouts (e.g., API key exposure)
-  - 95%+ of review content should be in per-line comments; the body should be just a couple sentences
-  - The review body will include quick action links for addressing feedback, so keep it concise
-- Do not nitpick unless instructed explicity to do so by the user's additional instructions. This includes: requesting documentation/docstrings/JSDoc. 
-- The review should be thoughtful. When evaluating complex changes, consider the following conceptual approach:
-  - 1) conceptualize the changes made. make sure you understand it.
-  - 2) evaluate conceptual approach. leave feedback as needed.
-  - 3) if the conceptual approach looks sound, evaluate the implementation. leave feedback as needed. consider everything, but especially edge cases, security, correctness, and performance. leave feedback as needed.
-  - 4) only leave nitpick/housekeeping comments if instructed explicity to do so by the user's additional instructions.
+  - All specific feedback MUST go in inline review comments with file paths and line numbers from the diff
+  - The vast majority of review content should be in inline review comments; the body should be brief and only summarize the urgency of the review and any cross-cutting concerns.
   `,
     },
     {