shockbot/mcp/review.test.ts

import { describe, expect, it, vi } from "vitest";
import {
  buildCommentableMap,
  type CommentableLines,
  clearStrandedPendingReview,
  commentableLinesForFile,
  createReviewWithStrandedRecovery,
  type DroppedComment,
  duplicateReviewDecision,
  formatDroppedCommentsNote,
  MAX_DROPPED_COMMENT_LINES,
  type ReviewCommentInput,
  reviewSkipDecision,
  validateInlineComments,
} from "./review.ts";
import type { ToolContext } from "./server.ts";

describe("commentableLinesForFile", () => {
  it("returns empty sets for missing patches (binary or no changes)", () => {
    const result = commentableLinesForFile(undefined);
    expect(result.LEFT.size).toBe(0);
    expect(result.RIGHT.size).toBe(0);
  });

  it("collects added lines on RIGHT, removed lines on LEFT, context on both", () => {
    const patch = ["@@ -10,3 +10,4 @@", " ctx1", "-old", "+new", "+new2", " ctx2"].join("\n");
    const { LEFT, RIGHT } = commentableLinesForFile(patch);
    expect([...LEFT].sort((a, b) => a - b)).toEqual([10, 11, 12]);
    expect([...RIGHT].sort((a, b) => a - b)).toEqual([10, 11, 12, 13]);
  });

  it("handles multiple hunks", () => {
    const patch = ["@@ -1,2 +1,2 @@", " a", "-b", "+B", "@@ -20,1 +20,2 @@", " x", "+y"].join("\n");
    const { LEFT, RIGHT } = commentableLinesForFile(patch);
    expect(RIGHT.has(2)).toBe(true); // +B
    expect(RIGHT.has(21)).toBe(true); // +y
    expect(LEFT.has(2)).toBe(true); // -b
    expect(LEFT.has(20)).toBe(true); // context x
    expect(RIGHT.has(20)).toBe(true); // context x
  });

  it("ignores the 'no newline at end of file' marker", () => {
    const patch = ["@@ -1,1 +1,1 @@", "-old", "\\ No newline at end of file", "+new"].join("\n");
    const { LEFT, RIGHT } = commentableLinesForFile(patch);
    expect(LEFT.has(1)).toBe(true);
    expect(RIGHT.has(1)).toBe(true);
    expect(LEFT.size).toBe(1);
    expect(RIGHT.size).toBe(1);
  });

  it("parses hunk headers without explicit counts", () => {
    // single-line hunks can omit ",<count>"
    const patch = ["@@ -5 +5 @@", "-old", "+new"].join("\n");
    const { LEFT, RIGHT } = commentableLinesForFile(patch);
    expect(LEFT.has(5)).toBe(true);
    expect(RIGHT.has(5)).toBe(true);
  });
});

function buildMap(entries: Array<[string, string]>): Map<string, CommentableLines> {
  const map = new Map<string, CommentableLines>();
  for (const [file, patch] of entries) {
    map.set(file, commentableLinesForFile(patch));
  }
  return map;
}

