fix(review): persist review state where it is read back

bin/cli.tsmodified+24-2

Changes to bin/cli.ts

		⋯1,613 linesShow the 1,613 lines above this hunk@@ -1614,6 +1614,8 @@ cli
1614	1614	let headSha = ''
1615	1615	let seenFingerprints: string[] = []
1616	1616	let gitProvider: GitProvider | null = null
	1617	+ let prBody: string | null = null
	1618	+ let wasPaused = false
1617	1619
1618	1620	if (prNumber) {
1619	1621	gitProvider = await providerFor(config, 'reviewing a pull request', log)
		⋯7 linesShow the 7 lines above this hunk@@ -1627,7 +1629,13 @@ cli
1627	1629	const pr = prs.find(candidate => candidate.number === number)
1628	1630	const state = parseReviewState(pr?.body)
1629	1631	seenFingerprints = state?.fingerprints ?? []
1630		- headSha = state?.reviewedSha ?? ''
	1632	+ prBody = pr?.body ?? null
	1633	+ wasPaused = Boolean(state?.paused)
	1634	+
	1635	+ // The commit being reviewed now, not the one reviewed last time.
	1636	+ // Reading it from the stored state recorded a stale sha, so the next
	1637	+ // run could not tell whether this commit had been seen.
	1638	+ headSha = await gitProvider.getPullRequestHeadSha(number)
1631	1639	}
1632	1640	else {
1633	1641	const base = options.base || config.repository?.baseBranch || 'main'
		⋯100 linesShow the 100 lines above this hunk@@ -1734,7 +1742,21 @@ cli
1734	1742	}
1735	1743
1736	1744	assertSupports(gitProvider!, 'inlineReviewComments', 'createReview', 'posting a review')
1737		- await gitProvider.createReview(Number.parseInt(prNumber, 10), prepared)
	1745	+ const reviewedNumber = Number.parseInt(prNumber, 10)
	1746	+ await gitProvider.createReview(reviewedNumber, prepared)
	1747	+
	1748	+ // The review has landed; recording where it got to is bookkeeping, so a
	1749	+ // failure here must not present as a failed review.
	1750	+ try {
	1751	+ const { upsertReviewState } = await import('../src/review/marker')
	1752	+ await gitProvider.updatePullRequest(reviewedNumber, {
	1753	+ body: upsertReviewState(prBody, { ...prepared.state, ...(wasPaused ? { paused: true } : {}) }),
	1754	+ })
	1755	+ }
	1756	+ catch (error) {
	1757	+ logger.warn(`Could not record review state on PR #${prNumber}: ${error instanceof Error ? error.message : String(error)}`)
	1758	+ }
	1759	+
