From d857e0673195a5eab29f70bca3b8512d25f3541c Mon Sep 17 00:00:00 2001
From: Colin McDonnell <colinmcd94@gmail.com>
Date: Thu, 14 May 2026 00:01:15 +0000
Subject: [PATCH] postrun: tighten unsubmitted-review gate to require
 create_pull_request_review for Review mode (#724)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The gate at `getUnsubmittedReview` accepted `toolState.finalSummaryWritten`
as a valid Review exit, contradicting the post-failure error message which
already says Review's only valid exit is `create_pull_request_review`.
This let any caller that flipped `finalSummaryWritten` — including a
`task`-dispatched `reviewfrog` subagent calling `pullfrog_report_progress`
in violation of its prose-only read-only contract — silence the gate even
when the orchestrator never submitted a review.

Split per-mode: Review requires `toolState.review`, IncrementalReview keeps
the existing `||` (its post-failure message explicitly accepts
`report_progress` as a "no review warranted" exit). Test split mirrors the
new semantics.

closes #648
---
 agents/postRun.test.ts | 15 ++++++++++++++-
 agents/postRun.ts      | 19 ++++++++++++++++---
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/agents/postRun.test.ts b/agents/postRun.test.ts
index 1f271d1..d05a10f 100644
--- a/agents/postRun.test.ts
+++ b/agents/postRun.test.ts
@@ -29,9 +29,22 @@ describe("getUnsubmittedReview", () => {
     ).toBeNull();
   });
 
-  it("returns null when report_progress wrote a final summary", () => {
+  it("fires for Review even when report_progress wrote a final summary", () => {
+    // Review's only valid exit is `create_pull_request_review`. a summary
+    // comment is not a substitute, and accepting it here previously let
+    // subagent-flipped `finalSummaryWritten` silence the gate.
     expect(
       getUnsubmittedReview(makeToolState({ selectedMode: "Review", finalSummaryWritten: true }))
+    ).toBe("Review");
+  });
+
+  it("returns null for IncrementalReview when report_progress wrote a final summary", () => {
+    // IncrementalReview treats `report_progress` as a legitimate
+    // "no review warranted" exit, matching the post-failure error message.
+    expect(
+      getUnsubmittedReview(
+        makeToolState({ selectedMode: "IncrementalReview", finalSummaryWritten: true })
+      )
     ).toBeNull();
   });
 
diff --git a/agents/postRun.ts b/agents/postRun.ts
index 2ffb74c..cd7b035 100644
--- a/agents/postRun.ts
+++ b/agents/postRun.ts
@@ -30,13 +30,26 @@ import {
  *
  * the gate is anchored to `hadProgressComment` so silent runs (non-issue
  * events, dispatcher skipped seeding) don't fire a nudge there's no UI for.
+ *
+ * `Review` and `IncrementalReview` have different valid exits:
+ *   - Review: only `create_pull_request_review` counts. `report_progress` is
+ *     not a substitute — a Review run that exits with just a summary comment
+ *     has produced nothing reviewable on the PR. matches the hard-fail
+ *     message at `expected = "create_pull_request_review"` below.
+ *   - IncrementalReview: `report_progress` is a legitimate "no review
+ *     warranted" exit, so either toolState flag short-circuits.
+ * splitting per mode also closes the bypass where a subagent (e.g. a
+ * `task`-dispatched `reviewfrog` lens) calls `report_progress` and silences
+ * the gate even though the orchestrator never submitted a review.
  */
 export function getUnsubmittedReview(toolState: ToolState): "Review" | "IncrementalReview" | null {
   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;
+  if (mode === "Review") return toolState.review ? null : "Review";
+  if (mode === "IncrementalReview") {
+    return toolState.review || toolState.finalSummaryWritten ? null : "IncrementalReview";
+  }
+  return null;
 }
 
 /**