diff --git a/mcp/review.test.ts b/mcp/review.test.ts index c70b41d..a83010b 100644 --- a/mcp/review.test.ts +++ b/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"); + }); +}); diff --git a/mcp/review.ts b/mcp/review.ts index 6eb56a5..63021cf 100644 --- a/mcp/review.ts +++ b/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.