feat(pr-review): refuse the first submit_review while reviewed files are unread

apps/ai/src/chat/review-coverage.test.ts

@@ -0,0 +1,71 @@
+/**
+ * What a review pass has read. Fed the real renderers' answers, since the tracker reads them back.
+ */
+import type { PullRequestFile } from "@maple/domain/http"
+import { renderFollowUp } from "@maple/backend/services/pr-review/findings"
+import { assert, describe, it } from "vitest"
+import { renderChangedFiles, renderFileDiffs } from "../mcp/tools/pull-request"
+import { makeReviewCoverage, unreadRefusal } from "./review-coverage"
+
+const file = (path: string): PullRequestFile => ({
+	path,
+	previousPath: null,
+	status: "modified",
+	additions: 1,
+	deletions: 0,
+	patch: "@@ -1 +1 @@\n+x",
+})
+
+const FILES = [file("src/a.ts"), file("src/b.ts"), file("src/a.test.ts"), file("docs/a.md")]
+const listing = renderChangedFiles("octo/shop", 7, FILES).content[0]!.text
+const diffs = (...paths: Array<string>) => renderFileDiffs(FILES, paths).content[0]!.text
+
+describe("makeReviewCoverage", () => {
+	it("owes nothing before the file list is read", () => {
+		assert.deepEqual(makeReviewCoverage("Review pull request #7.").unread(), [])
+	})
+
+	it("owes every reviewed file until a diff answer shows it", () => {
+		const coverage = makeReviewCoverage("Review pull request #7.")
+		coverage.observe("pr_changed_files", listing)
+		assert.deepEqual(coverage.unread(), ["src/a.ts", "src/b.ts", "src/a.test.ts"])
+		coverage.observe("pr_file_diff", diffs("src/a.ts", "src/a.test.ts"))
+		assert.deepEqual(coverage.unread(), ["src/b.ts"])
+		coverage.observe("pr_file_diff", diffs("src/b.ts"))
+		assert.deepEqual(coverage.unread(), [])
+	})
+
+	it("counts only pr_file_diff answers as reads", () => {
+		const coverage = makeReviewCoverage("Review pull request #7.")
+		coverage.observe("pr_changed_files", listing)
+		coverage.observe("sandbox_read_file", diffs("src/a.ts", "src/b.ts", "src/a.test.ts"))
+		assert.lengthOf(coverage.unread(), 3)
+	})
+
+	it("owes a later push only the files its kickoff says changed", () => {
+		const followUp = renderFollowUp({ previousSha: "abcdef1234", changedPaths: ["src/b.ts"], open: [] })
+		const coverage = makeReviewCoverage(["Review pull request #7.", ...followUp].join("\n"))
+		coverage.observe("pr_changed_files", listing)
+		assert.deepEqual(coverage.unread(), ["src/b.ts"])
+
+		const nothing = makeReviewCoverage(
+			renderFollowUp({ previousSha: "abcdef1234", changedPaths: [], open: [] }).join("\n"),
+		)
+		nothing.observe("pr_changed_files", listing)
+		assert.deepEqual(nothing.unread(), [])
+	})
+})
+
+describe("unreadRefusal", () => {
+	it("names the files and says a second call goes through", () => {
+		const message = unreadRefusal(["src/b.ts"])
+		assert.include(message, "1 reviewed file: src/b.ts.")
+		assert.include(message, "call submit_review again now")
+	})
+
+	it("bounds a long list", () => {
+		const message = unreadRefusal(Array.from({ length: 45 }, (_, i) => `src/f${i}.ts`))
+		assert.include(message, "src/f29.ts, and 15 more.")
+		assert.notInclude(message, "src/f30.ts")
+	})
+})

apps/ai/src/chat/review-coverage.ts

