fix(cli): apply the review and scan flags that were accepted and ignored

bin/cli.tsmodified+13-2

Changes to bin/cli.ts

		⋯1,609 linesShow the 1,609 lines above this hunk@@ -1610,9 +1610,9 @@ cli
1610	1610	// runs was the one path missing both.
1611	1611	if (prNumber) {
1612	1612	const gitProvider = await providerFor(config, 'reviewing a pull request', log)
1613		- const { runReviewForPR } = await import('../src/review/run')
	1613	+ const { reviewPullRequest } = await import('../src/review/run')
1614	1614
1615		- const status = await runReviewForPR({
	1615	+ const { status, result } = await reviewPullRequest({
1616	1616	config,
1617	1617	provider: gitProvider,
1618	1618	prNumber: Number.parseInt(prNumber, 10),
		⋯7 linesShow the 7 lines above this hunk@@ -1626,6 +1626,17 @@ cli
1626	1626	})
1627	1627
1628	1628	logger.success(`✅ ${status}`)
	1629	+
	1630	+ // `--format` and `--fail-on` were accepted on this path and applied
	1631	+ // only on the local one, so a pull request review could not be
	1632	+ // rendered as JSON or made to fail a pipeline. The review is posted
	1633	+ // regardless; these govern what the terminal sees and the exit code.
	1634	+ if (result && format !== 'pretty')
	1635	+ process.stdout.write(formatReview(result, format))
	1636	+
	1637	+ if (result && options.failOn && shouldFail(result.findings, options.failOn as 'critical'))
	1638	+ process.exit(1)
	1639	+
1629	1640	return
1630	1641	}
1631	1642

docs/cli/review.mdmodified+5-0

Changes to docs/cli/review.md

		⋯43 linesShow the 43 lines above this hunk@@ -44,6 +44,11 @@ fetched, costing one API call rather than a full review:
44	44	Without `--auto` none of these apply except `enabled`, because a person at a
45	45	terminal has already decided they want the review.
46	46
	47	+`--format` and `--fail-on` apply here too. The review is posted to the pull
	48	+request regardless; they govern what the terminal sees and the exit code, so
	49	+`buddy review 123 --fail-on major` can gate a pipeline on the review it just
	50	+left.
	51	+
47	52	A pull request review runs the same path as [`@buddy review`](/features/pr-conversations),
48	53	so both assemble the same context: the analyzers, the guidelines and the
49	54	learnings `@buddy remember` has recorded — all read from the base branch, never

src/buddy.tsmodified+14-2

Changes to src/buddy.ts

		⋯2,061 linesShow the 2,061 lines above this hunk@@ -2062,7 +2062,7 @@ export class Buddy {
2062	2062	*/
2063	2063	async checkPackages(packageNames: string[]): Promise<PackageUpdate[]> {
2064	2064	this.logger.info(`Checking specific packages: ${packageNames.join(', ')}`)
2065		- return this.registryClient.getUpdatesForPackages(packageNames)
	2065	+ return this.applyStrategy(await this.registryClient.getUpdatesForPackages(packageNames))
2066	2066	}
2067	2067
2068	2068	/**
		⋯1 lineShow the 1 line above this hunk@@ -2070,7 +2070,19 @@ export class Buddy {
2070	2070	*/
2071	2071	async checkPackagesWithPattern(pattern: string): Promise<PackageUpdate[]> {
2072	2072	this.logger.info(`Checking packages with pattern: ${pattern}`)
2073		- return this.registryClient.getUpdatesWithPattern(pattern)
	2073	+ return this.applyStrategy(await this.registryClient.getUpdatesWithPattern(pattern))
	2074	+ }
	2075	+
	2076	+ /**
	2077	+ * Apply the configured strategy to a targeted check.
	2078	+ *
	2079	+ * The full scan filtered by strategy and these two paths did not, so
	2080	+ * `buddy scan --strategy patch --packages react` folded `--strategy` into
	2081	+ * the config and then listed the major anyway.
	2082	+ */
	2083	+ private applyStrategy(updates: PackageUpdate[]): PackageUpdate[] {
	2084	+ const strategy = this.config.packages?.strategy
	2085	+ return strategy ? this.filterUpdatesByStrategy(updates, strategy) : updates
2074	2086	}
2075	2087
2076	2088	/**

src/review/index.tsmodified+2-1

Changes to src/review/index.ts

		⋯31 linesShow the 31 lines above this hunk@@ -32,7 +32,8 @@ export type { RefFileReader } from './guidelines'
32	32	export { prepareReview, renderFinding } from './poster'
33	33	export { resolveBuddyThreads } from './resolve'
34	34	export type { ResolveThreadsOptions } from './resolve'
35		-export { runReviewForPR } from './run'
	35	+export { reviewPullRequest, runReviewForPR } from './run'
	36	+export type { ReviewOutcome } from './run'
36	37	export type { RunReviewOptions } from './run'
37	38	export type { InlineComment, PreparedReview } from './poster'
38	39	export {

src/review/run.tsmodified+60-13

Changes to src/review/run.ts

		⋯9 linesShow the 9 lines above this hunk@@ -10,6 +10,7 @@ import { parseUnifiedDiff } from './diff'
10	10	import { reviewDiff } from './engine'
11	11	import { composeInstructions, loadGuidelines } from './guidelines'
12	12	import type { ReviewProfile } from './engine'
	13	+import type { ReviewResult } from './findings'
13	14	import type { ReviewTrigger } from './filters'
14	15	import { reviewSkipReason } from './filters'
15	16	import { needsReview, parseReviewState, upsertReviewState } from './marker'
		⋯44 linesShow the 44 lines above this hunk@@ -60,30 +61,49 @@ export interface RunReviewOptions {
60	61	* @param options - Repository context and review settings
61	62	* @returns A short status line describing what happened
62	63	*/
63		-export async function runReviewForPR(options: RunReviewOptions): Promise<string> {
	64	+/** What a pull request review produced, for callers that need more than a status line. */
	65	+export interface ReviewOutcome {
	66	+ /** A short status line describing what happened */
	67	+ status: string
	68	+ /** The review, when one was computed — absent when skipped, paused or empty */
	69	+ result?: ReviewResult
	70	+}
	71	+
	72	+/**
	73	+ * Review a pull request and report both the status and the findings.
	74	+ *
	75	+ * `runReviewForPR` returns only the status line, which is all a comment reply
	76	+ * needs. The CLI needs the findings too: `--format` renders them and
	77	+ * `--fail-on` gates the exit code on them, and both were accepted on the pull
	78	+ * request path and then never applied, because the findings never came back.
	79	+ *
	80	+ * @param options - Repository context and review settings
	81	+ * @returns The status, and the review when one was computed
	82	+ */
	83	+export async function reviewPullRequest(options: RunReviewOptions): Promise<ReviewOutcome> {
64	84	const logger = options.logger ?? getDefaultLogger()
65	85	const { config, provider, prNumber } = options
66	86
67	87	const prs = await provider.getPullRequests('open')
68	88	const pr = prs.find(candidate => candidate.number === prNumber)
69	89	if (!pr)
70		- return `Could not find open pull request #${prNumber}.`
	90	+ return { status: `Could not find open pull request #${prNumber}.` }
71	91
72	92	// Checked before the diff is fetched or a model is contacted, so an ignored
73	93	// pull request costs one API call rather than a review's worth of tokens.
74	94	const skip = reviewSkipReason(config, pr, options.trigger ?? 'requested')
75	95	if (skip) {
76	96	logger.info(`🔍 Skipping PR #${prNumber}: ${skip}`)
77		- return skip
	97	+ return { status: skip }
78	98	}
79	99
80	100	const state = parseReviewState(pr.body)
81	101	if (state?.paused && !options.full)
82		- return 'Reviews are paused on this pull request. Say `@buddy resume` to restart them.'
	102	+ return { status: 'Reviews are paused on this pull request. Say `@buddy resume` to restart them.' }
83	103
84	104	const diff = await provider.getPullRequestDiff(prNumber)
85	105	if (!diff.trim())
86		- return 'There are no changes to review.'
	106	+ return { status: 'There are no changes to review.' }
87	107
88	108	const parsed = parseUnifiedDiff(diff)
89	109	const changedFiles = parsed.files.map(file => file.path)
		⋯1 lineShow the 1 line above this hunk@@ -91,7 +111,7 @@ export async function runReviewForPR(options: RunReviewOptions): Promise<string>
91	111
92	112	if (options.skipIfReviewed && !needsReview(state, headSha)) {
93	113	logger.info(`🔍 PR #${prNumber} already reviewed at ${headSha}`)
94		- return 'Already reviewed at this commit.'
	114	+ return { status: 'Already reviewed at this commit.' }
95	115	}
96	116
97	117	const ai = options.ai === undefined ? createAiClient(config, logger) : options.ai
		⋯13 linesShow the 13 lines above this hunk@@ -111,7 +131,7 @@ export async function runReviewForPR(options: RunReviewOptions): Promise<string>
111	131
112	132	if (!ai) {
113	133	if (analysis.findings.length === 0)
114		- return 'No AI provider configured and static analysis found nothing to report.'
	134	+ return { status: 'No AI provider configured and static analysis found nothing to report.' }
115	135
116	136	const prepared = prepareReview(
117	137	{
		⋯6 linesShow the 6 lines above this hunk@@ -124,15 +144,23 @@ export async function runReviewForPR(options: RunReviewOptions): Promise<string>
124	144	{ headSha, requestChangesOn: config.ai?.review?.requestChangesOn },
125	145	)
126	146
	147	+ const staticResult: ReviewResult = {
	148	+ summary: 'Static analysis only — no AI provider is configured.',
	149	+ walkthrough: [],
	150	+ findings: analysis.findings,
	151	+ effort: 1,
	152	+ omittedFiles: [],
	153	+ }
	154	+
127	155	if (options.dryRun) {
128	156	reportDryRun(prepared, logger)
129		- return `Would post ${analysis.findings.length} static-analysis finding(s).`
	157	+ return { status: `Would post ${analysis.findings.length} static-analysis finding(s).`, result: staticResult }
130	158	}
131	159
132	160	assertSupports(provider, 'inlineReviewComments', 'createReview', 'posting a review')
133	161	await provider.createReview(prNumber, prepared)
134	162	await persistReviewState(provider, prNumber, pr.body, prepared.state, state?.paused, logger)
135		- return `Posted ${analysis.findings.length} static-analysis finding(s).`
	163	+ return { status: `Posted ${analysis.findings.length} static-analysis finding(s).`, result: staticResult }
136	164	}
137	165
138	166	// Guidelines and learnings are read from the base branch: both are inlined
		⋯33 linesShow the 33 lines above this hunk@@ -172,18 +200,37 @@ export async function runReviewForPR(options: RunReviewOptions): Promise<string>
172	200
173	201	if (options.dryRun) {
174	202	reportDryRun(prepared, logger)
175		- return `Would post ${result.findings.length} finding(s).`
	203	+ return { status: `Would post ${result.findings.length} finding(s).`, result }
176	204	}
177	205
178	206	assertSupports(provider, 'inlineReviewComments', 'createReview', 'posting a review')
179	207	await provider.createReview(prNumber, prepared)
180	208	await persistReviewState(provider, prNumber, pr.body, prepared.state, state?.paused, logger)
181	209
182		- return result.findings.length === 0
183		- ? 'Reviewed — nothing to report.'
184		- : `Reviewed — ${result.findings.length} finding(s) posted.`
	210	+ return {
	211	+ status: result.findings.length === 0
	212	+ ? 'Reviewed — nothing to report.'
	213	+ : `Reviewed — ${result.findings.length} finding(s) posted.`,
	214	+ result,
	215	+ }
	216	+}
	217	+
	218	+/**
	219	+ * Review a pull request end to end and post the result.
	220	+ *
	221	+ * Shared by the CLI, the `@buddy review` command and the automatic
	222	+ * trigger, so all three assemble context — guidelines, learnings, analyzer
	223	+ * findings — the same way rather than drifting apart.
	224	+ *
	225	+ * @param options - Repository context and review settings
	226	+ * @returns A short status line describing what happened
	227	+ */
	228	+export async function runReviewForPR(options: RunReviewOptions): Promise<string> {
	229	+ return (await reviewPullRequest(options)).status
185	230	}
186	231
	232	+
	233	+
187	234	/**
188	235	* Print a review instead of posting it.
189	236	*

test/review-parity.test.tsmodified+54-1

Changes to test/review-parity.test.ts

		⋯5 linesShow the 5 lines above this hunk@@ -6,7 +6,7 @@ import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
6	6	import { promises as fs } from 'node:fs'
7	7	import { tmpdir } from 'node:os'
8	8	import { join } from 'node:path'
9		-import { runReviewForPR } from '../src/review/run'
	9	+import { reviewPullRequest, runReviewForPR } from '../src/review/run'
10	10	import { NO_CAPABILITIES } from '../src/git/provider'
11	11	import { Logger } from '../src/utils/logger'
12	12
		⋯214 linesShow the 214 lines above this hunk@@ -227,3 +227,56 @@ describe('analyzers on a pull request', () => {
227	227	expect(seen.reviews[0].body).toContain('Static analysis only')
228	228	})
229	229	})
	230	+
	231	+/**
	232	+ * `--format` and `--fail-on` were accepted on the pull request path and applied
	233	+ * only on the local one — the findings never came back to the command, so a
	234	+ * pull request review could not be rendered as JSON or fail a pipeline.
	235	+ */
	236	+describe('reviewPullRequest returns the findings', () => {
	237	+ const config: BuddyConfig = {
	238	+ repository: { provider: 'github', owner: 'o', name: 'r', baseBranch: 'main' },
	239	+ }
	240	+
	241	+ /** An AI client that reports one finding on the added line. */
	242	+ function findingAi(seen: Seen): AiClient {
	243	+ return {
	244	+ name: 'stub',
	245	+ complete: async (request: { system?: string, messages: Array<{ content: string }> }) => {
	246	+ seen.prompts.push(request.messages.map(m => m.content).join('\n'))
	247	+ const json = {
	248	+ summary: 'one problem',
	249	+ walkthrough: [],
	250	+ findings: [{ path: 'src/app.ts', line: 2, severity: 'critical', category: 'security', message: 'hard-coded token' }],
	251	+ }
	252	+ return { text: JSON.stringify(json), json, outputTokens: 1 }
	253	+ },
	254	+ } as unknown as AiClient
	255	+ }
	256	+
	257	+ it('success case - the findings come back with the status', async () => {
	258	+ const { provider, seen } = stub()
	259	+
	260	+ const outcome = await reviewPullRequest({ config, provider, prNumber: 7, ai: findingAi(seen), logger: Logger.silent() })
	261	+
	262	+ expect(outcome.status).toContain('1 finding')
	263	+ expect(outcome.result?.findings.map(f => f.severity)).toEqual(['critical'])
	264	+ })
	265	+
	266	+ it('success case - a skipped review carries no result', async () => {
	267	+ const { provider } = stub()
	268	+
	269	+ const outcome = await reviewPullRequest({ config, provider, prNumber: 999, ai: null, logger: Logger.silent() })
	270	+
	271	+ expect(outcome.status).toContain('Could not find')
	272	+ expect(outcome.result).toBeUndefined()
	273	+ })
	274	+
	275	+ it('success case - the string form is the status alone', async () => {
	276	+ const { provider, seen } = stub()
	277	+
	278	+ const status = await runReviewForPR({ config, provider, prNumber: 7, ai: findingAi(seen), logger: Logger.silent() })
	279	+
	280	+ expect(typeof status).toBe('string')
	281	+ })
	282	+})

test/scan-strategy.test.tsadded+49-0

Changes to test/scan-strategy.test.ts

		@@ -0,0 +1,49 @@
	1	+import type { PackageUpdate } from '../src/types'
	2	+import { describe, expect, it } from 'bun:test'
	3	+import { Buddy } from '../src/buddy'
	4	+
	5	+function update(name: string, updateType: PackageUpdate['updateType']): PackageUpdate {
	6	+ return {
	7	+ name,
	8	+ currentVersion: '1.0.0',
	9	+ newVersion: updateType === 'major' ? '2.0.0' : updateType === 'minor' ? '1.1.0' : '1.0.1',
	10	+ updateType,
	11	+ dependencyType: 'dependencies',
	12	+ file: 'package.json',
	13	+ } as PackageUpdate
	14	+}
	15	+
	16	+/**
	17	+ * `buddy scan --strategy patch --packages react` folded `--strategy` into the
	18	+ * config and then listed the major anyway: the full scan filtered by strategy
	19	+ * and the targeted `--packages` / `--pattern` paths did not.
	20	+ */
	21	+describe('strategy on targeted checks', () => {
	22	+ function buddyWith(strategy: 'all' | 'major' | 'minor' | 'patch' | undefined) {
	23	+ const buddy = new Buddy({ packages: strategy ? { strategy } : ({} as never) }, process.cwd())
	24	+ const client = (buddy as never as { registryClient: Record<string, unknown> }).registryClient
	25	+ client.getUpdatesForPackages = async () => [update('react', 'major'), update('lodash', 'patch')]
	26	+ client.getUpdatesWithPattern = async () => [update('@types/node', 'minor'), update('@types/bun', 'patch')]
	27	+ return buddy
	28	+ }
	29	+
	30	+ it('success case - --packages honours the strategy', async () => {
	31	+ const updates = await buddyWith('patch').checkPackages(['react', 'lodash'])
	32	+
	33	+ expect(updates.map(u => u.name)).toEqual(['lodash'])
	34	+ })
	35	+
	36	+ it('success case - --pattern honours the strategy', async () => {
	37	+ const updates = await buddyWith('patch').checkPackagesWithPattern('@types/*')
	38	+
	39	+ expect(updates.map(u => u.name)).toEqual(['@types/bun'])
	40	+ })
	41	+
	42	+ it('success case - all admits everything, as the scan does', async () => {
	43	+ expect(await buddyWith('all').checkPackages(['react', 'lodash'])).toHaveLength(2)
	44	+ })
	45	+
	46	+ it('edge case - no strategy configured filters nothing', async () => {
	47	+ expect(await buddyWith(undefined).checkPackages(['react', 'lodash'])).toHaveLength(2)
	48	+ })
	49	+})