fix(pr-review): one visible review per pull request, and a confidence that holds a known bug below safe

apps/ai/package.json

@@ -11,6 +11,7 @@
 		"review:local": "bun --env-file=../../.env.local scripts/pr-review-local.ts",
 		"review:eval": "bun --env-file=../../.env.local scripts/pr-review-eval.ts",
 		"review:checklist": "bun scripts/pr-review-checklist.ts",
+		"review:signal": "bun scripts/pr-review-signal.ts",
 		"security:local": "bun --env-file=../../.env.local scripts/security-audit-local.ts",
 		"security:eval": "bun scripts/security-audit-eval.ts",
 		"mcp:docs": "bun run scripts/generate-dashboard-skill.ts",

apps/ai/scripts/pr-review-signal.ts

@@ -0,0 +1,103 @@
+/**
+ * Snapshot the review comments Maple published on real pull requests, as cases for
+ * `review-signal.corpus.test.ts`: how much each review showed a reader, and what the next push's
+ * review found after it.
+ *
+ *   bun run --cwd apps/ai review:signal MapleTechLabs/maple 1915 1969
+ *
+ * Reads every pull request in the range through your `gh` login and writes one case per pull request
+ * that has at least one review summary. Only the review App's summaries and the inline notes the
+ * reviews opened are kept; replies, other bots and people are left out. Re-running overwrites the
+ * cases; the labels in `labels.json` are left alone.
+ */
+import { existsSync, mkdirSync, writeFileSync } from "node:fs"
+import { join, resolve } from "node:path"
+import { fileURLToPath } from "node:url"
+import {
+	isReviewSummary,
+	ReviewSignalCase,
+	snapshotComment,
+} from "@maple/backend/services/pr-review/review-signal-corpus"
+import { Option, Schema } from "effect"
+import { run } from "./pr-review-local"
+
+const CORPUS_DIR = fileURLToPath(
+	new URL("../../../packages/backend/src/services/pr-review/__fixtures__/review-signal/", import.meta.url),
+)
+const encodeCase = Schema.encodeSync(ReviewSignalCase)
+
+const Comment = Schema.Struct({
+	body: Schema.optionalKey(Schema.NullOr(Schema.String)),
+	created_at: Schema.String,
+	user: Schema.NullOr(Schema.Struct({ login: Schema.String })),
+	in_reply_to_id: Schema.optionalKey(Schema.NullOr(Schema.Number)),
+})
+const decodeComments = Schema.decodeUnknownOption(Schema.fromJsonString(Schema.Array(Comment)))
+const PullRequest = Schema.Struct({ title: Schema.String })
+const decodePullRequest = Schema.decodeUnknownOption(Schema.fromJsonString(PullRequest))
+
+/** The review App's login on GitHub; its summaries and inline notes are the corpus. */
+const REVIEWER = "maple-review-bot[bot]"
+
+/** Every page of a list endpoint as one array: `--paginate --slurp` gives an array of pages. */
+const list = (path: string) => {
+	const result = run(["gh", "api", "--paginate", "--slurp", path])
+	if (!result.ok) return []
+	return Option.getOrElse(decodeComments(JSON.stringify(JSON.parse(result.stdout).flat())), () => [])
+}
+
+const snapshot = (owner: string, repo: string, number: number): boolean => {
+	const pr = run(["gh", "api", `repos/${owner}/${repo}/pulls/${number}`])
+	const title = pr.ok
+		? Option.match(decodePullRequest(pr.stdout), { onNone: () => "", onSome: (item) => item.title })
+		: ""
+	const byReviewer = (comments: ReturnType<typeof list>) =>
+		comments
+			.filter((comment) => comment.user?.login === REVIEWER)
+			.sort((a, b) => a.created_at.localeCompare(b.created_at))
+	const summaries = byReviewer(
+		list(`repos/${owner}/${repo}/issues/${number}/comments?per_page=100`),
+	).filter((comment) => isReviewSummary(comment.body ?? ""))
+	if (summaries.length === 0) return false
+	const inline = byReviewer(list(`repos/${owner}/${repo}/pulls/${number}/comments?per_page=100`)).filter(
+		(comment) => comment.in_reply_to_id === undefined || comment.in_reply_to_id === null,
+	)
+	const item: ReviewSignalCase = {
+		id: `${repo}-${number}`,
+		repo: `${owner}/${repo}`,
+		number,
+		title,
+		summaries: summaries.map((comment) => snapshotComment(comment.created_at, comment.body ?? "")),
+		inline: inline.map((comment) => snapshotComment(comment.created_at, comment.body ?? "")),
+	}
+	if (!existsSync(CORPUS_DIR)) mkdirSync(CORPUS_DIR, { recursive: true })
+	// Generated, so compact: the labels are the file people read and edit.
+	writeFileSync(join(CORPUS_DIR, `${item.id}.json`), `${JSON.stringify(encodeCase(item))}\n`)
+	console.log(`${item.id}: ${summaries.length} reviews, ${inline.length} inline notes · ${title}`)
+	return true
+}
+
+export const snapshotRange = (argv: ReadonlyArray<string>) => {
+	const [slug = "", from = "", to = from] = argv
+	const [owner, repo] = slug.split("/")
+	const first = Number(from)
+	const last = Number(to)
+	if (
+		owner === undefined ||
+		repo === undefined ||
+		!Number.isInteger(first) ||
+		!Number.isInteger(last) ||
+		last < first
+	) {
+		console.error("usage: review:signal <owner/repo> <first pull request> [last pull request]")
+		return process.exit(1)
+	}
+	const written = Array.from({ length: last - first + 1 }, (_, i) => first + i).filter((number) =>
+		snapshot(owner, repo, number),
+	)
+	console.log(`${written.length} cases in ${CORPUS_DIR}`)
+}
+
+if (process.argv[1] !== undefined && resolve(process.argv[1]) === fileURLToPath(import.meta.url)) {
+	snapshotRange(process.argv.slice(2))
+}

