ReviewOS
Reload

also looking at this

stacks/buddy

fix: keep the low-tier promises — bulk-update label, event-name validation, workflow personalisation

#1434
Merged glennmichael123 wants to merge fix/low-tier-wiring into main
9 files +325 -13
bin/cli.tsmodified+33-0
@@ -290,6 +290,12 @@ interface CLIOptions {
290290 ignore?: string
291291 dryRun?: boolean
292292 respectLatest?: boolean
293 /** Comma-separated reviewers `update` assigns, overriding `pullRequest.reviewers` */
294 reviewers?: string
295 /** Comma-separated assignees `update` assigns, overriding `pullRequest.assignees` */
296 assignees?: string
297 /** Comma-separated labels `update` adds, overriding `pullRequest.labels` */
298 labels?: string
293299 /** Which settings pages `open-settings` should open: repo, org or both. */
294300 type?: string
295301 /** Set by the generated workflow: honour the `ai.review` filters. */
@@ -844,6 +850,9 @@ cli
844850 .option('--strategy <type>', 'Update strategy: major|minor|patch|all', { default: 'all' })
845851 .option('--ignore <names>', 'Comma-separated list of packages to ignore')
846852 .option('--dry-run', 'Preview changes without making them')
853 .option('--reviewers <names>', 'Comma-separated reviewers to assign (overrides pullRequest.reviewers)')
854 .option('--assignees <names>', 'Comma-separated assignees to assign (overrides pullRequest.assignees)')
855 .option('--labels <names>', 'Comma-separated labels to add (overrides pullRequest.labels)')
847856 .option('--respect-latest', 'Respect "latest", "*", and other dynamic version indicators (default: true)')
848857 .option('--no-respect-latest', 'Allow updating "latest", "*", and other dynamic version indicators')
849858 .example('buddy update')
@@ -864,9 +873,26 @@ cli
864873 ignore = options.ignore.split(',').map(p => p.trim())
865874 }
866875
876 // Per-run PR overrides, from the flags a generated workflow passes.
877 // Folded in only when given: `pullRequest` staying unset is itself a
878 // setting, and materialising an empty block would change what runs.
879 const splitList = (value?: string): string[] | undefined =>
880 value ? value.split(',').map(part => part.trim()).filter(Boolean) : undefined
881 const reviewers = splitList(options.reviewers)
882 const assignees = splitList(options.assignees)
883 const labels = splitList(options.labels)
884 const prOverrides = {
885 ...(reviewers ? { reviewers } : {}),
886 ...(assignees ? { assignees } : {}),
887 ...(labels ? { labels } : {}),
888 }
889
867890 const finalConfig: BuddyConfig = {
868891 ...config,
869892 verbose: options.verbose ?? config.verbose,
893 ...(Object.keys(prOverrides).length
894 ? { pullRequest: { ...config.pullRequest, ...prOverrides } }
895 : {}),
870896 packages: {
871897 ...config.packages,
872898 strategy: options.strategy ?? config.packages?.strategy ?? 'all',
@@ -1864,6 +1890,13 @@ cli
18641890 const logger = options.verbose ? Logger.verbose() : Logger.quiet()
18651891 const config = await resolveConfig(options.config)
18661892
1893 // Explicit `false` only, the same reading the dashboard uses: a
1894 // repository that never wrote a `workflows` block keeps the command.
1895 if (config.workflows?.enabled === false) {
1896 logger.info('\u2139\ufe0f Workflow generation is disabled by config (workflows.enabled: false)')
1897 return
1898 }
1899
18671900 console.log('⚠️ The "generate-workflows" command is deprecated.')
18681901 console.log('💡 Use "buddy setup" for a better interactive experience.\n')
18691902
docs/cli/update.mdmodified+3-0
@@ -78,6 +78,9 @@ buddy update [options]
7878- `--strategy <type>` - Update strategy: major|minor|patch|all (default: all)
7979- `--ignore <names>` - Comma-separated list of packages to ignore
8080- `--dry-run` - Preview changes without making them
81- `--reviewers <names>` - Comma-separated reviewers to assign, overriding `pullRequest.reviewers`
82- `--assignees <names>` - Comma-separated assignees to assign, overriding `pullRequest.assignees`
83- `--labels <names>` - Comma-separated labels to add, overriding `pullRequest.labels`
8184- `--respect-latest` - Leave `latest`, `*` and other dynamic ranges alone (default)
8285- `--no-respect-latest` - Update dynamic ranges too
8386
src/config-validation.tsmodified+66-0
@@ -1,6 +1,7 @@
11import type { BuddyConfig } from './types'
22import type { GitProviderName } from './git/provider'
33import { BUILTIN_ANALYZERS } from './analysis/engine'
4import { EVENT_NAMES } from './events'
45import { IMPLEMENTED_PROVIDERS, PROVIDER_TRACKING_ISSUES } from './git/provider'
56import { ConfigurationError } from './types'
67
@@ -177,6 +178,7 @@ export function validateConfig(config: BuddyConfig): ConfigIssue[] {
177178 validateSchedules(issues, config)
178179 validateReleaseNotes(issues, config)
179180 validateDashboard(issues, config)
181 validateNotifications(issues, config)
180182 validateAi(issues, config)
181183 validateAnalysis(issues, config)
182184
@@ -688,6 +690,70 @@ function validateDashboard(issues: ConfigIssue[], config: BuddyConfig): void {
688690 issues.push({ path: 'dashboard.title', message: `expected a non-empty string, got ${quote(dashboard.title)}` })
689691}
690692
693function validateNotifications(issues: ConfigIssue[], config: BuddyConfig): void {
694 const notifications = config.notifications
695 if (notifications === undefined)
696 return
697 if (!isPlainObject(notifications)) {
698 issues.push({ path: 'notifications', message: `expected an object, got ${quote(notifications)}` })
699 return
700 }
701
702 // An `events` filter naming an event that does not exist subscribes to
703 // nothing: the sink is built, the credential is read, and every
704 // notification is silently dropped. `EVENT_NAMES` exists for exactly this
705 // check.
706 const checkEvents = (path: string, events: unknown): void => {
707 if (events === undefined)
708 return
709 if (!Array.isArray(events)) {
710 issues.push({ path, message: `expected an array of event names, got ${quote(events)}` })
711 return
712 }
713 events.forEach((event, index) => {
714 checkEnum(issues, `${path}[${index}]`, event, EVENT_NAMES)
715 })
716 }
717
718 const checkEnvName = (path: string, value: unknown): void => {
719 if (value !== undefined && (typeof value !== 'string' || !value.trim()))
720 issues.push({ path, message: `expected an environment variable name, got ${quote(value)}` })
721 }
722
723 for (const channel of ['slack', 'discord'] as const) {
724 const target = notifications[channel]
725 if (target === undefined)
726 continue
727 if (!isPlainObject(target)) {
728 issues.push({ path: `notifications.${channel}`, message: `expected an object, got ${quote(target)}` })
729 continue
730 }
731 checkEnvName(`notifications.${channel}.webhookEnv`, target.webhookEnv)
732 checkEvents(`notifications.${channel}.events`, target.events)
733 }
734
735 const webhooks = notifications.webhooks
736 if (webhooks === undefined)
737 return
738 if (!Array.isArray(webhooks)) {
739 issues.push({ path: 'notifications.webhooks', message: `expected an array, got ${quote(webhooks)}` })
740 return
741 }
742 webhooks.forEach((webhook, index) => {
743 const base = `notifications.webhooks[${index}]`
744 if (!isPlainObject(webhook)) {
745 issues.push({ path: base, message: `expected an object, got ${quote(webhook)}` })
746 return
747 }
748 if (webhook.url === undefined)
749 issues.push({ path: `${base}.url`, message: 'a webhook needs a url' })
750 else
751 checkUrl(issues, `${base}.url`, webhook.url)
752 checkEnvName(`${base}.secretEnv`, webhook.secretEnv)
753 checkEvents(`${base}.events`, webhook.events)
754 })
755}
756
691757/**
692758 * Render validation issues as a single multi-line, human-readable block.
693759 *
src/pr/pr-generator.tsmodified+6-0
@@ -158,6 +158,12 @@ export class PullRequestGenerator {
158158 labels.push(update.name)
159159 }
160160
161 // The docs have promised `bulk-update` on PRs carrying five or more
162 // updates since before anything applied it; a filter watching for the
163 // label saw grouped PRs slip past unlabelled.
164 if (group.updates.length >= 5)
165 labels.push('bulk-update')
166
161167 return labels
162168 }
163169
src/templates/github-actions.tsmodified+25-12
@@ -12,6 +12,7 @@ export interface WorkflowConfig {
1212 conditions?: string[]
1313 }
1414 reviewers?: string[]
15 assignees?: string[]
1516 labels?: string[]
1617}
1718
@@ -65,6 +66,18 @@ export class GitHubActionsTemplate {
6566 * Generate GitHub Actions workflow
6667 */
6768 static generateWorkflow(config: WorkflowConfig): string {
69 // Reviewers, assignees and labels travel as flags. The workflow runs
70 // `./buddy update` against whatever buddy.config.ts the checkout carries,
71 // so a per-workflow override can only reach it through the command line —
72 // these fields were accepted and silently dropped before this.
73 const list = (values?: string[]): string =>
74 (values ?? []).map(value => value.replace(/'/g, '')).filter(Boolean).join(',')
75 const prFlags = [
76 list(config.reviewers) ? ` --reviewers '${list(config.reviewers)}'` : '',
77 list(config.assignees) ? ` --assignees '${list(config.assignees)}'` : '',
78 list(config.labels) ? ` --labels '${list(config.labels)}'` : '',
79 ].join('')
80
6881 const workflow = `name: ${config.name}
6982
7083on:
@@ -129,9 +142,9 @@ jobs:
129142 DRY_RUN="\$"
130143
131144 if [ "\$DRY_RUN" = "true" ]; then
132 ./buddy update --strategy "\$STRATEGY" --dry-run --verbose
145 ./buddy update --strategy "\$STRATEGY" --dry-run --verbose${prFlags}
133146 else
134 ./buddy update --strategy "\$STRATEGY" --verbose
147 ./buddy update --strategy "\$STRATEGY" --verbose${prFlags}
135148 fi
136149
137150 - name: Auto-merge eligible PRs
@@ -148,9 +161,10 @@ jobs:
148161 * Generate workflow for different scheduling strategies
149162 */
150163 static generateScheduledWorkflows(config?: BuddyConfig): Record<string, string> {
164 // Global reviewers and labels are deliberately not baked in: the runtime
165 // reads `pullRequest` from buddy.config.ts on every run, and freezing
166 // today's values into the YAML would shadow every later config edit.
151167 const autoMergeConfig = config?.pullRequest?.autoMerge
152 const reviewers = config?.pullRequest?.reviewers || []
153 const labels = config?.pullRequest?.labels || []
154168
155169 // Determine auto-merge settings based on configuration
156170 const dailyAutoMerge = autoMergeConfig?.enabled && autoMergeConfig.conditions?.includes('patch-only')
@@ -169,8 +183,6 @@ jobs:
169183 schedule: config?.schedule?.cron || '0 2 * * *', // 2 AM daily
170184 strategy: 'patch',
171185 autoMerge: dailyAutoMerge,
172 reviewers,
173 labels,
174186 }),
175187
176188 'dependency-updates-weekly.yml': this.generateWorkflow({
@@ -178,8 +190,6 @@ jobs:
178190 schedule: '0 2 * * 1', // 2 AM Monday
179191 strategy: 'minor',
180192 autoMerge: weeklyAutoMerge,
181 reviewers,
182 labels,
183193 }),
184194
185195 'dependency-updates-monthly.yml': this.generateWorkflow({
@@ -187,8 +197,6 @@ jobs:
187197 schedule: '0 2 1 * *', // 2 AM first of month
188198 strategy: 'major',
189199 autoMerge: monthlyAutoMerge,
190 reviewers,
191 labels,
192200 }),
193201 }
194202 }
@@ -602,6 +610,7 @@ jobs:
602610 strategy?: 'major' | 'minor' | 'patch' | 'all'
603611 autoMerge?: boolean | { enabled: boolean, strategy: 'merge' | 'squash' | 'rebase', conditions?: string[] }
604612 reviewers?: string[]
613 assignees?: string[]
605614 labels?: string[]
606615 },
607616 config?: BuddyConfig,
@@ -632,8 +641,12 @@ jobs:
632641 schedule: customConfig.schedule,
633642 strategy: customConfig.strategy || 'all',
634643 autoMerge: autoMergeConfig,
635 reviewers: customConfig.reviewers || config?.pullRequest?.reviewers || [],
636 labels: customConfig.labels || config?.pullRequest?.labels || [],
644 // Only the per-workflow overrides are baked in; global `pullRequest`
645 // settings are read from config at runtime, and freezing them into the
646 // YAML would shadow every later edit.
647 reviewers: customConfig.reviewers ?? [],
648 assignees: customConfig.assignees ?? [],
649 labels: customConfig.labels ?? [],
637650 })
638651 }
639652
test/cli-surface.test.tsmodified+28-1
@@ -1,3 +1,6 @@
1import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'
2import { tmpdir } from 'node:os'
3import { join } from 'node:path'
14import { describe, expect, it } from 'bun:test'
25
36/**
@@ -13,13 +16,15 @@ const CLI = new URL('../bin/cli.ts', import.meta.url).pathname
1316 * Run the CLI and capture what a user would see.
1417 *
1518 * @param args - Arguments to pass
19 * @param cwd - Directory to run in (default: this repository)
1620 * @returns Exit code and combined output
1721 */
18async function run(args: string[]): Promise<{ code: number, output: string }> {
22async function run(args: string[], cwd?: string): Promise<{ code: number, output: string }> {
1923 const proc = Bun.spawn(['bun', CLI, ...args], {
2024 stdout: 'pipe',
2125 stderr: 'pipe',
2226 env: { ...process.env, APP_ENV: 'test' },
27 ...(cwd ? { cwd } : {}),
2328 })
2429
2530 const [stdout, stderr] = await Promise.all([
@@ -66,6 +71,28 @@ describe('CLI surface', () => {
6671 expect(output).not.toContain('Opening GitHub Actions settings')
6772 }, 30000)
6873
74 it('success case - generate-workflows declines when workflows.enabled is false', async () => {
75 // `workflows.enabled` was declared, documented and read by nothing — the
76 // command generated the full set of workflow files regardless.
77 const dir = mkdtempSync(join(tmpdir(), 'buddy-workflows-'))
78 writeFileSync(join(dir, 'buddy.config.ts'), 'export default { workflows: { enabled: false } }\n')
79
80 try {
81 const { code, output } = await run(['generate-workflows', '--verbose'], dir)
82
83 expect(code).toBe(0)
84 expect(output).toContain('disabled by config')
85 // Bun's native glob rather than node:fs — another suite's fs module
86 // mock leaks into this file under CI's run order, and existsSync came
87 // back undefined there.
88 const entries = Array.from(new Bun.Glob('**/*').scanSync({ cwd: dir, onlyFiles: false }))
89 expect(entries).toEqual(['buddy.config.ts'])
90 }
91 finally {
92 rmSync(dir, { recursive: true, force: true })
93 }
94 }, 30000)
95
6996 it('failure case - an unknown flag is rejected rather than ignored', async () => {
7097 // clapp hard-errors on unknown options, which is why a documented-but-
7198 // unimplemented flag crashed rather than degrading.
test/config-validation.test.tsmodified+40-0
@@ -192,6 +192,46 @@ describe('config-validation', () => {
192192 })
193193 })
194194
195 describe('notifications', () => {
196 it('success case - a valid notifications block passes', () => {
197 const issues = validateConfig({
198 notifications: {
199 slack: { webhookEnv: 'SLACK_WEBHOOK_URL', events: ['pr.created', 'run.failed'] },
200 webhooks: [{ url: 'https://example.com/hook', secretEnv: 'HOOK_SECRET', events: ['scan.completed'] }],
201 },
202 })
203
204 expect(issues).toHaveLength(0)
205 })
206
207 it('failure case - a mistyped event name is reported, not silently dropped', () => {
208 // `events` naming an event that does not exist used to build the sink
209 // and then filter every real event out — notifications configured,
210 // credential read, nothing ever delivered.
211 const issues = validateConfig({
212 notifications: { slack: { events: ['pr.opened'] } },
213 })
214
215 expect(issues.some(issue => issue.path === 'notifications.slack.events[0]')).toBe(true)
216 })
217
218 it('failure case - a webhook without a url is reported', () => {
219 const issues = validateConfig({
220 notifications: { webhooks: [{ events: ['pr.created'] }] },
221 } as any)
222
223 expect(issues.some(issue => issue.path === 'notifications.webhooks[0].url')).toBe(true)
224 })
225
226 it('edge case - webhook event filters are validated too', () => {
227 const issues = validateConfig({
228 notifications: { webhooks: [{ url: 'https://example.com/hook', events: ['nope'] }] },
229 })
230
231 expect(issues.some(issue => issue.path === 'notifications.webhooks[0].events[0]')).toBe(true)
232 })
233 })
234
195235 describe('formatConfigIssues', () => {
196236 it('should render one line per issue', () => {
197237 const formatted = formatConfigIssues([
test/custom-workflows.test.tsadded+88-0
@@ -0,0 +1,88 @@
1import type { BuddyConfig } from '../src/types'
2import { describe, expect, it } from 'bun:test'
3import { GitHubActionsTemplate } from '../src/templates/github-actions'
4
5/**
6 * `workflows.custom[].assignees` was declared, validated and documented, and
7 * `generateCustomWorkflow` did not even accept the field; `reviewers` and
8 * `labels` were accepted and then dropped by `generateWorkflow`. The workflow
9 * runs `./buddy update` against the repository's own buddy.config.ts, so a
10 * per-workflow override can only travel as a command-line flag — these assert
11 * it does.
12 */
13describe('custom workflow personalisation', () => {
14 it('success case - reviewers, assignees and labels become update flags', () => {
15 const yaml = GitHubActionsTemplate.generateCustomWorkflow({
16 name: 'Team Updates',
17 schedule: '0 4 * * *',
18 strategy: 'patch',
19 reviewers: ['alice', 'bob'],
20 assignees: ['carol'],
21 labels: ['deps', 'automated'],
22 })
23
24 expect(yaml).toContain(`--reviewers 'alice,bob'`)
25 expect(yaml).toContain(`--assignees 'carol'`)
26 expect(yaml).toContain(`--labels 'deps,automated'`)
27 })
28
29 it('success case - both the dry-run and live invocations carry the flags', () => {
30 const yaml = GitHubActionsTemplate.generateCustomWorkflow({
31 name: 'Team Updates',
32 schedule: '0 4 * * *',
33 assignees: ['carol'],
34 })
35
36 // `./buddy update-check` also matches a bare 'update' substring.
37 const invocations = yaml.split('\n').filter(line => line.includes('./buddy update --'))
38 expect(invocations).toHaveLength(2)
39 for (const line of invocations)
40 expect(line).toContain(`--assignees 'carol'`)
41 })
42
43 it('edge case - no overrides means no flags', () => {
44 const yaml = GitHubActionsTemplate.generateCustomWorkflow({
45 name: 'Plain',
46 schedule: '0 4 * * *',
47 })
48
49 expect(yaml).not.toContain('--reviewers')
50 expect(yaml).not.toContain('--assignees')
51 expect(yaml).not.toContain('--labels')
52 })
53
54 it('edge case - a quote in a name cannot escape the shell argument', () => {
55 const yaml = GitHubActionsTemplate.generateCustomWorkflow({
56 name: 'Hostile',
57 schedule: '0 4 * * *',
58 labels: [`a'; rm -rf /; '`],
59 })
60
61 const flag = /--labels '([^']*)'/.exec(yaml)
62 expect(flag).not.toBeNull()
63 expect(flag?.[1]).not.toContain(`'`)
64 })
65
66 it('failure case - global pullRequest settings are not frozen into the YAML', () => {
67 // The runtime reads `pullRequest` from buddy.config.ts on every run;
68 // baking today's values into the workflow file would shadow every later
69 // config edit for as long as the file lives.
70 const config: BuddyConfig = {
71 pullRequest: { reviewers: ['global-reviewer'], labels: ['global-label'] },
72 }
73
74 const custom = GitHubActionsTemplate.generateCustomWorkflow(
75 { name: 'Plain', schedule: '0 4 * * *' },
76 config,
77 )
78 expect(custom).not.toContain('global-reviewer')
79 expect(custom).not.toContain('global-label')
80
81 const scheduled = GitHubActionsTemplate.generateScheduledWorkflows(config)
82 expect(Object.keys(scheduled)).not.toHaveLength(0)
83 for (const yaml of Object.values(scheduled)) {
84 expect(yaml).not.toContain('global-reviewer')
85 expect(yaml).not.toContain('global-label')
86 }
87 })
88})
test/pr-labels.test.tsmodified+36-0
@@ -124,3 +124,39 @@ describe('pull request labels', () => {
124124 expect(labels).not.toContain('should-not-appear')
125125 })
126126})
127
128/**
129 * The docs promise `bulk-update` on PRs carrying five or more updates under
130 * "Dynamic Labeling"; the promise predates any code applying it, so a filter
131 * watching for the label saw every grouped PR slip past unlabelled.
132 */
133describe('bulk-update label', () => {
134 const updateFor = (name: string): PackageUpdate => ({
135 name,
136 currentVersion: '1.0.0',
137 newVersion: '1.0.1',
138 updateType: 'patch',
139 dependencyType: 'dependencies',
140 file: 'package.json',
141 })
142
143 const groupOf = (count: number): UpdateGroup => ({
144 name: 'Patch Updates',
145 updateType: 'patch',
146 title: 'chore(deps): update all non-major dependencies',
147 body: '',
148 updates: Array.from({ length: count }, (_, index) => updateFor(`package-${index}`)),
149 })
150
151 const generator = new PullRequestGenerator({
152 repository: { provider: 'github', owner: 'stacksjs', name: 'buddy' },
153 })
154
155 it('success case - five updates are labelled bulk-update', () => {
156 expect(generator.generateLabels(groupOf(5))).toContain('bulk-update')
157 })
158
159 it('edge case - four updates are not a bulk update', () => {
160 expect(generator.generateLabels(groupOf(4))).not.toContain('bulk-update')
161 })
162})