Colin McDonnell
d857e06731
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
64 lines
2.2 KiB
TypeScript
64 lines
2.2 KiB
TypeScript
import { describe, expect, it } from "vitest";
|
|
import type { ToolState } from "../toolState.ts";
|
|
import { getUnsubmittedReview } from "./postRun.ts";
|
|
|
|
function makeToolState(overrides: Partial<ToolState> = {}): ToolState {
|
|
return {
|
|
progressComment: undefined,
|
|
hadProgressComment: true,
|
|
backgroundProcesses: new Map(),
|
|
usageEntries: [],
|
|
...overrides,
|
|
};
|
|
}
|
|
|
|
describe("getUnsubmittedReview", () => {
|
|
it("returns null when mode is not a review mode", () => {
|
|
expect(getUnsubmittedReview(makeToolState({ selectedMode: "Build" }))).toBeNull();
|
|
expect(getUnsubmittedReview(makeToolState())).toBeNull();
|
|
});
|
|
|
|
it("returns null when a review was already submitted", () => {
|
|
expect(
|
|
getUnsubmittedReview(
|
|
makeToolState({
|
|
selectedMode: "Review",
|
|
review: { id: 1, nodeId: "n", reviewedSha: undefined },
|
|
})
|
|
)
|
|
).toBeNull();
|
|
});
|
|
|
|
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();
|
|
});
|
|
|
|
it("returns null when there is no progress comment to anchor the failure to", () => {
|
|
expect(
|
|
getUnsubmittedReview(makeToolState({ selectedMode: "Review", hadProgressComment: false }))
|
|
).toBeNull();
|
|
});
|
|
|
|
it("returns the selected mode when the gate should fire", () => {
|
|
expect(getUnsubmittedReview(makeToolState({ selectedMode: "Review" }))).toBe("Review");
|
|
expect(getUnsubmittedReview(makeToolState({ selectedMode: "IncrementalReview" }))).toBe(
|
|
"IncrementalReview"
|
|
);
|
|
});
|
|
});
|