@@ -0,0 +1,45 @@
+/**
+ * Which of a pull request's reviewed files a review pass has actually read.
+ *
+ * The prompt says to read every hunk; this is what checks it. One tracker per run, fed every Maple
+ * tool answer the run dispatches, including the ones its `review_files` children dispatch through
+ * the same handlers. `submit_review` consults it once before recording.
+ */
+import { followUpScope } from "@maple/backend/services/pr-review/findings"
+import { pathsInDiffAnswer, reviewablePathsInListing } from "../mcp/tools/pull-request"
+
+export interface ReviewCoverage {
+	/** One successful tool answer, by tool name. */
+	readonly observe: (tool: string, answer: string) => void
+	/** Reviewed files with a patch whose diff no `pr_file_diff` answer has shown yet, in listing order. */
+	readonly unread: () => ReadonlyArray<string>
+}
+
+/** `kickoff` is the pass's first message: a later push's kickoff narrows which files are due. */
+export const makeReviewCoverage = (kickoff: string): ReviewCoverage => {
+	const scope = followUpScope(kickoff)
+	const inScope = scope === undefined ? () => true : (path: string) => scope.includes(path)
+	const due = new Set<string>()
+	const read = new Set<string>()
+	return {
+		observe: (tool, answer) => {
+			if (tool === "pr_changed_files") {
+				for (const path of reviewablePathsInListing(answer)) if (inScope(path)) due.add(path)
+			} else if (tool === "pr_file_diff") {
+				for (const path of pathsInDiffAnswer(answer)) read.add(path)
+			}
+		},
+		unread: () => [...due].filter((path) => !read.has(path)),
+	}
+}
+
+const LISTED_UNREAD = 30
+
+/** The one-time refusal `submit_review` answers with while files are unread. */
+export const unreadRefusal = (unread: ReadonlyArray<string>): string =>
+	[
+		`You have not read the diff of ${unread.length} reviewed ${unread.length === 1 ? "file" : "files"}: ` +
+			`${unread.slice(0, LISTED_UNREAD).join(", ")}${unread.length > LISTED_UNREAD ? `, and ${unread.length - LISTED_UNREAD} more` : ""}.`,
+		"Read them with pr_file_diff (several per call) or review_files, then call submit_review again. " +
+			"If you left them unread on purpose, call submit_review again now and it will be recorded; say in the summary what was not read.",
+	].join(" ")

apps/ai/src/chat/run.ts

@@ -21,13 +21,15 @@ import type { TenantContext } from "@maple/backend/services/auth/tenant-context"
 import { type AgentDefinition, agentForSession, chatAgent } from "./agents"
 import { profileForTurn } from "./profiles"
 import { makeTextSanitizer, toChatEvents, type ChatTurnEvent } from "./events"