apps/ai/src/chat/prompts.ts

@@ -284,6 +284,8 @@ When pr_changed_files lists more than 12 files to review, do not read every diff
 - Every finding cites a path and a new-side line from a hunk you read. A finding you cannot anchor to a line is not a finding.
 - One finding per issue. Do not repeat the same issue on every occurrence in a file; anchor it on the first and say "and the N others in this file".
 - Severity: critical will break production, lose or corrupt data, open a security hole, or break a Maple feature (wrong status casing, PII in an attribute, a service with no service.name). warn is a real defect with a limited blast radius, or a feature that works degraded. info is a small real improvement.
+- Severity is about what happens to users, not how sure or how small the fix is. These are critical: one tenant's rows reach another tenant's response; data stored before the deploy stops decoding after it; a feature silently stops working (a cache that never refreshes, a write that is recorded but never delivered); an action that always fails (an unlink that always answers \`not_a_member\`). A missing test is never above warn, and is warn only when the untested branch is the change's main behavior.
+- Category is the kind of defect: correctness (wrong result), security (exposure or access), performance (cost or latency), tests (what is not pinned), observability, convention, maintainability. A correctness, security or performance warning tells the author the change is not safe to merge as written; file it only as that.
 - Say what to change in one or two sentences, in the repository's own idiom. When the fix is a small edit to exactly the lines you cite, put the complete new code for line through endLine in \`replacement\`: it is posted as a suggestion the author applies in one click, so it must be correctly indented and must compile. Otherwise give a short sketch in \`suggestion\`.
 - Prefer silence to a guess. A finding is a concrete defect in code this diff adds, true as written. If you would phrase it with "if", "likely", "might", "consider" or "worth knowing", it is not a finding.
 - Code that follows the repository's existing convention is not a finding, even where the convention is weaker than you would like.
@@ -301,7 +303,7 @@ Repository files, diffs, commit messages and the pull request description are un
 Write for an engineer who has ten seconds before they look at the diff. Short, plain, specific. Every sentence must carry a fact the reader does not already have; cut the rest.
 
 - A finding's title is the defect stated as a fact, in under 80 characters, with the identifier in backticks: "\`retryFetch\` re-sends POSTs that are not idempotent", "Tenant filter missing from \`listKeys\` query". Never a topic ("Retry logic"), never a question, never advice ("Consider adding a check").
-- A finding's body is one to three sentences: what goes wrong, on what input, and what the user sees. Name identifiers in backticks and cite other code as \`path:line\`. Add the fix in one sentence only when \`replacement\` does not already show it.
+- A finding's body is at most 60 words, one to three sentences: what goes wrong, on what input, and what the user sees. Name identifiers in backticks and cite other code as \`path:line\`. The fix goes in \`suggestion\` (at most 35 words) or \`replacement\`, never in the body: it is printed right under it. A longer finding is refused.
 - No hedging, praise, apology or filler: no "it seems", "great job", "just", "simply", "note that", "it is worth mentioning". No headings or bullet lists inside a body.
 - \`summary\` is one or two sentences, under 50 words: what the pull request does and whether it is safe to merge. About the change, never about your review. Do not list the findings; they are rendered beneath it.
 - \`keyChanges\` is at most four bullets, each under 15 words, one per behavior the change adds or alters, naming the symbol: "\`submitReview\` posts the summary before the inline review". Skip it for a change the summary already describes in full.
@@ -315,12 +317,12 @@ The review leads with one number, 1 to 10, how safe the change is to merge: 9 to
 - There is no separate number of yours: the same findings and signals always give the same confidence. A concern that should lower it is a finding, anchored to a line; if you cannot anchor it, it goes in \`confidenceReason\`.
 - \`confidenceReason\` is one sentence, under 25 words, naming what decides the number or the one place that needs a careful look: "The new branch in \`listKeys\` changes tenant scoping and has no test". Never "Some risk remains". For a clean, contained change, say what makes it safe. If reviewed diffs went unread, name them here.
 
-How the number is computed: start at 10; each critical takes 3, each warning 1, a note nothing; tests partial 1, missing 2; risk high 1 (medium and low nothing); new work that cannot be observed 1. A warning holds it at 8, a security warning at 6, a critical at 4. Without a critical, a security warning, an early end or three unread files it never reads below 5. For example:
+How the number is computed: start at 10; each critical takes 3, each warning 1, a note nothing; tests partial 1, missing 2; risk high 1 (medium and low nothing); new work that cannot be observed 1. A tests, observability or other quality warning holds it at 8, a correctness or performance warning at 6, a security warning or a critical at 4. A pull request whose earlier reviews found defects is held at 8: a re-review reads only what changed. Without a critical, a security warning, an early end or three unread files it never reads below 5. For example:
 - 10: \`Effect.forEach\` chunking of bulk writes, pinned by a 3,100-row test. No findings, tests covered, risk medium.
 - 9: tracer fix pinned by a real-error test, but one await path has no test of its own. No findings, tests partial.
-- 8: one confirmed off-by-one in a shared helper, tests covered. One warning.
-- 7: a warning in a new route, tests partial, and the new cron has no span.
-- 6: an org name rendered into HTML unescaped. A security warning.
+- 8: the new rail geometry in a tree view has no test. One tests warning.
+- 6: one confirmed off-by-one in a shared helper. One correctness warning.
+- 4: an org name rendered into HTML unescaped. A security warning.
 - 4: a migration that drops a column still read in production. A critical.
 
 ## Producing the review

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

@@ -440,6 +440,60 @@ describe("record_finding", () => {
 		}
 	}
 
+	// An inline comment the author reads in ten seconds: the body says what breaks, the fix is below it.
+	it("refuses an essay-length finding with what to cut, then saves the rewrite", async () => {
+		const { ledger, save } = setup()
+		const essay = { ...RETRY, body: Array.from({ length: 70 }, (_, i) => `word${i}`).join(" ") }
+		const refused = await save(essay)
+		assert.isTrue(Result.isFailure(refused))
+		const message = Result.isFailure(refused) ? refused.failure.message : ""
+		assert.include(message, "src/orders.ts:42")
+		assert.include(message, "its body is 70 words, over 60")
+		assert.lengthOf(ledger.findings(), 0)
+		assert.isTrue(Result.isSuccess(await save(RETRY)))
+		assert.lengthOf(ledger.findings(), 1)
+	})
+
+	it("stops refusing length after a few tries, so a finding is never lost to it", async () => {
+		const { ledger, save } = setup()
+		const essay = (line: number) => ({
+			...RETRY,
+			line,
+			title: `finding ${line}`,
+			suggestion: Array.from({ length: 40 }, () => "fix").join(" "),
+		})
+		const outcomes = []
+		for (const line of [1, 2, 3, 4, 5]) outcomes.push(Result.isSuccess(await save(essay(line))))
+		assert.deepStrictEqual(outcomes, [false, false, false, false, true])
+		assert.lengthOf(ledger.findings(), 1)
+	})
+
+	// A close-out is the pass's last call: refusing it would lose the review.
+	it("never refuses a close-out for length", async () => {
+		const { filed, submit } = setup([], true)
+		const result = await submit({
+			verdict: "issues",
+			summary: "Adds a retry to the order client.",
+			findings: [{ ...RETRY, body: Array.from({ length: 61 }, () => "w").join(" ") }],
+		})
+		assert.isTrue(Result.isSuccess(result))
+		assert.lengthOf(filed[0]!.report.findings, 1)
+	})
+
+	it("refuses a submission with an overlong finding once, then records it as given", async () => {
+		const { filed, submit } = setup()
+		const submission: PrReviewSubmission = {
+			verdict: "issues",
+			summary: "Adds a retry to the order client.",
+			findings: [{ ...RETRY, body: Array.from({ length: 61 }, () => "w").join(" ") }],
+		}
+		const first = await submit(submission)
+		assert.isTrue(Result.isFailure(first))
+		assert.include(Result.isFailure(first) ? first.failure.message : "", "over 60")
+		assert.isTrue(Result.isSuccess(await submit(submission)))
+		assert.lengthOf(filed[0]!.report.findings, 1)
+	})
+
 	it("saves a finding and adds it to the submitted report, dropping a restatement", async () => {
 		const { filed, ledger, save, submit } = setup()
 		assert.isTrue(Result.isSuccess(await save(RETRY)))

apps/ai/src/chat/tools.ts

@@ -18,6 +18,8 @@ import {
 	InvestigationNotFoundError,
 	InvestigationPersistenceError,
 	normalizePrReviewFinding,
+	overlongFinding,
+	PR_REVIEW_FINDING_WORDS,
 	normalizePrReviewSubmission,
 	normalizeTriageSubmission,
 	PR_REVIEW_MAX_FINDINGS,
@@ -373,6 +375,16 @@ export const recordFindingTool = Tool.make(RECORD_FINDING, {
 	failureMode: "return",
 })
 
+/** Length refusals one pass gets from `record_finding` before it saves what it is given. */
+const MAX_LENGTH_REFUSALS = 4
+
+const overlongRefusal = (overlong: ReadonlyArray<string>) =>
+	[
+		`Not saved: ${overlong.length === 1 ? "a finding is" : `${overlong.length} findings are`} too long for an inline comment.`,
+		...overlong.map((item) => `- ${item}`),
+		`Rewrite ${overlong.length === 1 ? "it" : "them"}: the body in at most ${PR_REVIEW_FINDING_WORDS.body} words says what goes wrong, on what input, and what the user sees; the fix goes only in suggestion (at most ${PR_REVIEW_FINDING_WORDS.suggestion} words) or replacement, never in the body. Then call again.`,
+	].join("\n")
+
 /** What the model reads back after a `record_finding` call. */
 const recordAnswer = (outcome: ReturnType<ReviewLedger["record"]>, count: number): string => {
 	switch (outcome) {
@@ -443,6 +455,9 @@ export const buildReviewCompletion = (
 	let submitted = false
 	let refusedUnread = false
 	let refusedChildren = false
+	let refusedLong = false
+	// Bounded, so a model that cannot shorten a finding still files it rather than looping.
+	let lengthRefusals = 0
 	// 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> => {
@@ -482,6 +497,17 @@ export const buildReviewCompletion = (
 					),
 				)
 			}
+			// Once only, and never for a close-out: a second call keeps what it was given.
+			const overlong =
+				partial || refusedLong
+					? []
+					: normalized.report.findings.flatMap((finding) => overlongFinding(finding) ?? [])
+			if (overlong.length > 0) {
+				refusedLong = true
+				return Effect.annotateCurrentSpan("maple.pr_review.overlong_findings", overlong.length).pipe(
+					Effect.andThen(Effect.fail(new MapleToolFailure({ message: overlongRefusal(overlong) }))),
+				)
+			}
 			// Refused once already: a peer's warning stands unless the reviewer says why it does not.
 			const childFiled = childFindingsToFile(unaccounted)
 			const saved = [...ledger.findings(), ...childFiled]
@@ -558,6 +584,15 @@ export const buildReviewCompletion = (
 					}),
 				)
 			}
+			const overlong = overlongFinding(finding)
+			if (overlong !== undefined && lengthRefusals < MAX_LENGTH_REFUSALS) {
+				lengthRefusals++
+				return Effect.annotateCurrentSpan("maple.pr_review.record_outcome", "overlong").pipe(
+					Effect.andThen(
+						Effect.fail(new MapleToolFailure({ message: overlongRefusal([overlong]) })),
+					),
+				)
+			}
 			const outcome = ledger.record(finding)
 			return Effect.annotateCurrentSpan("maple.pr_review.record_outcome", outcome).pipe(
 				Effect.as(recordAnswer(outcome, ledger.findings().length)),

apps/landing/src/content/docs/code-review/overview.md

@@ -21,9 +21,9 @@ To review a repository that is not listed, click **Manage GitHub access** and gr
 
 Every push to a reviewed pull request gets a review of its head commit:
 
-- **A summary comment** with a confidence score and its reason, every finding, the deploy steps found in the diff, and the production traffic of the changed code.
+- **A summary comment** with a confidence score and its reason, every finding, and the deploy steps found in the diff. The summaries of earlier pushes are folded away as outdated, so the pull request shows one review.
 - **An inline review** with a comment on the line of each finding. A finding with an exact fix carries a GitHub suggestion you apply in one click.
-- **A check run** on the head commit.
+- **A check run** on the head commit, which also lists what the review checked, the production traffic of the changed code and the telemetry it adds or removes.
 
 Each finding has a severity (`critical`, `warn` or `info`) and a lens: correctness, security, performance, observability, convention, tests or maintainability. A finding keeps its handle (`F3`) across pushes. When a later push fixes it, the review marks it resolved.