ReviewOS
Reload

also looking at this

stacks/buddy

fix(test): defuse the suite's landmines, and cover the dossier's untested modules

#1436
Merged glennmichael123 wants to merge fix/test-landmines into main
11 files +617 -12

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+12-0
@@ -1645,6 +1645,10 @@ cli
16451645 // `--auto` is what the generated workflow passes. Without it this is
16461646 // someone at a terminal, who has already decided they want a review.
16471647 trigger: options.auto ? 'automatic' : 'requested',
1648 // The workflow fires on every edited event, so without this the
1649 // same head commit would be reviewed again each time the PR body
1650 // changes. A person re-running the command still gets their review.
1651 ...(options.auto ? { skipIfReviewed: true } : {}),
16481652 logger: log,
16491653 dryRun: Boolean(options.dryRun),
16501654 ...(options.summaryOnly ? { summaryOnly: true } : {}),
@@ -1707,6 +1711,14 @@ cli
17071711
17081712 process.stdout.write(formatReview(result, format))
17091713
1714 // `--fix` was accepted here and silently ignored: the light branch
1715 // returned before the block that applies suggestions, so the flag
1716 // worked everywhere except the one mode fast enough for a hook.
1717 if (options.fix) {
1718 const applied = await applyFixes(result.findings, Boolean(options.yes), logger)
1719 logger.success(`\u{1f527} Applied ${applied} suggestion(s)`)
1720 }
1721
17101722 if (options.failOn && shouldFail(result.findings, options.failOn as 'critical'))
17111723 process.exit(1)
17121724 return
src/review/poster.tsmodified+13-3
@@ -42,14 +42,22 @@ export interface PreparedReview {
4242 * as prose only, since a half-right suggestion costs more to undo than to
4343 * write by hand.
4444 *
45 * On a platform whose `reviewSuggestions` capability is false the block
46 * renders as a plain code fence instead: Bitbucket shows a literal
47 * suggestion fence as noise, and the one-click apply it promises does not
48 * exist there.
49 *
4550 * @param finding - Finding to render
51 * @param options - Whether the platform renders suggestion blocks (default: it does)
4652 */
47export function renderFinding(finding: ReviewFinding): string {
53export function renderFinding(finding: ReviewFinding, options?: { suggestions?: boolean }): string {
4854 const source = finding.tool ? ` \`[${finding.tool}]\`` : ''
4955 let body = `**${SEVERITY_BADGE[finding.severity]}** · \`${finding.category}\`${source}\n\n${finding.message}`
5056
5157 if (finding.suggestion) {
52 body += `\n\n\`\`\`suggestion\n${finding.suggestion}\n\`\`\``
58 body += options?.suggestions === false
59 ? `\n\nSuggested replacement:\n\n\`\`\`\n${finding.suggestion}\n\`\`\``
60 : `\n\n\`\`\`suggestion\n${finding.suggestion}\n\`\`\``
5361 }
5462
5563 return body
@@ -70,13 +78,15 @@ export function prepareReview(
7078 requestChangesOn?: 'never' | 'critical'
7179 /** Fingerprints already reported, carried forward into the new state */
7280 seenFingerprints?: string[]
81 /** Whether the platform renders suggestion blocks (default: it does) */
82 suggestions?: boolean
7383 },
7484): PreparedReview {
7585 const comments: InlineComment[] = result.findings.map(finding => ({
7686 path: finding.path,
7787 line: finding.line,
7888 side: 'RIGHT' as const,
79 body: renderFinding(finding),
89 body: renderFinding(finding, { suggestions: options.suggestions !== false }),
8090 }))
8191
8292 const counts = countBySeverity(result.findings)
src/review/run.tsmodified+49-5
@@ -141,7 +141,11 @@ export async function reviewPullRequest(options: RunReviewOptions): Promise<Revi
141141 effort: 1,
142142 omittedFiles: [],
143143 },
144 { headSha, requestChangesOn: config.ai?.review?.requestChangesOn },
144 {
145 headSha,
146 requestChangesOn: config.ai?.review?.requestChangesOn,
147 suggestions: provider.capabilities().reviewSuggestions,
148 },
145149 )
146150
147151 const staticResult: ReviewResult = {
@@ -158,9 +162,15 @@ export async function reviewPullRequest(options: RunReviewOptions): Promise<Revi
158162 }
159163
160164 assertSupports(provider, 'inlineReviewComments', 'createReview', 'posting a review')
161 await provider.createReview(prNumber, prepared)
165 const submission = await provider.createReview(prNumber, prepared)
166 if (!submission.posted)
167 return { status: 'The review could not be posted.', result: staticResult }
168
162169 await persistReviewState(provider, prNumber, pr.body, prepared.state, state?.paused, logger)
163 return { status: `Posted ${analysis.findings.length} static-analysis finding(s).`, result: staticResult }
170 return {
171 status: `Posted ${analysis.findings.length} static-analysis finding(s).${placementNote(submission, prepared, logger)}`,
172 result: staticResult,
173 }
164174 }
165175
166176 // Guidelines and learnings are read from the base branch: both are inlined
@@ -196,6 +206,7 @@ export async function reviewPullRequest(options: RunReviewOptions): Promise<Revi
196206 headSha,
197207 requestChangesOn: config.ai?.review?.requestChangesOn,
198208 seenFingerprints: options.full ? [] : state?.fingerprints ?? [],
209 suggestions: provider.capabilities().reviewSuggestions,
199210 })
200211
201212 if (options.dryRun) {
@@ -204,17 +215,50 @@ export async function reviewPullRequest(options: RunReviewOptions): Promise<Revi
204215 }
205216
206217 assertSupports(provider, 'inlineReviewComments', 'createReview', 'posting a review')
207 await provider.createReview(prNumber, prepared)
218 const submission = await provider.createReview(prNumber, prepared)
219
220 // A review that never landed must not be recorded as this commit's review:
221 // persisting the state anyway would make every later run skip a review
222 // nobody can see.
223 if (!submission.posted)
224 return { status: 'The review could not be posted.', result }
225
208226 await persistReviewState(provider, prNumber, pr.body, prepared.state, state?.paused, logger)
209227
210228 return {
211229 status: result.findings.length === 0
212230 ? 'Reviewed — nothing to report.'
213 : `Reviewed — ${result.findings.length} finding(s) posted.`,
231 : `Reviewed — ${result.findings.length} finding(s) posted.${placementNote(submission, prepared, logger)}`,
214232 result,
215233 }
216234}
217235
236/**
237 * Describe inline comments the platform dropped, or nothing.
238 *
239 * `createReview` reports how many line-anchored comments were actually
240 * placed. A comment anchored to a line outside the diff is dropped by the
241 * platform, and a review that silently loses findings reads as cleaner than
242 * it is.
243 *
244 * @param submission - What the provider reports it published
245 * @param prepared - What was asked for
246 * @param logger - Logger for the warning
247 * @returns A sentence to append to the status, or an empty string
248 */
249function placementNote(
250 submission: { inlineComments: number },
251 prepared: PreparedReview,
252 logger: Logger,
253): string {
254 const dropped = prepared.comments.length - submission.inlineComments
255 if (dropped <= 0)
256 return ''
257
258 logger.warn(`⚠️ ${dropped} of ${prepared.comments.length} inline comment(s) could not be placed on the diff`)
259 return ` ${dropped} of ${prepared.comments.length} inline comment(s) could not be placed on the diff.`
260}
261
218262/**
219263 * Review a pull request end to end and post the result.
220264 *
test/review-outcomes.test.tsadded+191-0
@@ -0,0 +1,191 @@
1import type { AiClient } from '../src/ai/types'
2import type { GitProvider, ReviewSubmission, ReviewSubmissionResult } from '../src/git/provider'
3import type { BuddyConfig, PullRequest } from '../src/types'
4import { describe, expect, it } from 'bun:test'
5import { NO_CAPABILITIES } from '../src/git/provider'
6import { reviewPullRequest } from '../src/review/run'
7import { Logger } from '../src/utils/logger'
8
9const DIFF = `diff --git a/src/app.ts b/src/app.ts
10index 111..222 100644
11--- a/src/app.ts
12+++ b/src/app.ts
13@@ -1,2 +1,4 @@
14 const a = 1
15+const b = 2
16+const c = 3
17 const d = 4
18`
19
20interface Seen {
21 reviews: ReviewSubmission[]
22 bodyUpdates: number
23}
24
25/**
26 * A provider carrying one pull request, with a scriptable review outcome.
27 *
28 * @param options - Capability toggle and what createReview should report
29 */
30function stub(options: {
31 suggestions?: boolean
32 submission?: (review: ReviewSubmission) => ReviewSubmissionResult
33} = {}): { provider: GitProvider, seen: Seen } {
34 const seen: Seen = { reviews: [], bodyUpdates: 0 }
35
36 const pr = {
37 number: 7,
38 title: 'add a thing',
39 body: 'description',
40 head: 'feature',
41 base: 'main',
42 state: 'open',
43 labels: [],
44 draft: false,
45 } as unknown as PullRequest
46
47 const provider = {
48 capabilities: () => ({
49 ...NO_CAPABILITIES,
50 inlineReviewComments: true,
51 reviewSuggestions: options.suggestions ?? true,
52 }),
53 getPullRequests: async () => [pr],
54 getPullRequestDiff: async () => DIFF,
55 getPullRequestHeadSha: async () => 'head1',
56 getFileContent: async () => null,
57 createReview: async (_prNumber: number, review: ReviewSubmission) => {
58 seen.reviews.push(review)
59 return options.submission
60 ? options.submission(review)
61 : { posted: true, inlineComments: review.comments?.length ?? 0 }
62 },
63 updatePullRequest: async () => {
64 seen.bodyUpdates++
65 return pr
66 },
67 } as unknown as GitProvider
68
69 return { provider, seen }
70}
71
72/** An AI client that reports these findings for any diff. */
73function reportingAi(findings: unknown[]): AiClient {
74 return {
75 name: 'stub',
76 complete: async () => ({
77 // The engine reads `json`, not `text` — a stub answering in prose is
78 // indistinguishable from a review that found nothing.
79 json: { summary: 'looked at it', walkthrough: [], findings },
80 outputTokens: 1,
81 }),
82 } as unknown as AiClient
83}
84
85const FINDING = {
86 path: 'src/app.ts',
87 line: 2,
88 severity: 'minor',
89 category: 'style',
90 message: 'Name this better.',
91 suggestion: 'const better = 2',
92}
93
94const config: BuddyConfig = { analysis: { enabled: false } }
95
96/**
97 * `createReview` reports what publishing achieved — `ReviewSubmissionResult`
98 * was declared and implemented by all three providers, and the one caller
99 * ignored it: a review that failed to post was reported as posted, and
100 * inline comments the platform dropped vanished without a trace.
101 */
102describe('review submission outcomes', () => {
103 it('failure case - a review that was not posted says so and records nothing', async () => {
104 const { provider, seen } = stub({ submission: () => ({ posted: false, inlineComments: 0 }) })
105
106 const { status } = await reviewPullRequest({
107 config,
108 provider,
109 prNumber: 7,
110 ai: reportingAi([FINDING]),
111 logger: Logger.silent(),
112 })
113
114 expect(status).toBe('The review could not be posted.')
115 // Persisting the state anyway would make every later run skip a review
116 // nobody can see.
117 expect(seen.bodyUpdates).toBe(0)
118 })
119
120 it('failure case - dropped inline comments are named in the status', async () => {
121 const { provider } = stub({ submission: () => ({ posted: true, inlineComments: 1 }) })
122
123 const { status } = await reviewPullRequest({
124 config,
125 provider,
126 prNumber: 7,
127 ai: reportingAi([
128 FINDING,
129 { ...FINDING, line: 3, message: 'This one anchors nowhere.' },
130 ]),
131 logger: Logger.silent(),
132 })
133
134 expect(status).toContain('1 of 2 inline comment(s) could not be placed on the diff.')
135 })
136
137 it('success case - a fully placed review reports no shortfall', async () => {
138 const { provider, seen } = stub()
139
140 const { status } = await reviewPullRequest({
141 config,
142 provider,
143 prNumber: 7,
144 ai: reportingAi([FINDING]),
145 logger: Logger.silent(),
146 })
147
148 expect(status).toContain('1 finding(s) posted.')
149 expect(status).not.toContain('could not be placed')
150 expect(seen.bodyUpdates).toBe(1)
151 })
152})
153
154/**
155 * `ProviderCapabilities.reviewSuggestions` was declared, documented and set
156 * by every provider — and read by nothing, so Bitbucket received literal
157 * ```suggestion fences promising a one-click apply it does not have.
158 */
159describe('suggestion blocks follow the capability', () => {
160 it('success case - a capable platform gets the suggestion block', async () => {
161 const { provider, seen } = stub({ suggestions: true })
162
163 await reviewPullRequest({
164 config,
165 provider,
166 prNumber: 7,
167 ai: reportingAi([FINDING]),
168 logger: Logger.silent(),
169 })
170
171 const body = seen.reviews[0]?.comments?.[0]?.body ?? ''
172 expect(body).toContain('```suggestion')
173 })
174
175 it('failure case - a platform without suggestions gets a plain fence', async () => {
176 const { provider, seen } = stub({ suggestions: false })
177
178 await reviewPullRequest({
179 config,
180 provider,
181 prNumber: 7,
182 ai: reportingAi([FINDING]),
183 logger: Logger.silent(),
184 })
185
186 const body = seen.reviews[0]?.comments?.[0]?.body ?? ''
187 expect(body).not.toContain('```suggestion')
188 expect(body).toContain('Suggested replacement:')
189 expect(body).toContain(FINDING.suggestion)
190 })
191})
test/review-parity.test.tsmodified+2-2
@@ -50,9 +50,9 @@ function stub(files: Record<string, string> = {}): { provider: GitProvider, seen
5050 seen.filesRead.push({ path, ref })
5151 return files[path] ?? null
5252 },
53 createReview: async (prNumber: number, review: { body: string }) => {
53 createReview: async (prNumber: number, review: { body: string, comments?: unknown[] }) => {
5454 seen.reviews.push({ pr: prNumber, body: review.body })
55 return { id: 1 }
55 return { posted: true, inlineComments: review.comments?.length ?? 0 }
5656 },
5757 updatePullRequest: async () => pr,
5858 } as unknown as GitProvider