describe("validateInlineComments", () => {
  const patch = ["@@ -10,2 +10,3 @@", " ctx", "-old", "+new", "+new2"].join("\n");
  const diffMap = buildMap([["src/foo.ts", patch]]);

  const base = (overrides: Partial<ReviewCommentInput>): ReviewCommentInput => ({
    path: "src/foo.ts",
    line: 11,
    side: "RIGHT",
    body: "LGTM",
    ...overrides,
  });

  it("keeps comments anchored to added lines on RIGHT", () => {
    const result = validateInlineComments([base({ line: 12 })], diffMap);
    expect(result.valid).toHaveLength(1);
    expect(result.dropped).toHaveLength(0);
  });

  it("keeps comments anchored to removed lines on LEFT", () => {
    const result = validateInlineComments([base({ line: 11, side: "LEFT" })], diffMap);
    expect(result.valid).toHaveLength(1);
    expect(result.dropped).toHaveLength(0);
  });

  it("drops comments on files not in the diff", () => {
    const result = validateInlineComments([base({ path: "other/bar.ts" })], diffMap);
    expect(result.valid).toHaveLength(0);
    expect(result.dropped).toHaveLength(1);
    expect(result.dropped[0].reason).toContain("file not in PR diff");
  });

  it("distinguishes binary/no-patch files from files with hunks", () => {
    // file present in the PR but with no patch data (binary file).
    const binaryMap = buildMap([
      ["src/foo.ts", patch],
      ["assets/logo.png", undefined as unknown as string],
    ]);
    const result = validateInlineComments([base({ path: "assets/logo.png", line: 1 })], binaryMap);
    expect(result.valid).toHaveLength(0);
    expect(result.dropped).toHaveLength(1);
    expect(result.dropped[0].reason).toContain("no textual diff");
    expect(result.dropped[0].reason).not.toContain("not inside a diff hunk");
  });

  it("drops comments on lines outside diff hunks", () => {
    const result = validateInlineComments([base({ line: 500 })], diffMap);
    expect(result.valid).toHaveLength(0);
    expect(result.dropped).toHaveLength(1);
    expect(result.dropped[0].reason).toContain("line 500");
    expect(result.dropped[0].reason).toContain("RIGHT");
  });

  it("drops comments whose side mismatches the hunk (added line on LEFT)", () => {
    // line 12 is "+new" — only in RIGHT. Asking for it on LEFT should drop.
    const result = validateInlineComments([base({ line: 12, side: "LEFT" })], diffMap);
    expect(result.valid).toHaveLength(0);
    expect(result.dropped).toHaveLength(1);
  });

  it("drops multi-line comments where start_line is out of range", () => {
    const result = validateInlineComments([base({ line: 12, start_line: 3 })], diffMap);
    expect(result.valid).toHaveLength(0);
    expect(result.dropped).toHaveLength(1);
    expect(result.dropped[0].reason).toContain("start_line 3");
  });

  it("keeps multi-line comments fully inside a hunk", () => {
    const result = validateInlineComments([base({ line: 12, start_line: 11 })], diffMap);
    expect(result.valid).toHaveLength(1);
    expect(result.dropped).toHaveLength(0);
  });

  it("drops inverted ranges (start_line > line) with a precise reason", () => {
    // both 11 and 12 anchor in the hunk, but GitHub 422s with "invalid line
    // numbers" when start_line > line. dropping locally avoids the opaque
    // remote failure and tells the agent exactly what to fix.
    const result = validateInlineComments([base({ line: 11, start_line: 12 })], diffMap);
    expect(result.valid).toHaveLength(0);
    expect(result.dropped).toHaveLength(1);
    expect(result.dropped[0].reason).toMatch(/start_line 12 is after line 11/);
    expect(result.dropped[0].reason).toMatch(/start_line <= line/);
  });

  it("partitions a batch — valid and invalid comments survive independently", () => {
    const result = validateInlineComments(
      [base({ line: 12 }), base({ line: 9999 }), base({ path: "missing.ts" })],
      diffMap
    );
    expect(result.valid).toHaveLength(1);
    expect(result.dropped).toHaveLength(2);
  });

  it("defaults side to RIGHT when omitted", () => {
    const result = validateInlineComments([{ path: "src/foo.ts", line: 12, body: "" }], diffMap);
    expect(result.valid).toHaveLength(1);
  });
});

