ReviewOS
Reload

also looking at this

stacks/buddy

fix!: make strategies and labels behave as documented

#1399
Merged glennmichael123 wants to merge fix/strategy-and-label-semantics into main
7 files +187 -27

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.

.github/workflows/buddy.ymlmodified+1-1
@@ -254,7 +254,7 @@ jobs:
254254 echo "" >> $GITHUB_STEP_SUMMARY
255255
256256 if [ "$" = "schedule" ]; then
257 echo "⏰ **Scheduled Check**: Automatically checks every minute" >> $GITHUB_STEP_SUMMARY
257 echo "⏰ **Scheduled Check**: Daily cleanup tick — rebase requests are handled on PR edit, in real time" >> $GITHUB_STEP_SUMMARY
258258 else
259259 echo "🖱️ **Manual Check**: Manually triggered from Actions tab" >> $GITHUB_STEP_SUMMARY
260260 fi
src/buddy.tsmodified+43-21
@@ -27,7 +27,7 @@ import { appendUpgradeReport } from './upgrades/wire'
2727import { EventBus } from './events/bus'
2828import { createSinks } from './events/sinks'
2929import type { PackageRule } from './rules/engine'
30import { applyRules, groupsToRules, mergeGroupEffects, resolveRuleEffects } from './rules/engine'
30import { applyRules, groupsToRules, mergeGroupEffects, resolveRuleEffects, strategyAllows } from './rules/engine'
3131import { applyTemplate, templateTokensForGroup } from './pr/templates'
3232import { RegistryClient } from './registry/registry-client'
3333import { PackageScanner } from './scanner/package-scanner'
@@ -363,6 +363,30 @@ export class Buddy {
363363 return merged.length > 0 ? merged : undefined
364364 }
365365
366 /**
367 * Every label a pull request for this group should carry.
368 *
369 * Three sources union here: the automatic set derived from the update types
370 * and manifests in the group, whatever `pullRequest.labels` declares, and the
371 * labels any matching rule asked for.
372 *
373 * Centralised because the five call sites had drifted — only the creation
374 * path folded in rule labels, so refreshing an existing pull request quietly
375 * dropped them, and `pullRequest.labels` was read by nothing at all despite
376 * being declared and validated.
377 *
378 * @param group - Updates the pull request will contain
379 * @param prGenerator - Generator supplying the automatic labels
380 * @returns Labels to set on the pull request
381 */
382 private labelsFor(group: UpdateGroup, prGenerator: PullRequestGenerator): string[] {
383 return [...new Set([
384 ...prGenerator.generateLabels(group),
385 ...(this.config.pullRequest?.labels ?? []),
386 ...this.groupEffectsFor(group.updates).labels,
387 ])]
388 }
389
366390 /** Every rule governing this run, legacy groups compiled in first. */
367391 private activeRules(): PackageRule[] {
368392 return [...groupsToRules(this.config.packages?.groups), ...(this.config.packages?.rules ?? [])]
@@ -795,7 +819,7 @@ export class Buddy {
795819 }
796820
797821 // Generate dynamic labels for the update
798 const dynamicLabels = prGenerator.generateLabels(group)
822 const dynamicLabels = this.labelsFor(group, prGenerator)
799823
800824 // Update existing PR with new content
801825 await gitProvider.updatePullRequest(existingPR.number, {
@@ -841,7 +865,7 @@ export class Buddy {
841865 await gitProvider.commitChanges(branchName, this.commitMessageFor(group, '(updated)'), packageJsonUpdates, this.config.repository.baseBranch || 'main')
842866 }
843867
844 const dynamicLabels = prGenerator.generateLabels(group)
868 const dynamicLabels = this.labelsFor(group, prGenerator)
845869 await gitProvider.updatePullRequest(existingPRForBranch.number, {
846870 title: prTitle,
847871 body: prBody,
@@ -887,7 +911,7 @@ export class Buddy {
887911 }
888912
889913 await gitProvider.reopenPullRequest!(recentlyClosed.number)
890 const dynamicLabels = prGenerator.generateLabels(group)
914 const dynamicLabels = this.labelsFor(group, prGenerator)
891915 await gitProvider.updatePullRequest(recentlyClosed.number, {
892916 title: prTitle,
893917 body: prBody,
@@ -958,7 +982,7 @@ export class Buddy {
958982 // Reopen and update the existing PR
959983 await gitProvider.reopenPullRequest!(recentlyClosed.number)
960984 // Point the reopened PR at the new branch (in case branch name changed)
961 const dynamicLabels = prGenerator.generateLabels(group)
985 const dynamicLabels = this.labelsFor(group, prGenerator)
962986 await gitProvider.updatePullRequest(recentlyClosed.number, {
963987 title: prTitle,
964988 body: prBody,
@@ -1054,10 +1078,10 @@ export class Buddy {
10541078 // Commit changes (Renovate-style: branch is created from base, changes applied fresh)
10551079 await gitProvider.commitChanges(branchName, this.commitMessageFor(group), packageJsonUpdates, this.config.repository.baseBranch || 'main')
10561080
1057 // Generate dynamic labels based on update types and package types,
1058 // then union whatever the matching rules asked for.
1081 // Reviewers and assignees still union here; labels are resolved by
1082 // `labelsFor`, which every pull-request path shares.
10591083 const groupEffects = this.groupEffectsFor(group.updates)
1060 const dynamicLabels = [...new Set([...prGenerator.generateLabels(group), ...groupEffects.labels])]
1084 const dynamicLabels = this.labelsFor(group, prGenerator)
10611085
10621086 // Create pull request
10631087 const pr = await gitProvider.createPullRequest({
@@ -1863,7 +1887,16 @@ export class Buddy {
18631887 }
18641888
18651889 /**
1866 * Filter updates by strategy
1890 * Filter updates by strategy.
1891 *
1892 * Defers to `strategyAllows`, the same predicate the rules engine uses for a
1893 * rule-level `strategy`, so `packages.strategy` and a per-rule strategy can
1894 * never again mean different things on the same set of updates.
1895 *
1896 * @param updates - Updates to filter
1897 * @param strategy - Ceiling on semver impact: `all` admits everything,
1898 * `major` only majors, `minor` minors and patches, `patch` only patches
1899 * @returns The updates the strategy permits
18671900 */
18681901 private filterUpdatesByStrategy(
18691902 updates: PackageUpdate[],
@@ -1872,18 +1905,7 @@ export class Buddy {
18721905 if (strategy === 'all')
18731906 return updates
18741907
1875 return updates.filter((update) => {
1876 switch (strategy) {
1877 case 'major':
1878 return update.updateType === 'major'
1879 case 'minor':
1880 return update.updateType === 'major' || update.updateType === 'minor'
1881 case 'patch':
1882 return true // Include all types for patch strategy
1883 default:
1884 return true
1885 }
1886 })
1908 return updates.filter(update => strategyAllows(strategy, update.updateType))
18871909 }
18881910
18891911 /**
src/commands/dispatcher.tsmodified+0-2
@@ -46,12 +46,10 @@ export const HELP_TEXT = `**Buddy commands**
4646| \`@buddy review\` | Review new changes since the last review |
4747| \`@buddy full-review\` | Review the whole diff again, ignoring previous findings |
4848| \`@buddy summary\` | Post a fresh summary without inline findings |
49| \`@buddy resolve\` | Resolve the review threads this bot opened |
5049| \`@buddy pause\` / \`resume\` | Stop or restart reviewing this pull request |
5150| \`@buddy rebase\` | Rebase a dependency update pull request |
5251| \`@buddy merge\` | Re-check auto-merge conditions now |
5352| \`@buddy fix-ci\` | Diagnose and try to fix failing checks |
54| \`@buddy plan\` | Produce an implementation plan for an issue |
5553| \`@buddy remember <text>\` | Record a durable note for future runs |
5654| \`@buddy help\` | Show this table |
5755
src/rules/engine.tsmodified+19-2
@@ -283,8 +283,25 @@ export function mergeGroupEffects(effects: ResolvedEffects[]): {
283283 }
284284}
285285
286/** Whether a strategy permits an update of this type. */
287function strategyAllows(strategy: NonNullable<PackageRule['strategy']>, updateType: 'major' | 'minor' | 'patch'): boolean {
286/**
287 * Whether a strategy permits an update of this type.
288 *
289 * A strategy names the greatest semver impact a run may propose, so each one
290 * admits its own update type and everything gentler beneath it. Exported so the
291 * global filter in `src/buddy.ts` resolves the same question through the same
292 * function — the two carried separate switches once, and disagreed, which made
293 * `packages.strategy` and a rule-level `strategy` mean different things.
294 *
295 * @param strategy - Ceiling on semver impact
296 * @param updateType - The impact of the update being considered
297 * @returns Whether the update is within the ceiling
298 * @example
299 * ```ts
300 * strategyAllows('minor', 'patch') // true
301 * strategyAllows('patch', 'major') // false
302 * ```
303 */
304export function strategyAllows(strategy: NonNullable<PackageRule['strategy']>, updateType: 'major' | 'minor' | 'patch'): boolean {
288305 switch (strategy) {
289306 case 'all':
290307 return true
src/setup.tsmodified+1-1
@@ -1703,7 +1703,7 @@ ${generateComposerSetupSteps()}
17031703 echo "" >> \$GITHUB_STEP_SUMMARY
17041704
17051705 if [ "\$" = "schedule" ]; then
1706 echo "⏰ **Scheduled Check**: Automatically checks every minute" >> \$GITHUB_STEP_SUMMARY
1706 echo "⏰ **Scheduled Check**: Daily cleanup tick — rebase requests are handled on PR edit, in real time" >> \$GITHUB_STEP_SUMMARY
17071707 else
17081708 echo "🖱️ **Manual Check**: Manually triggered from Actions tab" >> \$GITHUB_STEP_SUMMARY
17091709 fi
test/package-rules.test.tsmodified+37-0
@@ -8,6 +8,7 @@ import {
88 mergeGroupEffects,
99 resolveRuleEffects,
1010 ruleMatches,
11 strategyAllows,
1112} from '../src/rules/engine'
1213
1314function makeUpdate(overrides: Partial<PackageUpdate> = {}): PackageUpdate {
@@ -214,3 +215,39 @@ describe('legacy group compilation', () => {
214215 expect(groupsToRules()).toEqual([])
215216 })
216217})
218
219describe('strategy semantics', () => {
220 const types = ['major', 'minor', 'patch'] as const
221
222 it('success case - each strategy admits its own type and everything gentler', () => {
223 expect(types.filter(t => strategyAllows('all', t))).toEqual(['major', 'minor', 'patch'])
224 expect(types.filter(t => strategyAllows('major', t))).toEqual(['major'])
225 expect(types.filter(t => strategyAllows('minor', t))).toEqual(['minor', 'patch'])
226 expect(types.filter(t => strategyAllows('patch', t))).toEqual(['patch'])
227 })
228
229 it('edge case - patch never admits a major', () => {
230 // `patch` once returned true for every update type, so the safest-sounding
231 // setting silently proposed breaking changes.
232 expect(strategyAllows('patch', 'major')).toBe(false)
233 })
234
235 it('edge case - minor never admits a major', () => {
236 // `minor` once admitted majors while excluding patches, inverting it.
237 expect(strategyAllows('minor', 'major')).toBe(false)
238 expect(strategyAllows('minor', 'patch')).toBe(true)
239 })
240
241 it('success case - a rule-level strategy filters the same way as the predicate', () => {
242 // The global filter in `src/buddy.ts` and the rules engine each carried
243 // their own switch and disagreed. Both now resolve through this predicate.
244 for (const strategy of ['all', 'major', 'minor', 'patch'] as const) {
245 const kept = applyRules(
246 types.map(t => makeUpdate({ updateType: t })),
247 [{ matchPackages: ['*'], strategy }],
248 ).map(r => r.update.updateType)
249
250 expect(kept).toEqual(types.filter(t => strategyAllows(strategy, t)))
251 }
252 })
253})
test/pr-labels.test.tsadded+86-0
@@ -0,0 +1,86 @@
1import type { BuddyConfig, PackageUpdate, UpdateGroup } from '../src/types'
2import { describe, expect, it } from 'bun:test'
3import { Buddy } from '../src/buddy'
4import { PullRequestGenerator } from '../src/pr/pr-generator'
5
6/**
7 * Labels reaching a pull request come from three places, and each one was
8 * broken or missing at some point: the automatic set, `pullRequest.labels`
9 * (declared and validated but read by nothing), and rule effects (folded in
10 * only on the creation path, so refreshing a pull request dropped them).
11 */
12describe('pull request labels', () => {
13 const update: PackageUpdate = {
14 name: 'react',
15 currentVersion: '18.0.0',
16 newVersion: '18.0.1',
17 updateType: 'patch',
18 dependencyType: 'dependencies',
19 file: 'package.json',
20 }
21
22 const group: UpdateGroup = {
23 name: 'Patch Updates',
24 updateType: 'patch',
25 title: 'chore(deps): update react to v18.0.1',
26 body: '',
27 updates: [update],
28 }
29
30 const baseConfig: BuddyConfig = {
31 repository: { provider: 'github', owner: 'stacksjs', name: 'buddy' },
32 }
33
34 /** Resolve labels the way every pull-request path now does. */
35 function labelsFor(config: BuddyConfig): string[] {
36 const buddy = new Buddy(config)
37 // @ts-expect-error - accessing private method for testing
38 return buddy.labelsFor(group, new PullRequestGenerator(config))
39 }
40
41 it('success case - always includes the automatic set', () => {
42 const labels = labelsFor(baseConfig)
43 expect(labels).toContain('dependencies')
44 expect(labels).toContain('patch')
45 expect(labels).toContain('npm')
46 })
47
48 it('success case - includes labels declared in pullRequest.labels', () => {
49 // This key was declared, validated and documented, but the live path never
50 // read it, so a configured label silently never reached a pull request.
51 const labels = labelsFor({ ...baseConfig, pullRequest: { labels: ['deps', 'automated'] } })
52 expect(labels).toContain('deps')
53 expect(labels).toContain('automated')
54 expect(labels).toContain('dependencies')
55 })
56
57 it('success case - includes labels a matching rule asked for', () => {
58 const labels = labelsFor({
59 ...baseConfig,
60 packages: { strategy: 'all', rules: [{ matchPackages: ['react'], labels: ['frontend'] }] },
61 })
62 expect(labels).toContain('frontend')
63 })
64
65 it('success case - unions all three sources without duplicating', () => {
66 const labels = labelsFor({
67 ...baseConfig,
68 pullRequest: { labels: ['dependencies', 'deps'] },
69 packages: { strategy: 'all', rules: [{ matchPackages: ['react'], labels: ['deps', 'frontend'] }] },
70 })
71
72 expect(labels).toContain('deps')
73 expect(labels).toContain('frontend')
74 // `dependencies` is automatic and also configured; it must appear once.
75 expect(labels.filter(label => label === 'dependencies')).toHaveLength(1)
76 expect(labels.filter(label => label === 'deps')).toHaveLength(1)
77 })
78
79 it('edge case - a rule that does not match contributes nothing', () => {
80 const labels = labelsFor({
81 ...baseConfig,
82 packages: { strategy: 'all', rules: [{ matchPackages: ['vue'], labels: ['should-not-appear'] }] },
83 })
84 expect(labels).not.toContain('should-not-appear')
85 })
86})