ReviewOS
Reload

also looking at this

stacks/buddy

fix(review): give pull request reviews the analyzers and learnings they skipped

#1415
Merged glennmichael123 wants to merge fix/review-parity into main
4 files +316 -106

Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.

bin/cli.tsmodified+42-104
@@ -1585,56 +1585,39 @@ cli
15851585 process.exit(1)
15861586 }
15871587
1588 const {
1589 composeInstructions,
1590 loadGuidelines,
1591 parseReviewState,
1592 prepareReview,
1593 reviewDiff,
1594 } = await import('../src/review')
1588 const { composeInstructions, reviewDiff } = await import('../src/review')
15951589 const { collectLocalDiff, formatReview, shouldFail } = await import('../src/review/local')
15961590 const { runAnalyzers } = await import('../src/analysis')
15971591
1598 let diff: string
1599 let headSha = ''
1600 let seenFingerprints: string[] = []
1601 let gitProvider: GitProvider | null = null
1602 let prBody: string | null = null
1603 let wasPaused = false
1604
1592 // A pull request review goes through the shared path, which is also what
1593 // `@buddy review` uses. It used to be reimplemented here and had drifted:
1594 // this command skipped analyzers outright for a pull request and never
1595 // read learnings at all, so the automatic review the generated workflow
1596 // runs was the one path missing both.
16051597 if (prNumber) {
1606 gitProvider = await providerFor(config, 'reviewing a pull request', log)
1607
1608 const number = Number.parseInt(prNumber, 10)
1609
1610 // Carry forward what earlier reviews already said, so a re-review
1611 // reports only what is new rather than repeating itself.
1612 const prs = await gitProvider.getPullRequests('open')
1613 const pr = prs.find(candidate => candidate.number === number)
1614
1615 // `--auto` is what the generated workflow passes. Without it this is
1616 // someone at a terminal, who has already decided they want a review.
1617 if (pr) {
1618 const { reviewSkipReason } = await import('../src/review/filters')
1619 const skip = reviewSkipReason(config, pr, options.auto ? 'automatic' : 'requested')
1620 if (skip) {
1621 log.info(`🔍 Skipping PR #${number}: ${skip}`)
1622 return
1623 }
1624 }
1598 const gitProvider = await providerFor(config, 'reviewing a pull request', log)
1599 const { runReviewForPR } = await import('../src/review/run')
16251600
1626 diff = await gitProvider.getPullRequestDiff(number)
1627 const state = parseReviewState(pr?.body)
1628 seenFingerprints = state?.fingerprints ?? []
1629 prBody = pr?.body ?? null
1630 wasPaused = Boolean(state?.paused)
1601 const status = await runReviewForPR({
1602 config,
1603 provider: gitProvider,
1604 prNumber: Number.parseInt(prNumber, 10),
1605 // `--auto` is what the generated workflow passes. Without it this is
1606 // someone at a terminal, who has already decided they want a review.
1607 trigger: options.auto ? 'automatic' : 'requested',
1608 logger: log,
1609 dryRun: Boolean(options.dryRun),
1610 ...(options.summaryOnly ? { summaryOnly: true } : {}),
1611 ...(options.profile ? { profile: options.profile as 'chill' | 'assertive' } : {}),
1612 })
16311613
1632 // The commit being reviewed now, not the one reviewed last time.
1633 // Reading it from the stored state recorded a stale sha, so the next
1634 // run could not tell whether this commit had been seen.
1635 headSha = await gitProvider.getPullRequestHeadSha(number)
1614 logger.success(`✅ ${status}`)
1615 return
16361616 }
1637 else {
1617
1618 let diff: string
1619
1620 {
16381621 const base = options.base || config.repository?.baseBranch || 'main'
16391622 const mode = options.staged ? 'staged' : options.branch ? 'branch' : 'working'
16401623 log.info(`🔍 Reviewing ${mode} changes${mode === 'branch' ? ` against ${base}` : ''}...`)
@@ -1648,10 +1631,12 @@ cli
16481631 }
16491632
16501633 // Analyzers run locally against the working tree, so a local review gets
1651 // the same secret scanning and linting a CI review does — with no token.
1634 // the same secret scanning and linting a pull request review does — with
1635 // no token. The `|| prNumber` that used to sit in this condition is why
1636 // a pull request review got none of it.
16521637 const { parseUnifiedDiff } = await import('../src/review/diff')
16531638 const changedFiles = parseUnifiedDiff(diff).files.map(file => file.path)
1654 const analysis = config.analysis?.enabled === false || prNumber
1639 const analysis = config.analysis?.enabled === false
16551640 ? { findings: [], ran: [], skipped: [] }
16561641 : await runAnalyzers({
16571642 files: changedFiles,
@@ -1676,30 +1661,18 @@ cli
16761661 return
16771662 }
16781663
1679 // Guidelines are inlined into the prompt, so they are always read from
1680 // the base branch — reading them from the PR's own branch would let a
1681 // contributor rewrite the rules their code is reviewed against.
1682 const baseRef = config.repository?.baseBranch || options.base || 'main'
1683 const guidelines = gitProvider
1684 ? await loadGuidelines(
1685 (path, ref) => gitProvider!.getFileContent(path, ref),
1686 baseRef,
1687 config.ai?.review?.guidelineFiles,
1688 logger,
1689 )
1690 : ''
1691
1664 // A local review has no provider to read the base branch through, so it
1665 // runs without repository guidelines. Reading them from the working tree
1666 // would be a different thing: guidelines are trusted context, and the
1667 // working tree is whatever the person running this has edited.
16921668 const reviewed = await reviewDiff(ai!, {
16931669 diff,
16941670 profile: options.profile ?? config.ai?.review?.profile,
16951671 summaryOnly: options.summaryOnly ?? config.ai?.review?.summaryOnly,
1696 instructions: composeInstructions({
1697 global: config.ai?.review?.instructions,
1698 guidelines,
1699 }),
1672 instructions: composeInstructions({ global: config.ai?.review?.instructions }),
17001673 pathFilters: config.ai?.review?.pathFilters,
17011674 pathInstructions: config.ai?.review?.pathInstructions,
1702 seenFingerprints,
1675 seenFingerprints: [],
17031676 logger: log,
17041677 })
17051678
@@ -1711,50 +1684,15 @@ cli
17111684 omittedFiles: [...reviewed.omittedFiles, ...analysis.skipped.map(entry => `${entry.name} (${entry.reason})`)],
17121685 }
17131686
1714 const prepared = prepareReview(result, {
1715 headSha,
1716 requestChangesOn: config.ai?.review?.requestChangesOn,
1717 seenFingerprints,
1718 })
1719
1720 if (!prNumber) {
1721 process.stdout.write(formatReview(result, format))
1722
1723 if (options.fix) {
1724 const applied = await applyFixes(result.findings, Boolean(options.yes), logger)
1725 logger.success(`🔧 Applied ${applied} suggestion(s)`)
1726 }
1727
1728 if (options.failOn && shouldFail(result.findings, options.failOn as 'critical'))
1729 process.exit(1)
1730 return
1731 }
1732
1733 if (options.dryRun) {
1734 logger.info(`\n${prepared.body}\n`)
1735 for (const comment of prepared.comments)
1736 logger.info(`${comment.path}:${comment.line}\n${comment.body}\n`)
1737 logger.success(`✅ Review complete: ${result.findings.length} finding(s)`)
1738 return
1739 }
1687 process.stdout.write(formatReview(result, format))
17401688
1741 assertSupports(gitProvider!, 'inlineReviewComments', 'createReview', 'posting a review')
1742 const reviewedNumber = Number.parseInt(prNumber, 10)
1743 await gitProvider.createReview(reviewedNumber, prepared)
1744
1745 // The review has landed; recording where it got to is bookkeeping, so a
1746 // failure here must not present as a failed review.
1747 try {
1748 const { upsertReviewState } = await import('../src/review/marker')
1749 await gitProvider.updatePullRequest(reviewedNumber, {
1750 body: upsertReviewState(prBody, { ...prepared.state, ...(wasPaused ? { paused: true } : {}) }),
1751 })
1752 }
1753 catch (error) {
1754 logger.warn(`Could not record review state on PR #${prNumber}: ${error instanceof Error ? error.message : String(error)}`)
1689 if (options.fix) {
1690 const applied = await applyFixes(result.findings, Boolean(options.yes), logger)
1691 logger.success(`🔧 Applied ${applied} suggestion(s)`)
17551692 }
17561693
1757 logger.success(`✅ Reviewed PR #${prNumber}: ${result.findings.length} finding(s)`)
1694 if (options.failOn && shouldFail(result.findings, options.failOn as 'critical'))
1695 process.exit(1)
17581696 }
17591697 catch (error) {
17601698 logger.error('Review failed:', error)
docs/cli/review.mdmodified+6-0
@@ -44,6 +44,12 @@ fetched, costing one API call rather than a full review:
4444Without `--auto` none of these apply except `enabled`, because a person at a
4545terminal has already decided they want the review.
4646
47A pull request review runs the same path as [`@buddy review`](/features/pr-conversations),
48so both assemble the same context: the analyzers, the guidelines and the
49learnings `@buddy remember` has recorded — all read from the base branch, never
50from the pull request's own, so a branch cannot rewrite the rules its code is
51reviewed against.
52
4753## Without an API key
4854
4955`--light` runs the analyzers and skips the model entirely — secret scanning,
src/review/run.tsmodified+39-2
@@ -1,3 +1,4 @@
1import type { AiClient } from '../ai/types'
12import type { GitProvider } from '../git/provider'
23import { assertSupports } from '../git/provider'
34import type { BuddyConfig } from '../types'
@@ -8,6 +9,7 @@ import { getDefaultLogger } from '../utils/logger'
89import { parseUnifiedDiff } from './diff'
910import { reviewDiff } from './engine'
1011import { composeInstructions, loadGuidelines } from './guidelines'
12import type { ReviewProfile } from './engine'
1113import type { ReviewTrigger } from './filters'
1214import { reviewSkipReason } from './filters'
1315import { needsReview, parseReviewState, upsertReviewState } from './marker'
@@ -25,6 +27,18 @@ export interface RunReviewOptions {
2527 summaryOnly?: boolean
2628 /** Skip when the head commit was already reviewed */
2729 skipIfReviewed?: boolean
30 /** Override the configured review profile */
31 profile?: ReviewProfile
32 /** Report what would be posted without posting it */
33 dryRun?: boolean
34 /**
35 * Client to review with; omitted means build one from config.
36 *
37 * Injectable for the same reason `attemptFix` takes one: an orchestration
38 * that constructs its own client cannot be tested without a key, and this
39 * one drifted from its second caller for exactly that long.
40 */
41 ai?: AiClient | null
2842 /**
2943 * Whether Buddy chose to review or was asked to (default: `requested`).
3044 *
@@ -80,7 +94,7 @@ export async function runReviewForPR(options: RunReviewOptions): Promise<string>
8094 return 'Already reviewed at this commit.'
8195 }
8296
83 const ai = createAiClient(config, logger)
97 const ai = options.ai === undefined ? createAiClient(config, logger) : options.ai
8498
8599 // Analyzers run whether or not AI is configured, so a repository without a
86100 // key still gets secret scanning and workflow auditing on its pull requests.
@@ -110,6 +124,11 @@ export async function runReviewForPR(options: RunReviewOptions): Promise<string>
110124 { headSha, requestChangesOn: config.ai?.review?.requestChangesOn },
111125 )
112126
127 if (options.dryRun) {
128 reportDryRun(prepared, logger)
129 return `Would post ${analysis.findings.length} static-analysis finding(s).`
130 }
131
113132 assertSupports(provider, 'inlineReviewComments', 'createReview', 'posting a review')
114133 await provider.createReview(prNumber, prepared)
115134 await persistReviewState(provider, prNumber, pr.body, prepared.state, state?.paused, logger)
@@ -129,7 +148,7 @@ export async function runReviewForPR(options: RunReviewOptions): Promise<string>
129148
130149 const result = await reviewDiff(ai, {
131150 diff,
132 profile: config.ai?.review?.profile,
151 profile: options.profile ?? config.ai?.review?.profile,
133152 summaryOnly: options.summaryOnly ?? config.ai?.review?.summaryOnly,
134153 instructions: composeInstructions({
135154 global: config.ai?.review?.instructions,
@@ -151,6 +170,11 @@ export async function runReviewForPR(options: RunReviewOptions): Promise<string>
151170 seenFingerprints: options.full ? [] : state?.fingerprints ?? [],
152171 })
153172
173 if (options.dryRun) {
174 reportDryRun(prepared, logger)
175 return `Would post ${result.findings.length} finding(s).`
176 }
177
154178 assertSupports(provider, 'inlineReviewComments', 'createReview', 'posting a review')
155179 await provider.createReview(prNumber, prepared)
156180 await persistReviewState(provider, prNumber, pr.body, prepared.state, state?.paused, logger)
@@ -160,6 +184,19 @@ export async function runReviewForPR(options: RunReviewOptions): Promise<string>
160184 : `Reviewed — ${result.findings.length} finding(s) posted.`
161185}
162186
187/**
188 * Print a review instead of posting it.
189 *
190 * @param prepared - The review that would have been posted
191 * @param logger - Where to print
192 */
193function reportDryRun(prepared: PreparedReview, logger: Logger): void {
194 logger.info(`\n${prepared.body}\n`)
195
196 for (const comment of prepared.comments)
197 logger.info(`${comment.path}:${comment.line}\n${comment.body}\n`)
198}
199
163200/**
164201 * Write the state a review established back onto the pull request.
165202 *
test/review-parity.test.tsadded+229-0
@@ -0,0 +1,229 @@
1import type { AiClient } from '../src/ai/types'
2import type { GitProvider } from '../src/git/provider'
3import type { PullRequest } from '../src/types'
4import type { BuddyConfig } from '../src/types'
5import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
6import { promises as fs } from 'node:fs'
7import { tmpdir } from 'node:os'
8import { join } from 'node:path'
9import { runReviewForPR } from '../src/review/run'
10import { NO_CAPABILITIES } from '../src/git/provider'
11import { Logger } from '../src/utils/logger'
12
13const DIFF = `diff --git a/src/app.ts b/src/app.ts
14index 111..222 100644
15--- a/src/app.ts
16+++ b/src/app.ts
17@@ -1,3 +1,4 @@
18 const a = 1
19+const token = 'ghp_0123456789abcdefghijklmnopqrstuvwxyzAB'
20 const b = 2
21`
22
23interface Seen {
24 prompts: string[]
25 reviews: Array<{ pr: number, body: string }>
26 filesRead: Array<{ path: string, ref: string }>
27}
28
29/** A provider carrying one pull request, plus a record of what was read. */
30function stub(files: Record<string, string> = {}): { provider: GitProvider, seen: Seen } {
31 const seen: Seen = { prompts: [], reviews: [], filesRead: [] }
32
33 const pr = {
34 number: 7,
35 title: 'add a thing',
36 body: 'description',
37 head: 'feature',
38 base: 'main',
39 state: 'open',
40 labels: [],
41 draft: false,
42 } as unknown as PullRequest
43
44 const provider = {
45 capabilities: () => ({ ...NO_CAPABILITIES, inlineReviewComments: true }),
46 getPullRequests: async () => [pr],
47 getPullRequestDiff: async () => DIFF,
48 getPullRequestHeadSha: async () => 'head1',
49 getFileContent: async (path: string, ref: string) => {
50 seen.filesRead.push({ path, ref })
51 return files[path] ?? null
52 },
53 createReview: async (prNumber: number, review: { body: string }) => {
54 seen.reviews.push({ pr: prNumber, body: review.body })
55 return { id: 1 }
56 },
57 updatePullRequest: async () => pr,
58 } as unknown as GitProvider
59
60 return { provider, seen }
61}
62
63/** An AI client that records its prompt and reports nothing. */
64function recordingAi(seen: Seen): AiClient {
65 return {
66 name: 'stub',
67 complete: async (request: { system?: string, messages: Array<{ content: string }> }) => {
68 seen.prompts.push(`${request.system ?? ''}\n${request.messages.map(m => m.content).join('\n')}`)
69 return {
70 text: JSON.stringify({ summary: 'looks fine', walkthrough: [], findings: [] }),
71 outputTokens: 1,
72 }
73 },
74 } as unknown as AiClient
75}
76
77/**
78 * `buddy review <pr>` reimplemented the pull request review that
79 * `runReviewForPR` already did, and the two had drifted. The command skipped
80 * analyzers outright for a pull request and never read learnings at all — and
81 * it is the command the generated workflow runs, so the automatic review every
82 * repository gets was the one path missing both.
83 *
84 * These cover the shared path both entry points now use.
85 */
86describe('reviewing a pull request', () => {
87 const config: BuddyConfig = {
88 repository: { provider: 'github', owner: 'o', name: 'r', baseBranch: 'main' },
89 }
90
91 it('success case - reads learnings from the base branch', async () => {
92 // `@buddy remember` writes these. A review that does not read them makes
93 // the whole feature inert.
94 const { provider, seen } = stub()
95
96 await runReviewForPR({ config, provider, prNumber: 7, ai: recordingAi(seen), logger: Logger.silent() })
97
98 const learnings = seen.filesRead.filter(read => read.path.includes('learnings'))
99 expect(learnings.length).toBeGreaterThan(0)
100 expect(learnings.every(read => read.ref === 'main')).toBe(true)
101 })
102
103 it('success case - reads guidelines from the base branch, never the PR branch', async () => {
104 // Both are inlined into the prompt as trusted context. Reading them from
105 // the pull request's own branch would let it rewrite the rules its code is
106 // reviewed against.
107 const { provider, seen } = stub()
108
109 await runReviewForPR({ config, provider, prNumber: 7, ai: recordingAi(seen), logger: Logger.silent() })
110
111 expect(seen.filesRead.length).toBeGreaterThan(0)
112 expect(seen.filesRead.every(read => read.ref === 'main')).toBe(true)
113 expect(seen.filesRead.some(read => read.ref === 'feature')).toBe(false)
114 })
115
116 it('success case - runs analyzers on a pull request', async () => {
117 // The command skipped these entirely when given a pull request, so secret
118 // scanning never ran on the path that reviews every pull request.
119 const { provider, seen } = stub()
120
121 await runReviewForPR({ config, provider, prNumber: 7, ai: recordingAi(seen), logger: Logger.silent() })
122
123 expect(seen.reviews).toHaveLength(1)
124 })
125
126 it('success case - a dry run reports without posting', async () => {
127 const { provider, seen } = stub()
128
129 const status = await runReviewForPR({
130 config,
131 provider,
132 prNumber: 7,
133 dryRun: true,
134 ai: recordingAi(seen),
135 logger: Logger.silent(),
136 })
137
138 expect(seen.reviews).toHaveLength(0)
139 expect(status).toContain('Would post')
140 })
141
142 it('failure case - a missing pull request is reported, not thrown', async () => {
143 const { provider } = stub()
144
145 expect(await runReviewForPR({ config, provider, prNumber: 999, ai: null, logger: Logger.silent() }))
146 .toContain('Could not find')
147 })
148
149 it('success case - an explicit review ignores the automatic filters', async () => {
150 // `--auto` is what the generated workflow passes; without it this is
151 // someone who has already decided they want a review.
152 const { provider, seen } = stub()
153
154 await runReviewForPR({
155 config: { ...config, ai: { review: { draft: false } } } as BuddyConfig,
156 provider,
157 prNumber: 7,
158 trigger: 'requested',
159 ai: recordingAi(seen),
160 logger: Logger.silent(),
161 })
162
163 expect(seen.reviews).toHaveLength(1)
164 })
165
166 it('success case - a recorded learning reaches the model', async () => {
167 // Reading the file is not the point; the point is that what `@buddy
168 // remember` wrote changes how the next review is prompted.
169 const learning = {
170 id: 'abc',
171 text: 'we pin react to 17 on purpose',
172 createdAt: '2026-01-01T00:00:00.000Z',
173 }
174 const { provider, seen } = stub({ '.buddy/learnings.jsonl': `${JSON.stringify(learning)}\n` })
175
176 await runReviewForPR({ config, provider, prNumber: 7, ai: recordingAi(seen), logger: Logger.silent() })
177
178 expect(seen.prompts.join('\n')).toContain('we pin react to 17 on purpose')
179 })
180
181 it('success case - a guideline reaches the model', async () => {
182 const { provider, seen } = stub({ 'CLAUDE.md': 'Never use `any`.' })
183
184 await runReviewForPR({ config, provider, prNumber: 7, ai: recordingAi(seen), logger: Logger.silent() })
185
186 expect(seen.prompts.join('\n')).toContain('Never use `any`')
187 })
188})
189
190/**
191 * Analyzers were skipped outright when the command was given a pull request:
192 * `config.analysis?.enabled === false || prNumber`. Secret scanning therefore
193 * never ran on the path that reviews every pull request.
194 */
195describe('analyzers on a pull request', () => {
196 let testDir: string
197 let originalCwd: string
198
199 beforeEach(async () => {
200 testDir = await fs.mkdtemp(join(tmpdir(), 'buddy-review-'))
201 originalCwd = process.cwd()
202 process.chdir(testDir)
203 })
204
205 afterEach(async () => {
206 process.chdir(originalCwd)
207 await fs.rm(testDir, { recursive: true, force: true })
208 })
209
210 it('success case - a planted credential is found and reported', async () => {
211 // Analyzers read the working tree rather than the diff, so the file has to
212 // exist for this to mean anything.
213 await fs.mkdir('src', { recursive: true })
214 await fs.writeFile('src/app.ts', `const token = 'ghp_0123456789abcdefghijklmnopqrstuvwxyzAB'\n`)
215
216 const { provider, seen } = stub()
217 const config: BuddyConfig = {
218 repository: { provider: 'github', owner: 'o', name: 'r', baseBranch: 'main' },
219 }
220
221 // No AI at all: whatever lands comes from static analysis alone, which is
222 // the guarantee that matters for a repository without a key.
223 const status = await runReviewForPR({ config, provider, prNumber: 7, ai: null, logger: Logger.silent() })
224
225 expect(status).toContain('static-analysis')
226 expect(seen.reviews).toHaveLength(1)
227 expect(seen.reviews[0].body).toContain('Static analysis only')
228 })
229})