fix(mcp): stop tripping simple-git block-unsafe guard on every write

.changeset/mcp-block-unsafe-guard.md

@@ -0,0 +1,30 @@
+---
+"@contentrain/mcp": patch
+---
+
+fix(mcp): stop tripping simple-git's block-unsafe guard on every write
+
+`git commit` (and every other worktree operation) began failing with
+`Use of "EDITOR"/"GIT_ASKPASS" is not permitted…` in any host that exports
+those variables (VS Code, Claude Code, CI). Nothing in Contentrain changed — a
+transitive bump of `simple-git` to `>= 3.34` pulled in `@simple-git/argv-parser`,
+whose block-unsafe guard rejects a `git` invocation when a guard-listed variable
+is passed **explicitly** through `.env()`. The transaction layer had been
+spreading `...process.env` into `.env()` to set the commit author, so the guard
+saw those inherited variables and refused to run.
+
+- Commit identity now flows through `-c user.name` / `-c user.email` config
+  (`authorConfig`) instead of `.env()`. These keys are not on any unsafe list and
+  git honours them for both author and committer, so the guard is never touched —
+  regardless of what the host exports. `CONTENTRAIN_AUTHOR_NAME/EMAIL` overrides
+  are preserved.
+- The one instance that legitimately needs the inherited environment — network
+  push/fetch, which relies on the host's askpass/SSH/proxy setup — keeps `.env()`
+  but opts out of the affected guard categories via `unsafe` (`NETWORK_UNSAFE`),
+  leaving arg-injection protections intact. This closes the same latent failure
+  in `contentrain_submit`.
+- Both concerns are centralized in `git/identity.ts` so no future call site can
+  reintroduce a `process.env` spread. `simple-git` is pinned to `^3.36.0`.
+
+Covered by `tests/git/identity.test.ts`, including a control that asserts the old
+`.env(process.env)` path still trips the guard.

packages/mcp/package.json

@@ -163,7 +163,7 @@
   "dependencies": {
     "@contentrain/types": "workspace:*",
     "@modelcontextprotocol/sdk": "^1.12.0",
-    "simple-git": "^3.27.0",
+    "simple-git": "^3.36.0",
     "typescript": "^5.7.0",
     "zod": "^3.24.0"
   },

packages/mcp/src/git/branch-lifecycle.ts

@@ -1,6 +1,7 @@
 import { simpleGit, type SimpleGit } from 'simple-git'
 import { CONTENTRAIN_BRANCH, type ContentrainConfig } from '@contentrain/types'
 import { readConfig } from '../core/config.js'
