also looking at this
fix: let the satisfied-PR closer consult the manifest, and reject phantom paths from PR bodies
#1447
2 files
+134
-11
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.
| @@ -1610,6 +1610,33 @@ export class Buddy { | ||
| 1610 | 1610 | * 'behind' — current is behind the target |
| 1611 | 1611 | * 'failed' — could not compare (non-semver, malformed, etc.) |
| 1612 | 1612 | */ |
| 1613 | /** | |
| 1614 | * Read the version package.json declares for every dependency, keyed by | |
| 1615 | * package name. Range prefixes stay in place — compareVersionsSafe strips | |
| 1616 | * them. Returns an empty map when the manifest is missing or unreadable. | |
| 1617 | */ | |
| 1618 | private readDeclaredNpmVersions(): Map<string, string> { | |
| 1619 | const declared = new Map<string, string>() | |
| 1620 | try { | |
| 1621 | const fs = require('node:fs') | |
| 1622 | const path = require('node:path') | |
| 1623 | const manifestPath = path.join(this.projectPath, 'package.json') | |
| 1624 | if (!fs.existsSync(manifestPath)) | |
| 1625 | return declared | |
| 1626 | const manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf8')) | |
| 1627 | for (const section of ['dependencies', 'devDependencies', 'peerDependencies', 'optionalDependencies']) { | |
| 1628 | for (const [name, version] of Object.entries(manifest[section] ?? {})) { | |
| 1629 | if (typeof version === 'string' && !declared.has(name)) | |
| 1630 | declared.set(name, version) | |
| 1631 | } | |
| 1632 | } | |
| 1633 | } | |
| 1634 | catch (error) { | |
| 1635 | this.logger.debug('Could not read declared versions from package.json:', error) | |
| 1636 | } | |
| 1637 | return declared | |
| 1638 | } | |
| 1639 | ||
| 1613 | 1640 | private compareVersionsSafe(current: string, target: string): 'at-or-beyond' | 'behind' | 'failed' { |
| 1614 | 1641 | try { |
| 1615 | 1642 | const cleanTarget = target.replace(/^[v^~>=<@]+/, '') |
| @@ -2736,6 +2763,17 @@ export class Buddy { | ||
| 2736 | 2763 | * Reads the embedded manifest when present, falling back to scraping the |
| 2737 | 2764 | * rendered tables for PRs opened before manifests existed. |
| 2738 | 2765 | */ |
| 2766 | /** | |
| 2767 | * A regex capture from a PR body counts as a repository path only when it | |
| 2768 | * looks like one — markdown links, badges and URLs (renovate-era bodies are | |
| 2769 | * full of them) must not reach the removed-file check as phantom paths. | |
| 2770 | */ | |
| 2771 | private isPlausibleRepoPath(candidate: string): boolean { | |
| 2772 | if (!candidate || /[\s()[\]!`]|:\/\//.test(candidate)) | |
| 2773 | return false | |
| 2774 | return /^[\w@.-]+(?:\/[\w@.-]+)*$/.test(candidate) | |
| 2775 | } | |
| 2776 | ||
| 2739 | 2777 | private extractFilePathsFromPRBody(prBody: string): string[] { |
| 2740 | 2778 | const manifest = parseManifest(prBody) |
| 2741 | 2779 | if (manifest) |
| @@ -2749,7 +2787,7 @@ export class Buddy { | ||
| 2749 | 2787 | let match |
| 2750 | 2788 | while ((match = tableRowRegex.exec(prBody)) !== null) { |
| 2751 | 2789 | const filePath = match[1].trim() |
| 2752 | if (filePath && !filePaths.includes(filePath)) { | |
| 2790 | if (this.isPlausibleRepoPath(filePath) && !filePaths.includes(filePath)) { | |
| 2753 | 2791 | filePaths.push(filePath) |
| 2754 | 2792 | } |
| 2755 | 2793 | } |
| @@ -2758,7 +2796,7 @@ export class Buddy { | ||
| 2758 | 2796 | const boldFileRegex = /\*\*([^*]+\.(?:json|yaml|yml|lock))\*\*/g |
| 2759 | 2797 | while ((match = boldFileRegex.exec(prBody)) !== null) { |
| 2760 | 2798 | const filePath = match[1].trim() |
| 2761 | if (filePath && !filePaths.includes(filePath)) { | |
| 2799 | if (this.isPlausibleRepoPath(filePath) && !filePaths.includes(filePath)) { | |
| 2762 | 2800 | filePaths.push(filePath) |
| 2763 | 2801 | } |
| 2764 | 2802 | } |
| @@ -2769,7 +2807,7 @@ export class Buddy { | ||
| 2769 | 2807 | while ((match = simpleTableRowRegex.exec(prBody)) !== null) { |
| 2770 | 2808 | const filePath = match[1].trim() |
| 2771 | 2809 | // Only consider paths that look like file paths (contain / or end with common extensions) |
| 2772 | if (filePath && (filePath.includes('/') || /\.(?:json|yaml|yml|lock)$/.test(filePath)) && !filePaths.includes(filePath)) { | |
| 2810 | if (this.isPlausibleRepoPath(filePath) && (filePath.includes('/') || /\.(?:json|yaml|yml|lock)$/.test(filePath)) && !filePaths.includes(filePath)) { | |
| 2773 | 2811 | filePaths.push(filePath) |
| 2774 | 2812 | } |
| 2775 | 2813 | } |
| @@ -2778,7 +2816,7 @@ export class Buddy { | ||
| 2778 | 2816 | const filePathRegex = /(?:^|\s)([\w-]+(?:\/[\w.-]+)*\/[\w.-]+\.(?:json|yaml|yml|lock))(?:\s|$)/gm |
| 2779 | 2817 | while ((match = filePathRegex.exec(prBody)) !== null) { |
| 2780 | 2818 | const filePath = match[1].trim() |
| 2781 | if (filePath && !filePaths.includes(filePath)) { | |
| 2819 | if (this.isPlausibleRepoPath(filePath) && !filePaths.includes(filePath)) { | |
| 2782 | 2820 | filePaths.push(filePath) |
| 2783 | 2821 | } |
| 2784 | 2822 | } |
| @@ -3008,13 +3046,19 @@ export class Buddy { | ||
| 3008 | 3046 | strategy: 'all', |
| 3009 | 3047 | }, |
| 3010 | 3048 | } |
| 3011 | const scanBuddy = new Buddy(scanConfig) | |
| 3049 | const scanBuddy = new Buddy(scanConfig, this.projectPath) | |
| 3012 | 3050 | const currentScanResult = await scanBuddy.scanForUpdates() |
| 3013 | 3051 | const currentUpdatesMap = new Map<string, PackageUpdate>() |
| 3014 | 3052 | for (const update of currentScanResult.updates) { |
| 3015 | 3053 | currentUpdatesMap.set(update.name, update) |
| 3016 | 3054 | } |
| 3017 | 3055 | |
| 3056 | // Positive evidence for packages the scan no longer lists: what the | |
| 3057 | // manifest itself declares. A fully satisfied PR consists entirely of | |
| 3058 | // packages absent from an outdated-scan, so without this the closer | |
| 3059 | // could never close one. | |
| 3060 | const declaredVersions = this.readDeclaredNpmVersions() | |
| 3061 | ||
| 3018 | 3062 | // Safety: if the scan returned 0 updates, something may be wrong |
| 3019 | 3063 | // (bun outdated failure, missing node_modules, rate limiting, etc.). |
| 3020 | 3064 | // Never close PRs based on an empty scan — that would wipe them all out. |
| @@ -3059,10 +3103,23 @@ export class Buddy { | ||
| 3059 | 3103 | // Package not in current scan. This could mean: |
| 3060 | 3104 | // a) Already at the target version (genuinely satisfied) |
| 3061 | 3105 | // b) Scan missed it (strategy filtering, API failure, etc.) |
| 3062 | // We can't distinguish these cases, so we count it as unverifiable. | |
| 3063 | // The PR will only be closed if we have ZERO unverifiable packages | |
| 3064 | // (i.e., every package was positively confirmed as satisfied). | |
| 3065 | this.logger.debug(`PR #${pr.number}: ${prUpdate.name} not in current scan (unverifiable)`) | |
| 3106 | // The manifest distinguishes them: a declared version at or | |
| 3107 | // beyond the target is positive confirmation of (a). | |
| 3108 | const declaredVersion = declaredVersions.get(prUpdate.name) | |
| 3109 | if (declaredVersion) { | |
| 3110 | const declaredComparison = this.compareVersionsSafe(declaredVersion, prUpdate.newVersion) | |
| 3111 | if (declaredComparison === 'at-or-beyond') { | |
| 3112 | this.logger.debug(`PR #${pr.number}: ${prUpdate.name} declared at ${declaredVersion}, at or beyond ${prUpdate.newVersion}`) | |
| 3113 | verifiedSatisfied++ | |
| 3114 | continue | |
| 3115 | } | |
| 3116 | if (declaredComparison === 'behind') { | |
| 3117 | this.logger.debug(`PR #${pr.number}: ${prUpdate.name} declared at ${declaredVersion}, still behind ${prUpdate.newVersion} — the scan missed it`) | |
| 3118 | verifiedStillNeeded++ | |
| 3119 | continue | |
| 3120 | } | |
| 3121 | } | |
| 3122 | this.logger.debug(`PR #${pr.number}: ${prUpdate.name} not in current scan and not declared (unverifiable)`) | |
| 3066 | 3123 | unverifiable++ |
| 3067 | 3124 | continue |
| 3068 | 3125 | } |
| @@ -3114,9 +3171,10 @@ export class Buddy { | ||
| 3114 | 3171 | // All packages were verified as satisfied (no unverifiable ones) |
| 3115 | 3172 | const packagesAlreadyUpdated = prUpdates.filter((u) => { |
| 3116 | 3173 | const current = currentUpdatesMap.get(u.name) |
| 3117 | if (!current) | |
| 3174 | const version = current?.currentVersion ?? declaredVersions.get(u.name) | |
| 3175 | if (!version) | |
| 3118 | 3176 | return false |
| 3119 | return this.compareVersionsSafe(current.currentVersion, u.newVersion) === 'at-or-beyond' | |
| 3177 | return this.compareVersionsSafe(version, u.newVersion) === 'at-or-beyond' | |
| 3120 | 3178 | }) |
| 3121 | 3179 | |
| 3122 | 3180 | let closeComment = `🤖 **Auto-closing satisfied PR**\n\n` |
| @@ -364,6 +364,46 @@ describe('buddy orchestration', () => { | ||
| 364 | 364 | expect(provider.deleteBranch).toHaveBeenCalledWith('buddy/update-lodash') |
| 365 | 365 | }) |
| 366 | 366 | |
| 367 | it('success case - closes a fully satisfied PR by consulting the manifest when its packages left the scan', async () => { | |
| 368 | // The scan lists only what is still outdated, so a fully satisfied PR | |
| 369 | // has no packages in it at all — the manifest is the positive evidence. | |
| 370 | const projectDir = fs.mkdtempSync(path.join(os.tmpdir(), 'buddy-close-')) | |
| 371 | fs.writeFileSync(path.join(projectDir, 'package.json'), JSON.stringify({ | |
| 372 | devDependencies: { typescript: '^7.0.2' }, | |
| 373 | })) | |
| 374 | restorers.push(() => fs.rmSync(projectDir, { recursive: true, force: true })) | |
| 375 | ||
| 376 | stubScan([makeUpdate({ name: 'react', currentVersion: '18.0.0', newVersion: '18.2.0' })]) | |
| 377 | const provider = makeCloseProvider([makePR({ | |
| 378 | number: 125, | |
| 379 | head: 'buddy/update-typescript', | |
| 380 | body: `updates${serializeManifest([makeUpdate({ name: 'typescript', currentVersion: '6.0.3', newVersion: '7.0.2' })])}`, | |
| 381 | })]) | |
| 382 | const buddy = new Buddy(baseConfig(), projectDir) | |
| 383 | ||
| 384 | await buddy.checkAndCloseSatisfiedPRs(provider, false) | |
| 385 | ||
| 386 | expect(provider.closePullRequest).toHaveBeenCalledWith(125) | |
| 387 | const [, comment] = provider.createComment.mock.calls[0] | |
| 388 | expect(comment).toContain('typescript') | |
| 389 | }) | |
| 390 | ||
| 391 | it('failure case - keeps the PR when the manifest declares a version still behind the target', async () => { | |
| 392 | const projectDir = fs.mkdtempSync(path.join(os.tmpdir(), 'buddy-close-')) | |
| 393 | fs.writeFileSync(path.join(projectDir, 'package.json'), JSON.stringify({ | |
| 394 | dependencies: { lodash: '^4.17.20' }, | |
| 395 | })) | |
| 396 | restorers.push(() => fs.rmSync(projectDir, { recursive: true, force: true })) | |
| 397 | ||
| 398 | stubScan([makeUpdate({ name: 'react', currentVersion: '18.0.0', newVersion: '18.2.0' })]) | |
| 399 | const provider = makeCloseProvider([lodashPR()]) | |
| 400 | const buddy = new Buddy(baseConfig(), projectDir) | |
| 401 | ||
| 402 | await buddy.checkAndCloseSatisfiedPRs(provider, false) | |
| 403 | ||
| 404 | expect(provider.closePullRequest).not.toHaveBeenCalled() | |
| 405 | }) | |
| 406 | ||
| 367 | 407 | it('failure case - never closes on an empty scan (mass-closure guard)', async () => { |
| 368 | 408 | // A scan returning nothing may mean bun outdated failed or the registry |
| 369 | 409 | // rate-limited — closing every PR on that evidence would wipe them out. |
| @@ -430,6 +470,31 @@ describe('buddy orchestration', () => { | ||
| 430 | 470 | }) |
| 431 | 471 | }) |
| 432 | 472 | |
| 473 | describe('extractFilePathsFromPRBody', () => { | |
| 474 | it('failure case - renovate-era badge tables yield no phantom paths', () => { | |
| 475 | const buddy = new Buddy(baseConfig()) as any | |
| 476 | const body = [ | |
| 477 | '| Package | Change | Age | Adoption | Passing | Confidence |', | |
| 478 | '|---|---|---|---|---|---|', | |
| 479 | '| [typescript](https://github.com/microsoft/TypeScript/tree/master) | [`6.0.3` -> `7.0.2`](https://renovatebot.com/diffs/npm/typescript/6.0.3/7.0.2) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | ok | ok |', | |
| 480 | ].join('\n') | |
| 481 | ||
| 482 | expect(buddy.extractFilePathsFromPRBody(body)).toEqual([]) | |
| 483 | }) | |
| 484 | ||
| 485 | it('success case - real file cells and bold paths still extract', () => { | |
| 486 | const buddy = new Buddy(baseConfig()) as any | |
| 487 | const body = [ | |
| 488 | '| [lodash](https://npmjs.com/lodash) | ^4.17.20 → ^4.17.21 | **package.json** | ✅ |', | |
| 489 | 'Also touches **.github/workflows/ci.yml** along the way.', | |
| 490 | ].join('\n') | |
| 491 | ||
| 492 | const paths = buddy.extractFilePathsFromPRBody(body) | |
| 493 | expect(paths).toContain('package.json') | |
| 494 | expect(paths).toContain('.github/workflows/ci.yml') | |
| 495 | }) | |
| 496 | }) | |
| 497 | ||
| 433 | 498 | describe('compareVersionsSafe', () => { |
| 434 | 499 | it('distinguishes at-or-beyond, behind, and failed', () => { |
| 435 | 500 | const buddy = new Buddy(baseConfig()) as any |