ReviewOS
Reload

also looking at this

stacks/buddy

fix(review): honour the ai.review settings that nothing read

#1406
Merged glennmichael123 wants to merge fix/honour-review-filters into main
7 files +282 -8

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+16-1
@@ -291,6 +291,8 @@ interface CLIOptions {
291291 respectLatest?: boolean
292292 /** Which settings pages `open-settings` should open: repo, org or both. */
293293 type?: string
294 /** Set by the generated workflow: honour the `ai.review` filters. */
295 auto?: boolean
294296}
295297
296298cli
@@ -1558,6 +1560,7 @@ cli
15581560 .option('--fail-on <severity>', 'Exit non-zero when a finding at or above this severity is found')
15591561 .option('--profile <name>', 'Review profile: chill|assertive')
15601562 .option('--summary-only', 'Post only the summary, without inline findings')
1563 .option('--auto', 'Treat this as an automatic trigger, honouring the ai.review filters')
15611564 .option('--dry-run', 'Print the review instead of posting it')
15621565 .example('buddy review 42')
15631566 .example('buddy review --staged --light')
@@ -1621,12 +1624,24 @@ cli
16211624 gitProvider = await providerFor(config, 'reviewing a pull request', log)
16221625
16231626 const number = Number.parseInt(prNumber, 10)
1624 diff = await gitProvider.getPullRequestDiff(number)
16251627
16261628 // Carry forward what earlier reviews already said, so a re-review
16271629 // reports only what is new rather than repeating itself.
16281630 const prs = await gitProvider.getPullRequests('open')
16291631 const pr = prs.find(candidate => candidate.number === number)
1632
1633 // `--auto` is what the generated workflow passes. Without it this is
1634 // someone at a terminal, who has already decided they want a review.
1635 if (pr) {
1636 const { reviewSkipReason } = await import('../src/review/filters')
1637 const skip = reviewSkipReason(config, pr, options.auto ? 'automatic' : 'requested')
1638 if (skip) {
1639 log.info(`🔍 Skipping PR #${number}: ${skip}`)
1640 return
1641 }
1642 }
1643
1644 diff = await gitProvider.getPullRequestDiff(number)
16301645 const state = parseReviewState(pr?.body)
16311646 seenFingerprints = state?.fingerprints ?? []
16321647 prBody = pr?.body ?? null
docs/cli/review.mdmodified+24-0
@@ -20,6 +20,30 @@ The default includes staged changes deliberately: a pre-commit review that
2020ignored what you just staged would miss the very changes you are about to
2121commit.
2222
23## Reviewing a pull request
24
25Pass a number to review a pull request instead of local changes:
26
27```bash
28buddy review 123
29```
30
31Add `--auto` when Buddy is choosing to review rather than being asked — which
32is what the generated workflow does. It honours the filters under
33`ai.review`, so a draft or a `wip` pull request is skipped before the diff is
34fetched, costing one API call rather than a full review:
35
36| Setting | Skips when |
37|---|---|
38| `enabled: false` | Always — including an explicit `@buddy review` |
39| `autoReview: false` | The trigger is automatic; asking still works |
40| `drafts` | The pull request is a draft and this is not `true` |
41| `ignoreTitleKeywords` | The title contains one of them, case-insensitively |
42| `ignoreUsernames` | The author is listed |
43
44Without `--auto` none of these apply except `enabled`, because a person at a
45terminal has already decided they want the review.
46
2347## Without an API key
2448
2549`--light` runs the analyzers and skips the model entirely — secret scanning,
src/review/filters.tsadded+102-0
@@ -0,0 +1,102 @@
1import type { BuddyConfig, PullRequest } from '../types'
2
3/**
4 * How a review came to be running.
5 *
6 * The distinction matters because every filter here except `enabled` describes
7 * when Buddy should review *on its own*. Someone who types `@buddy review` on a
8 * draft has answered the question the filter exists to ask.
9 */
10export type ReviewTrigger = 'automatic' | 'requested'
11
12/** The pull request fields a filter decision needs. */
13export type ReviewCandidate = Pick<PullRequest, 'title' | 'author' | 'draft'>
14
15/**
16 * Why this pull request should not be reviewed, if it should not be.
17 *
18 * These settings were declared, validated and documented long before anything
19 * read them, so a repository that set `ai.review.enabled: false` still got
20 * reviews. Each one is honoured here, in one place, rather than being scattered
21 * across the two paths that post reviews.
22 *
23 * `undefined` means "not configured" and never means "off": reviews already
24 * require both a workflow trigger and a provider key, so treating an unset flag
25 * as a refusal would silently disable reviews for every existing repository.
26 * Only an explicit `false` turns something off.
27 *
28 * @param config - Resolved Buddy configuration
29 * @param pr - Pull request under consideration
30 * @param trigger - Whether Buddy chose to review, or was asked to
31 * @returns A human-readable reason to skip, or `null` to go ahead
32 * @example
33 * ```ts
34 * const skip = reviewSkipReason(config, pr, 'automatic')
35 * if (skip)
36 * return skip
37 * ```
38 */
39export function reviewSkipReason(
40 config: BuddyConfig,
41 pr: ReviewCandidate,
42 trigger: ReviewTrigger,
43): string | null {
44 const review = config.ai?.review
45
46 // The one filter that also refuses an explicit request: `enabled: false` is
47 // a statement about the repository, not about a particular pull request.
48 if (review?.enabled === false)
49 return 'AI review is disabled for this repository (`ai.review.enabled` is false).'
50
51 if (trigger === 'requested')
52 return null
53
54 if (review?.autoReview === false)
55 return 'Automatic review is off (`ai.review.autoReview` is false). Ask for one with `@buddy review`.'
56
57 if (pr.draft && review?.drafts !== true)
58 return 'This pull request is a draft. Set `ai.review.drafts` to true, or ask with `@buddy review`.'
59
60 const keyword = matchedKeyword(pr.title, review?.ignoreTitleKeywords)
61 if (keyword)
62 return `The title contains \`${keyword}\` (\`ai.review.ignoreTitleKeywords\`).`
63
64 if (matchesUsername(pr.author, review?.ignoreUsernames))
65 return `Pull requests from \`${pr.author}\` are ignored (\`ai.review.ignoreUsernames\`).`
66
67 return null
68}
69
70/**
71 * The first configured keyword appearing in a title.
72 *
73 * Matched case-insensitively as a substring, so `wip` catches `WIP:` and
74 * `[wip]` without asking anyone to write a regular expression.
75 *
76 * @param title - Pull request title
77 * @param keywords - Configured keywords, if any
78 * @returns The keyword that matched, or `null`
79 */
80function matchedKeyword(title: string, keywords: string[] | undefined): string | null {
81 if (!keywords?.length)
82 return null
83
84 const haystack = title.toLowerCase()
85 return keywords.find(keyword => keyword.trim() && haystack.includes(keyword.toLowerCase().trim())) ?? null
86}
87
88/**
89 * Whether an author is on the ignore list.
90 *
91 * Compared case-insensitively, because GitHub logins are.
92 *
93 * @param author - Pull request author's login
94 * @param usernames - Configured logins to ignore, if any
95 */
96function matchesUsername(author: string, usernames: string[] | undefined): boolean {
97 if (!usernames?.length)
98 return false
99
100 const login = author.toLowerCase()
101 return usernames.some(name => name.toLowerCase().trim() === login)
102}
src/review/run.tsmodified+18-0
@@ -8,6 +8,8 @@ import { getDefaultLogger } from '../utils/logger'
88import { parseUnifiedDiff } from './diff'
99import { reviewDiff } from './engine'
1010import { composeInstructions, loadGuidelines } from './guidelines'
11import type { ReviewTrigger } from './filters'
12import { reviewSkipReason } from './filters'
1113import { needsReview, parseReviewState, upsertReviewState } from './marker'
1214import type { PreparedReview } from './poster'
1315import { prepareReview } from './poster'
@@ -23,6 +25,14 @@ export interface RunReviewOptions {
2325 summaryOnly?: boolean
2426 /** Skip when the head commit was already reviewed */
2527 skipIfReviewed?: boolean
28 /**
29 * Whether Buddy chose to review or was asked to (default: `requested`).
30 *
31 * Defaults to the permissive value so a caller that has not been taught the
32 * distinction keeps working: an unasked-for filter should never silently
33 * swallow a review someone requested.
34 */
35 trigger?: ReviewTrigger
2636 logger?: Logger
2737}
2838
@@ -45,6 +55,14 @@ export async function runReviewForPR(options: RunReviewOptions): Promise<string>
4555 if (!pr)
4656 return `Could not find open pull request #${prNumber}.`
4757
58 // Checked before the diff is fetched or a model is contacted, so an ignored
59 // pull request costs one API call rather than a review's worth of tokens.
60 const skip = reviewSkipReason(config, pr, options.trigger ?? 'requested')
61 if (skip) {
62 logger.info(`🔍 Skipping PR #${prNumber}: ${skip}`)
63 return skip
64 }
65
4866 const state = parseReviewState(pr.body)
4967 if (state?.paused && !options.full)
5068 return 'Reviews are paused on this pull request. Say `@buddy resume` to restart them.'
src/setup.tsmodified+5-3
@@ -1587,10 +1587,12 @@ ${generateComposerSetupSteps()}
15871587 - name: Install dependencies
15881588 run: bun install
15891589
1590 # Skips itself when ai.review.enabled is false, so adding the trigger
1591 # does not turn reviews on by itself.
1590 # --auto honours the ai.review filters: enabled, autoReview, drafts,
1591 # ignoreTitleKeywords and ignoreUsernames. Adding the trigger therefore
1592 # does not turn reviews on by itself, and a draft or WIP pull request
1593 # costs one API call rather than a full review.
15921594 - name: Review pull request
1593 run: bunx @buddysh/buddy review \${{ github.event.pull_request.number }} --verbose
1595 run: bunx @buddysh/buddy review \${{ github.event.pull_request.number }} --auto --verbose
15941596 env:
15951597 GITHUB_TOKEN: \${{ secrets.GITHUB_TOKEN }}
15961598 ANTHROPIC_API_KEY: \${{ secrets.ANTHROPIC_API_KEY }}
src/types.tsmodified+14-4
@@ -298,7 +298,14 @@ export interface BuddyConfig {
298298
299299 /** AI pull request review settings */
300300 review?: {
301 /** Enable AI review (default: false until you opt in) */
301 /**
302 * Set to `false` to switch AI review off for this repository, including
303 * when someone asks with `@buddy review`.
304 *
305 * Leaving it unset does not mean off: review already requires both a
306 * workflow trigger and a provider key, so this is the override, not the
307 * opt-in.
308 */
302309 enabled?: boolean
303310 /**
304311 * How thorough to be. `chill` reports only confident defects;
@@ -306,11 +313,14 @@ export interface BuddyConfig {
306313 * be filtered downstream.
307314 */
308315 profile?: 'chill' | 'assertive'
309 /** Review draft pull requests too (default: false) */
316 /** Review draft pull requests on an automatic trigger (default: false) */
310317 drafts?: boolean
311 /** Review automatically on open and push (default: true when enabled) */
318 /** Review automatically on open and push (default: true) */
312319 autoReview?: boolean
313 /** Skip pull requests whose title contains any of these, e.g. `wip` */
320 /**
321 * Skip pull requests whose title contains any of these, e.g. `wip`.
322 * Matched case-insensitively as a substring.
323 */
314324 ignoreTitleKeywords?: string[]
315325 /** Skip pull requests opened by these users */
316326 ignoreUsernames?: string[]
test/review-filters.test.tsadded+103-0
@@ -0,0 +1,103 @@
1import type { BuddyConfig } from '../src/types'
2import { describe, expect, it } from 'bun:test'
3import { reviewSkipReason } from '../src/review/filters'
4
5/**
6 * These five settings were typed, validated and documented long before
7 * anything read them, so a repository that set `ai.review.enabled: false`
8 * still got reviews.
9 */
10describe('review filters', () => {
11 const pr = { title: 'Add retry to the fetch helper', author: 'alice', draft: false }
12
13 /** Config carrying only the review settings under test. */
14 function withReview(review: NonNullable<NonNullable<BuddyConfig['ai']>['review']>): BuddyConfig {
15 return { repository: { provider: 'github', owner: 'stacksjs', name: 'buddy' }, ai: { review } }
16 }
17
18 describe('enabled', () => {
19 it('success case - an unset flag reviews, rather than silently refusing', () => {
20 // Reviews already need a workflow trigger and a provider key. Treating
21 // unset as off would disable review for every existing repository.
22 expect(reviewSkipReason(withReview({}), pr, 'automatic')).toBeNull()
23 expect(reviewSkipReason(withReview({}), pr, 'requested')).toBeNull()
24 })
25
26 it('failure case - false refuses even an explicit request', () => {
27 // `enabled` describes the repository, not one pull request, so asking
28 // does not override it.
29 const config = withReview({ enabled: false })
30
31 expect(reviewSkipReason(config, pr, 'automatic')).toContain('disabled')
32 expect(reviewSkipReason(config, pr, 'requested')).toContain('disabled')
33 })
34 })
35
36 describe('autoReview', () => {
37 it('failure case - false stops the automatic trigger', () => {
38 expect(reviewSkipReason(withReview({ autoReview: false }), pr, 'automatic')).toContain('Automatic review is off')
39 })
40
41 it('success case - an explicit request still works', () => {
42 expect(reviewSkipReason(withReview({ autoReview: false }), pr, 'requested')).toBeNull()
43 })
44 })
45
46 describe('drafts', () => {
47 const draft = { ...pr, draft: true }
48
49 it('failure case - drafts are skipped on an automatic trigger by default', () => {
50 expect(reviewSkipReason(withReview({}), draft, 'automatic')).toContain('draft')
51 })
52
53 it('success case - opting in reviews them', () => {
54 expect(reviewSkipReason(withReview({ drafts: true }), draft, 'automatic')).toBeNull()
55 })
56
57 it('success case - asking for a review of a draft overrides the default', () => {
58 expect(reviewSkipReason(withReview({}), draft, 'requested')).toBeNull()
59 })
60 })
61
62 describe('ignoreTitleKeywords', () => {
63 it('failure case - matches case-insensitively as a substring', () => {
64 // `wip` has to catch `WIP:` and `[wip]` without anyone writing a regex.
65 const config = withReview({ ignoreTitleKeywords: ['wip'] })
66
67 expect(reviewSkipReason(config, { ...pr, title: 'WIP: refactor' }, 'automatic')).toContain('wip')
68 expect(reviewSkipReason(config, { ...pr, title: '[wip] refactor' }, 'automatic')).toContain('wip')
69 })
70
71 it('success case - an unrelated title passes', () => {
72 expect(reviewSkipReason(withReview({ ignoreTitleKeywords: ['wip'] }), pr, 'automatic')).toBeNull()
73 })
74
75 it('edge case - an empty keyword does not match everything', () => {
76 // A blank entry left in a config array would otherwise skip every PR.
77 expect(reviewSkipReason(withReview({ ignoreTitleKeywords: ['', ' '] }), pr, 'automatic')).toBeNull()
78 })
79 })
80
81 describe('ignoreUsernames', () => {
82 it('failure case - skips a listed author, ignoring case', () => {
83 const config = withReview({ ignoreUsernames: ['Dependabot[bot]'] })
84
85 expect(reviewSkipReason(config, { ...pr, author: 'dependabot[bot]' }, 'automatic')).toContain('dependabot[bot]')
86 })
87
88 it('success case - an unlisted author passes', () => {
89 expect(reviewSkipReason(withReview({ ignoreUsernames: ['bob'] }), pr, 'automatic')).toBeNull()
90 })
91
92 it('edge case - a partial match is not a match', () => {
93 // Substring matching here would let `bob` ignore `bobby`.
94 expect(reviewSkipReason(withReview({ ignoreUsernames: ['ali'] }), pr, 'automatic')).toBeNull()
95 })
96 })
97
98 it('edge case - no ai config at all reviews normally', () => {
99 const config: BuddyConfig = { repository: { provider: 'github', owner: 'stacksjs', name: 'buddy' } }
100
101 expect(reviewSkipReason(config, pr, 'automatic')).toBeNull()
102 })
103})