also looking at this
fix: give GitLab/Bitbucket pipelines the review job, and stop cleanup lying
#1432
5 files
+152
-8
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.
| @@ -752,8 +752,15 @@ export class Buddy { | ||
| 752 | 752 | } |
| 753 | 753 | |
| 754 | 754 | await gitProvider.closePullRequest(existingPR.number) |
| 755 | await gitProvider.deleteBranch(existingPR.head) | |
| 756 | this.logger.success(`✅ Auto-closed PR #${existingPR.number} and deleted branch ${existingPR.head}`) | |
| 755 | // The pull request is closed either way; a branch that would | |
| 756 | // not delete is worth a warning, not a failed run. | |
| 757 | try { | |
| 758 | await gitProvider.deleteBranch(existingPR.head) | |
| 759 | this.logger.success(`✅ Auto-closed PR #${existingPR.number} and deleted branch ${existingPR.head}`) | |
| 760 | } | |
| 761 | catch (deleteError) { | |
| 762 | this.logger.warn(`⚠️ Auto-closed PR #${existingPR.number} but could not delete ${existingPR.head}:`, deleteError) | |
| 763 | } | |
| 757 | 764 | continue |
| 758 | 765 | } |
| 759 | 766 | catch (error) { |
| @@ -1608,19 +1608,29 @@ export class GitHubProvider implements GitProvider { | ||
| 1608 | 1608 | this.logger.info(`✅ Deleted branch ${branchName} via git`) |
| 1609 | 1609 | } |
| 1610 | 1610 | catch (error) { |
| 1611 | // If git push fails, it might be because the branch doesn't exist remotely | |
| 1612 | // or we don't have push permissions. Try to delete locally and ignore errors. | |
| 1611 | // The local tracking branch is tidied either way; it is not what the | |
| 1612 | // caller asked about and its absence is not an error. | |
| 1613 | 1613 | try { |
| 1614 | // Also delete local tracking branch if it exists | |
| 1615 | 1614 | await this.runCommand('git', ['branch', '-D', branchName]) |
| 1616 | this.logger.info(`✅ Deleted local branch ${branchName}`) | |
| 1617 | 1615 | } |
| 1618 | 1616 | catch { |
| 1619 | 1617 | // Ignore local deletion errors - branch might not exist locally |
| 1620 | 1618 | } |
| 1621 | 1619 | |
| 1620 | // A branch that is already gone is the outcome the caller wanted, so | |
| 1621 | // that one failure is a success. Every other failure — no push | |
| 1622 | // permission, network, protected branch — is reported by throwing, the | |
| 1623 | // way the GitLab and Bitbucket providers already do. This used to | |
| 1624 | // swallow all of them, which made `cleanupStaleBranches`'s `failed` | |
| 1625 | // bucket unreachable: every branch was reported deleted whether the | |
| 1626 | // push succeeded or not. | |
| 1627 | if (/remote ref does not exist|unable to delete '[^']*': remote ref/i.test(formatError(error))) { | |
| 1628 | this.logger.info(`ℹ️ Branch ${branchName} was already gone`) | |
| 1629 | return | |
| 1630 | } | |
| 1631 | ||
| 1622 | 1632 | this.logger.warn(`⚠️ Failed to delete remote branch ${branchName}: ${formatError(error)}`) |
| 1623 | // Don't throw - branch deletion failures are not critical | |
| 1633 | throw error | |
| 1624 | 1634 | } |
| 1625 | 1635 | } |
| 1626 | 1636 | |
| @@ -2360,7 +2360,12 @@ export async function generateCoreWorkflows(preset: WorkflowPreset, _repoInfo: R | ||
| 2360 | 2360 | // workflows into a GitLab repository would produce files that never run, |
| 2361 | 2361 | // which reads as a successful setup until the first schedule does nothing. |
| 2362 | 2362 | const provider = _repoInfo.provider ?? 'github' |
| 2363 | const template = ciTemplateFor(provider) | |
| 2363 | // The review job is what makes the pipeline more than a scheduler. The | |
| 2364 | // GitHub workflow has always carried one; these two were generated without | |
| 2365 | // it because the option existed and the call site never passed it. The | |
| 2366 | // command honours `ai.review` at run time and works without a key, so the | |
| 2367 | // job is as safe to include here as it is there. | |
| 2368 | const template = ciTemplateFor(provider, { review: true }) | |
| 2364 | 2369 | |
| 2365 | 2370 | if (template) { |
| 2366 | 2371 | fs.writeFileSync(template.path, template.content) |
| @@ -0,0 +1,67 @@ | ||
| 1 | import { describe, expect, it } from 'bun:test' | |
| 2 | import { GitHubProvider } from '../src/git/github-provider' | |
| 3 | import { Logger } from '../src/utils/logger' | |
| 4 | ||
| 5 | /** A provider whose git commands are scripted, recording what was run. */ | |
| 6 | function provider(script: (args: string[]) => Promise<string>) { | |
| 7 | const prov = new GitHubProvider('token', 'owner', 'repo', false, undefined, 'https://api.github.test', Logger.silent()) | |
| 8 | const ran: string[][] = [] | |
| 9 | ;(prov as never as { runCommand: (cmd: string, args: string[]) => Promise<string> }).runCommand | |
| 10 | = async (_cmd, args) => { | |
| 11 | ran.push(args) | |
| 12 | return script(args) | |
| 13 | } | |
| 14 | return { prov, ran } | |
| 15 | } | |
| 16 | ||
| 17 | /** | |
| 18 | * `deleteBranch` swallowed every failure "because branch deletion failures are | |
| 19 | * not critical" — so `cleanupStaleBranches`, whose per-branch `catch` fills the | |
| 20 | * `failed` bucket, could never reach it. Every branch was reported deleted | |
| 21 | * whether the push succeeded or not, and `buddy update-check` printed | |
| 22 | * "N branches deleted, 0 failed" over a remote that still had them. | |
| 23 | */ | |
| 24 | describe('deleting a branch', () => { | |
| 25 | it('success case - a successful push resolves', async () => { | |
| 26 | const { prov, ran } = provider(async () => '') | |
| 27 | ||
| 28 | await expect(prov.deleteBranch('buddy/update-x')).resolves.toBeUndefined() | |
| 29 | expect(ran[0]).toEqual(['push', 'origin', '--delete', 'buddy/update-x']) | |
| 30 | }) | |
| 31 | ||
| 32 | it('failure case - a refused push is reported, not swallowed', async () => { | |
| 33 | // No permission, network, protected branch: the caller asked for a | |
| 34 | // deletion and did not get one. GitLab and Bitbucket already throw here. | |
| 35 | const { prov } = provider(async (args) => { | |
| 36 | if (args[0] === 'push') | |
| 37 | throw new Error('remote: error: GH006: Protected branch update failed') | |
| 38 | return '' | |
| 39 | }) | |
| 40 | ||
| 41 | await expect(prov.deleteBranch('main')).rejects.toThrow('Protected branch') | |
| 42 | }) | |
| 43 | ||
| 44 | it('edge case - a branch that is already gone counts as deleted', async () => { | |
| 45 | // The outcome the caller wanted has already happened. Failing cleanup | |
| 46 | // over it would leave the same branch "failing" on every run. | |
| 47 | const { prov } = provider(async (args) => { | |
| 48 | if (args[0] === 'push') | |
| 49 | throw new Error("error: unable to delete 'buddy/old': remote ref does not exist") | |
| 50 | return '' | |
| 51 | }) | |
| 52 | ||
| 53 | await expect(prov.deleteBranch('buddy/old')).resolves.toBeUndefined() | |
| 54 | }) | |
| 55 | ||
| 56 | it('success case - the local tracking branch is tidied even when the push fails', async () => { | |
| 57 | const { prov, ran } = provider(async (args) => { | |
| 58 | if (args[0] === 'push') | |
| 59 | throw new Error('fatal: could not read from remote') | |
| 60 | return '' | |
| 61 | }) | |
| 62 | ||
| 63 | await prov.deleteBranch('buddy/x').catch(() => {}) | |
| 64 | ||
| 65 | expect(ran.some(args => args[0] === 'branch' && args[1] === '-D')).toBe(true) | |
| 66 | }) | |
| 67 | }) | |
| @@ -0,0 +1,55 @@ | ||
| 1 | import { afterEach, beforeEach, describe, expect, it } from 'bun:test' | |
| 2 | import { promises as fs } from 'node:fs' | |
| 3 | import { tmpdir } from 'node:os' | |
| 4 | import { join } from 'node:path' | |
| 5 | import { generateCoreWorkflows, getWorkflowPreset } from '../src/setup' | |
| 6 | import { Logger } from '../src/utils/logger' | |
| 7 | ||
| 8 | /** | |
| 9 | * `ciTemplateFor` took a `{ review }` option and its only call site never | |
| 10 | * passed it, so a GitLab or Bitbucket repository set up by `buddy setup` got | |
| 11 | * a pipeline that scheduled updates and never reviewed a merge request. The | |
| 12 | * GitHub workflow has always carried the review job. | |
| 13 | */ | |
| 14 | describe('generated GitLab and Bitbucket pipelines', () => { | |
| 15 | let dir: string | |
| 16 | let originalCwd: string | |
| 17 | ||
| 18 | beforeEach(async () => { | |
| 19 | dir = await fs.mkdtemp(join(tmpdir(), 'buddy-pipeline-')) | |
| 20 | originalCwd = process.cwd() | |
| 21 | process.chdir(dir) | |
| 22 | }) | |
| 23 | ||
| 24 | afterEach(async () => { | |
| 25 | process.chdir(originalCwd) | |
| 26 | await fs.rm(dir, { recursive: true, force: true }) | |
| 27 | }) | |
| 28 | ||
| 29 | it('success case - the GitLab pipeline carries the review job', async () => { | |
| 30 | await generateCoreWorkflows( | |
| 31 | getWorkflowPreset('standard'), | |
| 32 | { owner: 'group', name: 'repo', provider: 'gitlab' }, | |
| 33 | false, | |
| 34 | Logger.silent(), | |
| 35 | ) | |
| 36 | ||
| 37 | const pipeline = await fs.readFile(join(dir, '.gitlab-ci.yml'), 'utf-8') | |
| 38 | ||
| 39 | expect(pipeline).toContain('buddy:review:') | |
| 40 | expect(pipeline).toContain('buddy review $CI_MERGE_REQUEST_IID') | |
| 41 | }) | |
| 42 | ||
| 43 | it('success case - the Bitbucket pipeline carries the review step', async () => { | |
| 44 | await generateCoreWorkflows( | |
| 45 | getWorkflowPreset('standard'), | |
| 46 | { owner: 'workspace', name: 'repo', provider: 'bitbucket' }, | |
| 47 | false, | |
| 48 | Logger.silent(), | |
| 49 | ) | |
| 50 | ||
| 51 | const pipeline = await fs.readFile(join(dir, 'bitbucket-pipelines.yml'), 'utf-8') | |
| 52 | ||
| 53 | expect(pipeline).toContain('buddy review $BITBUCKET_PR_ID') | |
| 54 | }) | |
| 55 | }) | |