From c444561e85b90bc4ddb1fefd16c05e03fb0f2cde Mon Sep 17 00:00:00 2001 From: AHMET BAYHAN BAYRAMOGLU <49499275+ABB65@users.noreply.github.com> Date: Thu, 9 Jul 2026 14:47:33 +0300 Subject: [PATCH] fix(mcp): make mergeBranch source deletion opt-in and guard protected branches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The GitHub/GitLab providers' mergeBranch deleted the merged SOURCE branch by default (opt-out via removeSourceBranch: false), with no guard. Because it deletes whatever `branch` it is given, a driver merging a long-lived branch — contentrain→main (publish) or main→contentrain (sync) — would delete contentrain or main. A destructive default that shipped in a minor: a regression, and a data-loss hazard. - Opt-in, not opt-out: like git merge and the platform merge APIs, mergeBranch now leaves the source branch alone by default. Callers that want cr/* cleanup pass removeSourceBranch: true. - Mandatory guard: even when opted in, cleanupSourceBranch never deletes the merge target (into), the contentrain content branch, or the repo default branch (getDefaultBranch; fail-safe skips if unresolvable). Mirrors the LocalProvider's cr/*-only guard; defends against head/base confusion. Both providers. LocalProvider path unchanged (merges only cr/* + guards its own remote cleanup). Studio's explicit removeSourceBranch: false stays valid. Tests: github+gitlab branch-ops — default no-delete, opt-in deletes, guard protects into/default/contentrain even when opted in, fail-safe on unresolvable default. 112/112 across providers + http integration. --- .changeset/merge-source-delete-optin-guard.md | 12 +++ .../mcp/src/providers/github/branch-ops.ts | 38 +++++++-- .../mcp/src/providers/gitlab/branch-ops.ts | 61 ++++++++++---- .../tests/providers/github/branch-ops.test.ts | 81 ++++++++++++++++--- .../tests/providers/gitlab/branch-ops.test.ts | 61 ++++++++++++-- packages/types/src/provider.ts | 14 ++-- 6 files changed, 221 insertions(+), 46 deletions(-) create mode 100644 .changeset/merge-source-delete-optin-guard.md diff --git a/.changeset/merge-source-delete-optin-guard.md b/.changeset/merge-source-delete-optin-guard.md new file mode 100644 index 0000000..b596f60 --- /dev/null +++ b/.changeset/merge-source-delete-optin-guard.md @@ -0,0 +1,12 @@ +--- +"@contentrain/mcp": patch +--- + +Fix: `RepoProvider.mergeBranch` no longer deletes the source branch by default (regression), and never deletes a protected branch even when asked + +The GitHub/GitLab providers' `mergeBranch` deleted the merged **source** branch by default (opt-out via `removeSourceBranch: false`). Because the primitive deletes whatever `branch` it is given, a driver merging a long-lived branch — `contentrain → main` (publish) or `main → contentrain` (sync) — would delete `contentrain` or `main`. This was a destructive-default change that shipped in a minor; it is a regression. + +- **Opt-in, not opt-out.** Like `git merge` and the platform merge APIs, `mergeBranch` now leaves the source branch in place by default. Callers that want the merged branch removed (e.g. `cr/*` review-branch cleanup) pass `removeSourceBranch: true`. +- **Mandatory guard.** Even when opted in, the cleanup NEVER deletes the merge target (`into`), the `contentrain` content branch, or the repo's default branch (resolved via `getDefaultBranch`; fail-safe skips the delete if it can't be resolved). This mirrors the LocalProvider's existing `cr/*`-only guard and defends against head/base confusion. + +Applies to both the GitHub and GitLab providers. The LocalProvider path is unchanged (it already merges only `cr/*` branches and guards its remote cleanup). Studio's explicit `removeSourceBranch: false` pin remains valid and harmless. diff --git a/packages/mcp/src/providers/github/branch-ops.ts b/packages/mcp/src/providers/github/branch-ops.ts index 1b98510..76deb3d 100644 --- a/packages/mcp/src/providers/github/branch-ops.ts +++ b/packages/mcp/src/providers/github/branch-ops.ts @@ -1,3 +1,4 @@ +import { CONTENTRAIN_BRANCH } from '@contentrain/types' import type { Branch, FileDiff, MergeResult } from '../../core/contracts/index.js' import type { GitHubClient } from './client.js' import type { RepoRef } from './types.js' @@ -102,11 +103,11 @@ export async function mergeBranch( base: into, head: branch, }) - const remote = await cleanupSourceBranch(client, repo, branch, opts) + const remote = await cleanupSourceBranch(client, repo, branch, into, opts) return { merged: true, sha: response.data.sha, pullRequestUrl: null, ...(remote ? { remote } : {}) } } catch (error) { if (isNotModified(error)) { - const remote = await cleanupSourceBranch(client, repo, branch, opts) + const remote = await cleanupSourceBranch(client, repo, branch, into, opts) return { merged: true, sha: null, pullRequestUrl: null, ...(remote ? { remote } : {}) } } throw error @@ -114,18 +115,41 @@ export async function mergeBranch( } /** - * Best-effort post-merge deletion of the source branch so merged cr/* - * branches don't pile up on the remote. Opt out per call with - * `removeSourceBranch: false` (e.g. a driver that owns cleanup separately). - * Never throws — the merge itself already succeeded. + * Post-merge deletion of the source branch — **opt-in**. `mergeBranch` is a + * general merge primitive: like `git merge` and GitHub's merge API it leaves + * the source branch alone by default. A caller that wants the merged branch + * removed (e.g. cr/* review-branch cleanup) opts in with + * `removeSourceBranch: true`. + * + * Even when opted in, a long-lived branch is NEVER deleted: not the merge + * target (`into`), not the `contentrain` content branch, and not the repo's + * default branch. This mirrors the LocalProvider's `deleteRemoteBranch` + * guard and defends against head/base confusion and `contentrain→main` / + * `main→contentrain` flows. If the default branch can't be resolved, the + * delete is skipped (fail safe). Never throws — the merge already succeeded. */ async function cleanupSourceBranch( client: GitHubClient, repo: RepoRef, branch: string, + into: string, opts?: { removeSourceBranch?: boolean }, ): Promise { - if (opts?.removeSourceBranch === false) return undefined + if (opts?.removeSourceBranch !== true) return undefined + + // Free guards first — no API call for the obvious protected refs. + if (branch === into || branch === CONTENTRAIN_BRANCH) { + return { deleted: false, skipped: 'protected' } + } + try { + if (branch === await getDefaultBranch(client, repo)) { + return { deleted: false, skipped: 'protected' } + } + } catch { + // Cannot verify the default branch — refuse to delete rather than risk it. + return { deleted: false, skipped: 'protected' } + } + try { await deleteBranch(client, repo, branch) return { deleted: true } diff --git a/packages/mcp/src/providers/gitlab/branch-ops.ts b/packages/mcp/src/providers/gitlab/branch-ops.ts index 425c0dd..a491e53 100644 --- a/packages/mcp/src/providers/gitlab/branch-ops.ts +++ b/packages/mcp/src/providers/gitlab/branch-ops.ts @@ -1,3 +1,4 @@ +import { CONTENTRAIN_BRANCH } from '@contentrain/types' import type { Branch, FileDiff, MergeResult } from '../../core/contracts/index.js' import type { GitLabClient } from './client.js' import type { ProjectRef } from './types.js' @@ -125,24 +126,54 @@ export async function mergeBranch( const webUrl = (mr as { web_url?: string }).web_url ?? null const merged = mergeSha !== null - // 3. Best-effort source-branch cleanup so merged cr/* branches don't - // pile up on the remote. Opt out per call with - // `removeSourceBranch: false` (e.g. a driver that owns cleanup - // separately). Never throws — the merge itself already succeeded. - let remote: MergeResult['remote'] - if (merged && opts?.removeSourceBranch !== false) { - try { - await deleteBranch(client, project, branch) - remote = { deleted: true } - } catch (error) { - const message = error instanceof Error ? error.message : String(error) - remote = /404|not found/i.test(message) - ? { deleted: false, skipped: 'not-found' } - : { deleted: false, warning: `Could not delete "${branch}": ${message}` } + // 3. Source-branch cleanup — OPT-IN (`removeSourceBranch: true`). Like git + // and GitLab's own merge, `mergeBranch` leaves the source alone by + // default. Even when opted in, a long-lived branch is NEVER deleted: + // not the merge target (`into`), not the `contentrain` content branch, + // not the project's default branch. Mirrors the LocalProvider guard; + // defends against head/base confusion and contentrain↔default flows. + // Never throws — the merge itself already succeeded. + const remote = merged ? await cleanupSourceBranch(client, project, branch, into, opts) : undefined + + return { merged, sha: mergeSha, pullRequestUrl: webUrl, ...(remote ? { remote } : {}) } +} + +/** + * Opt-in ({@link mergeBranch} `removeSourceBranch: true`) deletion of the + * merged source branch, with the same guard as the GitHub provider and the + * LocalProvider: never delete the merge target, the `contentrain` content + * branch, or the project default branch — even when opted in. Fail-safe + * skips the delete if the default branch cannot be resolved. Never throws. + */ +async function cleanupSourceBranch( + client: GitLabClient, + project: ProjectRef, + branch: string, + into: string, + opts?: { removeSourceBranch?: boolean }, +): Promise { + if (opts?.removeSourceBranch !== true) return undefined + + if (branch === into || branch === CONTENTRAIN_BRANCH) { + return { deleted: false, skipped: 'protected' } + } + try { + if (branch === await getDefaultBranch(client, project)) { + return { deleted: false, skipped: 'protected' } } + } catch { + return { deleted: false, skipped: 'protected' } } - return { merged, sha: mergeSha, pullRequestUrl: webUrl, ...(remote ? { remote } : {}) } + try { + await deleteBranch(client, project, branch) + return { deleted: true } + } catch (error) { + const message = error instanceof Error ? error.message : String(error) + return /404|not found/i.test(message) + ? { deleted: false, skipped: 'not-found' } + : { deleted: false, warning: `Could not delete "${branch}": ${message}` } + } } export async function isMerged( diff --git a/packages/mcp/tests/providers/github/branch-ops.test.ts b/packages/mcp/tests/providers/github/branch-ops.test.ts index 513e620..0350894 100644 --- a/packages/mcp/tests/providers/github/branch-ops.test.ts +++ b/packages/mcp/tests/providers/github/branch-ops.test.ts @@ -5,6 +5,7 @@ import { mergeBranch } from '../../../src/providers/github/branch-ops.js' interface Mocks { merge?: ReturnType deleteRef?: ReturnType + get?: ReturnType } function mockClient(overrides: Mocks = {}): GitHubClient { @@ -12,6 +13,8 @@ function mockClient(overrides: Mocks = {}): GitHubClient { rest: { repos: { merge: overrides.merge ?? vi.fn().mockResolvedValue({ data: { sha: 'merge-sha' } }), + // getDefaultBranch (used by the cleanup guard) — default 'main'. + get: overrides.get ?? vi.fn().mockResolvedValue({ data: { default_branch: 'main' } }), }, git: { deleteRef: overrides.deleteRef ?? vi.fn().mockResolvedValue(undefined), @@ -23,21 +26,26 @@ function mockClient(overrides: Mocks = {}): GitHubClient { const repo = { owner: 'o', name: 'r' } describe('mergeBranch', () => { - it('merges and deletes the source ref by default', async () => { + it('does NOT delete the source ref by default (opt-in)', async () => { const merge = vi.fn().mockResolvedValue({ data: { sha: 'merge-sha' } }) - const deleteRef = vi.fn().mockResolvedValue(undefined) + const deleteRef = vi.fn() const client = mockClient({ merge, deleteRef }) const result = await mergeBranch(client, repo, 'cr/feat', 'contentrain') expect(merge).toHaveBeenCalledWith({ owner: 'o', repo: 'r', base: 'contentrain', head: 'cr/feat' }) + expect(deleteRef).not.toHaveBeenCalled() + expect(result).toEqual({ merged: true, sha: 'merge-sha', pullRequestUrl: null }) + }) + + it('deletes the source ref when removeSourceBranch: true', async () => { + const deleteRef = vi.fn().mockResolvedValue(undefined) + const client = mockClient({ deleteRef }) + + const result = await mergeBranch(client, repo, 'cr/feat', 'contentrain', { removeSourceBranch: true }) + expect(deleteRef).toHaveBeenCalledWith({ owner: 'o', repo: 'r', ref: 'heads/cr/feat' }) - expect(result).toEqual({ - merged: true, - sha: 'merge-sha', - pullRequestUrl: null, - remote: { deleted: true }, - }) + expect(result.remote).toEqual({ deleted: true }) }) it('keeps the source ref when removeSourceBranch is false', async () => { @@ -50,12 +58,59 @@ describe('mergeBranch', () => { expect(result.remote).toBeUndefined() }) - it('treats an already-merged (304) response as merged and still cleans up', async () => { + // ─── Guard: never delete a long-lived branch, even when opted in ─── + + it('refuses to delete the merge target (into), even with removeSourceBranch: true', async () => { + const deleteRef = vi.fn() + const client = mockClient({ deleteRef }) + + // A caller confusing head/base: merging main INTO main, asking to delete. + const result = await mergeBranch(client, repo, 'main', 'main', { removeSourceBranch: true }) + + expect(deleteRef).not.toHaveBeenCalled() + expect(result.remote).toEqual({ deleted: false, skipped: 'protected' }) + }) + + it('refuses to delete the repo default branch (e.g. main→contentrain), even with removeSourceBranch: true', async () => { + const deleteRef = vi.fn() + const get = vi.fn().mockResolvedValue({ data: { default_branch: 'main' } }) + const client = mockClient({ deleteRef, get }) + + const result = await mergeBranch(client, repo, 'main', 'contentrain', { removeSourceBranch: true }) + + expect(deleteRef).not.toHaveBeenCalled() + expect(result.remote).toEqual({ deleted: false, skipped: 'protected' }) + }) + + it('refuses to delete the contentrain content branch (contentrain→main), even with removeSourceBranch: true', async () => { + const deleteRef = vi.fn() + const client = mockClient({ deleteRef }) + + const result = await mergeBranch(client, repo, 'contentrain', 'main', { removeSourceBranch: true }) + + expect(deleteRef).not.toHaveBeenCalled() + expect(result.remote).toEqual({ deleted: false, skipped: 'protected' }) + }) + + it('fails safe (no delete) when the default branch cannot be resolved', async () => { + const deleteRef = vi.fn() + const get = vi.fn().mockRejectedValue(new Error('Server Error')) + const client = mockClient({ deleteRef, get }) + + const result = await mergeBranch(client, repo, 'cr/feat', 'contentrain', { removeSourceBranch: true }) + + expect(deleteRef).not.toHaveBeenCalled() + expect(result.remote).toEqual({ deleted: false, skipped: 'protected' }) + }) + + // ─── Opted-in cleanup edge cases ─── + + it('treats an already-merged (204) response as merged and still cleans up when opted in', async () => { const merge = vi.fn().mockRejectedValue(Object.assign(new Error('not modified'), { status: 204 })) const deleteRef = vi.fn().mockResolvedValue(undefined) const client = mockClient({ merge, deleteRef }) - const result = await mergeBranch(client, repo, 'cr/feat', 'contentrain') + const result = await mergeBranch(client, repo, 'cr/feat', 'contentrain', { removeSourceBranch: true }) expect(result.merged).toBe(true) expect(result.sha).toBeNull() @@ -66,7 +121,7 @@ describe('mergeBranch', () => { const deleteRef = vi.fn().mockRejectedValue(Object.assign(new Error('Reference does not exist'), { status: 422 })) const client = mockClient({ deleteRef }) - const result = await mergeBranch(client, repo, 'cr/feat', 'contentrain') + const result = await mergeBranch(client, repo, 'cr/feat', 'contentrain', { removeSourceBranch: true }) expect(result.merged).toBe(true) expect(result.remote).toEqual({ deleted: false, skipped: 'not-found' }) @@ -76,7 +131,7 @@ describe('mergeBranch', () => { const deleteRef = vi.fn().mockRejectedValue(Object.assign(new Error('Forbidden'), { status: 403 })) const client = mockClient({ deleteRef }) - const result = await mergeBranch(client, repo, 'cr/feat', 'contentrain') + const result = await mergeBranch(client, repo, 'cr/feat', 'contentrain', { removeSourceBranch: true }) expect(result.merged).toBe(true) expect(result.remote?.deleted).toBe(false) @@ -88,7 +143,7 @@ describe('mergeBranch', () => { const deleteRef = vi.fn() const client = mockClient({ merge, deleteRef }) - await expect(mergeBranch(client, repo, 'cr/feat', 'contentrain')).rejects.toThrow('Conflict') + await expect(mergeBranch(client, repo, 'cr/feat', 'contentrain', { removeSourceBranch: true })).rejects.toThrow('Conflict') expect(deleteRef).not.toHaveBeenCalled() }) }) diff --git a/packages/mcp/tests/providers/gitlab/branch-ops.test.ts b/packages/mcp/tests/providers/gitlab/branch-ops.test.ts index 5590e5c..4bf92a3 100644 --- a/packages/mcp/tests/providers/gitlab/branch-ops.test.ts +++ b/packages/mcp/tests/providers/gitlab/branch-ops.test.ts @@ -139,7 +139,7 @@ describe('getBranchDiff', () => { }) describe('mergeBranch', () => { - it('opens an MR, accepts it and deletes the source branch — returns merged: true with the merge SHA', async () => { + it('opens an MR and accepts it, but does NOT delete the source branch by default (opt-in)', async () => { const mrCreate = vi.fn().mockResolvedValue({ iid: 42, web_url: 'https://gitlab.com/o/r/-/merge_requests/42', @@ -164,15 +164,26 @@ describe('mergeBranch', () => { 42, { shouldRemoveSourceBranch: false, squash: false }, ) - expect(branchRemove).toHaveBeenCalledWith('o/r', 'cr/feat') + expect(branchRemove).not.toHaveBeenCalled() expect(result).toEqual({ merged: true, sha: 'merge-sha-1', pullRequestUrl: 'https://gitlab.com/o/r/-/merge_requests/42', - remote: { deleted: true }, }) }) + it('deletes the source branch when removeSourceBranch: true', async () => { + const mrCreate = vi.fn().mockResolvedValue({ iid: 42, web_url: 'u' }) + const mrAccept = vi.fn().mockResolvedValue({ merge_commit_sha: 'merge-sha-1' }) + const branchRemove = vi.fn().mockResolvedValue(undefined) + const client = mockClient({ mrCreate, mrAccept, branchRemove }) + + const result = await mergeBranch(client, { projectId: 'o/r' }, 'cr/feat', 'contentrain', { removeSourceBranch: true }) + + expect(branchRemove).toHaveBeenCalledWith('o/r', 'cr/feat') + expect(result.remote).toEqual({ deleted: true }) + }) + it('keeps the source branch when removeSourceBranch is false', async () => { const mrCreate = vi.fn().mockResolvedValue({ iid: 43, web_url: 'https://gitlab.com/o/r/-/merge_requests/43' }) const mrAccept = vi.fn().mockResolvedValue({ merge_commit_sha: 'merge-sha-2' }) @@ -185,13 +196,53 @@ describe('mergeBranch', () => { expect(result.remote).toBeUndefined() }) + // ─── Guard: never delete a long-lived branch, even when opted in ─── + + it('refuses to delete the merge target, the default branch, or contentrain — even with removeSourceBranch: true', async () => { + const cases: Array<[string, string]> = [ + ['contentrain', 'contentrain'], // branch === into + ['main', 'contentrain'], // branch === default branch + ['contentrain', 'main'], // branch === contentrain content branch + ] + for (const [branch, into] of cases) { + const mrAccept = vi.fn().mockResolvedValue({ merge_commit_sha: 's' }) + const branchRemove = vi.fn() + const client = mockClient({ + mrCreate: vi.fn().mockResolvedValue({ iid: 1, web_url: 'u' }), + mrAccept, + branchRemove, + projectShow: vi.fn().mockResolvedValue({ default_branch: 'main' }), + }) + + const result = await mergeBranch(client, { projectId: 'o/r' }, branch, into, { removeSourceBranch: true }) + + expect(branchRemove).not.toHaveBeenCalled() + expect(result.remote).toEqual({ deleted: false, skipped: 'protected' }) + } + }) + + it('fails safe (no delete) when the default branch cannot be resolved', async () => { + const branchRemove = vi.fn() + const client = mockClient({ + mrCreate: vi.fn().mockResolvedValue({ iid: 1, web_url: 'u' }), + mrAccept: vi.fn().mockResolvedValue({ merge_commit_sha: 's' }), + branchRemove, + projectShow: vi.fn().mockRejectedValue(new Error('500')), + }) + + const result = await mergeBranch(client, { projectId: 'o/r' }, 'cr/feat', 'contentrain', { removeSourceBranch: true }) + + expect(branchRemove).not.toHaveBeenCalled() + expect(result.remote).toEqual({ deleted: false, skipped: 'protected' }) + }) + it('surfaces a cleanup failure as a warning without failing the merge', async () => { const mrCreate = vi.fn().mockResolvedValue({ iid: 44, web_url: 'https://gitlab.com/o/r/-/merge_requests/44' }) const mrAccept = vi.fn().mockResolvedValue({ merge_commit_sha: 'merge-sha-3' }) const branchRemove = vi.fn().mockRejectedValue(new Error('403 Forbidden')) const client = mockClient({ mrCreate, mrAccept, branchRemove }) - const result = await mergeBranch(client, { projectId: 'o/r' }, 'cr/feat', 'contentrain') + const result = await mergeBranch(client, { projectId: 'o/r' }, 'cr/feat', 'contentrain', { removeSourceBranch: true }) expect(result.merged).toBe(true) expect(result.remote?.deleted).toBe(false) @@ -204,7 +255,7 @@ describe('mergeBranch', () => { const branchRemove = vi.fn().mockRejectedValue(new Error('404 Branch Not Found')) const client = mockClient({ mrCreate, mrAccept, branchRemove }) - const result = await mergeBranch(client, { projectId: 'o/r' }, 'cr/feat', 'contentrain') + const result = await mergeBranch(client, { projectId: 'o/r' }, 'cr/feat', 'contentrain', { removeSourceBranch: true }) expect(result.remote).toEqual({ deleted: false, skipped: 'not-found' }) }) diff --git a/packages/types/src/provider.ts b/packages/types/src/provider.ts index 08e7b05..8674d6b 100644 --- a/packages/types/src/provider.ts +++ b/packages/types/src/provider.ts @@ -224,12 +224,14 @@ export interface RepoProvider extends RepoReader, RepoWriter { deleteBranch(name: string): Promise getBranchDiff(branch: string, base?: string): Promise /** - * Merge `branch` into `into`. By default the source branch's remote copy - * is removed after a successful merge (best-effort — reported via - * `MergeResult.remote`, never a thrown error). Pass - * `opts.removeSourceBranch: false` to keep it (e.g. a driver that owns - * cleanup separately). LocalProvider ignores the option: its cleanup is - * governed by `config.remoteBranchCleanup`. + * Merge `branch` into `into`. Like `git merge` and the platform merge APIs, + * the source branch is left in place by default. Pass + * `opts.removeSourceBranch: true` to also delete it after a successful merge + * (best-effort — reported via `MergeResult.remote`, never a thrown error). + * Even when opted in, a long-lived branch is never deleted: not `into`, not + * the `contentrain` content branch, and not the repo's default branch. + * LocalProvider ignores the option: its cleanup is governed by + * `config.remoteBranchCleanup` and its own `cr/*`-only guard. */ mergeBranch(branch: string, into: string, opts?: { removeSourceBranch?: boolean }): Promise isMerged(branch: string, into?: string): Promise