guard against duplicate create_pull_request_review calls in the same session (#553)

the agent occasionally submits twice in one Review-mode run — once with
substantive feedback, then again with the canonical "Reviewed — no issues
found." body when the prompt's branch logic re-classifies non-blocking
observations as "no actionable issues" (see colinhacks/zod#5897). the
second submission is always redundant noise on the PR.

duplicateReviewDecision short-circuits the second call when toolState.review
is already populated for the current checkout sha. legitimate follow-up
reviews after new commits still go through because the new-commits-mid-review
path advances toolState.checkoutSha past the prior reviewedSha before
returning, so the next call sees a different sha and is allowed.

mcp/review.test.ts

@@ -6,6 +6,7 @@ import {
   commentableLinesForFile,
   createReviewWithStrandedRecovery,
   type DroppedComment,
+  duplicateReviewDecision,
   formatDroppedCommentsNote,
   MAX_DROPPED_COMMENT_LINES,
   type ReviewCommentInput,
@@ -643,3 +644,66 @@ describe("reviewSkipDecision", () => {
     expect(decision).toBeNull();
   });
 });
+
+describe("duplicateReviewDecision", () => {
+  // regression: colinhacks/zod#5897 had two reviews submitted from the same
+  // workflow run 8 seconds apart — a substantive review followed by an empty
+  // "Reviewed — no issues found." follow-up. the agent re-classified the
+  // first review's non-blocking observations as "no actionable issues" and
+  // submitted the canonical body per modes.ts. this guard makes the second
+  // call a no-op without burning a GitHub API call or polluting the PR.
+
+  it("allows the first submission when no prior review exists", () => {
+    const decision = duplicateReviewDecision({
+      existing: undefined,
+      currentCheckoutSha: "sha1",
+    });
+    expect(decision).toBeNull();
+  });
+
+  it("blocks a second submission when checkoutSha matches the prior reviewedSha", () => {
+    // exact reproduction of the zod#5897 shape: same session, same checked-out
+    // SHA, second create_pull_request_review call.
+    const decision = duplicateReviewDecision({
+      existing: { id: 100, reviewedSha: "sha1" },
+      currentCheckoutSha: "sha1",
+    });
+    expect(decision?.kind).toBe("already-submitted");
+    expect(decision?.reviewId).toBe(100);
+    expect(decision?.reason).toContain("already submitted");
+    expect(decision?.reason).toContain("checkout_pr");
+  });
+
+  it("allows a follow-up when checkoutSha advanced past the prior reviewedSha", () => {
+    // the new-commits-mid-review path advances toolState.checkoutSha to the
+    // new HEAD before returning, and the agent is told to call checkout_pr
+    // again — both paths leave checkoutSha != reviewedSha. those are real
+    // follow-up reviews and must go through.
+    const decision = duplicateReviewDecision({
+      existing: { id: 100, reviewedSha: "sha-old" },
+      currentCheckoutSha: "sha-new",
+    });
+    expect(decision).toBeNull();
+  });
+
+  it("blocks when checkoutSha is missing — cannot prove the SHA moved", () => {
+    // if the agent never called checkout_pr, we have no anchor to compare
+    // against. assume duplicate rather than letting a second review through
+    // — the prior review still satisfies the agent's intent.
+    const decision = duplicateReviewDecision({
+      existing: { id: 100, reviewedSha: "sha1" },
+      currentCheckoutSha: undefined,
+    });
+    expect(decision?.kind).toBe("already-submitted");
+  });
+
+  it("blocks when prior reviewedSha is missing — cannot prove the SHA moved", () => {
+    // belt-and-suspenders: if for any reason the prior review didn't capture
+    // a reviewedSha, treat the second call as a duplicate to be safe.
+    const decision = duplicateReviewDecision({
+      existing: { id: 100, reviewedSha: undefined },
+      currentCheckoutSha: "sha1",
+    });
+    expect(decision?.kind).toBe("already-submitted");
+  });
+});

mcp/review.ts

@@ -170,6 +170,58 @@ export type ReviewSkipDecision =
   | { kind: "no-issues"; reason: string }
   | { kind: "empty-downgraded-approve"; reason: string };
 
+/**
+ * decision returned by duplicateReviewDecision when a session has already
+ * submitted a review and the current call would be a duplicate.
+ */
+export type DuplicateReviewDecision = {
+  kind: "already-submitted";
+  reviewId: number;
+  reason: string;
+};
+
+/**
+ * decide whether a second create_pull_request_review call in the same session
+ * is a duplicate of an earlier submission.
+ *
+ * the agent is instructed to call create_pull_request_review exactly once per
+ * Review-mode session (see action/modes.ts), but in practice it sometimes
+ * submits twice — once with substantive feedback, then again with the
+ * canonical "Reviewed — no issues found." body when the prompt's branch
+ * logic re-classifies non-blocking observations. the second submission is
+ * always redundant: the first review is the record, and the duplicate just
+ * adds noise to the PR.
+ *
+ * legitimate follow-up reviews after new commits ARE allowed: the
+ * new-commits-mid-review path advances toolState.checkoutSha past the
+ * previously reviewed sha, and a subsequent checkout_pr advances it again.
+ * any call where checkoutSha has moved past the prior reviewedSha is a real
+ * follow-up and goes through. anything else — same sha, or no checkoutSha
+ * to compare against — is a duplicate.
+ */
+export function duplicateReviewDecision(params: {
+  existing: { id: number; reviewedSha: string | undefined } | undefined;
+  currentCheckoutSha: string | undefined;
+}): DuplicateReviewDecision | null {
+  const existing = params.existing;
+  if (!existing) return null;
+  // checkoutSha advanced past the prior reviewed sha — legitimate follow-up
+  // (e.g. after checkout_pr re-fetched new commits the agent was nudged to
+  // pull). only treat as a duplicate when we cannot prove the SHA moved.
+  if (
+    params.currentCheckoutSha &&
+    existing.reviewedSha &&
+    params.currentCheckoutSha !== existing.reviewedSha
+  ) {
+    return null;
+  }
+  return {
+    kind: "already-submitted",
+    reviewId: existing.id,
+    reason: `review ${existing.id} was already submitted in this session; ignoring duplicate call (call \`checkout_pr\` again first if new commits were pushed)`,
+  };
+}
+
 /**
  * decide whether to skip a review submission before any network call.
  *
@@ -300,6 +352,26 @@ export function CreatePullRequestReviewTool(ctx: ToolContext) {
       // set issue context (PRs are issues)
       ctx.toolState.issueNumber = pull_number;
 
+      // guard against duplicate review submissions in the same session.
+      // see duplicateReviewDecision for the rationale — short version: the
+      // agent occasionally submits twice (substantive review + canonical
+      // "no issues found" follow-up) and the second is always redundant.
+      // legit re-reviews after new commits are still allowed because
+      // checkout_pr advances toolState.checkoutSha past the prior reviewedSha.
+      const dup = duplicateReviewDecision({
+        existing: ctx.toolState.review,
+        currentCheckoutSha: ctx.toolState.checkoutSha,
+      });
+      if (dup) {
+        log.info(`skipping duplicate review submission: ${dup.reason}`);
+        return {
+          success: true,
+          skipped: true,
+          reason: dup.reason,
+          reviewId: dup.reviewId,
+        };
+      }
+
       // skip empty COMMENT reviews before any GitHub call. see reviewSkipDecision
       // for the cases (no-issues vs empty-downgraded-approve) and why GitHub 422s
       // the shape we'd otherwise POST.