Colin McDonnell
8c01ee3251
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.
710 lines
28 KiB
TypeScript
710 lines
28 KiB
TypeScript
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
|
|
// "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");
|
|
});
|
|
});
|