From 5bcfae990abaf82577969f579f1b74d32bf025a8 Mon Sep 17 00:00:00 2001 From: David Blass Date: Wed, 11 Mar 2026 04:24:09 +0000 Subject: [PATCH] restructure dashboard UI, add mode instructions, post-review follow-up dispatch (#453) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * add mode instructions and restructure dashboard sidebar - add modeInstructions JSONB field to Repo model for per-mode user instructions - thread modeInstructions through settings API, run-context API, RepoSettings, ToolContext, and selectMode runtime - merge user-defined mode instructions with hardcoded orchestrator guidance, with IncrementalReview inheriting from Review - reduce visible built-in modes from 7 to 4 (Build, Review, Plan, Fix) with editable Instructions textareas - add TRIGGERS group header to sidebar above Mentions, Pull requests, Issues - add wiki/modes.md documenting triggers and modes conceptual model Made-with: Cursor * fix leaping comment deletion and address review feedback - wrap post-createReview operations in try/finally so deleteProgressComment runs even when updateReview or reportReviewNodeId throws - add parseModeInstructions runtime guard to filter non-string values from the JSONB field before passing to buildOrchestratorGuidance - add useEffect sync for localInstructions when props change - guard onBlur to skip save when instructions haven't changed - update wiki/modes.md to reflect V2 is implemented (no longer "proposed") Made-with: Cursor * harden review cleanup, fix type cast, stabilize mode instructions state - wrap deleteProgressComment in try/catch inside finally to prevent masking original errors - replace `as Record` cast with runtime parseModeInstructions + useMemo - fix wiki dual-prompt table to reflect mode.prompt fallback status Made-with: Cursor * fix wiki tense and heading ambiguity from PR review Made-with: Cursor * fix review "edited" badge by using pending review + submit flow create review as PENDING first (no event/body), build the footer with the now-known review ID, then submitReview with the full body. single atomic publish — no updateReview edit needed. Made-with: Cursor * add post-agent follow-up re-review dispatch After the agent exits, check if PR HEAD moved past the reviewed commit and dispatch a follow-up re-review. This closes the gap where push webhooks are suppressed during in-flight reviews. Made-with: Cursor * add silent flag to follow-up re-review dispatch Made-with: Cursor * restructure dashboard for consistency and clarity - consolidate tools into single grouped card (was 4 separate cards) - merge coding + autofix CI into one section - remove redundant trigger section descriptions - add bidirectional crosslinks between modes and triggers - inline instruction links (review/plan/build) into descriptions - add save status indicators to all sections - restructure flags with grouped built-in/custom cards - flatten sidebar (remove dividers and group headers) - tighten all descriptions Made-with: Cursor * update PR screenshots for new dashboard layout Made-with: Cursor * extend review context inline instead of dispatching new workflow when commits are pushed during a review, the agent now handles them inline: create_pull_request_review detects HEAD movement, returns instructions to pull and review the incremental diff, and the agent submits a second review covering only the new changes. this avoids the cost of spinning up a full new workflow run. also fixes a bug where reviewedSha was set to the submission HEAD (current) rather than the checkout HEAD (what was actually reviewed), which caused commits pushed between checkout and submission to be silently missed by postReviewCleanup. the workflow dispatch is kept as a safety net for agent timeout/error. Made-with: Cursor * polish dashboard UI: fix debug markers, crosslinks, title consistency, descriptions - remove all red debug borders/labels and CM component - remove all inline style={{}} debug outlines from crosslinks - fix ambiguous crosslinks: Build→"Coding ↓", Plan→"Enrich issues ↓" - add missing "Edit build instructions ↑" backlink on Auto-address reviews - normalize card title weight to text-sm font-semibold across all cards - rename "Default" subcard to "Setup" with broader description - fix Mentions description to imperative tone - broaden Flags section description to cover built-in and custom - remove useless fragments in ModesSection and ToolsSettings - restructure Agent section: remove ConsoleSection wrappers, add sidebar indent support Made-with: Cursor * extract PR quick links as standalone card, consistent with issues - PR quick links is now its own card under Reviews (was a sub-toggle inside Review PRs disabled state) - Review PRs OFF sets prCreated="none" instead of auto-falling back to "links" - Review PRs card hides sub-toggles when disabled (re-review/approve don't apply) - Both PRs and Issues now have identical Quick links card structure Made-with: Cursor * update reviews screenshot with standalone quick links card Made-with: Cursor * polish dashboard UI: revert quick links to inline toggles, fix fonts and spacing - revert standalone PR/issue Quick Links cards back to inline toggles inside Review PRs and Enrich Issues cards (fixes prCreated state coupling bug) - restore original font-medium card titles across all trigger/settings cards - fix sidebar: add CONSOLE heading, remove nested indentation, remove truncation - right-justify Enrich Issues mode dropdown, group description with label - move instructions links inline with behavior descriptions - replace text save indicators with icon spinner/checkmark - standardize section title spacing, move footer below danger zone Made-with: Cursor * fix formatting for biome lint Made-with: Cursor * address PR review feedback: cleanup guard, shared util, wiki update - clear ctx.toolState.review after read to prevent double-execution of postReviewCleanup - forward authorPermission in safety-net re-review dispatch - extract parseModeInstructions to utils/schemas/modeInstructions.ts - update wiki/modes.md: remove stale v1/v2 language, fix dashboard layout - add typecheck to pre-push hook Made-with: Cursor * add action typecheck to pre-push, fix exactOptionalPropertyTypes errors Made-with: Cursor * fix duplicate actuallyReviewedSha from rebase Made-with: Cursor * remove PR screenshots Made-with: Cursor * add label/textarea association for mode instruction accessibility Made-with: Cursor --------- Co-authored-by: Colin McDonnell --- entry | 167 ++++++++++++++++++++++++++++++++++------- external.ts | 2 - main.ts | 31 +++++++- mcp/checkout.ts | 3 +- mcp/review.ts | 60 ++++++++++++--- mcp/selectMode.ts | 32 ++++++-- mcp/server.ts | 11 ++- mcp/subagent.ts | 5 +- post | 1 - utils/instructions.ts | 19 +---- utils/payload.ts | 2 - utils/reviewCleanup.ts | 99 ++++++++++++++++++++++++ utils/runContext.ts | 4 +- 13 files changed, 361 insertions(+), 75 deletions(-) create mode 100644 utils/reviewCleanup.ts diff --git a/entry b/entry index eb81fc9..e5210b4 100755 --- a/entry +++ b/entry @@ -140142,7 +140142,6 @@ ${params.instructions}`; system: subagentSystemPreamble, user: params.instructions, eventInstructions: "", - repo: "", event: "", runtime: "" }; @@ -140182,6 +140181,9 @@ async function runSubagent(params) { completeSubagent({ ctx: params.ctx, subagent: params.subagent, success: false }); return { success: false, error: errorMessage }; } finally { + if (mcpServer.toolState.review) { + params.ctx.toolState.review = mcpServer.toolState.review; + } await mcpServer.stop(); } }); @@ -144834,6 +144836,7 @@ function CheckoutPrTool(ctx) { if (!headRepo) { throw new Error(`PR #${pull_number} source repository was deleted`); } + ctx.toolState.checkoutSha = pr.data.head.sha; const formatResult = await fetchAndFormatPrDiff({ octokit: ctx.octokit, owner: ctx.repo.owner, @@ -147233,8 +147236,34 @@ function CreatePullRequestReviewTool(ctx) { } const reviewId = result.data.id; const reviewNodeId = result.data.node_id; - await reportReviewNodeId(ctx, reviewNodeId); - await deleteProgressComment(ctx); + const actuallyReviewedSha = ctx.toolState.checkoutSha ?? params.commit_id; + ctx.toolState.review = { + id: reviewId, + nodeId: reviewNodeId, + reviewedSha: actuallyReviewedSha + }; + const headMovedDuringReview = ctx.toolState.checkoutSha && params.commit_id !== ctx.toolState.checkoutSha; + if (headMovedDuringReview) { + const fromSha = ctx.toolState.checkoutSha; + const toSha = params.commit_id; + ctx.toolState.checkoutSha = toSha; + 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, + newCommits: { + from: fromSha, + to: toSha, + instructions: `New commits were pushed while you were reviewing. Run \`git pull\` to fetch them, then review the incremental diff with \`git diff ${fromSha}...HEAD\`. Submit another review covering only the new changes. Do not repeat feedback from your previous review.` + } + }; + } return { success: true, reviewId, @@ -147912,9 +147941,9 @@ Each task in the \`tasks\` array should include: After all tasks complete, consolidate into a **single** review: - merge the \`comments\` arrays from all subagent outputs -- if subagents found actionable issues: submit one \`${ghPullfrogMcpName}/create_pull_request_review\` with \`approved: false\`, the merged comments, and a unified summary body -- if no subagent found actionable issues: submit with \`approved: true\` and a brief positive summary (no inline comments) -- call \`${ghPullfrogMcpName}/report_progress\` with the summary +- if subagents found actionable issues: submit one \`${ghPullfrogMcpName}/create_pull_request_review\` with \`approved: false\`, the merged comments, and an **empty body** (do NOT include a summary \u2014 inline comments speak for themselves and a top-level comment clutters the PR conversation on every re-review) +- if no subagent found actionable issues: submit with \`approved: true\` and an **empty body** (no inline comments, no summary) +- do NOT call \`${ghPullfrogMcpName}/report_progress\` \u2014 incremental reviews should be silent Use max effort for thorough reviews.`, Plan: `### Checklist @@ -147982,8 +148011,15 @@ Use auto effort.`, - if the task involved labeling, commenting, or other GitHub operations, perform those directly 5. Use mini effort for simple research tasks, auto for typical tasks, max for complex multi-file changes.` }; -function buildOrchestratorGuidance(mode, overrideGuidance) { - const guidance = overrideGuidance ?? modeGuidance[mode.name] ?? ""; +var modeInstructionParent = { + IncrementalReview: "Review", + Fix: "Build" +}; +function buildOrchestratorGuidance(mode, opts = {}) { + const hardcoded = opts.overrideGuidance ?? modeGuidance[mode.name] ?? mode.prompt ?? ""; + const lookupKey = modeInstructionParent[mode.name] ?? mode.name; + const userInstructions = opts.modeInstructions?.[lookupKey] ?? ""; + const guidance = [hardcoded, userInstructions].filter(Boolean).join("\n\n"); return { modeName: mode.name, description: mode.description, @@ -148029,6 +148065,7 @@ function SelectModeTool(ctx) { }; } ctx.toolState.selectedMode = selectedMode.name; + const guidanceOpts = { modeInstructions: ctx.modeInstructions }; if (selectedMode.name === "Plan") { const issueNumber = params.issue_number ?? ctx.payload.event.issue_number; if (issueNumber !== void 0) { @@ -148037,13 +148074,16 @@ function SelectModeTool(ctx) { ctx.toolState.existingPlanCommentId = existing.commentId; ctx.toolState.previousPlanBody = existing.body; return { - ...buildOrchestratorGuidance(selectedMode, modeGuidance.PlanEdit), + ...buildOrchestratorGuidance(selectedMode, { + ...guidanceOpts, + overrideGuidance: modeGuidance.PlanEdit + }), previousPlanBody: existing.body }; } } } - return buildOrchestratorGuidance(selectedMode); + return buildOrchestratorGuidance(selectedMode, guidanceOpts); }) }); } @@ -148546,7 +148586,11 @@ async function startSubagentMcpServer(params) { const subagentCtx = { ...params.ctx, toolState: subagentToolState }; const tools = buildSubagentTools(subagentCtx); const startResult = await selectMcpPort(subagentCtx, tools); - return { url: startResult.url, stop: () => startResult.server.stop() }; + return { + url: startResult.url, + stop: () => startResult.server.stop(), + toolState: subagentToolState + }; } // modes.ts @@ -151032,7 +151076,6 @@ function buildRuntimeContext(ctx) { "~pullfrog": _, prompt: _p, eventInstructions: _ei, - repoInstructions: _r, event: _e, ...payloadRest } = ctx.payload; @@ -151220,14 +151263,10 @@ var orchestratorPriorityOrder = `## Priority Order In case of conflict between instructions, follow this precedence (highest to lowest): 1. Security rules and system instructions (non-overridable) 2. User prompt -3. Event-level instructions -4. Repo-level instructions`; +3. Event-level instructions`; function buildContextSections(ctx) { const isPr = ctx.payload.event.is_pr === true; const relatedLabel = isPr ? "--- related PR ---" : "--- related issue ---"; - const repoSection = ctx.repo ? `************* REPO-LEVEL INSTRUCTIONS ************* - -${ctx.repo}` : ""; const eventInstructionsSection = ctx.eventInstructions ? `************* EVENT-LEVEL INSTRUCTIONS ************* ${ctx.eventInstructions}` : ""; @@ -151248,7 +151287,7 @@ ${metadataSection}` : `************* EVENT CONTEXT ************* ${titleBodySection} ${metadataSection}`; - return [repoSection, eventInstructionsSection, userSection].filter(Boolean).join("\n\n"); + return [eventInstructionsSection, userSection].filter(Boolean).join("\n\n"); } function buildCommonInputs(ctx) { const eventTitleBody = buildEventTitleBody(ctx.payload.event); @@ -151256,7 +151295,6 @@ function buildCommonInputs(ctx) { const runtime = buildRuntimeContext(ctx); const user = ctx.payload.prompt; const eventInstructions = ctx.payload.eventInstructions ?? ""; - const repo = ctx.payload.repoInstructions ?? ""; const event = [eventTitleBody, eventMetadata].filter(Boolean).join("\n\n---\n\n"); const userQuoted = user ? user.split("\n").map((line) => `> ${line}`).join("\n") : ""; return { @@ -151265,7 +151303,6 @@ function buildCommonInputs(ctx) { runtime, user, eventInstructions, - repo, event, userQuoted }; @@ -151336,7 +151373,6 @@ If the task clearly requires no work, skip delegation. Call \`${ghPullfrogMcpNam }); const contextSections = buildContextSections({ payload: ctx.payload, - repo: inputs.repo, eventInstructions: inputs.eventInstructions, eventTitleBody: inputs.eventTitleBody, eventMetadata: inputs.eventMetadata, @@ -151352,7 +151388,6 @@ If the task clearly requires no work, skip delegation. Call \`${ghPullfrogMcpNam system, user: inputs.user, eventInstructions: inputs.eventInstructions, - repo: inputs.repo, event: inputs.event, runtime: inputs.runtime }; @@ -151435,7 +151470,6 @@ var JsonPayload = type({ prompt: "string", "triggerer?": "string | undefined", "eventInstructions?": "string", - "repoInstructions?": "string", "event?": "object", "effort?": Effort.or("undefined"), "timeout?": "string | undefined", @@ -151530,7 +151564,6 @@ function resolvePayload(resolvedPromptInput, repoSettings) { triggerer: jsonPayload?.triggerer ?? // it's not a common use case but GITHUB_ACTOR can be a user when the workflow is manually triggered by a user through GitHub Actions UI (!isPullfrog(process.env.GITHUB_ACTOR) ? process.env.GITHUB_ACTOR : void 0), eventInstructions: jsonPayload?.eventInstructions, - repoInstructions: jsonPayload?.repoInstructions, event, effort: inputs.effort ?? jsonPayload?.effort ?? "auto", timeout: inputs.timeout ?? jsonPayload?.timeout, @@ -151545,6 +151578,73 @@ function resolvePayload(resolvedPromptInput, repoSettings) { }; } +// utils/reviewCleanup.ts +var RE_REVIEW_PREAMBLE = "Incrementally re-review the new commits on this pull request. Use the IncrementalReview mode."; +async function postReviewCleanup(ctx) { + const review = ctx.toolState.review; + if (!review) return; + delete ctx.toolState.review; + await bestEffort(() => reportReviewNodeId(ctx, review.nodeId), "reportReviewNodeId"); + if (review.reviewedSha) { + await bestEffort( + () => dispatchFollowUpReReview(ctx, review.reviewedSha), + "follow-up re-review dispatch" + ); + } + await bestEffort(() => deleteProgressComment(ctx), "delete progress comment"); +} +async function bestEffort(fn2, label) { + try { + await fn2(); + } catch (error49) { + log.debug(`${label} failed: ${error49}`); + } +} +async function dispatchFollowUpReReview(ctx, reviewedSha) { + const issueNumber = ctx.payload.event.issue_number; + if (!issueNumber) return; + const pr = await ctx.octokit.rest.pulls.get({ + owner: ctx.repo.owner, + repo: ctx.repo.name, + pull_number: issueNumber + }); + if (pr.data.head.sha === reviewedSha) return; + if (pr.data.state !== "open") return; + if (pr.data.draft) return; + log.info( + `safety net: pr HEAD moved from ${reviewedSha.slice(0, 7)} to ${pr.data.head.sha.slice(0, 7)} and agent did not review inline \u2014 dispatching follow-up re-review` + ); + const event = { + trigger: "pull_request_synchronize", + issue_number: issueNumber, + is_pr: true, + title: pr.data.title, + body: null, + branch: pr.data.head.ref, + before_sha: reviewedSha, + silent: true + }; + if (ctx.payload.event.authorPermission) { + event.authorPermission = ctx.payload.event.authorPermission; + } + const payload = { + "~pullfrog": true, + version: ctx.payload.version, + agent: ctx.payload.agent, + prompt: "", + eventInstructions: RE_REVIEW_PREAMBLE, + event, + effort: "max" + }; + await ctx.octokit.rest.actions.createWorkflowDispatch({ + owner: ctx.repo.owner, + repo: ctx.repo.name, + workflow_id: "pullfrog.yml", + ref: pr.data.base.repo.default_branch, + inputs: { prompt: JSON.stringify(payload) } + }); +} + // utils/run.ts async function handleAgentResult(ctx) { if (!ctx.result.success) { @@ -151583,12 +151683,12 @@ var defaultSettings = { modes: [], setupScript: null, postCheckoutScript: null, - repoInstructions: "", web: "enabled", search: "enabled", push: "restricted", shell: "restricted", - prApproveEnabled: false + prApproveEnabled: false, + modeInstructions: {} }; var defaultRunContext = { settings: defaultSettings, @@ -151820,6 +151920,7 @@ async function main() { } const octokit = createOctokit(tokenRef.mcpToken); const runInfo = await resolveRun({ octokit }); + let toolContext; try { var _stack = []; try { @@ -151866,7 +151967,7 @@ async function main() { timer.checkpoint("lifecycleHooks::setup"); const modes2 = [...computeModes(), ...runContext.repoSettings.modes]; const outputSchema = resolveOutputSchema(); - const toolContext = { + toolContext = { repo: runContext.repo, payload, octokit, @@ -151877,6 +151978,7 @@ async function main() { modes: modes2, postCheckoutScript: runContext.repoSettings.postCheckoutScript, prApproveEnabled: runContext.repoSettings.prApproveEnabled, + modeInstructions: runContext.repoSettings.modeInstructions, toolState, runId: runInfo.runId, jobId: runInfo.jobId, @@ -151947,8 +152049,14 @@ ${instructions.user}` : null, "output_schema was provided but agent did not call set_output \u2014 structured output is required" ); } + if (toolContext) { + await postReviewCleanup(toolContext).catch((error49) => { + log.debug(`post-review cleanup failed: ${error49}`); + }); + } await writeJobSummary(toolState); if (toolState.output) { + log.info(`::pullfrog-output::${Buffer.from(toolState.output).toString("base64")}`); core5.setOutput("result", toolState.output); } return await handleAgentResult({ @@ -151974,6 +152082,11 @@ ${instructions.user}` : null, await reportErrorToComment({ toolState, error: errorMessage }); } catch { } + if (toolContext) { + await postReviewCleanup(toolContext).catch((error50) => { + log.debug(`post-review cleanup failed: ${error50}`); + }); + } return { success: false, error: errorMessage diff --git a/external.ts b/external.ts index 3a25734..eda3d82 100644 --- a/external.ts +++ b/external.ts @@ -288,8 +288,6 @@ export interface WriteablePayload { triggerer?: string | undefined; /** event-level instructions for this trigger type (flag-expanded server-side) */ eventInstructions?: string | undefined; - /** repo-level instructions (flag-expanded server-side) */ - repoInstructions?: string | undefined; /** event data from webhook payload - discriminated union based on trigger field */ event: PayloadEvent; /** effort level for model selection (mini, auto, max) - defaults to "auto" */ diff --git a/main.ts b/main.ts index d20eb59..745e3fb 100644 --- a/main.ts +++ b/main.ts @@ -1,7 +1,12 @@ // changes to tool permissions should be reflected in wiki/granular-tools.md import * as core from "@actions/core"; -import { initToolState, startMcpHttpServer, type ToolState } from "./mcp/server.ts"; +import { + initToolState, + startMcpHttpServer, + type ToolContext, + type ToolState, +} from "./mcp/server.ts"; import { computeModes } from "./modes.ts"; import { type ActivityTimeout, @@ -21,6 +26,7 @@ import { resolveInstructions } from "./utils/instructions.ts"; import { executeLifecycleHook } from "./utils/lifecycle.ts"; import { normalizeEnv } from "./utils/normalizeEnv.ts"; import { resolvePayload, resolvePromptInput } from "./utils/payload.ts"; +import { postReviewCleanup } from "./utils/reviewCleanup.ts"; import { handleAgentResult } from "./utils/run.ts"; import { resolveRunContextData } from "./utils/runContextData.ts"; import { createTempDirectory, setupGit } from "./utils/setup.ts"; @@ -112,6 +118,7 @@ export async function main(): Promise { const octokit = createOctokit(tokenRef.mcpToken); const runInfo = await resolveRun({ octokit }); + let toolContext: ToolContext | undefined; try { // enable debug logging if --debug flag was used @@ -173,7 +180,7 @@ export async function main(): Promise { const outputSchema = resolveOutputSchema(); // mcpServerUrl and tmpdir are set after server starts — delegate tool reads them at call time - const toolContext = { + toolContext = { repo: runContext.repo, payload, octokit, @@ -184,6 +191,7 @@ export async function main(): Promise { modes, postCheckoutScript: runContext.repoSettings.postCheckoutScript, prApproveEnabled: runContext.repoSettings.prApproveEnabled, + modeInstructions: runContext.repoSettings.modeInstructions, toolState, runId: runInfo.runId, jobId: runInfo.jobId, @@ -265,9 +273,20 @@ export async function main(): Promise { ); } + // post-agent review cleanup: reportReviewNodeId → follow-up dispatch → delete progress comment. + // runs after the agent exits so ordering is architecturally guaranteed (no LLM involvement). + // best-effort: cleanup failures must not turn a successful agent run into a failure. + if (toolContext) { + await postReviewCleanup(toolContext).catch((error) => { + log.debug(`post-review cleanup failed: ${error}`); + }); + } + await writeJobSummary(toolState); + // emit structured output marker for test validation if (toolState.output) { + log.info(`::pullfrog-output::${Buffer.from(toolState.output).toString("base64")}`); core.setOutput("result", toolState.output); } @@ -291,6 +310,14 @@ export async function main(): Promise { } catch { // error reporting failed, but don't let it mask the original error } + + // best-effort review cleanup (e.g., agent timed out after submitting a review) + if (toolContext) { + await postReviewCleanup(toolContext).catch((error) => { + log.debug(`post-review cleanup failed: ${error}`); + }); + } + return { success: false, error: errorMessage, diff --git a/mcp/checkout.ts b/mcp/checkout.ts index baaaf5b..d06d95e 100644 --- a/mcp/checkout.ts +++ b/mcp/checkout.ts @@ -355,7 +355,6 @@ export function CheckoutPrTool(ctx: ToolContext) { postCheckoutScript: ctx.postCheckoutScript, }); - // fetch PR metadata to return result const pr = await ctx.octokit.rest.pulls.get({ owner: ctx.repo.owner, repo: ctx.repo.name, @@ -367,6 +366,8 @@ export function CheckoutPrTool(ctx: ToolContext) { throw new Error(`PR #${pull_number} source repository was deleted`); } + ctx.toolState.checkoutSha = pr.data.head.sha; + // fetch PR files and format with line numbers const formatResult = await fetchAndFormatPrDiff({ octokit: ctx.octokit, diff --git a/mcp/review.ts b/mcp/review.ts index 98be317..dbbe3b6 100644 --- a/mcp/review.ts +++ b/mcp/review.ts @@ -4,7 +4,6 @@ import { apiFetch } from "../utils/apiFetch.ts"; import { getApiUrl } from "../utils/apiUrl.ts"; import { buildPullfrogFooter } from "../utils/buildPullfrogFooter.ts"; import { log } from "../utils/cli.ts"; -import { deleteProgressComment } from "./comment.ts"; import type { ToolContext } from "./server.ts"; import { execute, tool } from "./shared.ts"; @@ -81,7 +80,6 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) { event = "COMMENT"; } - // compose the request const params: RestEndpointMethodTypes["pulls"]["createReview"]["parameters"] = { owner: ctx.repo.owner, repo: ctx.repo.name, @@ -91,7 +89,6 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) { if (commit_id) { params.commit_id = commit_id; } else { - // get the PR to determine the head commit if commit_id not provided const pr = await ctx.octokit.rest.pulls.get({ owner: ctx.repo.owner, repo: ctx.repo.name, @@ -101,9 +98,7 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) { } if (comments.length > 0) { type ReviewComment = (typeof params.comments & {})[number]; - // convert comments to the format expected by GitHub API params.comments = comments.map((comment) => { - // build comment body with suggestion block if provided let commentBody = comment.body || ""; if (comment.suggestion !== undefined) { const suggestionBlock = "```suggestion\n" + comment.suggestion + "\n```"; @@ -139,12 +134,49 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) { const reviewId = result.data.id; const reviewNodeId = result.data.node_id; - // report review node ID to server so the in-flight dedup check - // in the synchronize webhook handler sees this run as "review submitted." - // awaited (not fire-and-forget) to guarantee the signal lands before - // any subsequent push's webhook checks for in-flight runs. - await reportReviewNodeId(ctx, reviewNodeId); - await deleteProgressComment(ctx); + // 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, + }; + + // detect commits pushed since checkout and guide the agent to review them + // inline instead of dispatching a separate workflow run + const headMovedDuringReview = + ctx.toolState.checkoutSha && params.commit_id !== ctx.toolState.checkoutSha; + + if (headMovedDuringReview) { + const fromSha = ctx.toolState.checkoutSha!; + const toSha = params.commit_id!; + // advance checkoutSha so the next review submission tracks correctly + ctx.toolState.checkoutSha = toSha; + + 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, + newCommits: { + from: fromSha, + to: toSha, + instructions: + `New commits were pushed while you were reviewing. ` + + `Run \`git pull\` to fetch them, then review the incremental diff ` + + `with \`git diff ${fromSha}...HEAD\`. Submit another review covering ` + + `only the new changes. Do not repeat feedback from your previous review.`, + }, + }; + } return { success: true, @@ -202,7 +234,11 @@ async function createAndSubmitWithFooter( }); } -async function reportReviewNodeId(ctx: ToolContext, reviewNodeId: string): Promise { +/** + * report the review node ID to the server so the WorkflowRun is marked as "review submitted". + * exported for use in main.ts post-agent cleanup. + */ +export async function reportReviewNodeId(ctx: ToolContext, reviewNodeId: string): Promise { for (let remaining = 2; remaining >= 0; remaining--) { try { const response = await apiFetch({ diff --git a/mcp/selectMode.ts b/mcp/selectMode.ts index 28ee91b..952d5ba 100644 --- a/mcp/selectMode.ts +++ b/mcp/selectMode.ts @@ -153,9 +153,9 @@ Each task in the \`tasks\` array should include: After all tasks complete, consolidate into a **single** review: - merge the \`comments\` arrays from all subagent outputs -- if subagents found actionable issues: submit one \`${ghPullfrogMcpName}/create_pull_request_review\` with \`approved: false\`, the merged comments, and a unified summary body -- if no subagent found actionable issues: submit with \`approved: true\` and a brief positive summary (no inline comments) -- call \`${ghPullfrogMcpName}/report_progress\` with the summary +- if subagents found actionable issues: submit one \`${ghPullfrogMcpName}/create_pull_request_review\` with \`approved: false\`, the merged comments, and an **empty body** (do NOT include a summary — inline comments speak for themselves and a top-level comment clutters the PR conversation on every re-review) +- if no subagent found actionable issues: submit with \`approved: true\` and an **empty body** (no inline comments, no summary) +- do NOT call \`${ghPullfrogMcpName}/report_progress\` — incremental reviews should be silent Use max effort for thorough reviews.`, @@ -234,8 +234,21 @@ type OrchestratorGuidance = { orchestratorGuidance: string; }; -function buildOrchestratorGuidance(mode: Mode, overrideGuidance?: string): OrchestratorGuidance { - const guidance = overrideGuidance ?? modeGuidance[mode.name] ?? ""; +const modeInstructionParent: Record = { + IncrementalReview: "Review", + Fix: "Build", +}; + +type BuildGuidanceOpts = { + modeInstructions?: Record; + overrideGuidance?: string; +}; + +function buildOrchestratorGuidance(mode: Mode, opts: BuildGuidanceOpts = {}): OrchestratorGuidance { + const hardcoded = opts.overrideGuidance ?? modeGuidance[mode.name] ?? mode.prompt ?? ""; + const lookupKey = modeInstructionParent[mode.name] ?? mode.name; + const userInstructions = opts.modeInstructions?.[lookupKey] ?? ""; + const guidance = [hardcoded, userInstructions].filter(Boolean).join("\n\n"); return { modeName: mode.name, description: mode.description, @@ -295,6 +308,8 @@ export function SelectModeTool(ctx: ToolContext) { ctx.toolState.selectedMode = selectedMode.name; + const guidanceOpts: BuildGuidanceOpts = { modeInstructions: ctx.modeInstructions }; + if (selectedMode.name === "Plan") { const issueNumber = params.issue_number ?? ctx.payload.event.issue_number; if (issueNumber !== undefined) { @@ -303,14 +318,17 @@ export function SelectModeTool(ctx: ToolContext) { ctx.toolState.existingPlanCommentId = existing.commentId; ctx.toolState.previousPlanBody = existing.body; return { - ...buildOrchestratorGuidance(selectedMode, modeGuidance.PlanEdit), + ...buildOrchestratorGuidance(selectedMode, { + ...guidanceOpts, + overrideGuidance: modeGuidance.PlanEdit, + }), previousPlanBody: existing.body, }; } } } - return buildOrchestratorGuidance(selectedMode); + return buildOrchestratorGuidance(selectedMode, guidanceOpts); }), }); } diff --git a/mcp/server.ts b/mcp/server.ts index b2e9926..ee7a34d 100644 --- a/mcp/server.ts +++ b/mcp/server.ts @@ -44,6 +44,8 @@ export interface ToolState { pushDest?: StoredPushDest; // issue or PR number (same number space in GitHub) issueNumber?: number; + // PR HEAD sha at checkout time — used to detect new commits pushed during a review + checkoutSha?: string; selectedMode?: string; // per-subagent lifecycle tracking (keyed by subagent uuid) subagents: Map; @@ -54,6 +56,7 @@ export interface ToolState { review?: { id: number; nodeId: string; + reviewedSha: string | undefined; }; dependencyInstallation?: { status: "not_started" | "in_progress" | "completed" | "failed"; @@ -103,6 +106,7 @@ export interface ToolContext { modes: Mode[]; postCheckoutScript: string | null; prApproveEnabled: boolean; + modeInstructions: Record; toolState: ToolState; runId: number | undefined; jobId: string | undefined; @@ -384,6 +388,7 @@ export async function startMcpHttpServer( export type ManagedMcpServer = { url: string; stop: () => Promise; + toolState: ToolState; }; type StartSubagentMcpServerParams = { @@ -412,5 +417,9 @@ export async function startSubagentMcpServer( const subagentCtx: ToolContext = { ...params.ctx, toolState: subagentToolState }; const tools = buildSubagentTools(subagentCtx); const startResult = await selectMcpPort(subagentCtx, tools); - return { url: startResult.url, stop: () => startResult.server.stop() }; + return { + url: startResult.url, + stop: () => startResult.server.stop(), + toolState: subagentToolState, + }; } diff --git a/mcp/subagent.ts b/mcp/subagent.ts index 2fa0e92..e40978e 100644 --- a/mcp/subagent.ts +++ b/mcp/subagent.ts @@ -114,7 +114,6 @@ export function buildSubagentInstructions( system: subagentSystemPreamble, user: params.instructions, eventInstructions: "", - repo: "", event: "", runtime: "", }; @@ -169,6 +168,10 @@ export async function runSubagent(params: RunSubagentParams): Promise { + const review = ctx.toolState.review; + if (!review) return; + delete ctx.toolState.review; + + // mark review as submitted — unlocks webhook dedup for new pushes + await bestEffort(() => reportReviewNodeId(ctx, review.nodeId), "reportReviewNodeId"); + + // dispatch follow-up if PR HEAD moved past the reviewed commit + if (review.reviewedSha) { + await bestEffort( + () => dispatchFollowUpReReview(ctx, review.reviewedSha!), + "follow-up re-review dispatch" + ); + } + + await bestEffort(() => deleteProgressComment(ctx), "delete progress comment"); +} + +async function bestEffort(fn: () => Promise, label: string): Promise { + try { + await fn(); + } catch (error) { + log.debug(`${label} failed: ${error}`); + } +} + +async function dispatchFollowUpReReview(ctx: ToolContext, reviewedSha: string): Promise { + const issueNumber = ctx.payload.event.issue_number; + if (!issueNumber) return; + + const pr = await ctx.octokit.rest.pulls.get({ + owner: ctx.repo.owner, + repo: ctx.repo.name, + pull_number: issueNumber, + }); + + if (pr.data.head.sha === reviewedSha) return; + if (pr.data.state !== "open") return; + if (pr.data.draft) return; + + log.info( + `safety net: pr HEAD moved from ${reviewedSha.slice(0, 7)} to ${pr.data.head.sha.slice(0, 7)} ` + + `and agent did not review inline — dispatching follow-up re-review` + ); + + const event: WriteablePayload["event"] = { + trigger: "pull_request_synchronize", + issue_number: issueNumber, + is_pr: true, + title: pr.data.title, + body: null, + branch: pr.data.head.ref, + before_sha: reviewedSha, + silent: true, + }; + if (ctx.payload.event.authorPermission) { + event.authorPermission = ctx.payload.event.authorPermission; + } + + const payload: WriteablePayload = { + "~pullfrog": true, + version: ctx.payload.version, + agent: ctx.payload.agent, + prompt: "", + eventInstructions: RE_REVIEW_PREAMBLE, + event, + effort: "max", + }; + + await ctx.octokit.rest.actions.createWorkflowDispatch({ + owner: ctx.repo.owner, + repo: ctx.repo.name, + workflow_id: "pullfrog.yml", + ref: pr.data.base.repo.default_branch, + inputs: { prompt: JSON.stringify(payload) }, + }); +} diff --git a/utils/runContext.ts b/utils/runContext.ts index 3f123f4..1cee2ec 100644 --- a/utils/runContext.ts +++ b/utils/runContext.ts @@ -14,12 +14,12 @@ export interface RepoSettings { modes: Mode[]; setupScript: string | null; postCheckoutScript: string | null; - repoInstructions: string; web: ToolPermission; search: ToolPermission; push: PushPermission; shell: ShellPermission; prApproveEnabled: boolean; + modeInstructions: Record; } export interface RunContext { @@ -32,12 +32,12 @@ const defaultSettings: RepoSettings = { modes: [], setupScript: null, postCheckoutScript: null, - repoInstructions: "", web: "enabled", search: "enabled", push: "restricted", shell: "restricted", prApproveEnabled: false, + modeInstructions: {}, }; const defaultRunContext: RunContext = {