1738	1760	logger.success(`✅ Reviewed PR #${prNumber}: ${result.findings.length} finding(s)`)
1739	1761	}
1740	1762	catch (error) {

src/commands/handlers.tsmodified+8-10

Changes to src/commands/handlers.ts

		⋯3 linesShow the 3 lines above this hunk@@ -4,7 +4,6 @@ import type { CommandContext, CommandHandler, CommandOutcome } from './dispatche
4	4	import process from 'node:process'
5	5	import { createAiClient } from '../ai'
6	6	import { attemptFix } from '../ci/fix'
7		-import { serializeReviewState } from '../review/marker'
8	7
9	8	/** Everything the built-in handlers need to act. */
10	9	export interface HandlerDeps {
		⋯133 linesShow the 133 lines above this hunk@@ -144,16 +143,15 @@ async function setPaused(deps: HandlerDeps, prNumber: number, paused: boolean):
144	143	if (!pr)
145	144	return
146	145
147		- const { parseReviewState } = await import('../review/marker')
	146	+ const { parseReviewState, upsertReviewState } = await import('../review/marker')
148	147	const state = parseReviewState(pr.body)
149	148
150		- const marker = serializeReviewState({
151		- reviewedSha: state?.reviewedSha ?? '',
152		- fingerprints: state?.fingerprints ?? [],
153		- reviewedAt: state?.reviewedAt ?? new Date().toISOString(),
154		- ...(paused ? { paused: true } : {}),
	149	+ await deps.provider.updatePullRequest(prNumber, {
	150	+ body: upsertReviewState(pr.body, {
	151	+ reviewedSha: state?.reviewedSha ?? '',
	152	+ fingerprints: state?.fingerprints ?? [],
	153	+ reviewedAt: state?.reviewedAt ?? new Date().toISOString(),
	154	+ ...(paused ? { paused: true } : {}),
	155	+ }),
155	156	})
156		-
157		- const body = pr.body.replace(/<!--\s*buddy:review[\s\S]*?-->/, '').trimEnd()
158		- await deps.provider.updatePullRequest(prNumber, { body: `${body}\n\n${marker}` })
159	157	}

src/review/marker.tsmodified+22-0

Changes to src/review/marker.ts

		⋯62 linesShow the 62 lines above this hunk@@ -63,6 +63,28 @@ export function parseReviewState(body: string | null | undefined): ReviewState |
63	63	}
64	64	}
65	65
	66	+/**
	67	+ * Replace the review-state marker in a pull request body.
	68	+ *
	69	+ * The marker has to live on the pull request itself: `parseReviewState` reads
	70	+ * `pr.body` and nothing else, so state written anywhere else — a review
	71	+ * summary, a comment — is written and never read back.
	72	+ *
	73	+ * @param body - Current pull request body
	74	+ * @param state - State to store
	75	+ * @returns The body with exactly one, current, marker
	76	+ * @example
	77	+ * ```ts
	78	+ * await provider.updatePullRequest(number, {
	79	+ * body: upsertReviewState(pr.body, { reviewedSha, fingerprints, reviewedAt }),
	80	+ * })
	81	+ * ```
	82	+ */
	83	+export function upsertReviewState(body: string | null | undefined, state: Omit<ReviewState, 'schemaVersion'>): string {
	84	+ const withoutMarker = (body ?? '').replace(MARKER_REGEX, '').trimEnd()
	85	+ return `${withoutMarker}\n\n${serializeReviewState(state)}`
	86	+}
	87	+
66	88	/**
67	89	* Whether a pull request needs reviewing.
68	90	*

src/review/poster.tsmodified+12-0

Changes to src/review/poster.ts

		@@ -1,4 +1,5 @@
1	1	import type { ReviewFinding, ReviewResult } from './findings'
	2	+import type { ReviewState } from './marker'
2	3	import { fingerprint } from './findings'
3	4	import { serializeReviewState } from './marker'
4	5
		⋯20 linesShow the 20 lines above this hunk@@ -25,6 +26,12 @@ export interface PreparedReview {
25	26	comments: InlineComment[]
26	27	/** `REQUEST_CHANGES` only when configured and warranted */
27	28	event: 'COMMENT' | 'REQUEST_CHANGES'
	29	+ /**
	30	+ * State this review established, for the caller to persist onto the pull
	31	+ * request. The copy inside `body` is an audit trail of what this particular
	32	+ * review saw; only the copy on `pr.body` is ever read back.
	33	+ */
	34	+ state: Omit<ReviewState, 'schemaVersion'>
28	35	}
29	36
30	37	/**
		⋯52 linesShow the 52 lines above this hunk@@ -83,6 +90,11 @@ export function prepareReview(
83	90	body: renderSummary(result, counts, options.headSha, fingerprints),
84	91	comments,
85	92	event: options.requestChangesOn === 'critical' && hasCritical ? 'REQUEST_CHANGES' : 'COMMENT',
	93	+ state: {
	94	+ reviewedSha: options.headSha,
	95	+ fingerprints,
	96	+ reviewedAt: new Date().toISOString(),
	97	+ },
86	98	}
87	99	}
88	100

src/review/run.tsmodified+39-1

Changes to src/review/run.ts

		⋯7 linesShow the 7 lines above this hunk@@ -8,7 +8,8 @@ import { getDefaultLogger } from '../utils/logger'
8	8	import { parseUnifiedDiff } from './diff'
9	9	import { reviewDiff } from './engine'
10	10	import { composeInstructions, loadGuidelines } from './guidelines'
11		-import { needsReview, parseReviewState } from './marker'
	11	+import { needsReview, parseReviewState, upsertReviewState } from './marker'
	12	+import type { PreparedReview } from './poster'
12	13	import { prepareReview } from './poster'
13	14
14	15	/** Inputs to a full review of a pull request. */
		⋯78 linesShow the 78 lines above this hunk@@ -93,6 +94,7 @@ export async function runReviewForPR(options: RunReviewOptions): Promise<string>
