diff --git a/agents/claude.ts b/agents/claude.ts index dd083ea..af8baba 100644 --- a/agents/claude.ts +++ b/agents/claude.ts @@ -810,6 +810,7 @@ export const claude = agent({ stopScript: ctx.stopScript, summaryFilePath: ctx.summaryFilePath, summarySeed: ctx.summarySeed, + getUnsubmittedReview: ctx.getUnsubmittedReview, reflectionPrompt: ctx.learningsFilePath ? buildLearningsReflectionPrompt(ctx.learningsFilePath) : undefined, diff --git a/agents/opencode.ts b/agents/opencode.ts index 9f3eca9..e0bd2e2 100644 --- a/agents/opencode.ts +++ b/agents/opencode.ts @@ -1150,6 +1150,7 @@ export const opencode = agent({ stopScript: ctx.stopScript, summaryFilePath: ctx.summaryFilePath, summarySeed: ctx.summarySeed, + getUnsubmittedReview: ctx.getUnsubmittedReview, reflectionPrompt: ctx.learningsFilePath ? buildLearningsReflectionPrompt(ctx.learningsFilePath) : undefined, diff --git a/agents/postRun.test.ts b/agents/postRun.test.ts index 8e9c813..e63dbf4 100644 --- a/agents/postRun.test.ts +++ b/agents/postRun.test.ts @@ -328,6 +328,138 @@ describe("runPostRunRetryLoop — reflection turn", () => { expect(mockedGetGitStatus).not.toHaveBeenCalled(); }); + it("nudges the agent when Review mode finished without submitting (no report_progress exit offered)", async () => { + // simulate the canonical bug: the agent picked Review mode, talked itself + // out of acting, and stopped without calling create_pull_request_review. + // the gate should fire a single resume that ONLY offers + // create_pull_request_review (Review mode's contract forbids + // report_progress as an exit). once the resume flips the toolState + // (modeled here by the closure returning null on the second call), the + // loop should exit cleanly with success=true. + let submitted = false; + const resume = vi + .fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise>() + .mockImplementation(async () => { + submitted = true; + return successResult({ output: "review submitted" }); + }); + + const result = await runPostRunRetryLoop({ + initialResult: successResult({ output: "analysis turn" }), + initialUsage: undefined, + stopScript: null, + getUnsubmittedReview: () => (submitted ? null : "Review"), + resume, + }); + + expect(result.success).toBe(true); + expect(resume).toHaveBeenCalledTimes(1); + const prompt = resume.mock.calls[0]?.[0].prompt ?? ""; + expect(prompt).toContain("MISSING REVIEW OUTPUT"); + expect(prompt).toContain("Review mode"); + expect(prompt).toContain("create_pull_request_review"); + // Review mode contract: nudge must NOT offer report_progress as an exit + // (would contradict modes.ts step 5 "ALWAYS submit ... do NOT call + // report_progress"). + expect(prompt).not.toContain("report_progress"); + }); + + it("offers `report_progress` in the IncrementalReview nudge for the trivial-skip path", async () => { + // IncrementalReview's mode prompt step 7 has a legitimate "no new + // issues, non-substantive changes only → report_progress" exit. the + // nudge must offer it so the agent can satisfy the gate without + // submitting a vacuous re-review. + let satisfied = false; + const resume = vi + .fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise>() + .mockImplementation(async () => { + satisfied = true; + return successResult({ output: "no review warranted, reported" }); + }); + + const result = await runPostRunRetryLoop({ + initialResult: successResult({ output: "analysis turn" }), + initialUsage: undefined, + stopScript: null, + getUnsubmittedReview: () => (satisfied ? null : "IncrementalReview"), + resume, + }); + + expect(result.success).toBe(true); + const prompt = resume.mock.calls[0]?.[0].prompt ?? ""; + expect(prompt).toContain("IncrementalReview mode"); + expect(prompt).toContain("create_pull_request_review"); + expect(prompt).toContain("report_progress"); + }); + + it("hard-fails IncrementalReview after MAX_POST_RUN_RETRIES with mode-appropriate error copy", async () => { + // parallel to the persistent-stop-hook test: if the agent keeps stopping + // without producing a review, the run must fail loudly with a clear error + // so the user knows something went wrong instead of seeing a deleted + // progress comment and silence. IncrementalReview's error string lists + // both exits the gate accepts. + const resume = vi + .fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise>() + .mockResolvedValue(successResult({ output: "still didn't submit" })); + + const result = await runPostRunRetryLoop({ + initialResult: successResult({ output: "analysis turn" }), + initialUsage: undefined, + stopScript: null, + getUnsubmittedReview: () => "IncrementalReview", + resume, + }); + + expect(result.success).toBe(false); + expect(result.error).toContain("IncrementalReview"); + expect(result.error).toContain("create_pull_request_review"); + expect(result.error).toContain("report_progress"); + expect(result.error).toContain("3 retry attempts"); + expect(resume).toHaveBeenCalledTimes(3); + }); + + it("hard-fails Review with a mode-appropriate error string (no `report_progress`)", async () => { + // Review mode's contract is "always submit a review" (modes.ts step 5). + // the terminal error must mirror the nudge copy: only + // create_pull_request_review is named — listing report_progress would + // contradict the mode prompt and confuse triage. + const resume = vi + .fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise>() + .mockResolvedValue(successResult({ output: "still didn't submit" })); + + const result = await runPostRunRetryLoop({ + initialResult: successResult({ output: "analysis turn" }), + initialUsage: undefined, + stopScript: null, + getUnsubmittedReview: () => "Review", + resume, + }); + + expect(result.success).toBe(false); + expect(result.error).toContain("Review mode"); + expect(result.error).toContain("create_pull_request_review"); + expect(result.error).not.toContain("report_progress"); + }); + + it("does not fire the unsubmitted-review gate when review was submitted", async () => { + // negative case: the closure returns null (review or progress was + // recorded), the gate must stay silent regardless of mode. + const resume = vi + .fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise>() + .mockResolvedValue(successResult()); + + const result = await runPostRunRetryLoop({ + initialResult: successResult({ output: "review submitted inline" }), + initialUsage: undefined, + stopScript: null, + getUnsubmittedReview: () => null, + resume, + }); + + expect(result.success).toBe(true); + expect(resume).not.toHaveBeenCalled(); + }); + it("aggregates usage across every gate retry", async () => { // billing/reporting rely on the usage summary reflecting the full run, // not just the final retry's slice. regression gate. diff --git a/agents/postRun.ts b/agents/postRun.ts index 57f0dc1..f499057 100644 --- a/agents/postRun.ts +++ b/agents/postRun.ts @@ -115,6 +115,32 @@ export function buildSummaryStalePrompt(filePath: string): string { ].join("\n"); } +export function buildUnsubmittedReviewPrompt(mode: "Review" | "IncrementalReview"): string { + // mode-aware: Review mode's contract is "always submit one review" — its + // mode prompt forbids `report_progress`, so the nudge here must not offer + // it as an exit. IncrementalReview legitimately allows a report_progress + // exit when there are no new issues since the last review (mode prompt + // step 7), so the nudge mirrors that contract. + if (mode === "Review") { + return [ + `MISSING REVIEW OUTPUT — you selected Review mode but stopped without calling \`create_pull_request_review\`. the user has no visible signal that this run produced anything; the progress comment will be deleted on exit and no review will appear on the PR.`, + "", + "call `create_pull_request_review` now with your aggregated review (body + inline comments). if you found no actionable issues, submit with `approved: true` and a body opening with `No new issues found.` per the mode prompt — Review mode does not have a no-submit exit. the first call may error once with a diff-coverage nudge — retry the same call to proceed.", + "", + "do NOT stop again until `create_pull_request_review` has been called successfully.", + ].join("\n"); + } + return [ + `MISSING REVIEW OUTPUT — you selected IncrementalReview mode but stopped without calling \`create_pull_request_review\` or \`report_progress\`. the user has no visible signal that this run produced anything; the progress comment will be deleted on exit and no review will appear on the PR.`, + "", + "do exactly one of:", + "- if you have findings: call `create_pull_request_review` now with your aggregated review (body + inline comments). the first call may error once with a diff-coverage nudge — retry the same call to proceed.", + "- if there are genuinely no actionable findings since the last review (e.g. only formatting / comment / lockfile changes): call `report_progress` with a 1-2 sentence summary explaining that no review was warranted.", + "", + "do NOT stop again until one of those tools has been called successfully.", + ].join("\n"); +} + /** * check the post-run gates: did the stop hook pass, is the working tree * clean, and (when applicable) did the agent touch the rolling PR summary @@ -132,6 +158,7 @@ export async function collectPostRunIssues(params: { stopScript: string | null | undefined; summaryFilePath?: string | undefined; summarySeed?: string | undefined; + getUnsubmittedReview?: (() => "Review" | "IncrementalReview" | null) | undefined; }): Promise { const issues: PostRunIssues = {}; if (params.stopScript) { @@ -144,12 +171,24 @@ export async function collectPostRunIssues(params: { const stale = await isSummaryUnchanged(params.summaryFilePath, params.summarySeed); if (stale) issues.summaryStale = { filePath: params.summaryFilePath }; } + if (params.getUnsubmittedReview) { + const mode = params.getUnsubmittedReview(); + if (mode) issues.unsubmittedReview = mode; + } return issues; } export function buildPostRunPrompt(issues: PostRunIssues): string { + // order matches the terminal hard-fail order in `runPostRunRetryLoop` so + // the prompt's emphasis (which gate the agent should fix first) lines up + // with the user-visible failure message reported when retries exhaust. + // both hard-fail gates first (`stopHook` → `unsubmittedReview`), then the + // soft gates (`dirtyTree` → `summaryStale`). const parts: string[] = []; if (issues.stopHook) parts.push(buildStopHookPrompt(issues.stopHook)); + if (issues.unsubmittedReview) { + parts.push(buildUnsubmittedReviewPrompt(issues.unsubmittedReview)); + } if (issues.dirtyTree) parts.push(buildCommitPrompt(issues.dirtyTree)); if (issues.summaryStale) parts.push(buildSummaryStalePrompt(issues.summaryStale.filePath)); return parts.join("\n\n---\n\n"); @@ -214,6 +253,8 @@ export async function runPostRunRetryLoop(params: { summaryFilePath?: string | undefined; /** exact bytes of the seeded summary file used for the unchanged-check. */ summarySeed?: string | undefined; + /** see {@link AgentRunContext.getUnsubmittedReview}. */ + getUnsubmittedReview?: (() => "Review" | "IncrementalReview" | null) | undefined; resume: (context: { prompt: string; previousResult: R }) => Promise; canResume?: ((result: R) => boolean) | undefined; reflectionPrompt?: string | undefined; @@ -236,6 +277,7 @@ export async function runPostRunRetryLoop(params: { stopScript: params.stopScript, summaryFilePath: summaryStaleNudged ? undefined : params.summaryFilePath, summarySeed: summaryStaleNudged ? undefined : params.summarySeed, + getUnsubmittedReview: params.getUnsubmittedReview, }); if (issues.summaryStale) summaryStaleNudged = true; finalIssues = issues; @@ -321,10 +363,14 @@ export async function runPostRunRetryLoop(params: { // false-positive failures right after it just passed. if (gateResumeCount > 0 && result.success && hasPostRunIssues(finalIssues)) { // re-check the gates that can actually fail the run (stop hook / - // dirty tree). summary-stale is intentionally NOT re-checked here: - // we already delivered the one-shot nudge, and a still-unchanged - // file at this point is the agent's deliberate choice. - finalIssues = await collectPostRunIssues({ stopScript: params.stopScript }); + // dirty tree / unsubmitted review). summary-stale is intentionally + // NOT re-checked here: we already delivered the one-shot nudge, and + // a still-unchanged file at this point is the agent's deliberate + // choice. + finalIssues = await collectPostRunIssues({ + stopScript: params.stopScript, + getUnsubmittedReview: params.getUnsubmittedReview, + }); } if (result.success && finalIssues.stopHook) { @@ -340,5 +386,25 @@ export async function runPostRunRetryLoop(params: { }; } + if (result.success && finalIssues.unsubmittedReview) { + const retryNote = + gateResumeCount > 0 + ? ` after ${gateResumeCount} retry ${gateResumeCount === 1 ? "attempt" : "attempts"}` + : ""; + // mode-aware: Review's contract requires a review submission; only + // IncrementalReview accepts `report_progress` as an exit. mirroring + // the nudge prompt avoids contradicting the agent-facing copy. + const expected = + finalIssues.unsubmittedReview === "Review" + ? "create_pull_request_review" + : "create_pull_request_review or report_progress"; + return { + ...result, + success: false, + error: `${finalIssues.unsubmittedReview} mode finished without calling ${expected}${retryNote}`, + usage: aggregatedUsage, + }; + } + return { ...result, usage: aggregatedUsage }; } diff --git a/agents/shared.ts b/agents/shared.ts index 66be562..8435ace 100644 --- a/agents/shared.ts +++ b/agents/shared.ts @@ -54,13 +54,24 @@ export interface PostRunIssues { * seed, i.e. the agent never touched it. soft gate — nudges once via a * resume turn but never fails the run, parallel to dirtyTree semantics. */ summaryStale?: SummaryStale; + /** + * populated when the agent selected a review mode but the post-run check + * over toolState shows neither a `create_pull_request_review` submission + * nor a final `report_progress` write happened. derived inline from + * `toolState.selectedMode` + `toolState.review` + `toolState.finalSummaryWritten` + * — no parallel toolState flag is stored. carries the mode name so the + * resume prompt can reference it. handled like `stopHook`: nudge via + * resume, hard-fail if still unsatisfied after `MAX_POST_RUN_RETRIES`. + */ + unsubmittedReview?: "Review" | "IncrementalReview"; } export function hasPostRunIssues(issues: PostRunIssues): boolean { return ( issues.stopHook !== undefined || issues.dirtyTree !== undefined || - issues.summaryStale !== undefined + issues.summaryStale !== undefined || + issues.unsubmittedReview !== undefined ); } @@ -147,6 +158,15 @@ export interface AgentRunContext { */ onActivityTimeout?: (() => void) | undefined; onToolUse?: ((event: AgentToolUseEvent) => void) | undefined; + /** + * post-run check derived from toolState: returns the selected mode when + * the agent picked Review / IncrementalReview but neither submitted a + * review nor wrote a final progress comment, otherwise `null`. main.ts + * supplies the closure so the agent harness has no direct toolState + * dependency; the closure fires synchronously after each agent attempt + * so it sees the latest mutations from any MCP tool calls. + */ + getUnsubmittedReview?: (() => "Review" | "IncrementalReview" | null) | undefined; } export interface Agent { diff --git a/main.ts b/main.ts index 6bd587c..2264cc3 100644 --- a/main.ts +++ b/main.ts @@ -927,6 +927,19 @@ export async function main(): Promise { summaryFilePath: toolState.summaryFilePath, summarySeed: toolState.summarySeed, learningsFilePath: toolState.learningsFilePath, + // post-run gate: derive "review mode finished without producing + // anything visible" inline from toolState. no parallel toolState flag — + // the absence of `review` and `finalSummaryWritten` is the signal. + // skipped when there's no progress comment to anchor the failure to + // (e.g. silent runs / non-issue events) so the gate doesn't fire + // on runs where there's nothing to display anyway. + getUnsubmittedReview: () => { + const mode = toolState.selectedMode; + if (mode !== "Review" && mode !== "IncrementalReview") return null; + if (toolState.review || toolState.finalSummaryWritten) return null; + if (!toolState.hadProgressComment) return null; + return mode; + }, onActivityTimeout: onInnerActivityTimeout, onToolUse: (event) => { const wasTracked = recordDiffReadFromToolUse({ @@ -1021,6 +1034,22 @@ export async function main(): Promise { await persistLearnings(toolContext); } + // when the agent harness returns success=false (e.g. unsubmitted-review + // gate exhausted retries, stop-hook persistently failing), surface the + // error in the progress comment so the user sees it instead of a + // deleted-comment void. mirrors the catch-block error reporting for + // thrown errors. runs before the stranded-comment cleanup below so + // the comment is still around to update; reportErrorToComment sets + // wasUpdated=true and the !result.success guard skips deletion. + if (!result.success && toolContext && toolState.progressComment) { + await reportErrorToComment({ + toolState, + error: result.error || "agent run failed", + }).catch((error) => { + log.debug(`failure error report failed: ${error}`); + }); + } + // clean up stranded progress comments. the comment is stale unless // report_progress wrote a final summary to it — three sub-cases all reduce // to !finalSummaryWritten: @@ -1034,14 +1063,34 @@ export async function main(): Promise { // so progressComment is already null by the time we get here for that path. // uses finalSummaryWritten (not todoTracker.enabled or wasUpdated) so cleanup // survives API failures in report_progress where cancel() ran but the write - // didn't succeed, and isn't fooled by writes to *other* artifacts. - if (toolContext && toolState.progressComment && !toolState.finalSummaryWritten) { + // didn't succeed, and isn't fooled by writes to *other* artifacts. skipped + // entirely on result.success===false: the error message just written above + // is the user's only signal that the run happened — deleting it would + // restore the same empty-void UX this commit fixes. + if ( + toolContext && + result.success && + toolState.progressComment && + !toolState.finalSummaryWritten + ) { await deleteProgressComment(toolContext).catch((error) => { log.debug(`stranded progress comment cleanup failed: ${error}`); }); } - await writeJobSummary(toolState); + // best-effort: failures writing the actions step summary must not throw + // past this point. on the result.success===false branch above we already + // wrote `result.error` to the progress comment, and a throw here would + // jump to the outer catch which calls reportErrorToComment again with + // the (less actionable) writeJobSummary error — silently overwriting the + // gate's failure message in the progress comment. the step-summary write + // is informational; let it fail silently rather than corrupt user-facing + // output. + try { + await writeJobSummary(toolState); + } catch (error) { + log.debug(`job summary write failed: ${error}`); + } // emit structured output marker for test validation if (toolState.output) { diff --git a/mcp/server.ts b/mcp/server.ts index a1005c1..32dce5f 100644 --- a/mcp/server.ts +++ b/mcp/server.ts @@ -68,6 +68,28 @@ export type StoredPushDest = { localBranch: string; }; +/** + * mutable per-run record of facts that occurred during execution. shared + * between the action process and the MCP server (one process — toolState is + * just a JS object passed by reference into both surfaces). + * + * design rule: ToolState is LITERAL. each field records a thing that + * happened — `review` is set when `create_pull_request_review` succeeded, + * `finalSummaryWritten` flips when `report_progress` wrote a non-plan body, + * `selectedMode` is set when `select_mode` was called. fields should never + * encode the absence of an event ("unsubmittedReview", "missingArtifact"), + * speculative state, or values derived from other fields. + * + * any predicate the rest of the code needs ("the agent picked review mode but + * never produced a review or progress write") is computed inline at the call + * site, not stored. derived state in this struct invariably drifts from the + * literal fields under refactors and is the wrong layer for the check. + * + * write narrowly: prefer adding state inside the tool that mutates it (e.g. + * `create_pull_request_review` populates `toolState.review`) and reading + * narrowly elsewhere. don't introduce flags from main.ts that mirror what an + * MCP tool already records. + */ export interface ToolState { // where we're allowed to push - base repo initially, fork URL for fork PRs // set by setupGit, updated by checkout_pr. always set before push validation. diff --git a/modes.ts b/modes.ts index b70b26c..e74d860 100644 --- a/modes.ts +++ b/modes.ts @@ -304,9 +304,9 @@ ${PR_SUMMARY_FORMAT}`, 6. **build the review body** — a single "Reviewed changes" section: summarize at the logical-change level, not per-file. each bullet starts with a past-tense verb (e.g. \`- Extracted shared CLI runtime into a single module\`, \`- Renamed package to pullfrog\`). avoid file paths unless they add clarity. if the changes can be described in one sentence, use one sentence — no bullets needed. do NOT include a separate "Prior review feedback" checklist; that's tracked in the rolling PR summary snapshot for the next agent run, and surfacing it in the user-facing body is noise (changes that addressed prior feedback are already covered by the Reviewed-changes bullets). in some cases you may receive a complete diff for the whole pull request instead of an incremental one — when this happens, you will need to determine what changes have happened since Pullfrog's most recent review. -7. Submit — Do NOT call \`report_progress\` or \`create_issue_comment\` — the review is the final record and the progress comment will be cleaned up automatically. Follow these rules: +7. Submit — every run must end with EXACTLY ONE of \`${t("create_pull_request_review")}\` (substantive review) or \`${t("report_progress")}\` (no-review acknowledgement). do NOT call \`create_issue_comment\` for review output. Follow these rules: - note: the first create_pull_request_review submission may error with a one-time diff-coverage nudge listing unread TOC regions. retry the same call to proceed — optionally after reading the listed ranges. the pre-flight will not block again this session. - - IF NO NEW ISSUES, NON-SUBSTANTIVE CHANGES ONLY (trivial formatting, import reordering, comment tweaks): do NOT submit a review. Do NOT call \`report_progress\`. Exit — the progress comment will be cleaned up automatically. + - IF NO NEW ISSUES, NON-SUBSTANTIVE CHANGES ONLY (trivial formatting, import reordering, comment tweaks): do NOT submit a review. Instead call \`${t("report_progress")}\` with a 1-2 sentence note explaining no review was warranted (e.g. "No new issues. Changes since last review are formatting-only."). this leaves a visible signal that the run completed. - ELSE IF NEW CRITICAL ISSUES (blocks merge): call \`${t("create_pull_request_review")}\` with \`approved: false\`, all comments, and the review body. body opens with a GitHub alert blockquote (e.g. \`> [!CAUTION]\\n> This PR introduces ...\`), then the Reviewed-changes summary. - ELSE IF NEW RECOMMENDED CHANGES (non-critical): call \`${t("create_pull_request_review")}\` with \`approved: false\`, all comments, and the review body. body opens with \`> [!IMPORTANT]\\n> ...\` alert, then the Reviewed-changes summary. - ELSE IF NO NEW ISSUES, SUBSTANTIVE CHANGES (new functionality, behavior changes, or fixes to prior review feedback): call \`${t("create_pull_request_review")}\` to create a PR review. If all previous reviews have been properly addressed and no new issues were discovered, you can set \`approved: true\`. body opens with \`No new issues. Reviewed the following changes:\\n\`, then the Reviewed-changes summary.`,