describe("buildCommentableMap", () => {
  it("returns the cached snapshot when toolState matches PR and checkoutSha", async () => {
    // simulates checkout_pr having pre-populated the cache. the cache pins the
    // commentable lines to checkoutSha so review-time validation matches what
    // GitHub anchors to, even if the PR is updated mid-run.
    const cached = buildMap([["src/foo.ts", "@@ -1,1 +1,2 @@\n ctx\n+new"]]);
    const paginate = vi.fn();
    const ctx = {
      octokit: { paginate, rest: { pulls: { listFiles: {} } } },
      repo: { owner: "o", name: "r" },
      toolState: {
        commentableLinesByFile: cached,
        commentableLinesPullNumber: 42,
        commentableLinesCheckoutSha: "sha1",
        checkoutSha: "sha1",
      },
    } as unknown as ToolContext;

    const result = await buildCommentableMap(ctx, 42);

    expect(result).toBe(cached);
    expect(paginate).not.toHaveBeenCalled();
  });

  it("ignores the cached snapshot when it was built for a different PR", async () => {
    // without this guard, checkout_pr(B) followed by review(A) would validate
    // A's inline comments against B's diff — silently dropping valid anchors.
    const cached = buildMap([["src/foo.ts", "@@ -1,1 +1,2 @@\n ctx\n+new"]]);
    const freshFile = { filename: "src/bar.ts", patch: "@@ -1,1 +1,2 @@\n ctx\n+added" };
    const paginate = vi.fn().mockResolvedValue([freshFile]);
    const ctx = {
      octokit: { paginate, rest: { pulls: { listFiles: {} } } },
      repo: { owner: "o", name: "r" },
      toolState: {
        commentableLinesByFile: cached,
        commentableLinesPullNumber: 99,
        commentableLinesCheckoutSha: "sha1",
        checkoutSha: "sha1",
      },
    } as unknown as ToolContext;

    const result = await buildCommentableMap(ctx, 42);

    expect(paginate).toHaveBeenCalledTimes(1);
    expect(result).not.toBe(cached);
    expect(result.get("src/bar.ts")?.RIGHT.has(2)).toBe(true);
  });

  it("ignores the cached snapshot when checkoutSha has moved since it was built", async () => {
    // simulates a second checkout_pr(42) that bumped checkoutSha but failed
    // before repopulating the cache (e.g., listFiles rate-limited). without
    // the sha guard, review would reuse the stale snapshot against the new
    // anchor and either drop valid comments or let invalid ones through.
    const cached = buildMap([["src/foo.ts", "@@ -1,1 +1,2 @@\n ctx\n+new"]]);
    const freshFile = { filename: "src/bar.ts", patch: "@@ -1,1 +1,2 @@\n ctx\n+added" };
    const paginate = vi.fn().mockResolvedValue([freshFile]);
    const ctx = {
      octokit: { paginate, rest: { pulls: { listFiles: {} } } },
      repo: { owner: "o", name: "r" },
      toolState: {
        commentableLinesByFile: cached,
        commentableLinesPullNumber: 42,
        commentableLinesCheckoutSha: "sha-old",
        checkoutSha: "sha-new",
      },
    } as unknown as ToolContext;

    const result = await buildCommentableMap(ctx, 42);

    expect(paginate).toHaveBeenCalledTimes(1);
    expect(result).not.toBe(cached);
  });

  it("falls back to listFiles when no cache exists", async () => {
    const file = { filename: "src/bar.ts", patch: "@@ -1,1 +1,2 @@\n ctx\n+added" };
    const paginate = vi.fn().mockResolvedValue([file]);
    const ctx = {
      octokit: { paginate, rest: { pulls: { listFiles: {} } } },
      repo: { owner: "o", name: "r" },
      toolState: {},
    } as unknown as ToolContext;

    const result = await buildCommentableMap(ctx, 42);

    expect(paginate).toHaveBeenCalledTimes(1);
    expect(result.get("src/bar.ts")?.RIGHT.has(2)).toBe(true);
  });
});