93	94
94	95	assertSupports(provider, 'inlineReviewComments', 'createReview', 'posting a review')
95	96	await provider.createReview(prNumber, prepared)
	97	+ await persistReviewState(provider, prNumber, pr.body, prepared.state, state?.paused, logger)
96	98	return `Posted ${analysis.findings.length} static-analysis finding(s).`
97	99	}
98	100
		⋯34 linesShow the 34 lines above this hunk@@ -133,8 +135,44 @@ export async function runReviewForPR(options: RunReviewOptions): Promise<string>
133	135
134	136	assertSupports(provider, 'inlineReviewComments', 'createReview', 'posting a review')
135	137	await provider.createReview(prNumber, prepared)
	138	+ await persistReviewState(provider, prNumber, pr.body, prepared.state, state?.paused, logger)
136	139
137	140	return result.findings.length === 0
138	141	? 'Reviewed — nothing to report.'
139	142	: `Reviewed — ${result.findings.length} finding(s) posted.`
140	143	}
	144	+
	145	+/**
	146	+ * Write the state a review established back onto the pull request.
	147	+ *
	148	+ * Separated from posting because the review itself has already landed by this
	149	+ * point: if updating the body fails — a token without write access to the
	150	+ * description, say — the findings are still on the pull request, and the worst
	151	+ * case is that the next run reviews the same commit again. That is a better
	152	+ * failure than losing the review to a bookkeeping error.
	153	+ *
	154	+ * @param provider - Git provider to write through
	155	+ * @param prNumber - Pull request to update
	156	+ * @param body - Current pull request body
	157	+ * @param state - State this review established
	158	+ * @param paused - Carried forward so persisting does not silently resume a
	159	+ * paused pull request
	160	+ * @param logger - Logger for the failure path
	161	+ */
	162	+async function persistReviewState(
	163	+ provider: GitProvider,
	164	+ prNumber: number,
	165	+ body: string | null | undefined,
	166	+ state: PreparedReview['state'],
	167	+ paused: boolean | undefined,
	168	+ logger: Logger,
	169	+): Promise<void> {
	170	+ try {
	171	+ await provider.updatePullRequest(prNumber, {
	172	+ body: upsertReviewState(body, { ...state, ...(paused ? { paused: true } : {}) }),
	173	+ })
	174	+ }
	175	+ catch (error) {
	176	+ logger.warn(`Could not record review state on PR #${prNumber}: ${error instanceof Error ? error.message : String(error)}`)
	177	+ }
	178	+}

test/review-engine.test.tsmodified+71-1

Changes to test/review-engine.test.ts

		⋯3 linesShow the 3 lines above this hunk@@ -4,7 +4,7 @@ import { describe, expect, it } from 'bun:test'
4	4	import { parseUnifiedDiff, renderDiffForReview } from '../src/review/diff'
5	5	import { defaultIncludePath, reviewDiff } from '../src/review/engine'
6	6	import { dedupeFindings, fingerprint, validateFindings } from '../src/review/findings'
7		-import { needsReview, parseReviewState, serializeReviewState } from '../src/review/marker'
	7	+import { needsReview, parseReviewState, serializeReviewState, upsertReviewState } from '../src/review/marker'
