also looking at this
fix(pr): apply the security label so security-only can fire
#1409
4 files
+87
-60
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.
| @@ -34,7 +34,7 @@ A PR qualifies when **any** listed condition accepts it. | ||
| 34 | 34 | | `security-only` | The PR carries the configured `security.label` (default `security`) | |
| 35 | 35 | | `all` | Any Buddy PR, including major upgrades | |
| 36 | 36 | |
| 37 | `security-only` reads a label, and Buddy does not apply that label on its own — a PR gets it because you put it there, either through `pullRequest.labels` or a package rule. Configure `security-only` without arranging for the label and nothing will ever match it. | |
| 37 | `security-only` reads a label, and Buddy applies it to any pull request whose updates resolve a published advisory — matched against OSV.dev, not against package names that merely sound security-related. Rename it with `security.label`. You can also add that label by hand or through a package rule, and the condition treats it the same way. | |
| 38 | 38 | |
| 39 | 39 | Unknown condition names are rejected by config validation rather than ignored, so a typo like `patch_only` fails loudly instead of widening what merges unreviewed. |
| 40 | 40 | |
| @@ -366,9 +366,14 @@ export class Buddy { | ||
| 366 | 366 | /** |
| 367 | 367 | * Every label a pull request for this group should carry. |
| 368 | 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. | |
| 369 | * Four sources union here: the automatic set derived from the update types | |
| 370 | * and manifests in the group, `security.label` when the group resolves a | |
| 371 | * published advisory, whatever `pullRequest.labels` declares, and the labels | |
| 372 | * any matching rule asked for. | |
| 373 | * | |
| 374 | * The security label matters beyond decoration: the `security-only` | |
| 375 | * auto-merge condition tests for it, so a group that resolves an advisory | |
| 376 | * could never satisfy that condition while nothing applied the label. | |
| 372 | 377 | * |
| 373 | 378 | * Centralised because the five call sites had drifted — only the creation |
| 374 | 379 | * path folded in rule labels, so refreshing an existing pull request quietly |
| @@ -380,8 +385,15 @@ export class Buddy { | ||
| 380 | 385 | * @returns Labels to set on the pull request |
| 381 | 386 | */ |
| 382 | 387 | private labelsFor(group: UpdateGroup, prGenerator: PullRequestGenerator): string[] { |
| 388 | // Label groups that actually resolve a published advisory. This once | |
| 389 | // matched a hardcoded list of security-adjacent package names, which both | |
| 390 | // missed every real vulnerability outside that list and mislabelled | |
| 391 | // routine updates to packages that merely sound security-related. | |
| 392 | const resolvesAdvisory = group.updates.some(update => (update.securityAdvisories?.length ?? 0) > 0) | |
| 393 | ||
| 383 | 394 | return [...new Set([ |
| 384 | 395 | ...prGenerator.generateLabels(group), |
| 396 | ...(resolvesAdvisory ? [this.config.security?.label ?? 'security'] : []), | |
| 385 | 397 | ...(this.config.pullRequest?.labels ?? []), |
| 386 | 398 | ...this.groupEffectsFor(group.updates).labels, |
| 387 | 399 | ])] |
| @@ -2219,62 +2231,6 @@ export class Buddy { | ||
| 2219 | 2231 | return existingUpdates.size === newUpdates.length |
| 2220 | 2232 | } |
| 2221 | 2233 | |
| 2222 | /** | |
| 2223 | * Generate dynamic labels for PR based on update types and configuration | |
| 2224 | */ | |
| 2225 | private generatePRLabels(group: UpdateGroup): string[] { | |
| 2226 | const labels = new Set<string>() | |
| 2227 | ||
| 2228 | // Always add dependencies label | |
| 2229 | labels.add('dependencies') | |
| 2230 | ||
| 2231 | // Add update type specific labels | |
| 2232 | const updateTypes = group.updates.map(u => u.updateType) | |
| 2233 | const hasUpdates = { | |
| 2234 | major: updateTypes.includes('major'), | |
| 2235 | minor: updateTypes.includes('minor'), | |
| 2236 | patch: updateTypes.includes('patch'), | |
| 2237 | } | |
| 2238 | ||
| 2239 | // Add specific update type labels | |
| 2240 | if (hasUpdates.major) { | |
| 2241 | labels.add('major') | |
| 2242 | } | |
| 2243 | if (hasUpdates.minor) { | |
| 2244 | labels.add('minor') | |
| 2245 | } | |
| 2246 | if (hasUpdates.patch) { | |
| 2247 | labels.add('patch') | |
| 2248 | } | |
| 2249 | ||
| 2250 | // Add additional contextual labels | |
| 2251 | if (group.updates.length > 5) { | |
| 2252 | labels.add('dependencies') // Use standard dependencies label instead of bulk-update | |
| 2253 | } | |
| 2254 | ||
| 2255 | // Label groups that actually resolve a published advisory. This used to | |
| 2256 | // match a hardcoded list of security-adjacent package names, which both | |
| 2257 | // missed every real vulnerability outside that list and mislabelled | |
| 2258 | // routine updates to packages that merely sound security-related. | |
| 2259 | const resolvesAdvisory = group.updates.some(update => (update.securityAdvisories?.length ?? 0) > 0) | |
| 2260 | if (resolvesAdvisory) { | |
| 2261 | labels.add(this.config.security?.label ?? 'security') | |
| 2262 | } | |
| 2263 | ||
| 2264 | // Add configured labels from config if they exist (but avoid duplicates) | |
| 2265 | if (this.config.pullRequest?.labels) { | |
| 2266 | this.config.pullRequest.labels.forEach((label) => { | |
| 2267 | // Only add if it's not 'dependencies' since we always add that | |
| 2268 | if (label !== 'dependencies') { | |
| 2269 | labels.add(label) | |
| 2270 | } | |
| 2271 | }) | |
| 2272 | } | |
| 2273 | ||
| 2274 | // Convert to array and return | |
| 2275 | return Array.from(labels) | |
| 2276 | } | |
| 2277 | ||
| 2278 | 2234 | /** |
| 2279 | 2235 | * Check if a PR should be auto-closed due to configuration changes |
| 2280 | 2236 | * This handles cases where: |
| @@ -233,3 +233,34 @@ describe('auto-merge evaluation', () => { | ||
| 233 | 233 | }) |
| 234 | 234 | }) |
| 235 | 235 | }) |
| 236 | ||
| 237 | describe('security-only, end to end', () => { | |
| 238 | /** | |
| 239 | * `security-only` tests for `security.label`, and until the label was | |
| 240 | * actually applied to a pull request the condition could never once fire. | |
| 241 | * These pin the contract from both sides. | |
| 242 | */ | |
| 243 | it('failure case - no security label means no match', () => { | |
| 244 | const decision = evaluateAutoMerge(makePR([makeUpdate({ updateType: 'major' })]), makeConfig(['security-only']), true) | |
| 245 | ||
| 246 | expect(decision.eligible).toBe(false) | |
| 247 | }) | |
| 248 | ||
| 249 | it('success case - the label Buddy now applies satisfies the condition', () => { | |
| 250 | const pr = makePR([makeUpdate({ updateType: 'major' })], { labels: ['dependencies', 'security'] }) | |
| 251 | ||
| 252 | const decision = evaluateAutoMerge(pr, makeConfig(['security-only']), true) | |
| 253 | ||
| 254 | // A major update, admitted purely because it resolves an advisory. | |
| 255 | expect(decision.eligible).toBe(true) | |
| 256 | expect(decision.reason).toContain('security advisory') | |
| 257 | }) | |
| 258 | ||
| 259 | it('success case - a renamed label is matched, not the default', () => { | |
| 260 | const config = makeConfig(['security-only']) | |
| 261 | config.security = { label: 'vulnerability' } | |
| 262 | ||
| 263 | expect(evaluateAutoMerge(makePR([makeUpdate()], { labels: ['vulnerability'] }), config, true).eligible).toBe(true) | |
| 264 | expect(evaluateAutoMerge(makePR([makeUpdate()], { labels: ['security'] }), config, true).eligible).toBe(false) | |
| 265 | }) | |
| 266 | }) | |
| @@ -76,6 +76,46 @@ describe('pull request labels', () => { | ||
| 76 | 76 | expect(labels.filter(label => label === 'deps')).toHaveLength(1) |
| 77 | 77 | }) |
| 78 | 78 | |
| 79 | describe('security advisories', () => { | |
| 80 | /** An update that resolves a published advisory. */ | |
| 81 | const vulnerable: PackageUpdate = { | |
| 82 | ...update, | |
| 83 | securityAdvisories: [{ | |
| 84 | id: 'GHSA-xxxx-yyyy-zzzz', | |
| 85 | aliases: ['CVE-2026-0001'], | |
| 86 | severity: 'high', | |
| 87 | summary: 'Prototype pollution', | |
| 88 | url: 'https://github.com/advisories/GHSA-xxxx-yyyy-zzzz', | |
| 89 | }], | |
| 90 | } | |
| 91 | ||
| 92 | /** Resolve labels for a group carrying the advisory. */ | |
| 93 | function labelsForVulnerable(config: BuddyConfig): string[] { | |
| 94 | const buddy = new Buddy(config) | |
| 95 | // @ts-expect-error - accessing private method for testing | |
| 96 | return buddy.labelsFor({ ...group, updates: [vulnerable] }, new PullRequestGenerator(config)) | |
| 97 | } | |
| 98 | ||
| 99 | it('success case - labels a group that resolves an advisory', () => { | |
| 100 | // `security-only` auto-merge tests for this label, so while nothing | |
| 101 | // applied it that condition could never once fire. | |
| 102 | expect(labelsForVulnerable(baseConfig)).toContain('security') | |
| 103 | }) | |
| 104 | ||
| 105 | it('success case - honours a configured label name', () => { | |
| 106 | const labels = labelsForVulnerable({ ...baseConfig, security: { label: 'vulnerability' } }) | |
| 107 | ||
| 108 | expect(labels).toContain('vulnerability') | |
| 109 | expect(labels).not.toContain('security') | |
| 110 | }) | |
| 111 | ||
| 112 | it('failure case - a routine update is not labelled security', () => { | |
| 113 | // The old implementation matched package names that merely sounded | |
| 114 | // security-related, which mislabelled routine updates. | |
| 115 | expect(labelsFor(baseConfig)).not.toContain('security') | |
| 116 | }) | |
| 117 | }) | |
| 118 | ||
| 79 | 119 | it('edge case - a rule that does not match contributes nothing', () => { |
| 80 | 120 | const labels = labelsFor({ |
| 81 | 121 | ...baseConfig, |