ReviewOS
Reload

also looking at this

stacks/buddy

fix(setup): make custom plugins able to run at all

#1416
Merged glennmichael123 wants to merge fix/json-plugins into main
5 files +401 -31

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.

CLAUDE.mdmodified+18-12
@@ -1187,30 +1187,36 @@ export JIRA_PROJECT_KEY="BUDDY" # Optional, defaults to BUDDY
11871187Create custom integrations by defining plugins in `.buddy/plugins/`:
11881188
11891189```jsonc
1190// .buddy/plugins/custom-integration.json
1190// .buddy/plugins/custom-notify.json
11911191{
1192 "name": "custom-integration",
1192 "name": "custom-notify",
11931193 "version": "1.0.0",
11941194 "enabled": true,
11951195 "triggers": [
1196 { "event": "setup*complete" },
1197 { "event": "validation*error" }
1196 { "event": "setup_complete" },
1197 { "event": "validation_error" }
11981198 ],
11991199 "hooks": [
12001200 {
1201 "name": "custom-notification",
1201 "name": "notify",
12021202 "priority": 10,
1203 "async": true,
1204 "handler": "// Custom JavaScript function"
1203 "action": {
1204 "type": "webhook",
1205 "url": "https://your-webhook.com/notify",
1206 "body": { "channel": "#builds" }
1207 }
12051208 }
1206 ],
1207 "configuration": {
1208 "webhook*url": "https://your-custom-webhook.com/notify",
1209 "api*key": "your-api-key"
1210 }
1209 ]
12111210}
12121211```
12131212
1213A hook declares an `action` rather than a function. JSON has no way to carry
1214one, and reading a string out of a config file to run as code would turn any
1215`.buddy/plugins/*.json` in a repository into arbitrary code execution on
1216whoever runs `buddy setup`. The payload names the event and the repository
1217and nothing else, plus whatever `body` adds — setup holds a token by the time
1218hooks run, and the plugin file chooses the URL.
1219
12141220### Plugin Events
12151221
12161222| Event | Description | Context |
README.mdmodified+16-10
@@ -438,9 +438,9 @@ export JIRA_PROJECT_KEY="BUDDY" # Optional, defaults to BUDDY renamed throughout
438438Create custom integrations by defining plugins in `.buddy/plugins/`:
439439
440440```jsonc
441// .buddy/plugins/custom-integration.json
441// .buddy/plugins/custom-notify.json
442442{
443 "name": "custom-integration",
443 "name": "custom-notify",
444444 "version": "1.0.0",
445445 "enabled": true,
446446 "triggers": [
@@ -449,19 +449,25 @@ Create custom integrations by defining plugins in `.buddy/plugins/`:
449449 ],
450450 "hooks": [
451451 {
452 "name": "custom-notification",
452 "name": "notify",
453453 "priority": 10,
454 "async": true,
455 "handler": "// Custom JavaScript function"
454 "action": {
455 "type": "webhook",
456 "url": "https://your-webhook.com/notify",
457 "body": { "channel": "#builds" }
458 }
456459 }
457 ],
458 "configuration": {
459 "webhook_url": "https://your-custom-webhook.com/notify",
460 "api_key": "your-api-key"
461 }
460 ]
462461}
463462```
464463
464A hook declares an `action` rather than a function. JSON has no way to carry
465one, and reading a string out of a config file to run as code would turn any
466`.buddy/plugins/*.json` in a repository into arbitrary code execution on
467whoever runs `buddy setup`. The payload names the event and the repository
468and nothing else, plus whatever `body` adds — setup holds a token by the time
469hooks run, and the plugin file chooses the URL.
470
465471### Plugin Events
466472
467473| Event | Description | Context |
docs/guide/configuration.mdmodified+18-3
@@ -315,12 +315,27 @@ Create custom integrations in `.buddy/plugins/`:
315315 { "event": "setup_complete" },
316316 { "event": "validation_error" }
317317 ],
318 "configuration": {
319 "webhook_url": "https://your-webhook.com/notify"
320 }
318 "hooks": [
319 {
320 "name": "notify",
321 "priority": 10,
322 "action": {
323 "type": "webhook",
324 "url": "https://your-webhook.com/notify",
325 "body": { "channel": "#builds" }
326 }
327 }
328 ]
321329}
322330```
323331
332A hook declares an `action` rather than a function. JSON has no way to carry
333one, and reading a string out of a config file to run as code would turn any
334`.buddy/plugins/*.json` in a repository into arbitrary code execution on
335whoever runs `buddy setup`. The payload names the event and the repository
336and nothing else, plus whatever `body` adds — setup holds a token by the time
337hooks run, and the plugin file chooses the URL.
338
324339## Environment Configuration
325340
326341Override settings via environment:
src/setup.tsmodified+143-6
@@ -112,7 +112,33 @@ export interface SetupHook {
112112 name: string
113113 priority: number
114114 async: boolean
115 handler: (context: SetupContext) => Promise<void> | void
115 /**
116 * What to run, for a plugin defined in TypeScript.
117 *
118 * Optional because a plugin loaded from `.buddy/plugins/*.json` cannot have
119 * one: JSON has no way to carry a function, so a file declaring `handler`
120 * produced a string, and calling it threw a `TypeError` that the hook runner
121 * caught and logged as an ordinary hook failure. Those plugins declare an
122 * `action` instead.
123 */
124 handler?: (context: SetupContext) => Promise<void> | void
125 /** What to do, for a plugin declared as JSON */
126 action?: SetupHookAction
127}
128
129/**
130 * What a JSON-declared hook does when it fires.
131 *
132 * Declarative rather than executable on purpose. Reading a string out of a
133 * config file and running it as code would make any `.buddy/plugins/*.json` in
134 * a repository into arbitrary code execution on whoever runs `buddy setup`.
135 */
136export interface SetupHookAction {
137 type: 'webhook'
138 /** Where to POST the notification */
139 url: string
140 /** Extra fields merged into the payload */
141 body?: Record<string, unknown>
116142}
117143
118144export interface PluginConfig {
@@ -423,6 +449,104 @@ export class ConfigurationMigrator {
423449}
424450
425451// Plugin Architecture Implementation
452/**
453 * Perform a hook's declared action.
454 *
455 * The payload names the event and the repository and nothing else. Setup holds
456 * a token by the time hooks run, and a plugin file committed to a repository
457 * decides the URL — so what goes to it is a fixed shape rather than whatever
458 * happens to be on the context.
459 *
460 * @param action - The action the hook declared
461 * @param context - Setup state the payload is drawn from
462 * @throws {Error} When the action type is unknown or the endpoint refuses it
463 */
464async function runHookAction(action: SetupHookAction, context: SetupContext): Promise<void> {
465 if (action.type !== 'webhook')
466 throw new Error(`Unknown hook action type: ${String(action.type)}`)
467
468 const response = await fetchWithTimeout(action.url, {
469 method: 'POST',
470 headers: { 'Content-Type': 'application/json' },
471 body: JSON.stringify({
472 event: context.step,
473 repository: `${context.repository.owner}/${context.repository.name}`,
474 ...action.body,
475 }),
476 })
477
478 if (!response.ok)
479 throw new Error(`Webhook returned ${response.status} ${response.statusText}`)
480}
481
482/**
483 * Read a plugin definition out of parsed JSON, or explain why it cannot be.
484 *
485 * The loader used to push whatever `JSON.parse` returned straight onto the
486 * plugin list, so a malformed file surfaced later as a `TypeError` inside the
487 * hook runner — at which point the message named neither the file nor what was
488 * wrong with it.
489 *
490 * @param raw - Parsed JSON from a plugin file
491 * @param file - File name, for the message
492 * @returns The plugin, or `null` with the reason logged
493 */
494function parseCustomPlugin(raw: unknown, file: string): SetupPlugin | null {
495 const reject = (reason: string): null => {
496 getDefaultLogger().warn(`⚠️ Ignoring plugin ${file}: ${reason}`)
497 return null
498 }
499
500 if (!raw || typeof raw !== 'object')
501 return reject('it is not a JSON object')
502
503 const candidate = raw as Record<string, unknown>
504 if (typeof candidate.name !== 'string' || !candidate.name)
505 return reject('it has no name')
506
507 if (!Array.isArray(candidate.triggers) || candidate.triggers.length === 0)
508 return reject('it declares no triggers')
509
510 if (!Array.isArray(candidate.hooks) || candidate.hooks.length === 0)
511 return reject('it declares no hooks')
512
513 const hooks: SetupHook[] = []
514 for (const entry of candidate.hooks as Array<Record<string, unknown>>) {
515 if (!entry || typeof entry.name !== 'string')
516 return reject('a hook has no name')
517
518 // The documented example carried `"handler": "// Custom JavaScript
519 // function"`, which could never run. Saying what to write instead beats
520 // failing later with `hook.handler is not a function`.
521 if (typeof entry.handler === 'string') {
522 return reject(
523 `hook "${entry.name}" declares a handler as text. JSON cannot carry a function — `
524 + 'use "action": { "type": "webhook", "url": "..." } instead.',
525 )
526 }
527
528 const action = entry.action as SetupHookAction | undefined
529 if (!action || action.type !== 'webhook' || typeof action.url !== 'string' || !action.url)
530 return reject(`hook "${entry.name}" needs an action with a type of "webhook" and a url`)
531
532 hooks.push({
533 name: entry.name,
534 priority: typeof entry.priority === 'number' ? entry.priority : 0,
535 async: entry.async !== false,
536 action,
537 })
538 }
539
540 return {
541 name: candidate.name,
542 version: typeof candidate.version === 'string' ? candidate.version : '0.0.0',
543 triggers: candidate.triggers as SetupTrigger[],
544 hooks,
545 configuration: (candidate.configuration ?? {}) as PluginConfig,
546 enabled: candidate.enabled !== false,
547 }
548}
549
426550export class PluginManager {
427551 private plugins: SetupPlugin[] = []
428552 private context: SetupContext | null = null
@@ -472,12 +596,20 @@ export class PluginManager {
472596
473597 for (const hook of sortedHooks) {
474598 try {
475 if (hook.async) {
476 await hook.handler(this.context)
599 if (typeof hook.handler === 'function') {
600 if (hook.async)
601 await hook.handler(this.context)
602 else
603 hook.handler(this.context)
604 }
605 else if (hook.action) {
606 await runHookAction(hook.action, this.context)
477607 }
478608 else {
479 hook.handler(this.context)
609 getDefaultLogger().warn(`⚠️ Hook ${hook.name} declares neither a handler nor an action; skipping`)
610 continue
480611 }
612
481613 getDefaultLogger().info(`✅ Executed hook: ${hook.name}`)
482614 }
483615 catch (error) {
@@ -592,8 +724,13 @@ export class PluginManager {
592724
593725 for (const file of pluginFiles) {
594726 try {
595 const pluginConfig = JSON.parse(fs.readFileSync(path.join('.buddy/plugins/', file), 'utf8'))
596 plugins.push(pluginConfig)
727 const parsed = parseCustomPlugin(
728 JSON.parse(fs.readFileSync(path.join('.buddy/plugins/', file), 'utf8')),
729 file,
730 )
731
732 if (parsed)
733 plugins.push(parsed)
597734 }
598735 catch (error) {
599736 getDefaultLogger().info(`⚠️ Failed to load plugin ${file}: ${error instanceof Error ? error.message : 'Unknown error'}`)
test/setup-plugins.test.tsadded+206-0
@@ -0,0 +1,206 @@
1import type { SetupContext } from '../src/setup'
2import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
3import { promises as fs } from 'node:fs'
4import { tmpdir } from 'node:os'
5import { join } from 'node:path'
6import { PluginManager } from '../src/setup'
7
8const realFetch = globalThis.fetch
9
10interface Posted {
11 url: string
12 body: unknown
13}
14
15/** Record every webhook POST and answer it. */
16function captureFetch(status = 200): Posted[] {
17 const posted: Posted[] = []
18
19 globalThis.fetch = (async (input: string, init?: RequestInit) => {
20 posted.push({ url: String(input), body: JSON.parse(String(init?.body ?? '{}')) })
21 return new Response('{}', { status })
22 }) as unknown as typeof fetch
23
24 return posted
25}
26
27function context(): SetupContext {
28 return {
29 step: 'setup_complete',
30 progress: {} as SetupContext['progress'],
31 // Setup holds a token by the time hooks run. Nothing here may reach a
32 // webhook whose URL a committed file chose.
33 config: { repository: { token: 'ghp_secret_value_here' } },
34 repository: { owner: 'acme', name: 'widgets' } as SetupContext['repository'],
35 analysis: {} as SetupContext['analysis'],
36 plugins: [],
37 }
38}
39
40/**
41 * `loadCustomPlugins` pushed whatever `JSON.parse` returned onto the plugin
42 * list, and `SetupHook.handler` is a function. JSON cannot carry one, so every
43 * documented custom plugin loaded, announced itself, and then failed on each
44 * hook with `hook.handler is not a function` — caught and logged as an
45 * ordinary hook failure.
46 */
47describe('custom plugins from .buddy/plugins', () => {
48 let testDir: string
49 let originalCwd: string
50
51 beforeEach(async () => {
52 testDir = await fs.mkdtemp(join(tmpdir(), 'buddy-plugins-'))
53 originalCwd = process.cwd()
54 process.chdir(testDir)
55 await fs.mkdir('.buddy/plugins', { recursive: true })
56 })
57
58 afterEach(async () => {
59 globalThis.fetch = realFetch
60 process.chdir(originalCwd)
61 await fs.rm(testDir, { recursive: true, force: true })
62 })
63
64 /** Write a plugin file and return the plugins discovered from it. */
65 async function discover(plugin: unknown, name = 'custom.json') {
66 await fs.writeFile(join('.buddy/plugins', name), JSON.stringify(plugin))
67 const plugins = await new PluginManager().discoverPlugins()
68 return plugins.filter(candidate => candidate.name === 'custom-integration')
69 }
70
71 const valid = {
72 name: 'custom-integration',
73 version: '1.0.0',
74 enabled: true,
75 triggers: [{ event: 'setup_complete' }],
76 hooks: [{
77 name: 'custom-notification',
78 priority: 10,
79 async: true,
80 action: { type: 'webhook', url: 'https://example.test/notify' },
81 }],
82 configuration: { api_key: 'k' },
83 }
84
85 describe('loading', () => {
86 it('success case - a declarative plugin loads', async () => {
87 const [plugin] = await discover(valid)
88
89 expect(plugin.version).toBe('1.0.0')
90 expect(plugin.hooks[0].action?.url).toBe('https://example.test/notify')
91 })
92
93 it('failure case - a handler written as text is refused with the fix', async () => {
94 // This is exactly what the README documented:
95 // `"handler": "// Custom JavaScript function"`.
96 const withHandler = {
97 ...valid,
98 hooks: [{ name: 'custom-notification', priority: 10, async: true, handler: '// Custom JavaScript function' }],
99 }
100
101 expect(await discover(withHandler)).toHaveLength(0)
102 })
103
104 it('failure case - a hook with no action is refused at load', async () => {
105 const noAction = { ...valid, hooks: [{ name: 'h', priority: 1, async: true }] }
106
107 expect(await discover(noAction)).toHaveLength(0)
108 })
109
110 it('failure case - a plugin with no triggers is refused', async () => {
111 expect(await discover({ ...valid, triggers: [] })).toHaveLength(0)
112 })
113
114 it('failure case - a plugin with no name is refused', async () => {
115 const { name, ...rest } = valid
116 expect(await discover(rest)).toHaveLength(0)
117 expect(name).toBe('custom-integration')
118 })
119
120 it('edge case - a file that is not an object is refused', async () => {
121 await fs.writeFile('.buddy/plugins/bad.json', '"just a string"')
122
123 expect(await new PluginManager().discoverPlugins()).toHaveLength(0)
124 })
125
126 it('edge case - unparseable JSON does not stop the other plugins', async () => {
127 await fs.writeFile('.buddy/plugins/broken.json', '{ not json')
128
129 expect(await discover(valid)).toHaveLength(1)
130 })
131 })
132
133 describe('running', () => {
134 /** A manager with one loaded plugin and a context set. */
135 async function loaded(plugin: unknown = valid) {
136 await fs.writeFile('.buddy/plugins/custom.json', JSON.stringify(plugin))
137 const manager = new PluginManager()
138
139 for (const discovered of await manager.discoverPlugins())
140 await manager.loadPlugin(discovered)
141
142 manager.setContext(context())
143 return manager
144 }
145
146 it('success case - the declared webhook is called', async () => {
147 const posted = captureFetch()
148 const manager = await loaded()
149
150 await manager.executePluginHooks({ event: 'setup_complete' })
151
152 expect(posted).toHaveLength(1)
153 expect(posted[0].url).toBe('https://example.test/notify')
154 })
155
156 it('success case - the payload names the event and repository', async () => {
157 const posted = captureFetch()
158 await (await loaded()).executePluginHooks({ event: 'setup_complete' })
159
160 expect(posted[0].body).toMatchObject({
161 event: 'setup_complete',
162 repository: 'acme/widgets',
163 })
164 })
165
166 it('failure case - the payload carries no credentials', async () => {
167 // A plugin file committed to a repository chooses the URL, so what is
168 // sent to it is a fixed shape rather than whatever is on the context.
169 const posted = captureFetch()
170 await (await loaded()).executePluginHooks({ event: 'setup_complete' })
171
172 expect(JSON.stringify(posted[0].body)).not.toContain('ghp_secret_value_here')
173 })
174
175 it('success case - extra body fields are merged in', async () => {
176 const posted = captureFetch()
177 const withBody = {
178 ...valid,
179 hooks: [{
180 name: 'h',
181 priority: 1,
182 async: true,
183 action: { type: 'webhook', url: 'https://example.test/notify', body: { channel: '#builds' } },
184 }],
185 }
186
187 await (await loaded(withBody)).executePluginHooks({ event: 'setup_complete' })
188
189 expect(posted[0].body).toMatchObject({ channel: '#builds' })
190 })
191
192 it('failure case - a refused webhook does not abort setup', async () => {
193 captureFetch(500)
194 const manager = await loaded()
195
196 await expect(manager.executePluginHooks({ event: 'setup_complete' })).resolves.toBeUndefined()
197 })
198
199 it('edge case - a trigger the plugin did not declare runs nothing', async () => {
200 const posted = captureFetch()
201 await (await loaded()).executePluginHooks({ event: 'pre_setup' })
202
203 expect(posted).toHaveLength(0)
204 })
205 })
206})