8	8	import { prepareReview, renderFinding } from '../src/review/poster'
9	9
10	10	const SAMPLE_DIFF = `diff --git a/src/app.ts b/src/app.ts
		⋯435 linesShow the 435 lines above this hunk@@ -446,3 +446,73 @@ describe('reviewDiff', () => {
446	446	expect(ai.requests[0].system).toContain('less certain')
447	447	})
448	448	})
	449	+
	450	+describe('review state round-trip', () => {
	451	+ it('success case - prepareReview exposes the state for the caller to persist', () => {
	452	+ // The marker was only ever written into the review summary, which nothing
	453	+ // reads back — so `skipIfReviewed` never skipped and dedupe never engaged.
	454	+ const prepared = prepareReview(
	455	+ { summary: 'Adds logging.', walkthrough: [], findings: [makeFinding()], effort: 2, omittedFiles: [] },
	456	+ { headSha: 'sha123' },
	457	+ )
	458	+
	459	+ expect(prepared.state.reviewedSha).toBe('sha123')
	460	+ expect(prepared.state.fingerprints.length).toBeGreaterThan(0)
	461	+ })
	462	+
	463	+ it('success case - state written to a PR body is readable again', () => {
	464	+ const prepared = prepareReview(
	465	+ { summary: 'x', walkthrough: [], findings: [makeFinding()], effort: 1, omittedFiles: [] },
	466	+ { headSha: 'abc' },
	467	+ )
	468	+
	469	+ const body = upsertReviewState('Original description.', prepared.state)
	470	+ const read = parseReviewState(body)
	471	+
	472	+ expect(read?.reviewedSha).toBe('abc')
	473	+ expect(read?.fingerprints).toEqual(prepared.state.fingerprints)
	474	+ expect(body).toContain('Original description.')
	475	+ })
	476	+
	477	+ it('success case - the round-trip satisfies needsReview', () => {
	478	+ // The whole point: a second run at the same commit must skip.
	479	+ const prepared = prepareReview(
	480	+ { summary: 'x', walkthrough: [], findings: [], effort: 1, omittedFiles: [] },
	481	+ { headSha: 'head-1' },
	482	+ )
	483	+
	484	+ const state = parseReviewState(upsertReviewState('desc', prepared.state))
	485	+
	486	+ expect(needsReview(state, 'head-1')).toBe(false)
	487	+ expect(needsReview(state, 'head-2')).toBe(true)
	488	+ })
	489	+
	490	+ it('edge case - replaces the existing marker rather than stacking them', () => {
	491	+ const first = upsertReviewState('desc', { reviewedSha: 'one', fingerprints: [], reviewedAt: 'x' })
	492	+ const second = upsertReviewState(first, { reviewedSha: 'two', fingerprints: [], reviewedAt: 'y' })
	493	+
	494	+ expect(second.match(/<!--\s*buddy:review/g)).toHaveLength(1)
	495	+ expect(parseReviewState(second)?.reviewedSha).toBe('two')
	496	+ expect(second).toContain('desc')
	497	+ })
	498	+
	499	+ it('edge case - persisting a review does not resume a paused pull request', () => {
	500	+ const paused = upsertReviewState('desc', { reviewedSha: 'one', fingerprints: [], reviewedAt: 'x', paused: true })
	501	+ const previous = parseReviewState(paused)
	502	+
	503	+ const next = upsertReviewState(paused, {
	504	+ reviewedSha: 'two',
	505	+ fingerprints: [],
	506	+ reviewedAt: 'y',
	507	+ ...(previous?.paused ? { paused: true } : {}),
	508	+ })
	509	+
	510	+ expect(parseReviewState(next)?.paused).toBe(true)
	511	+ })
	512	+
	513	+ it('edge case - tolerates an empty pull request body', () => {
	514	+ const body = upsertReviewState(null, { reviewedSha: 'x', fingerprints: [], reviewedAt: 'y' })
	515	+
	516	+ expect(parseReviewState(body)?.reviewedSha).toBe('x')
	517	+ })
	518	+})