diff --git a/apps/ai/src/chat/review-coverage.test.ts b/apps/ai/src/chat/review-coverage.test.ts new file mode 100644 index 0000000000..e29404df87 --- /dev/null +++ b/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) => 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") + }) +}) diff --git a/apps/ai/src/chat/review-coverage.ts b/apps/ai/src/chat/review-coverage.ts new file mode 100644 index 0000000000..bb0fb89008 --- /dev/null +++ b/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 +} + +/** `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() + const read = new Set() + 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 => + [ + `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(" ") diff --git a/apps/ai/src/chat/run.ts b/apps/ai/src/chat/run.ts index ed2fa4f75e..95b8ee5ddc 100644 --- a/apps/ai/src/chat/run.ts +++ b/apps/ai/src/chat/run.ts @@ -21,6 +21,7 @@ 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, @@ -28,6 +29,7 @@ import { 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 diff --git a/apps/ai/src/chat/tools.test.ts b/apps/ai/src/chat/tools.test.ts index 33249bc10e..47223c43c8 100644 --- a/apps/ai/src/chat/tools.test.ts +++ b/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): ReviewCoverage => ({ + observe: () => {}, + unread: () => unread, + }) + const recording = () => { + const filed: Array = [] + const submit: SubmitReview = (_org, _id, request) => Effect.sync(() => filed.push(request)) + return { filed, submit } + } + const submitWith = (submit: SubmitReview, unread: ReadonlyArray, 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) + }) + }) }) diff --git a/apps/ai/src/chat/tools.ts b/apps/ai/src/chat/tools.ts index babff544f2..62a6898c59 100644 --- a/apps/ai/src/chat/tools.ts +++ b/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>, + /** 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 => { + 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>, + 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", diff --git a/apps/ai/src/mcp/tools/llm-tools.test.ts b/apps/ai/src/mcp/tools/llm-tools.test.ts index d9632ca25e..1d8dfddca8 100644 --- a/apps/ai/src/mcp/tools/llm-tools.test.ts +++ b/apps/ai/src/mcp/tools/llm-tools.test.ts @@ -71,6 +71,25 @@ describe("buildMapleToolkit", () => { assert.equal(toolkit.tools.create_dashboard?.failureMode, "error") }) + it("hands every successful answer to onAnswer and never a failed one", async () => { + const seen: Array = [] + const executor: McpToolExecutorApi = { + execute: (_tenant, name) => + Effect.succeed( + name === "run_sql" + ? { isError: true, content: [{ type: "text" as const, text: "rejected" }] } + : { content: [{ type: "text" as const, text: `${name} ran` }] }, + ), + } + const { handlers } = buildMapleToolkit(executor, TENANT, { + surface: "chat", + onAnswer: (tool, answer) => seen.push(`${tool}: ${answer}`), + }) + await Effect.runPromise(handlers.list_services!({}, {} as never)) + await Effect.runPromise(Effect.result(handlers.run_sql!({ sql: "select 1" }, {} as never))) + assert.deepEqual(seen, ["list_services: list_services ran"]) + }) + it("fails a tool that reported an error with its message", async () => { const executor: McpToolExecutorApi = { execute: () => diff --git a/apps/ai/src/mcp/tools/llm-tools.ts b/apps/ai/src/mcp/tools/llm-tools.ts index e9d2c400a0..709beac55c 100644 --- a/apps/ai/src/mcp/tools/llm-tools.ts +++ b/apps/ai/src/mcp/tools/llm-tools.ts @@ -84,6 +84,8 @@ export interface BuildMapleToolsOptions { readonly surface: McpToolSurface /** The agent-session identity of the run, stamped on every tool span — see `withToolCallContent`. */ readonly sessionAttributes?: Readonly> + /** Sees every successful answer this build dispatches; a review pass tracks what it read here. */ + readonly onAnswer?: (tool: string, answer: string) => void } /** @@ -192,6 +194,7 @@ export const buildMapleToolkit = ( ? fail(toolResultText(result)) : Effect.succeed(toolResultText(result)), ), + Effect.tap((answer) => Effect.sync(() => options.onAnswer?.(definition.name, answer))), ) const handle = (params: unknown): Effect.Effect => { if (gated) { diff --git a/apps/ai/src/mcp/tools/pull-request.test.ts b/apps/ai/src/mcp/tools/pull-request.test.ts index 8df39a0a58..f94454654b 100644 --- a/apps/ai/src/mcp/tools/pull-request.test.ts +++ b/apps/ai/src/mcp/tools/pull-request.test.ts @@ -6,10 +6,12 @@ import { assert, describe, it } from "vitest" import { annotatePatch, classifyChangedFile, + pathsInDiffAnswer, renderChangedFiles, renderFileDiffs, renderPullRequestContext, reviewCallBudget, + reviewablePathsInListing, } from "./pull-request" describe("annotatePatch", () => { @@ -168,3 +170,51 @@ describe("renderPullRequestContext", () => { assert.isBelow(text.indexOf("typecheck: failure · 2 errors"), text.indexOf("lint: success")) }) }) + +/** Coverage reads these answers back, so the parsers are tested against the renderers themselves. */ +describe("reading the answers back", () => { + const file = (path: string, overrides: Partial<{ previousPath: string; patch: null }> = {}) => ({ + path, + previousPath: overrides.previousPath ?? null, + status: "modified" as const, + additions: 2, + deletions: 1, + patch: overrides.patch === null ? null : "@@ -1 +1,2 @@\n+x\n+y", + }) + + it("finds the reviewed files that have a patch in a pr_changed_files answer", () => { + const answer = renderChangedFiles("octo/shop", 7, [ + file("src/orders.ts"), + file("src/orders.test.ts"), + file("src/moved.ts", { previousPath: "src/old.ts" }), + file("infra/alchemy.run.ts"), + file("package.json"), + file("docs/orders.md"), + file("bun.lock"), + file("assets/logo.ts", { patch: null }), + ]).content[0]!.text + assert.deepEqual(reviewablePathsInListing(answer), [ + "src/orders.ts", + "src/orders.test.ts", + "src/moved.ts", + "infra/alchemy.run.ts", + "package.json", + ]) + }) + + it("finds only the files whose diff a pr_file_diff answer showed", () => { + const big = { + ...file("src/big.ts"), + patch: `@@ -1,0 +1,1400 @@\n${`+${"x".repeat(60)}\n`.repeat(1400)}`, + } + const answer = renderFileDiffs( + [file("src/a.ts"), big, file("src/c.ts")], + ["src/a.ts", "src/missing.ts", "src/big.ts", "src/c.ts"], + ).content[0]!.text + assert.include(answer, "Not included") + const shown = pathsInDiffAnswer(answer) + // The unknown path shows nothing, and the deferred one is named without its diff. + assert.deepEqual(shown, ["src/a.ts", "src/c.ts"]) + assert.include(answer, "request them in one more call: src/big.ts") + }) +}) diff --git a/apps/ai/src/mcp/tools/pull-request.ts b/apps/ai/src/mcp/tools/pull-request.ts index a6b9717a4b..13ec1492e0 100644 --- a/apps/ai/src/mcp/tools/pull-request.ts +++ b/apps/ai/src/mcp/tools/pull-request.ts @@ -134,6 +134,28 @@ const describeFile = (file: PullRequestFile): string => { /** Kinds a review reads: code, deploy and runtime config, and tests for the tests lens. */ const REVIEWED_KINDS: ReadonlySet = new Set(["source", "infra", "config", "test"]) +const REVIEWED_KIND_NAMES: ReadonlySet = new Set(REVIEWED_KINDS) +const LISTED_FILE = /^- (.+?)(?: \(was .+\))? · [a-z]+ · \+\d+\/-\d+ · ([a-z]+)(, no patch)?$/ +const DIFF_HEADER = /^## (.+) · [a-z]+ · \+\d+\/-\d+$/ + +/** + * The files a `pr_changed_files` answer asks the reviewer to read in `pr_file_diff`: the reviewed + * kinds that have a patch. Parsed back from {@link renderChangedFiles}, so the two change together. + */ +export const reviewablePathsInListing = (answer: string): ReadonlyArray => + answer.split("\n").flatMap((line) => { + const match = LISTED_FILE.exec(line) + if (match === null || match[3] !== undefined) return [] + return REVIEWED_KIND_NAMES.has(match[2]!) ? [match[1]!] : [] + }) + +/** The files whose diff a `pr_file_diff` answer actually showed, from {@link renderFileDiff}'s headers. */ +export const pathsInDiffAnswer = (answer: string): ReadonlyArray => + answer.split("\n").flatMap((line) => { + const match = DIFF_HEADER.exec(line) + return match === null ? [] : [match[1]!] + }) + /** * Tool calls a review of this many files should need: a diff and two lookups per file, plus the * file list, the pull request context, the repository's rules and the submission. Stated to the diff --git a/packages/backend/src/services/pr-review/findings.test.ts b/packages/backend/src/services/pr-review/findings.test.ts index 1e3fd2396c..6f4a9f2c68 100644 --- a/packages/backend/src/services/pr-review/findings.test.ts +++ b/packages/backend/src/services/pr-review/findings.test.ts @@ -2,6 +2,7 @@ import { PrReviewFinding, type PullRequestReviewThread } from "@maple/domain/htt import { assert, describe, it } from "vitest" import { dismissedFindings, + followUpScope, nextHandles, pathIgnored, renderFollowUp, @@ -140,3 +141,27 @@ describe("renderFollowUp", () => { assert.include(text, "- F1 · src/a.ts:10 · correctness · warn · off by one") }) }) + +describe("followUpScope", () => { + const kickoff = (changedPaths: ReadonlyArray | undefined) => + [ + "Review pull request #7 of octo/shop.", + "", + ...renderFollowUp({ previousSha: "abcdef1234", changedPaths, open: [] }), + ].join("\n") + + it("reads back the files renderFollowUp names", () => { + assert.deepEqual(followUpScope(kickoff(["src/a.ts", "src/b.ts"])), ["src/a.ts", "src/b.ts"]) + }) + + it("reads back the listed files when the list is cut", () => { + const paths = Array.from({ length: 70 }, (_, i) => `src/f${i}.ts`) + assert.deepEqual(followUpScope(kickoff(paths)), paths.slice(0, 60)) + }) + + it("is empty when nothing changed and undefined when the kickoff names no scope", () => { + assert.deepEqual(followUpScope(kickoff([])), []) + assert.isUndefined(followUpScope(kickoff(undefined))) + assert.isUndefined(followUpScope("Review pull request #7 of octo/shop.")) + }) +}) diff --git a/packages/backend/src/services/pr-review/findings.ts b/packages/backend/src/services/pr-review/findings.ts index 94a420a03b..9bfaeb961f 100644 --- a/packages/backend/src/services/pr-review/findings.ts +++ b/packages/backend/src/services/pr-review/findings.ts @@ -108,6 +108,11 @@ export const resolvedByHandle = ( return open.filter((finding) => wanted.has(finding.handle)) } +const NOTHING_CHANGED = + "No file changed since then (a rebase or an empty commit): check the open findings and submit." +const CHANGED_SINCE = "Files changed since then; review these, the rest was already reviewed: " +const MAX_LISTED_CHANGES = 60 + /** The kickoff section for a later push: what changed since, and what is still open. */ export const renderFollowUp = (input: { readonly previousSha: string @@ -118,8 +123,8 @@ export const renderFollowUp = (input: { if (input.changedPaths !== undefined) { lines.push( input.changedPaths.length === 0 - ? "No file changed since then (a rebase or an empty commit): check the open findings and submit." - : `Files changed since then; review these, the rest was already reviewed: ${input.changedPaths.slice(0, 60).join(", ")}${input.changedPaths.length > 60 ? `, and ${input.changedPaths.length - 60} more` : ""}.`, + ? NOTHING_CHANGED + : `${CHANGED_SINCE}${input.changedPaths.slice(0, MAX_LISTED_CHANGES).join(", ")}${input.changedPaths.length > MAX_LISTED_CHANGES ? `, and ${input.changedPaths.length - MAX_LISTED_CHANGES} more` : ""}.`, ) } if (input.open.length === 0) { @@ -136,6 +141,25 @@ export const renderFollowUp = (input: { return lines } +/** + * The files a later push's kickoff asks the reviewer to read, parsed back from + * {@link renderFollowUp}: none when nothing changed, `undefined` when the kickoff names no scope + * (a first review, or a push whose comparison failed) and every file is in scope. + */ +export const followUpScope = (kickoff: string): ReadonlyArray | undefined => { + for (const line of kickoff.split("\n")) { + if (line === NOTHING_CHANGED) return [] + if (line.startsWith(CHANGED_SINCE)) { + return line + .slice(CHANGED_SINCE.length) + .replace(/(, and \d+ more)?\.$/, "") + .split(", ") + .filter((path) => path !== "") + } + } + return undefined +} + const globToRegExp = (pattern: string): RegExp => { const escaped = pattern .trim()