describe("formatDroppedCommentsNote", () => {
  it("renders single-line dropped entries with `path:line`", () => {
    const dropped: DroppedComment[] = [
      {
        path: "src/foo.ts",
        line: 42,
        side: "RIGHT",
        reason: "line 42 (RIGHT) is not inside a diff hunk",
      },
    ];
    const note = formatDroppedCommentsNote(dropped);
    expect(note).toContain("**Note:** 1 inline comment(s) dropped");
    expect(note).toContain("`src/foo.ts:42` (RIGHT)");
    expect(note).toContain("line 42 (RIGHT) is not inside a diff hunk");
  });

  it("renders multi-line dropped entries with `path:start-end`", () => {
    const dropped: DroppedComment[] = [
      {
        path: "src/bar.ts",
        line: 20,
        startLine: 15,
        side: "LEFT",
        reason: "start_line 15 (LEFT) is not inside a diff hunk",
      },
    ];
    const note = formatDroppedCommentsNote(dropped);
    expect(note).toContain("`src/bar.ts:15-20` (LEFT)");
  });

  it("falls back to single-line format when startLine equals line", () => {
    const dropped: DroppedComment[] = [
      { path: "src/baz.ts", line: 7, startLine: 7, side: "RIGHT", reason: "file not in PR diff" },
    ];
    const note = formatDroppedCommentsNote(dropped);
    expect(note).toContain("`src/baz.ts:7` (RIGHT)");
    expect(note).not.toContain("7-7");
  });

  it("caps detail lines and reports the remainder so body stays under GitHub's size limit", () => {
    const overflow = MAX_DROPPED_COMMENT_LINES + 7;
    const dropped: DroppedComment[] = Array.from({ length: overflow }, (_, i) => ({
      path: `src/file${i}.ts`,
      line: i + 1,
      side: "RIGHT" as const,
      reason: "file not in PR diff",
    }));
    const note = formatDroppedCommentsNote(dropped);
    expect(note).toContain(`**Note:** ${overflow} inline comment(s) dropped`);
    // still reports the full count in the header
    expect(note).toContain(`${overflow} inline comment(s)`);
    // first entry shown, last entry elided
    expect(note).toContain("`src/file0.ts:1` (RIGHT)");
    expect(note).not.toContain(`src/file${overflow - 1}.ts`);
    expect(note).toContain("…and 7 more dropped comment(s) not shown");
  });

  it("does not add a truncation line when drops fit under the cap", () => {
    const dropped: DroppedComment[] = Array.from({ length: MAX_DROPPED_COMMENT_LINES }, (_, i) => ({
      path: `src/f${i}.ts`,
      line: i + 1,
      side: "RIGHT" as const,
      reason: "file not in PR diff",
    }));
    const note = formatDroppedCommentsNote(dropped);
    expect(note).not.toContain("more dropped comment(s) not shown");
  });
});

