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
README.mdmodified+6-0
@@ -1034,9 +1034,15 @@ bun test
10341034## Build From Source
10351035
10361036```bash
1037bun install # not optional — see the note below
10371038bun run build
10381039```
10391040
1041> [!NOTE]
1042> `bun install` is a hard prerequisite for every dev command. On a fresh clone without `node_modules`, `bun test` fails with over a hundred phantom errors (`Cannot find package 'ts-pantry'` and friends) that look like real bugs and are not.
1043>
1044> Git hooks (staged lint on `pre-commit`, gitlint on `commit-msg`) are declared in `package.json` but are not installed automatically — run `bunx bun-git-hooks` once after cloning to activate them.
1045
10401046## Changelog
10411047
10421048Please see our [releases](https://github.com/stacksjs/buddy/releases) page for more information on what has changed recently.
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/ai-provider.test.tsmodified+100-0
@@ -376,6 +376,106 @@ describe('anthropic provider', () => {
376376
377377 expect(response.usage.cachedInputTokens).toBe(900)
378378 })
379
380 it('edge case - the remaining stop reasons land in the normalized set', async () => {
381 process.env.ANTHROPIC_API_KEY = 'sk-ant-test'
382 const cases: Array<[string, string]> = [
383 ['stop_sequence', 'end'],
384 ['max_tokens', 'max_tokens'],
385 ['something_new', 'other'],
386 ]
387
388 for (const [wire, normalized] of cases) {
389 stubFetch({
390 id: 'msg',
391 type: 'message',
392 role: 'assistant',
393 model: 'claude-opus-5',
394 content: [{ type: 'text', text: 'x' }],
395 stop_reason: wire,
396 usage: { input_tokens: 1, output_tokens: 1 },
397 })
398
399 const response = await createAiClient({})!.complete({ messages: [{ role: 'user', content: 'hi' }] })
400 expect(response.stopReason).toBe(normalized as never)
401 }
402 })
403
404 it('success case - parses json only for schema-constrained requests', async () => {
405 process.env.ANTHROPIC_API_KEY = 'sk-ant-test'
406 stubFetch({
407 id: 'msg',
408 type: 'message',
409 role: 'assistant',
410 model: 'claude-opus-5',
411 content: [{ type: 'text', text: '{"answer":42}' }],
412 stop_reason: 'end_turn',
413 usage: { input_tokens: 1, output_tokens: 1 },
414 })
415
416 const unconstrained = await createAiClient({})!.complete({ messages: [{ role: 'user', content: 'hi' }] })
417 expect(unconstrained.json).toBeUndefined()
418
419 const constrained = await createAiClient({})!.complete({
420 messages: [{ role: 'user', content: 'hi' }],
421 jsonSchema: { type: 'object' },
422 })
423 expect(constrained.json).toEqual({ answer: 42 })
424 })
425
426 it('failure case - malformed json for a schema-constrained request throws', async () => {
427 process.env.ANTHROPIC_API_KEY = 'sk-ant-test'
428 stubFetch({
429 id: 'msg',
430 type: 'message',
431 role: 'assistant',
432 model: 'claude-opus-5',
433 content: [{ type: 'text', text: 'not json' }],
434 stop_reason: 'end_turn',
435 usage: { input_tokens: 1, output_tokens: 1 },
436 })
437
438 await expect(createAiClient({})!.complete({
439 messages: [{ role: 'user', content: 'hi' }],
440 jsonSchema: { type: 'object' },
441 })).rejects.toBeInstanceOf(AiProviderError)
442 })
443
444 it('success case - sends system, tools and the output config on the wire', async () => {
445 // Asserted at the fetch layer, through the real SDK — a second
446 // `mock.module('@anthropic-ai/sdk')` owner would clobber this suite's
447 // stubs process-wide, which is the same landmine the node:fs mock was.
448 process.env.ANTHROPIC_API_KEY = 'sk-ant-test'
449 let body: any
450 globalThis.fetch = (async (input: unknown, init?: { body?: unknown }) => {
451 const raw = init?.body ?? (input instanceof Request ? await input.clone().text() : undefined)
452 body = raw ? JSON.parse(String(raw)) : undefined
453 return new Response(JSON.stringify({
454 id: 'msg',
455 type: 'message',
456 role: 'assistant',
457 model: 'claude-opus-5',
458 content: [{ type: 'text', text: '{}' }],
459 stop_reason: 'end_turn',
460 usage: { input_tokens: 1, output_tokens: 1 },
461 }), { status: 200, headers: { 'Content-Type': 'application/json' } })
462 }) as unknown as typeof fetch
463
464 await createAiClient({})!.complete({
465 system: 'be brief',
466 effort: 'low',
467 jsonSchema: { type: 'object' },
468 tools: [{ name: 'grep', description: 'search', parameters: { type: 'object' } }],
469 messages: [{ role: 'user', content: 'hi' }],
470 })
471
472 expect(body.model).toBe('claude-opus-5')
473 expect(body.system).toBe('be brief')
474 // The default ceiling is sized to stay under the SDK's HTTP timeout.
475 expect(body.max_tokens).toBe(16000)
476 expect(body.tools).toEqual([{ name: 'grep', description: 'search', input_schema: { type: 'object' } }])
477 expect(body.output_config).toEqual({ effort: 'low', format: { type: 'json_schema', schema: { type: 'object' } } })
478 })
379479})
380480
381481describe('token budget', () => {
test/composer-constraint-updates.test.tsmodified+89-0
@@ -0,0 +1,89 @@
1import { afterEach, beforeEach, describe, expect, it, spyOn } from 'bun:test'
2import * as fs from 'node:fs'
3import { generateComposerUpdates } from '../src/utils/composer-parser'
4
5/**
6 * This file sat in the tree as zero bytes — a suite that ran, passed and
7 * asserted nothing, which reads as coverage in every listing. What its name
8 * promises is the constraint-style contract: an update must keep the shape
9 * the maintainer wrote (`^`, `~`, exact, compound range), because rewriting
10 * `^10.0` to a bare `10.48.29` silently pins a project that chose a range.
11 */
12describe('composer constraint updates', () => {
13 const composerJson = {
14 'require': {
15 'php': '^8.1',
16 'laravel/framework': '^10.0',
17 'symfony/console': '>=6.0,<7.0',
18 'monolog/monolog': '~3.0',
19 'guzzlehttp/guzzle': '7.8.1',
20 },
21 'require-dev': {
22 'phpunit/phpunit': '^10.0',
23 },
24 }
25
26 let readFileSpy: ReturnType<typeof spyOn>
27
28 beforeEach(() => {
29 readFileSpy = spyOn(fs, 'readFileSync').mockReturnValue(JSON.stringify(composerJson, null, 2))
30 })
31
32 afterEach(() => {
33 // Restore, or every later test that reads a real file gets composer.json.
34 readFileSpy?.mockRestore?.()
35 })
36
37 /** Run one update through the generator and hand back the rewritten JSON. */
38 async function updated(name: string, newVersion: string): Promise<Record<string, Record<string, string>>> {
39 const result = await generateComposerUpdates([{ name, newVersion, file: 'composer.json' }])
40 expect(result).toHaveLength(1)
41 return JSON.parse(result[0].content)
42 }
43
44 it('success case - a caret constraint keeps its caret', async () => {
45 const json = await updated('laravel/framework', '10.48.29')
46
47 expect(json.require['laravel/framework']).toBe('^10.48.29')
48 })
49
50 it('success case - a tilde constraint keeps its tilde', async () => {
51 const json = await updated('monolog/monolog', '3.9.0')
52
53 expect(json.require['monolog/monolog']).toBe('~3.9.0')
54 })
55
56 it('success case - an exact pin stays exact', async () => {
57 const json = await updated('guzzlehttp/guzzle', '7.9.2')
58
59 expect(json.require['guzzlehttp/guzzle']).toBe('7.9.2')
60 })
61
62 it('success case - a compound range moves only its floor', async () => {
63 // `>=6.0,<7.0` updated to 6.4.23 must not touch the ceiling: rewriting
64 // both bounds would widen the range the maintainer deliberately closed.
65 const json = await updated('symfony/console', '6.4.23')
66
67 expect(json.require['symfony/console']).toBe('>=6.4.23,<7.0')
68 })
69
70 it('success case - require-dev packages update with the same rules', async () => {
71 const json = await updated('phpunit/phpunit', '10.5.20')
72
73 expect(json['require-dev']['phpunit/phpunit']).toBe('^10.5.20')
74 })
75
76 it('edge case - only the named package changes', async () => {
77 const json = await updated('laravel/framework', '10.48.29')
78
79 expect(json.require['symfony/console']).toBe('>=6.0,<7.0')
80 expect(json.require['monolog/monolog']).toBe('~3.0')
81 expect(json.require.php).toBe('^8.1')
82 })
83
84 it('failure case - an unknown package leaves the file untouched', async () => {
85 const json = await updated('vendor/not-here', '1.0.0')
86
87 expect(json).toEqual(composerJson)
88 })
89})
test/dependency-file-parser.test.tsmodified+18-2
@@ -16,13 +16,25 @@ mock.module('ts-pantry', () => ({
1616
1717const mockReadFileSync = mock()
1818const mockExistsSync = mock()
19
20/**
21 * `mock.module` is process-global and permanent: whatever it installs is what
22 * every later test file gets too. Handing it the bare mocks broke suites that
23 * ran after this one — `existsSync` returned `undefined` everywhere once
24 * `afterEach` reset the mocks (which is how #1434's decline test failed on CI
25 * while passing alone). The replacement functions therefore delegate to the
26 * real fs except while this suite has armed them.
27 */
28let fsMocksArmed = false
1929mock.module('node:fs', () => {
2030 // eslint-disable-next-line ts/no-require-imports
2131 const originalFs = require('node:fs')
2232 return {
2333 ...originalFs,
24 readFileSync: mockReadFileSync,
25 existsSync: mockExistsSync,
34 readFileSync: (...args: unknown[]) =>
35 fsMocksArmed ? mockReadFileSync(...args) : originalFs.readFileSync(...args),
36 existsSync: (...args: unknown[]) =>
37 fsMocksArmed ? mockExistsSync(...args) : originalFs.existsSync(...args),
2638 }
2739})
2840
@@ -36,9 +48,13 @@ describe('Dependency File Parser', () => {
3648 // Set up default mock behaviors
3749 mockExistsSync.mockReturnValue(true)
3850 mockReadFileSync.mockReturnValue('mock content')
51 fsMocksArmed = true
3952 })
4053
4154 afterEach(() => {
55 // Hand the real fs back to everyone else before anything runs after us.
56 fsMocksArmed = false
57
4258 // Complete cleanup
4359 mockResolveDependencyFile.mockReset()
4460 mockReadFileSync.mockReset()
test/lock-file.test.tsadded+118-0
@@ -0,0 +1,118 @@
1import { afterEach, describe, expect, it } from 'bun:test'
2import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'
3import { tmpdir } from 'node:os'
4import { join } from 'node:path'
5import process from 'node:process'
6import {
7 detectRequiredPackageManagers,
8 getAllLockFilePaths,
9 hasLockFile,
10 regenerateLockFile,
11} from '../src/utils/lock-file'
12
13/**
14 * 188 lines, publicly re-exported, and no direct test — the dossier's
15 * shortlist. The spawn-heavy path is exercised through its non-fatal error
16 * contract; the pure parts are asserted directly.
17 */
18describe('lock-file', () => {
19 const cleanups: Array<() => void> = []
20
21 afterEach(() => {
22 while (cleanups.length > 0)
23 cleanups.pop()?.()
24 })
25
26 /** A temp directory removed after the test. */
27 function tempDir(): string {
28 const dir = mkdtempSync(join(tmpdir(), 'buddy-lock-'))
29 cleanups.push(() => rmSync(dir, { recursive: true, force: true }))
30 return dir
31 }
32
33 describe('getAllLockFilePaths', () => {
34 it('success case - names every lock file a regeneration may touch', () => {
35 const paths = getAllLockFilePaths()
36
37 // The staging step after regeneration adds exactly these; a manager
38 // whose lock file is missing here gets regenerated and then not
39 // committed, which looks like the update never ran.
40 for (const expected of ['bun.lock', 'bun.lockb', 'package-lock.json', 'yarn.lock', 'pnpm-lock.yaml', 'composer.lock'])
41 expect(paths).toContain(expected)
42 })
43 })
44
45 describe('hasLockFile', () => {
46 it('success case - finds each manager by its lock file', () => {
47 const dir = tempDir()
48 writeFileSync(join(dir, 'yarn.lock'), '')
49 writeFileSync(join(dir, 'composer.lock'), '{}')
50
51 expect(hasLockFile('yarn', dir)).toBe(true)
52 expect(hasLockFile('composer', dir)).toBe(true)
53 expect(hasLockFile('npm', dir)).toBe(false)
54 expect(hasLockFile('pnpm', dir)).toBe(false)
55 })
56
57 it('edge case - either bun lock format counts', () => {
58 const textual = tempDir()
59 writeFileSync(join(textual, 'bun.lock'), '')
60 expect(hasLockFile('bun', textual)).toBe(true)
61
62 const binary = tempDir()
63 writeFileSync(join(binary, 'bun.lockb'), '')
64 expect(hasLockFile('bun', binary)).toBe(true)
65
66 expect(hasLockFile('bun', tempDir())).toBe(false)
67 })
68 })
69
70 describe('detectRequiredPackageManagers', () => {
71 // These run from the repository root, whose bun.lock makes the JS
72 // manager detection deterministic.
73 it('success case - a package.json update needs the JS manager', () => {
74 expect(detectRequiredPackageManagers(['package.json'])).toEqual(['bun'])
75 expect(detectRequiredPackageManagers(['packages/app/package.json'])).toEqual(['bun'])
76 })
77
78 it('success case - a composer.json update needs composer', () => {
79 expect(detectRequiredPackageManagers(['composer.json'])).toEqual(['composer'])
80 })
81
82 it('success case - mixed updates need both, once each', () => {
83 const managers = detectRequiredPackageManagers([
84 'package.json',
85 'composer.json',
86 'sub/package.json',
87 ])
88
89 expect(managers.sort()).toEqual(['bun', 'composer'])
90 })
91
92 it('edge case - unrelated files need nothing', () => {
93 expect(detectRequiredPackageManagers(['deps.yaml', 'README.md', 'Dockerfile'])).toEqual([])
94 })
95 })
96
97 describe('regenerateLockFile', () => {
98 it('failure case - a missing binary resolves rather than throwing', async () => {
99 // The contract is non-fatal: a machine without the manager installed
100 // gets a result object naming the failure, not an unhandled rejection
101 // that sinks the whole update run.
102 const originalPath = process.env.PATH
103 process.env.PATH = tempDir()
104 cleanups.push(() => {
105 process.env.PATH = originalPath
106 })
107
108 const result = await regenerateLockFile('yarn', tempDir())
109
110 expect(result.success).toBe(false)
111 expect(result.packageManager).toBe('yarn')
112 // Which channel reports the missing binary is platform-dependent —
113 // node fires the `error` event, Bun closes with a negative code. The
114 // contract is only that the failure is named, not how it was caught.
115 expect(result.message).toMatch(/Failed to run yarn|exited with code/)
116 })
117 })
118})
test/preload.tsmodified+19-0
@@ -16,6 +16,18 @@
1616 */
1717import { afterEach, beforeEach } from 'bun:test'
1818
19/**
20 * Repair a dead working directory between tests.
21 *
22 * A suite that `chdir`s into a temp directory and then deletes it can leave
23 * `process.cwd()` pointing at nothing — every later test that touches the
24 * filesystem, spawns a process or captures `cwd` in `beforeEach` then fails
25 * with an unrelated-looking ENOENT. Suites defend individually; this is the
26 * net under all of them. Repair-only: a *live* cwd is a suite's own business,
27 * so this never moves one.
28 */
29const repoRoot = process.cwd()
30
1931/** Set by {@link allowNetwork} for the rare test that genuinely needs the wire. */
2032let networkAllowed = false
2133
@@ -58,6 +70,13 @@ beforeEach(() => {
5870 networkAllowed = false
5971 if (globalThis.fetch !== guardedFetch)
6072 globalThis.fetch = guardedFetch as unknown as typeof fetch
73
74 try {
75 process.cwd()
76 }
77 catch {
78 process.chdir(repoRoot)
79 }
6180})
6281
6382afterEach(() => {
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