+import { NETWORK_UNSAFE } from './identity.js'
 
 export interface CleanupResult {
   deleted: number
@@ -356,9 +357,15 @@ function contentrainRemoteName(): string {
  * simple-git instance hardened for network operations: `timeout.block` kills
  * the child after N ms without output (hung SSH passphrase prompts), and
  * `GIT_TERMINAL_PROMPT=0` refuses interactive HTTPS credential prompts.
+ *
+ * This is the one place that legitimately inherits the real environment — push
+ * needs the host's askpass/SSH/proxy setup to authenticate. Because `.env()` is
+ * therefore unavoidable, `unsafe` opts out of the block-unsafe guard categories
+ * that inherited variables (EDITOR, GIT_ASKPASS, …) would otherwise trip; see
+ * NETWORK_UNSAFE. Arg-injection protections stay intact.
  */
 function networkGit(projectRoot: string, timeoutMs: number): SimpleGit {
-  return simpleGit({ baseDir: projectRoot, timeout: { block: timeoutMs } })
+  return simpleGit({ baseDir: projectRoot, timeout: { block: timeoutMs }, unsafe: NETWORK_UNSAFE })
     .env({ ...process.env, GIT_TERMINAL_PROMPT: '0' })
 }
 

packages/mcp/src/git/identity.ts

@@ -0,0 +1,52 @@
+/**
+ * Git identity + guard-safe `simple-git` construction for MCP write operations.
+ *
+ * simple-git >= 3.34 bundles `@simple-git/argv-parser`, whose block-unsafe
+ * guard rejects a `git` invocation when any of ~18 "unsafe" variables (EDITOR,
+ * GIT_ASKPASS, PAGER, GIT_SSH_COMMAND, GIT_PROXY_COMMAND, …) is passed
+ * EXPLICITLY through `.env()`. Crucially, the guard only scans the object
+ * handed to `.env()` — it never inspects the inherited process environment.
+ *
+ * The rule this module enforces: NEVER spread `process.env` into `.env()`.
+ *   - Commit identity is supplied as `-c user.*` config (guard-safe: `user.name`
+ *     / `user.email` are not on any unsafe list, and git honours them for both
+ *     the author and the committer). See {@link authorConfig}.
+ *   - The rare instance that genuinely needs the inherited environment — network
+ *     push/fetch, which relies on the host's askpass/SSH/proxy setup to
+ *     authenticate — opts out of the affected guard categories via `unsafe`
+ *     instead of hiding the environment. See {@link NETWORK_UNSAFE}.
+ */
+
+const DEFAULT_AUTHOR_NAME = 'Contentrain'
+const DEFAULT_AUTHOR_EMAIL = 'ai@contentrain.io'
+
+/**
+ * Commit identity as `-c` config entries for `simpleGit(dir, { config })`.
+ * Passed as arguments (not env) so the block-unsafe guard is never triggered,
+ * regardless of what the host process exports. Sets author + committer alike.
+ */
+export function authorConfig(): string[] {
+  const name = process.env['CONTENTRAIN_AUTHOR_NAME'] ?? DEFAULT_AUTHOR_NAME
+  const email = process.env['CONTENTRAIN_AUTHOR_EMAIL'] ?? DEFAULT_AUTHOR_EMAIL
+  return [`user.name=${name}`, `user.email=${email}`]
+}
+
+/**
+ * Guard opt-outs for network `git` instances that MUST inherit the real
+ * environment (credential askpass helpers, SSH agent, proxy) to authenticate a
+ * push/fetch. Covers every guard category reachable from an inherited env var,
+ * so the command never trips regardless of what the host (VS Code, CI) exports
+ * — while still leaving arg-injection protections (custom binaries, `ext::`
+ * protocol, `--upload-pack`) intact.
+ */
+export const NETWORK_UNSAFE = {
+  allowUnsafeAskPass: true,
+  allowUnsafeConfigEnvCount: true,
+  allowUnsafeConfigPaths: true,
+  allowUnsafeDiffExternal: true,
+  allowUnsafeEditor: true,
+  allowUnsafeGitProxy: true,
+  allowUnsafePager: true,
+  allowUnsafeSshCommand: true,
+  allowUnsafeTemplateDir: true,
+}

packages/mcp/src/git/transaction.ts

@@ -6,6 +6,7 @@ import { randomUUID } from 'node:crypto'
 import { readConfig } from '../core/config.js'
 import { writeContext } from '../core/context.js'
 import { deleteRemoteBranch, type RemoteDeleteResult } from './branch-lifecycle.js'
+import { authorConfig } from './identity.js'
 import { branchTimestamp } from '../util/id.js'
 import { migrateLegacyBranches } from '../providers/local/migration.js'
 import type { SyncResult, WorkflowMode } from '@contentrain/types'
@@ -60,25 +61,6 @@ export async function ensureContentBranch(projectRoot: string): Promise<void> {
   }
 }
 
-/**
- * Commit identity for worktree operations, supplied via environment instead
- * of two `git config` spawns per transaction. Git honors GIT_AUTHOR_* /
- * GIT_COMMITTER_* for both the sync merges and the feature-branch commit, so
- * a worktree git built with this env needs no `git config user.*` calls.
- * `process.env` is spread so PATH/HOME and any GIT_* already set survive.
- */
-function authorEnv(): Record<string, string | undefined> {
-  const name = process.env['CONTENTRAIN_AUTHOR_NAME'] ?? 'Contentrain'
-  const email = process.env['CONTENTRAIN_AUTHOR_EMAIL'] ?? 'ai@contentrain.io'
-  return {
-    ...process.env,
-    GIT_AUTHOR_NAME: name,
-    GIT_AUTHOR_EMAIL: email,
-    GIT_COMMITTER_NAME: name,
-    GIT_COMMITTER_EMAIL: email,
-  }
-}
-
 async function selectiveSync(
   projectRoot: string,
   _worktreePath: string,
@@ -241,9 +223,10 @@ export async function createTransaction(
   // Create worktree on contentrain branch
   await git.raw(['worktree', 'add', worktreePath, CONTENTRAIN_BRANCH])
 
-  // Commit identity comes from the environment (see authorEnv) — no
-  // `git config user.*` spawns.
-  const wtGit = simpleGit(worktreePath).env(authorEnv())
+  // Commit identity comes from `-c user.*` config (see authorConfig) — passed
+  // as args, never via `.env()`, so simple-git's block-unsafe guard is never
+  // triggered by an inherited EDITOR/GIT_ASKPASS/etc.
+  const wtGit = simpleGit(worktreePath, { config: authorConfig() })
 
   // Sync contentrain with base branch (bring main changes into contentrain)
   try {
@@ -454,8 +437,9 @@ export async function mergeBranch(
   const worktreePath = join(tmpdir(), `cr-merge-${randomUUID()}`)
   await git.raw(['worktree', 'add', worktreePath, CONTENTRAIN_BRANCH])
 
-  // Commit identity from the environment (see authorEnv) — no config spawns.
-  const wtGit = simpleGit(worktreePath).env(authorEnv())
+  // Commit identity from `-c user.*` config (see authorConfig) — guard-safe,
+  // no `.env()` spread.
+  const wtGit = simpleGit(worktreePath, { config: authorConfig() })
 
   try {
     // Merge the feature branch into contentrain

packages/mcp/tests/git/identity.test.ts

@@ -0,0 +1,96 @@
+import { afterEach, beforeEach, describe, expect, it } from 'vitest'
+import { mkdtemp, rm, writeFile } from 'node:fs/promises'
+import { tmpdir } from 'node:os'
+import { join } from 'node:path'
+import { simpleGit } from 'simple-git'
+import { authorConfig, NETWORK_UNSAFE } from '../../src/git/identity.js'
+
+/**
+ * Regression coverage for the simple-git block-unsafe guard.
+ *
+ * simple-git >= 3.34 (`@simple-git/argv-parser`) rejects a `git` invocation
+ * when a guard-listed variable (EDITOR, GIT_ASKPASS, …) is passed EXPLICITLY
+ * through `.env()`. The previous code spread `process.env` into `.env()` to set
+ * the commit author, which tripped the guard in any host (VS Code, Claude Code,
+ * CI) that exports those variables. These tests pin the fix: identity via
+ * `-c user.*` config, and network instances opt out of the guard categories.
+ */
+
+const TOUCHED = [
+  'EDITOR',
+  'GIT_ASKPASS',
+  'GIT_AUTHOR_NAME',
+  'GIT_AUTHOR_EMAIL',
+  'GIT_COMMITTER_NAME',
+  'GIT_COMMITTER_EMAIL',
+  'CONTENTRAIN_AUTHOR_NAME',
+  'CONTENTRAIN_AUTHOR_EMAIL',
+] as const
+
+let repo: string
+const saved: Record<string, string | undefined> = {}
+
+beforeEach(async () => {
+  for (const key of TOUCHED) saved[key] = process.env[key]
+
+  // Simulate a host that exports guard-tripping variables (the real-world bug).
+  process.env['EDITOR'] = 'vi'
+  process.env['GIT_ASKPASS'] = '/usr/bin/true'
+  // Identity must come from our `-c` config, not ambient GIT_AUTHOR_*/defaults.
+  for (const key of TOUCHED.slice(2)) delete process.env[key]
+
+  repo = await mkdtemp(join(tmpdir(), 'cr-identity-'))
+  const git = simpleGit(repo)
+  await git.init()
+  await writeFile(join(repo, 'a.txt'), 'hello\n')
+})
+
+afterEach(async () => {
+  for (const key of TOUCHED) {
+    if (saved[key] === undefined) delete process.env[key]
+    else process.env[key] = saved[key]
+  }
+  await rm(repo, { recursive: true, force: true })
+})
+
+describe('git identity — block-unsafe guard', () => {
+  it('commits via `-c user.*` config while EDITOR/GIT_ASKPASS are set (guard not tripped)', async () => {
+    const git = simpleGit(repo, { config: authorConfig() })
+    await git.add('.')
+    await git.commit('add a')
+
+    const author = (await git.raw(['log', '-1', '--format=%an <%ae>'])).trim()
+    const committer = (await git.raw(['log', '-1', '--format=%cn <%ce>'])).trim()
+    expect(author).toBe('Contentrain <ai@contentrain.io>')
+    // `user.*` config sets committer identity too, matching the old env behavior.
+    expect(committer).toBe('Contentrain <ai@contentrain.io>')
+  })
+
+  it('honors CONTENTRAIN_AUTHOR_* overrides through the config path', async () => {
+    process.env['CONTENTRAIN_AUTHOR_NAME'] = 'Ada'
+    process.env['CONTENTRAIN_AUTHOR_EMAIL'] = 'ada@example.com'
+    const git = simpleGit(repo, { config: authorConfig() })
+    await git.add('.')
+    await git.commit('add a')
+
+    const author = (await git.raw(['log', '-1', '--format=%an <%ae>'])).trim()
+    expect(author).toBe('Ada <ada@example.com>')
+  })
+
+  it('control: spreading process.env into `.env()` trips the guard (the original bug)', async () => {
+    const git = simpleGit(repo).env({ ...process.env })
+    await expect(git.raw(['status'])).rejects.toThrow(/not permitted/i)
+  })
+
+  it('networkGit-style instance runs despite inherited env via NETWORK_UNSAFE', async () => {
+    const git = simpleGit({ baseDir: repo, unsafe: NETWORK_UNSAFE })
+      .env({ ...process.env, GIT_TERMINAL_PROMPT: '0' })
+    const status = await git.raw(['status', '--porcelain'])
+    expect(status).toContain('a.txt')
+  })
+
+  it('control: the same inherited-env instance without NETWORK_UNSAFE trips the guard', async () => {
+    const git = simpleGit({ baseDir: repo }).env({ ...process.env, GIT_TERMINAL_PROMPT: '0' })
+    await expect(git.raw(['status'])).rejects.toThrow(/not permitted/i)
+  })
+})