+import { makeReviewCoverage } from "./review-coverage"
 import { buildReviewFanout } from "./review-fanout"
 import {
 	accumulateUsage,
 	buildChatToolkit,
 	buildDiagnosisCompletion,
 	buildReplyCompletion,
 	buildReviewCompletion,
+	isAutonomousReviewTurn,
 	type RunCompletion,
 	SUBMIT_REVIEW,
 	type RunUsage,
@@ -134,12 +136,18 @@ export interface ChatRunOutcome {
 export const runChatTurn = (input: ChatRunInput) => {
 	const agent = input.agent ?? agentForSession(input.sessionId)
 	const profile = profileForTurn(agent, input.origin)
+	// Per run, and fed by the Maple handlers the review_files children share, so a group a child
+	// read counts as read.
+	const coverage = isAutonomousReviewTurn(input.sessionId, input.origin)
+		? makeReviewCoverage(input.text)
+		: undefined
 	const maple = buildChatToolkit(
 		input.toolExecutor,
 		input.tenant,
 		profile.ruleset,
 		profile.surface,
 		agentSessionSpanAttributes(input.model.tags),
+		coverage?.observe,
 	)
 	// One completion per session kind. The session id decides which, so a review session can never
 	// be handed the diagnosis tool or the other way round.
@@ -165,6 +173,7 @@ export const runChatTurn = (input: ChatRunInput) => {
 					input.model.name,
 					input.closeOut === true,
 					agentSessionSpanAttributes(input.model.tags),
+					coverage,
 				)) ??
 		(input.submitReply === undefined || input.stageEdit === undefined
 			? undefined

apps/ai/src/chat/tools.test.ts

@@ -7,7 +7,7 @@
 import { type ChatTurnOrigin, prReviewSessionId } from "@maple/domain/chat-session"
 import { MAPLE_NATIVE_SESSION_ID_ATTR } from "@maple/domain/gen-ai"
 import { ChatConnectorId, ExternalUserId, OrgId, UserId } from "@maple/domain/primitives"
-import { Effect, Schema } from "effect"
+import { Effect, Result, Schema } from "effect"
 import { assert, describe, it } from "vitest"
 import {
 	buildDiagnosisCompletion,
@@ -20,6 +20,8 @@ import {
 } from "./tools"
 import type { TenantContext } from "@maple/backend/services/auth/tenant-context"
 import { makeRecordingTracer } from "@maple/backend/testing/recording-tracer"
+import type { PrReviewSubmission } from "@maple/domain/http"
+import type { ReviewCoverage } from "./review-coverage"
 
 const orgId = Schema.decodeSync(OrgId)("org_test")
 const human = Schema.decodeSync(UserId)("user_test")
@@ -154,4 +156,74 @@ describe("buildReviewCompletion", () => {
 	it("gives a session that is not a review no review tool", () => {
 		assert.isUndefined(buildReview(INVESTIGATION_SESSION, { kind: "autonomous" }))
 	})
+
+	describe("with files unread", () => {
+		const coverage = (unread: ReadonlyArray<string>): ReviewCoverage => ({
+			observe: () => {},
+			unread: () => unread,
+		})
+		const recording = () => {
+			const filed: Array<unknown> = []
+			const submit: SubmitReview = (_org, _id, request) => Effect.sync(() => filed.push(request))
+			return { filed, submit }
+		}
+		const submitWith = (submit: SubmitReview, unread: ReadonlyArray<string>, partial = false) => {
+			const completion = buildReviewCompletion(
+				REVIEW_SESSION,
+				tenantFor(human),
+				{ kind: "autonomous" },
+				submit,
+				makeRunUsage(),
+				MODEL_NAME,
+				partial,
+				undefined,
+				coverage(unread),
+			)
+			assert.isDefined(completion)
+			return {
+				completion: completion!,
+				call: (submission: PrReviewSubmission) =>
+					Effect.runPromise(
+						Effect.result(completion!.handlers[SUBMIT_REVIEW](submission, {} as never)),
+					),
+			}
+		}
+		const CLEAN: PrReviewSubmission = { verdict: "clean", summary: "Adds a retry to the order client." }
+
+		it("refuses the first submission, naming the files, and records the second", async () => {
+			const { filed, submit } = recording()
+			const { completion, call } = submitWith(submit, ["src/b.ts"])
+
+			const first = await call(CLEAN)
+			assert.isTrue(Result.isFailure(first))
+			assert.include(Result.isFailure(first) ? first.failure.message : "", "src/b.ts")
+			assert.lengthOf(filed, 0)
+			assert.isFalse(completion.submitted())
+
+			const second = await call(CLEAN)
+			assert.isTrue(Result.isSuccess(second))
+			assert.lengthOf(filed, 1)
+			assert.isTrue(completion.submitted())
+		})
+
+		it("records straight away when every reviewed file was read", async () => {
+			const { filed, submit } = recording()
+			assert.isTrue(Result.isSuccess(await submitWith(submit, []).call(CLEAN)))
+			assert.lengthOf(filed, 1)
+		})
+
+		it("never holds a close-out or a not_applicable verdict", async () => {
+			const closeOut = recording()
+			assert.isTrue(Result.isSuccess(await submitWith(closeOut.submit, ["src/b.ts"], true).call(CLEAN)))
+			assert.lengthOf(closeOut.filed, 1)
+
+			const notApplicable = recording()
+			const result = await submitWith(notApplicable.submit, ["src/b.ts"]).call({
+				verdict: "not_applicable",
+				summary: "Only docs changed.",
+			})
+			assert.isTrue(Result.isSuccess(result))
+			assert.lengthOf(notApplicable.filed, 1)
+		})
+	})
 })

apps/ai/src/chat/tools.ts

@@ -27,6 +27,7 @@ import {
 	type PrReviewReplyNotFoundError,
 	PrReviewReplySubmission,
 	PrReviewSubmission,
+	type PrReviewVerdict,
 	SubmitDiagnosisRequest,
 	SubmitPrReviewRequest,
 } from "@maple/domain/http"
@@ -39,6 +40,7 @@ import type { McpToolSurface } from "@maple/domain/mcp-manifest"
 import { buildMapleToolkit, MapleToolFailure, summarizeToolFailure } from "../mcp/tools/llm-tools"
 import { toolHandlersWithContent } from "../platform/genai-spans"
 import type { TenantContext } from "@maple/backend/services/auth/tenant-context"
+import { type ReviewCoverage, unreadRefusal } from "./review-coverage"
 
 const decodeInvestigationIdOption = Schema.decodeUnknownOption(InvestigationId)
 const decodePrReviewIdOption = Schema.decodeUnknownOption(PrReviewId)
@@ -281,6 +283,8 @@ export const buildReviewCompletion = (
 	modelName: string,
 	partial = false,
 	sessionAttributes?: Readonly<Record<string, string>>,
+	/** What this pass has read. The first submission with files unread is refused once. */
+	coverage?: ReviewCoverage,
 ) => {
 	const reviewId = prReviewForSession(sessionId)
 	if (reviewId === undefined) return undefined
@@ -289,53 +293,61 @@ export const buildReviewCompletion = (
 	if (origin.kind !== "autonomous") return undefined
 	const toolkit = Toolkit.make(reviewTool)
 	let submitted = false
+	let refusedUnread = false
+	// Once only: a second call with files still unread is the agent submitting anyway. A close-out
+	// files what it has and a not_applicable verdict reads nothing, so neither is held.
+	const unreadToRefuse = (verdict: PrReviewVerdict): ReadonlyArray<string> => {
+		if (coverage === undefined || partial || refusedUnread || verdict === "not_applicable") return []
+		const unread = coverage.unread()
+		refusedUnread = unread.length > 0
+		return unread
+	}
+	const record = (submission: PrReviewSubmission) =>
+		Effect.suspend(() => {
+			const { report, filled, droppedFindings, resolved } = normalizePrReviewSubmission(submission)
+			const unread = unreadToRefuse(report.verdict)
+			if (unread.length > 0) {
+				return Effect.annotateCurrentSpan("maple.pr_review.unread_files", unread.length).pipe(
+					Effect.andThen(Effect.fail(new MapleToolFailure({ message: unreadRefusal(unread) }))),
+				)
+			}
+			return submitReview(
+				tenant.orgId,
+				reviewId,
+				new SubmitPrReviewRequest({
+					report,
+					model: modelName,
+					inputTokens: usage.input,
+					outputTokens: usage.output,
+					...(partial ? { partial: true } : undefined),
+					...(resolved.length > 0 ? { resolved } : undefined),
+				}),
+			).pipe(
+				Effect.tap(() =>
+					Effect.annotateCurrentSpan({
+						"maple.pr_review.filled_fields": filled.join(","),
+						"maple.pr_review.filled_count": filled.length,
+						"maple.pr_review.dropped_findings": droppedFindings,
+						"maple.pr_review.findings": report.findings.length,
+						"maple.pr_review.verdict": report.verdict,
+						"maple.pr_review.resolved": resolved.length,
+					}),
+				),
+				Effect.tap(() => Effect.sync(() => (submitted = true))),
+				Effect.as("Review recorded."),
+				Effect.catchCause((cause) =>
+					Effect.fail(
+						new MapleToolFailure({
+							message: `${SUBMIT_REVIEW} failed: ${summarizeToolFailure(cause)}`,
+						}),
+					),
+				),
+			)
+		})
 	return {
 		tool: SUBMIT_REVIEW,
 		toolkit,
-		...toolHandlersWithContent(
-			toolkit,
-			{
-				[SUBMIT_REVIEW]: (submission: PrReviewSubmission) =>
-					Effect.suspend(() => {
-						const { report, filled, droppedFindings, resolved } =
-							normalizePrReviewSubmission(submission)
-						return submitReview(
-							tenant.orgId,
-							reviewId,
-							new SubmitPrReviewRequest({
-								report,
-								model: modelName,
-								inputTokens: usage.input,
-								outputTokens: usage.output,
-								...(partial ? { partial: true } : undefined),
-								...(resolved.length > 0 ? { resolved } : undefined),
-							}),
-						).pipe(
-							Effect.tap(() =>
-								Effect.annotateCurrentSpan({
-									"maple.pr_review.filled_fields": filled.join(","),
-									"maple.pr_review.filled_count": filled.length,
-									"maple.pr_review.dropped_findings": droppedFindings,
-									"maple.pr_review.findings": report.findings.length,
-									"maple.pr_review.verdict": report.verdict,
-									"maple.pr_review.resolved": resolved.length,
-								}),
-							),
-						)
-					}).pipe(
-						Effect.tap(() => Effect.sync(() => (submitted = true))),
-						Effect.as("Review recorded."),
-						Effect.catchCause((cause) =>
-							Effect.fail(
-								new MapleToolFailure({
-									message: `${SUBMIT_REVIEW} failed: ${summarizeToolFailure(cause)}`,
-								}),
-							),
-						),
-					),
-			},
-			sessionAttributes,
-		),
+		...toolHandlersWithContent(toolkit, { [SUBMIT_REVIEW]: record }, sessionAttributes),
 		autonomous: isAutonomousReviewTurn(sessionId, origin),
 		submitted: () => submitted,
 	} satisfies RunCompletion
@@ -442,10 +454,12 @@ export const buildChatToolkit = (
 	ruleset: PermissionRuleset,
 	surface: McpToolSurface,
 	sessionAttributes?: Readonly<Record<string, string>>,
+	onAnswer?: (tool: string, answer: string) => void,
 ) =>
 	buildMapleToolkit(executor, tenant, {
 		surface,
 		...(sessionAttributes === undefined ? undefined : { sessionAttributes }),
+		...(onAnswer === undefined ? undefined : { onAnswer }),
 		// `deny` means the model never sees the tool. That is a stronger guarantee than refusing the
 		// call afterwards, and it is free — an unoffered tool cannot be called.
 		include: (name) => evaluatePermission(ruleset, name) !== "deny",