also looking at this
fix(rules): consult a rule's autoMerge when deciding to merge
#1422
2 files
+178
-1
| @@ -1,4 +1,5 @@ | ||
| 1 | import type { BuddyConfig, PackageUpdate, PRManifest } from '../types' | |
| 1 | import type { BuddyConfig, PackageUpdate, PRManifest, PRManifestUpdate } from '../types' | |
| 2 | import { groupsToRules, resolveRuleEffects } from '../rules/engine' | |
| 2 | 3 | import { parseManifest } from './pr-manifest' |
| 3 | 4 | |
| 4 | 5 | /** Branch prefix every buddy pull request is opened from. */ |
| @@ -116,9 +117,74 @@ export function evaluateAutoMerge( | ||
| 116 | 117 | if (manifest.truncated) |
| 117 | 118 | return { eligible: false, reason: `PR #${pr.number} has a truncated manifest, so its full update set cannot be verified` } |
| 118 | 119 | |
| 120 | // A rule that names auto-merge explicitly settles it in either direction; | |
| 121 | // the global conditions are the default for everything it does not cover. | |
| 122 | const ruled = evaluateRuleAutoMerge(manifest, config) | |
| 123 | if (ruled) | |
| 124 | return ruled | |
| 125 | ||
| 119 | 126 | return evaluateConditions(manifest, labels, settings) |
| 120 | 127 | } |
| 121 | 128 | |
| 129 | /** | |
| 130 | * Whether a package rule decides auto-merge for this pull request. | |
| 131 | * | |
| 132 | * `autoMerge` on a rule was resolved into `ResolvedEffects` and merged by | |
| 133 | * `mergeGroupEffects`, and nothing ever read either — so a rule saying "these | |
| 134 | * are safe to merge without review" did nothing, and one saying the opposite | |
| 135 | * did nothing either. | |
| 136 | * | |
| 137 | * The all-or-nothing reading is `mergeGroupEffects`'s, not a new invention: a | |
| 138 | * group qualifies only when every update in it is covered. One package no rule | |
| 139 | * has vouched for sends the whole pull request back to the global conditions. | |
| 140 | * | |
| 141 | * @param manifest - The pull request's embedded update manifest | |
| 142 | * @param config - Configuration supplying the rules | |
| 143 | * @returns A decision when a rule settles it, `null` to fall through | |
| 144 | */ | |
| 145 | function evaluateRuleAutoMerge(manifest: PRManifest, config: BuddyConfig): AutoMergeDecision | null { | |
| 146 | const rules = [...groupsToRules(config.packages?.groups), ...(config.packages?.rules ?? [])] | |
| 147 | if (rules.length === 0 || manifest.updates.length === 0) | |
| 148 | return null | |
| 149 | ||
| 150 | // A manifest sheds `type` and `dependencyType` before it sheds rows, and | |
| 151 | // does not mark itself truncated when it does. Rules match on both, so | |
| 152 | // evaluating one against a manifest missing them would decide on data that | |
| 153 | // is not there — fall through to the conditions instead. | |
| 154 | const complete = manifest.updates.every(update => update.type && update.dependencyType) | |
| 155 | if (!complete) | |
| 156 | return null | |
| 157 | ||
| 158 | const effects = manifest.updates.map(update => resolveRuleEffects(toPackageUpdate(update), rules)) | |
| 159 | ||
| 160 | if (effects.some(effect => effect.autoMerge === false)) | |
| 161 | return { eligible: false, reason: 'a package rule holds one of these updates back from auto-merge' } | |
| 162 | ||
| 163 | if (effects.every(effect => effect.autoMerge === true)) | |
| 164 | return { eligible: true, reason: 'every update is covered by a package rule that enables auto-merge' } | |
| 165 | ||
| 166 | return null | |
| 167 | } | |
| 168 | ||
| 169 | /** | |
| 170 | * Rebuild the update a manifest row describes, for rule matching. | |
| 171 | * | |
| 172 | * Only the fields `ruleMatches` reads are reconstructed; the manifest carries | |
| 173 | * nothing else and nothing here needs more. | |
| 174 | * | |
| 175 | * @param row - A manifest row | |
| 176 | */ | |
| 177 | function toPackageUpdate(row: PRManifestUpdate): PackageUpdate { | |
| 178 | return { | |
| 179 | name: row.name, | |
| 180 | currentVersion: row.current, | |
| 181 | newVersion: row.target, | |
| 182 | updateType: row.type, | |
| 183 | file: row.file, | |
| 184 | dependencyType: row.dependencyType, | |
| 185 | } as PackageUpdate | |
| 186 | } | |
| 187 | ||
| 122 | 188 | function evaluateConditions( |
| 123 | 189 | manifest: PRManifest, |
| 124 | 190 | labels: string[], |
| @@ -219,8 +285,13 @@ export function evaluateAutoMergeForUpdates( | ||
| 219 | 285 | target: update.newVersion, |
| 220 | 286 | type: update.updateType, |
| 221 | 287 | file: update.file, |
| 288 | dependencyType: update.dependencyType, | |
| 222 | 289 | })), |
| 223 | 290 | } |
| 224 | 291 | |
| 292 | const ruled = evaluateRuleAutoMerge(manifest, config) | |
| 293 | if (ruled) | |
| 294 | return ruled | |
| 295 | ||
| 225 | 296 | return evaluateConditions(manifest, labels, settings) |
| 226 | 297 | } |
| @@ -264,3 +264,109 @@ describe('security-only, end to end', () => { | ||
| 264 | 264 | expect(evaluateAutoMerge(makePR([makeUpdate()], { labels: ['security'] }), config, true).eligible).toBe(false) |
| 265 | 265 | }) |
| 266 | 266 | }) |
| 267 | ||
| 268 | /** | |
| 269 | * `autoMerge` on a package rule was resolved into `ResolvedEffects` and merged | |
| 270 | * by `mergeGroupEffects`, and nothing ever read either — so a rule saying | |
| 271 | * "these are safe to merge without review" did nothing, and one saying the | |
| 272 | * opposite did nothing either. | |
| 273 | */ | |
| 274 | describe('rule-level auto-merge', () => { | |
| 275 | /** A config whose global conditions would refuse everything. */ | |
| 276 | function withRules(rules: unknown[]): BuddyConfig { | |
| 277 | return { | |
| 278 | pullRequest: { autoMerge: { enabled: true, strategy: 'squash', conditions: ['patch-only'] } }, | |
| 279 | packages: { strategy: 'all', rules }, | |
| 280 | } as unknown as BuddyConfig | |
| 281 | } | |
| 282 | ||
| 283 | it('success case - a rule admits a major the conditions would refuse', () => { | |
| 284 | // `patch-only` alone would send this back. The rule is the point. | |
| 285 | const pr = makePR([makeUpdate({ name: 'react', updateType: 'major', newVersion: '19.0.0' })]) | |
| 286 | const config = withRules([{ matchPackages: ['react'], autoMerge: true }]) | |
| 287 | ||
| 288 | const decision = evaluateAutoMerge(pr, config, true) | |
| 289 | ||
| 290 | expect(decision.eligible).toBe(true) | |
| 291 | expect(decision.reason).toContain('package rule') | |
| 292 | }) | |
| 293 | ||
| 294 | it('failure case - a rule holds back what the conditions would admit', () => { | |
| 295 | // A patch that `patch-only` accepts, refused because a rule says so. | |
| 296 | const pr = makePR([makeUpdate({ name: 'left-pad' })]) | |
| 297 | const config = withRules([{ matchPackages: ['left-pad'], autoMerge: false }]) | |
| 298 | ||
| 299 | const decision = evaluateAutoMerge(pr, config, true) | |
| 300 | ||
| 301 | expect(decision.eligible).toBe(false) | |
| 302 | expect(decision.reason).toContain('holds one of these updates back') | |
| 303 | }) | |
| 304 | ||
| 305 | it('failure case - one uncovered package sends the whole PR back to the conditions', () => { | |
| 306 | // `mergeGroupEffects` reads this all-or-nothing, and so does this: a | |
| 307 | // package no rule has vouched for is enough. | |
| 308 | const pr = makePR([ | |
| 309 | makeUpdate({ name: 'react', updateType: 'major' }), | |
| 310 | makeUpdate({ name: 'lodash', updateType: 'major' }), | |
| 311 | ]) | |
| 312 | const config = withRules([{ matchPackages: ['react'], autoMerge: true }]) | |
| 313 | ||
| 314 | expect(evaluateAutoMerge(pr, config, true).eligible).toBe(false) | |
| 315 | }) | |
| 316 | ||
| 317 | it('success case - a blocking rule wins over an admitting one', () => { | |
| 318 | const pr = makePR([ | |
| 319 | makeUpdate({ name: 'react', updateType: 'patch' }), | |
| 320 | makeUpdate({ name: 'danger', updateType: 'patch' }), | |
| 321 | ]) | |
| 322 | const config = withRules([ | |
| 323 | { matchPackages: ['**'], autoMerge: true }, | |
| 324 | { matchPackages: ['danger'], autoMerge: false }, | |
| 325 | ]) | |
| 326 | ||
| 327 | const decision = evaluateAutoMerge(pr, config, true) | |
| 328 | ||
| 329 | expect(decision.eligible).toBe(false) | |
| 330 | }) | |
| 331 | ||
| 332 | it('success case - rules that say nothing about auto-merge change nothing', () => { | |
| 333 | // A rule setting only a group name must not accidentally decide this. | |
| 334 | const pr = makePR([makeUpdate({ updateType: 'major' })]) | |
| 335 | const config = withRules([{ matchPackages: ['**'], groupName: 'Everything' }]) | |
| 336 | ||
| 337 | const decision = evaluateAutoMerge(pr, config, true) | |
| 338 | ||
| 339 | expect(decision.eligible).toBe(false) | |
| 340 | expect(decision.reason).not.toContain('package rule') | |
| 341 | }) | |
| 342 | ||
| 343 | it('failure case - a rule cannot grant auto-merge on a shed manifest', () => { | |
| 344 | // A large manifest drops `type` and `dependencyType` without marking | |
| 345 | // itself truncated. Rules match on both, so a rule evaluated against one | |
| 346 | // would be deciding on data that is not there. | |
| 347 | const pr = makePR([makeUpdate({ name: 'react', updateType: 'major' })]) | |
| 348 | pr.body = pr.body.replace(/"type":"[a-z]+",/g, '').replace(/"dependencyType":"[^"]+",?/g, '') | |
| 349 | const config = withRules([{ matchPackages: ['react'], autoMerge: true }]) | |
| 350 | ||
| 351 | expect(evaluateAutoMerge(pr, config, true).eligible).toBe(false) | |
| 352 | }) | |
| 353 | ||
| 354 | it('success case - the same rule applies through evaluateAutoMergeForUpdates', () => { | |
| 355 | // The pre-PR path must agree with the post-PR one, or a group would be | |
| 356 | // opened as auto-mergeable and then refused at merge time. | |
| 357 | const decision = evaluateAutoMergeForUpdates( | |
| 358 | [makeUpdate({ name: 'react', updateType: 'major' })], | |
| 359 | [], | |
| 360 | withRules([{ matchPackages: ['react'], autoMerge: true }]), | |
| 361 | ) | |
| 362 | ||
| 363 | expect(decision.eligible).toBe(true) | |
| 364 | }) | |
| 365 | ||
| 366 | it('edge case - no rules at all leaves the conditions in charge', () => { | |
| 367 | const pr = makePR([makeUpdate()]) | |
| 368 | const config = makeConfig(['patch-only']) | |
| 369 | ||
| 370 | expect(evaluateAutoMerge(pr, config, true).eligible).toBe(true) | |
| 371 | }) | |
| 372 | }) | |