post-run gate: fail review-mode runs that don't submit a review or progress (#638)

* post-run gate: fail the run when review mode finishes without a review or progress review-mode runs that ended in a text-only assistant turn ("now I have enough to draft the review...") were silently swallowed: the progress comment was deleted by stranded-comment cleanup and no review appeared on the PR. user- visible result was identical to "the agent never ran." caught in https://github.com/pullfrog/app/actions/runs/25583698781. new post-run gate alongside stopHook / dirtyTree / summaryStale: derived inline from toolState (selectedMode in {Review, IncrementalReview} && !review && !finalSummaryWritten && hadProgressComment) — no parallel toolState flag. when it fires, the resume prompt nudges the agent to call either create_pull_request_review or report_progress; persistent failure after MAX_POST_RUN_RETRIES surfaces as AgentResult.error. also: when the post-run loop returns success=false, write the error to the progress comment before the stranded-comment cleanup runs, and skip the delete in that case. previously a !success run from the loop would lose the error message into the void. IncrementalReview's trivial-skip branch now calls report_progress with a brief "no review warranted" note instead of exiting silently — keeps the contract symmetric with the gate and gives the user a visible signal even on no-op review runs. documents the literal-record design rule on the ToolState interface so future fields don't drift back into derived/absence-encoding state. * review feedback: mode-aware nudge, gate-error preservation, prompt order addresses three findings from the auto-review on this PR: 1. Review mode nudge no longer offers `report_progress` as an exit. Review mode's contract (modes.ts step 5) forbids it; the gate previously sent contradictory copy. IncrementalReview's nudge still offers both since its trivial-skip path legitimately allows `report_progress`. 2. `writeJobSummary` is now wrapped in try/catch on the success-path cleanup. without this, a throw there jumped to the outer catch and overwrote the gate's failure message in the progress comment with the (less actionable) writeJobSummary error — restoring exactly the invisible-failure UX this PR fixes. step-summary writes are informational; let them fail silently. 3. `buildPostRunPrompt` reorders gates to match the terminal hard-fail order: `stopHook` → `unsubmittedReview` → `dirtyTree` → `summaryStale`. when both hard-fail gates co-fire (rare in review modes), the prompt's emphasis now matches the user-visible failure message. new test asserts the IncrementalReview nudge offers both exits while the Review nudge offers only `create_pull_request_review`. e2e validation already passed against pullfrog/preview-638-review-stop-hook PR #1 (gate fired once; agent recovered on second turn). * mode-aware terminal error copy second auto-review caught a residual contradiction: the terminal hard-fail error string reported "create_pull_request_review or report_progress" for both modes, even though the new mode-aware nudge tells Review-mode agents "Review mode does not have a no-submit exit". the error message now mirrors the nudge — Review names only `create_pull_request_review`, IncrementalReview lists both. additional Review-mode hard-fail test asserts the absence of `report_progress` in the error.
2026-05-09 00:14:31 +00:00
parent 653fae47a5
commit 85d25a6fe6
8 changed files with 301 additions and 10 deletions
@@ -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,
@@ -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,
@@ -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<AgentResult>>()
+      .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<AgentResult>>()
+      .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<AgentResult>>()
+      .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<AgentResult>>()
+      .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<AgentResult>>()
+      .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.
@@ -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<PostRunIssues> {
  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<R extends AgentResult>(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<R>;
  canResume?: ((result: R) => boolean) | undefined;
  reflectionPrompt?: string | undefined;
@@ -236,6 +277,7 @@ export async function runPostRunRetryLoop<R extends AgentResult>(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<R extends AgentResult>(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<R extends AgentResult>(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 };
 }
@@ -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 {