describe("clearStrandedPendingReview", () => {
  function pendingReviewError(status: number, message: string): Error {
    const err = new Error(message) as Error & { status: number };
    err.status = status;
    return err;
  }

  const baseParams = { owner: "o", repo: "r", pull_number: 42 };

  it("rethrows the original error when status is not 422", async () => {
    const err = pendingReviewError(500, "server exploded");
    const ctx = {
      octokit: {
        paginate: vi.fn(),
        rest: { pulls: { listReviews: {}, deletePendingReview: vi.fn() } },
      },
    } as unknown as ToolContext;
    await expect(clearStrandedPendingReview(ctx, { ...baseParams, originalErr: err })).rejects.toBe(
      err
    );
    expect(ctx.octokit.paginate).not.toHaveBeenCalled();
  });

  it("rethrows the original error when 422 does not mention pending review", async () => {
    // a 422 from an unrelated validation (e.g., invalid anchor) must not
    // trigger a destructive delete of the user's own draft.
    const err = pendingReviewError(422, "pull_request_review_thread is not part of the diff");
    const deletePendingReview = vi.fn();
    const ctx = {
      octokit: {
        paginate: vi.fn(),
        rest: { pulls: { listReviews: {}, deletePendingReview } },
      },
    } as unknown as ToolContext;
    await expect(clearStrandedPendingReview(ctx, { ...baseParams, originalErr: err })).rejects.toBe(
      err
    );
    expect(ctx.octokit.paginate).not.toHaveBeenCalled();
    expect(deletePendingReview).not.toHaveBeenCalled();
  });

  it("rethrows the original error when no PENDING review is found", async () => {
    // 422 claimed a pending exists but listReviews returns only SUBMITTED —
    // likely a transient GitHub inconsistency. retry won't help; surface the
    // original error so the caller sees why createReview failed.
    const err = pendingReviewError(422, "User already has a pending review for this pull request");
    const paginate = vi.fn().mockResolvedValue([{ id: 1, state: "COMMENTED" } as unknown as never]);
    const deletePendingReview = vi.fn();
    const ctx = {
      octokit: { paginate, rest: { pulls: { listReviews: {}, deletePendingReview } } },
    } as unknown as ToolContext;
    await expect(clearStrandedPendingReview(ctx, { ...baseParams, originalErr: err })).rejects.toBe(
      err
    );
    expect(paginate).toHaveBeenCalledTimes(1);
    expect(deletePendingReview).not.toHaveBeenCalled();
  });

  it("deletes the leftover PENDING review and resolves on success", async () => {
    const err = pendingReviewError(422, "User already has a pending review for this pull request");
    const paginate = vi.fn().mockResolvedValue([
      { id: 100, state: "COMMENTED" },
      { id: 101, state: "PENDING" },
    ] as unknown as never);
    const deletePendingReview = vi.fn().mockResolvedValue({ status: 204 });
    const ctx = {
      octokit: { paginate, rest: { pulls: { listReviews: {}, deletePendingReview } } },
    } as unknown as ToolContext;
    await expect(
      clearStrandedPendingReview(ctx, { ...baseParams, originalErr: err })
    ).resolves.toBeUndefined();
    expect(deletePendingReview).toHaveBeenCalledWith({
      owner: "o",
      repo: "r",
      pull_number: 42,
      review_id: 101,
    });
  });

  it("swallows a 404 from deletePendingReview (raced with another cleanup)", async () => {
    const err = pendingReviewError(422, "User already has a pending review for this pull request");
    const paginate = vi.fn().mockResolvedValue([{ id: 101, state: "PENDING" }] as unknown as never);
    const deletePendingReview = vi.fn().mockRejectedValue(pendingReviewError(404, "not found"));
    const ctx = {
      octokit: { paginate, rest: { pulls: { listReviews: {}, deletePendingReview } } },
    } as unknown as ToolContext;
    await expect(
      clearStrandedPendingReview(ctx, { ...baseParams, originalErr: err })
    ).resolves.toBeUndefined();
  });

  it("swallows a 422 from deletePendingReview (draft submitted by a concurrent caller)", async () => {
    const err = pendingReviewError(422, "User already has a pending review for this pull request");
    const paginate = vi.fn().mockResolvedValue([{ id: 101, state: "PENDING" }] as unknown as never);
    const deletePendingReview = vi
      .fn()
      .mockRejectedValue(pendingReviewError(422, "review has already been submitted"));
    const ctx = {
      octokit: { paginate, rest: { pulls: { listReviews: {}, deletePendingReview } } },
    } as unknown as ToolContext;
    await expect(
      clearStrandedPendingReview(ctx, { ...baseParams, originalErr: err })
    ).resolves.toBeUndefined();
  });

  it("rethrows the ORIGINAL 422 when listReviews fails so the real blocker isn't masked", async () => {
    // if listReviews throws a transient 502 during cleanup, we must surface
    // the pending-review 422 — not the 502 — so the caller sees the actual
    // reason createReview failed and can retry the cleanup. masking the 422
    // with a 502 previously sent agents chasing phantom server errors.
    const err = pendingReviewError(422, "User already has a pending review for this pull request");
    const paginate = vi.fn().mockRejectedValue(pendingReviewError(502, "bad gateway"));
    const deletePendingReview = vi.fn();
    const ctx = {
      octokit: { paginate, rest: { pulls: { listReviews: {}, deletePendingReview } } },
    } as unknown as ToolContext;
    await expect(clearStrandedPendingReview(ctx, { ...baseParams, originalErr: err })).rejects.toBe(
      err
    );
    expect(deletePendingReview).not.toHaveBeenCalled();
  });

  it("rethrows non-404/422 errors from deletePendingReview so the real cause surfaces", async () => {
    const err = pendingReviewError(422, "User already has a pending review for this pull request");
    const paginate = vi.fn().mockResolvedValue([{ id: 101, state: "PENDING" }] as unknown as never);
    const cleanupErr = pendingReviewError(500, "internal server error");
    const deletePendingReview = vi.fn().mockRejectedValue(cleanupErr);
    const ctx = {
      octokit: { paginate, rest: { pulls: { listReviews: {}, deletePendingReview } } },
    } as unknown as ToolContext;
    await expect(clearStrandedPendingReview(ctx, { ...baseParams, originalErr: err })).rejects.toBe(
      cleanupErr
    );
  });
});

