fix(mcp): mergeBranch source deletion is opt-in + guards main/contentrain (data-loss regression)

.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.

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,30 +103,53 @@ 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
   }
 }
 
 /**
- * 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<MergeResult['remote'] | undefined> {
-  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 }

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<MergeResult['remote'] | undefined> {
+  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(

packages/mcp/tests/providers/github/branch-ops.test.ts

@@ -5,13 +5,16 @@ import { mergeBranch } from '../../../src/providers/github/branch-ops.js'
 interface Mocks {
   merge?: ReturnType<typeof vi.fn>
   deleteRef?: ReturnType<typeof vi.fn>
+  get?: ReturnType<typeof vi.fn>
 }
 
 function mockClient(overrides: Mocks = {}): GitHubClient {
   return {
     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()
   })
 })

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' })
   })