describe("createReviewWithStrandedRecovery", () => {
  function pendingReviewError(status: number, message: string): Error {
    const err = new Error(message) as Error & { status: number };
    err.status = status;
    return err;
  }

  const params = {
    owner: "o",
    repo: "r",
    pull_number: 42,
    event: "COMMENT" as const,
  };

  it("returns createReview result directly when no stranded draft exists", async () => {
    const response = { data: { id: 1, node_id: "n1" } };
    const createReview = vi.fn().mockResolvedValue(response);
    const ctx = {
      octokit: {
        paginate: vi.fn(),
        rest: { pulls: { createReview, listReviews: {}, deletePendingReview: vi.fn() } },
      },
    } as unknown as ToolContext;
    await expect(createReviewWithStrandedRecovery(ctx, params)).resolves.toBe(response);
    expect(createReview).toHaveBeenCalledTimes(1);
  });

  it("clears a stranded PENDING draft and retries on pending-review 422 — covers the no-body path", async () => {
    // regression: the no-body review path (approve-with-no-feedback,
    // comments-only) used to call createReview directly. a prior body-path run
    // that crashed between createReview(PENDING) and submitReview would leave
    // a stranded PENDING draft; every subsequent no-body review would 422
    // with "already has a pending review" until a body-path run happened to
    // clear it. this test exercises the recovery: first createReview 422s,
    // clearStranded deletes the leftover, and the retry succeeds.
    const stranded = pendingReviewError(
      422,
      "User already has a pending review for this pull request"
    );
    const response = { data: { id: 2, node_id: "n2" } };
    const createReview = vi.fn().mockRejectedValueOnce(stranded).mockResolvedValueOnce(response);
    const paginate = vi.fn().mockResolvedValue([{ id: 77, state: "PENDING" }] as unknown as never);
    const deletePendingReview = vi.fn().mockResolvedValue({ status: 204 });
    const ctx = {
      octokit: {
        paginate,
        rest: { pulls: { createReview, listReviews: {}, deletePendingReview } },
      },
    } as unknown as ToolContext;
    await expect(createReviewWithStrandedRecovery(ctx, params)).resolves.toBe(response);
    expect(createReview).toHaveBeenCalledTimes(2);
    expect(deletePendingReview).toHaveBeenCalledWith({
      owner: "o",
      repo: "r",
      pull_number: 42,
      review_id: 77,
    });
  });

  it("rethrows non-pending 422s without retrying — avoids masking a real validation error", async () => {
    // if the 422 is unrelated to a stranded draft (e.g. body too long, bad
    // anchor), clearStrandedPendingReview rethrows and we must not retry
    // blindly — a retry would just hit the same validation and double the
    // GitHub API traffic for nothing.
    const err = pendingReviewError(422, "body is too long");
    const createReview = vi.fn().mockRejectedValue(err);
    const paginate = vi.fn();
    const deletePendingReview = vi.fn();
    const ctx = {
      octokit: {
        paginate,
        rest: { pulls: { createReview, listReviews: {}, deletePendingReview } },
      },
    } as unknown as ToolContext;
    await expect(createReviewWithStrandedRecovery(ctx, params)).rejects.toBe(err);
    expect(createReview).toHaveBeenCalledTimes(1);
    expect(deletePendingReview).not.toHaveBeenCalled();
  });
});

describe("reviewSkipDecision", () => {
  // GitHub 422s `event: "COMMENT"` reviews with no body + no comments
  // ("{\"message\":\"Unprocessable Entity\",\"errors\":[\"\"]}"). verified
  // empirically against repos/pullfrog/preview-546-run-issues-fixes/pulls/1
  // with and without commit_id set. the skip function must return a decision
  // for every shape that lands on that API call.

  it("skips with 'no-issues' when !approved + empty body + no comments", () => {
    const decision = reviewSkipDecision({
      approved: false,
      body: "",
      hasComments: false,
      prApproveEnabled: true,
    });
    expect(decision?.kind).toBe("no-issues");
    expect(decision?.reason).toContain("nothing to post");
  });

  it("treats null body the same as empty string", () => {
    const decision = reviewSkipDecision({
      approved: false,
      body: null,
      hasComments: false,
      prApproveEnabled: true,
    });
    expect(decision?.kind).toBe("no-issues");
  });

  it("treats undefined body the same as empty string", () => {
    const decision = reviewSkipDecision({
      approved: false,
      body: undefined,
      hasComments: false,
      prApproveEnabled: true,
    });
    expect(decision?.kind).toBe("no-issues");
  });

  it("skips with 'empty-downgraded-approve' when approved + !prApproveEnabled + empty", () => {
    // this is the F3 regression case — agent requests APPROVE, runtime
    // downgrades to COMMENT (prApproveEnabled off), and the empty COMMENT
    // 422s at GitHub. before this fix, the tool returned a stranded-success
    // shape that didn't map to any persisted review.
    const decision = reviewSkipDecision({
      approved: true,
      body: "",
      hasComments: false,
      prApproveEnabled: false,
    });
    expect(decision?.kind).toBe("empty-downgraded-approve");
    expect(decision?.reason).toContain("prApproveEnabled is disabled");
  });

  it("does NOT skip legitimate bare APPROVE (approved + prApproveEnabled + empty)", () => {
    // GitHub accepts empty APPROVE reviews — the stamp itself is the content.
    // skipping here would silently drop agents' real approvals.
    const decision = reviewSkipDecision({
      approved: true,
      body: "",
      hasComments: false,
      prApproveEnabled: true,
    });
    expect(decision).toBeNull();
  });

  it("does NOT skip when body is present (no-issues path)", () => {
    const decision = reviewSkipDecision({
      approved: false,
      body: "found some issues",
      hasComments: false,
      prApproveEnabled: true,
    });
    expect(decision).toBeNull();
  });

  it("does NOT skip when body is present (downgrade path)", () => {
    // approved+!prApproveEnabled with a body becomes a real COMMENT review
    // (downgrade + body). GitHub accepts those; don't skip.
    const decision = reviewSkipDecision({
      approved: true,
      body: "nits follow",
      hasComments: false,
      prApproveEnabled: false,
    });
    expect(decision).toBeNull();
  });

  it("does NOT skip when comments are present (no-issues path)", () => {
    const decision = reviewSkipDecision({
      approved: false,
      body: "",
      hasComments: true,
      prApproveEnabled: true,
    });
    expect(decision).toBeNull();
  });

  it("does NOT skip when comments are present (downgrade path)", () => {
    const decision = reviewSkipDecision({
      approved: true,
      body: "",
      hasComments: true,
      prApproveEnabled: false,
    });
    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
  // "No new 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");
  });
});