[Bug] scrubbedParentEnv strips GIT_CONFIG_KEY_n but keeps COUNT/VALUE, so every git call in a child fails

[deniskern]

Version: @deepseek-ai/dsh@0.2.0-rc.2 (macOS, npm global install)

What happens

scrubbedParentEnv() in packages/subprocess/subprocess removes every env name matching /KEY|PASSWORD|SECRET|TOKEN/i. Git's indexed config protocol uses GIT_CONFIG_COUNT, GIT_CONFIG_KEY_<n> and GIT_CONFIG_VALUE_<n>. The scrub drops GIT_CONFIG_KEY_<n> (it contains KEY) but keeps GIT_CONFIG_COUNT and GIT_CONFIG_VALUE_<n>, so every git call in a dsh child fails:

error: missing config key GIT_CONFIG_KEY_0
fatal: unable to parse command-line config

This hits anyone whose parent process exports Git config through the environment (IDEs, agent hosts and CI runners do this, e.g. for safe.directory). The function's own doc comment promises that child CLIs run normally.

Repro (against the installed package)

// repro.mjs: node repro.mjs <path-to-any-git-repo>
import { scrubbedParentEnv } from '<global node_modules>/@deepseek-ai/dsh/node_modules/@deepseek-ai/dsh-subprocess/lib/index.js'
import { spawnSync } from 'node:child_process'
process.env.GIT_CONFIG_COUNT = '1'
process.env.GIT_CONFIG_KEY_0 = 'core.abbrev'
process.env.GIT_CONFIG_VALUE_0 = '12'
const env = scrubbedParentEnv()
console.log(Object.keys(env).filter((k) => k.startsWith('GIT_CONFIG')))
console.log(spawnSync('git', ['rev-parse', '--git-dir'], { env, encoding: 'utf8', cwd: process.argv[2] }).stderr)

Output:

[ 'GIT_CONFIG_COUNT', 'GIT_CONFIG_VALUE_0' ]
error: missing config key GIT_CONFIG_KEY_0
fatal: unable to parse command-line config

Suggested fix

Treat the Git config tuple as one unit. Either exempt ^GIT_CONFIG_(COUNT|KEY_\d+|VALUE_\d+)$ from the credential filter (the key names are Git config names, not secrets), or drop the whole tuple whenever any part of it is dropped. The same scrub is shared by dsh-subprocess-local, dsh-mcp-client, dsh-web-app, dsh-host-open-in-app and dsh-plugin-manager, so one fix there covers every spawner.

Our current workaround: we remove GIT_CONFIG_COUNT and every GIT_CONFIG_KEY_n/GIT_CONFIG_VALUE_n before launching dsh, so Git never sees half a tuple.

[argszero]

Confirmed — and the half tuple also forwards the value it was meant to protect

Reproduced from source and by experiment on master @ 5badb1500 (macOS, git 2.50.1). The mechanism is exactly as you describe; there is a second consequence of the same three lines worth adding to the report.

The classifier is name-only. packages/subprocess/subprocess/src/index.ts:

export const SENSITIVE_ENV_PATTERN = /KEY|PASSWORD|SECRET|TOKEN/i            // :47
...
if (value !== undefined && !SENSITIVE_ENV_PATTERN.test(key)
    && !key.toUpperCase().startsWith(DSH_ENV_PREFIX)) env[key] = value      // :69

GIT_CONFIG_KEY_0 matches KEY and is dropped; GIT_CONFIG_COUNT and GIT_CONFIG_VALUE_0 match nothing and survive. The one base is shared by every spawner — childEnv() in packages/subprocess/subprocess-local/src/spawn.ts (:14, used at :473, exported from subprocess-local/src/index.ts:30) — so this is not specific to one backend.

Reproduction. A parent carrying the ordinary GIT_CONFIG_COUNT=1 / GIT_CONFIG_KEY_0 / GIT_CONFIG_VALUE_0 trio, the predicate above copied verbatim, child runs git config --get core.harnessprobe:

child environment	exit	stderr
what scrubbedParentEnv() produces: COUNT + VALUE_0	128	error: missing config key GIT_CONFIG_KEY_0 / fatal: unable to parse command-line config
the whole GIT_CONFIG_* group removed	1 (plain "not set")	—
KEY_0 re-supplied through the spec's explicit env	0	the value reads back

Row 3 is the documented contract working as designed ("a deliberately supplied entry survives, because explicit env layers merge after the scrub"), and it is also the escape hatch for anyone carrying a local patch: re-supplying the missing KEY_n repairs the tuple without touching the scrub.

The second consequence: the value half still reaches the child. printenv GIT_CONFIG_VALUE_0 in a child prints it. That matters because VALUE_n is where the credential goes — this repository's own .github/review-ownership/blame-ownership.mjs builds exactly this trio with http.<server>/.extraheader = AUTHORIZATION: basic <token>. A parent that inherited that shape therefore both breaks every git call and hands the base64 credential to the child the scrub exists to protect. Same blind spot as #9285: the rule classifies names one at a time, and in a group-structured name family no member stands for the group.

Fix shape. Decide group-structured names atomically: when any member of a GIT_CONFIG_COUNT / GIT_CONFIG_KEY_n / GIT_CONFIG_VALUE_n group is rejected, drop the group whole. Dropping the group is also the safe direction here, since VALUE_n is the credential-bearing half (above). The narrower variant — keep COUNT/KEY_n, drop only VALUE_n — also parses, but it silently changes configuration the parent deliberately set; removing the group whole is the one that both parses and never forwards the value.

09ecc4f423 does not cover this. That merge ("isolate plugin-manager Git children from host command-line config", PR #5265) adjusts packages/boot/plugin-manager/tests/git-environment.ts — test-only isolation. scrubbedParentEnv itself is unchanged, and your failure is on the harness's own child path.

A fix has one home and no consumer to chase: the doc comment at :56 states the function is exported deliberately so that "spawners that cannot route through the service (node-pty backends, SDK-managed transports) share the one scrub definition".

[whutlichao]

Follow-up with a tested patch. @deniskern's diagnosis and @argszero's confirmation cover the
mechanism, including the second consequence (the surviving VALUE_n half is the credential-bearing
one), so this comment only adds the part that is still missing: a fix that can be applied, plus
tests and measurements.

Shape of the fix

Treat the family as a group. Skip it in the per-name loop, decide it once, and drop it whole
whenever it is incomplete or credential-bearing:

export const GIT_CONFIG_ENV_FAMILY = /^GIT_CONFIG_(?:COUNT|KEY_\d+|VALUE_\d+)$/u

/** A config value carrying a credential, in the shapes Git itself documents. */
const GIT_CONFIG_CREDENTIAL_VALUE =
  /AUTHORIZATION|BEARER|PRIVATE[_-]?KEY|PASSWORD|SECRET|TOKEN|API[_-]?KEY|:\/\/[^/\s]*:[^/\s]*@/iu

/** Config names that exist to carry an authorization header. */
const GIT_CONFIG_CREDENTIAL_NAME = /(?:^|\.)extraheader$/iu

export function gitConfigEnvFamily(source: NodeJS.ProcessEnv): Record<string, string> | null {
  if (!Object.keys(source).some((name) => GIT_CONFIG_ENV_FAMILY.test(name))) return null
  const count = source.GIT_CONFIG_COUNT
  if (count === undefined || !/^(?:0|[1-9]\d*)$/u.test(count)) return null
  const total = Number(count)
  if (!Number.isSafeInteger(total)) return null
  // Exactly one KEY and one VALUE per pair: a stray index makes Git reject the
  // whole set, so an unexpected member drops the group instead of forwarding it.
  const members = Object.keys(source).filter((name) => GIT_CONFIG_ENV_FAMILY.test(name))
  if (members.length !== total * 2 + 1) return null
  const forwarded: Record<string, string> = { GIT_CONFIG_COUNT: String(total) }
  for (let index = 0; index < total; index += 1) {
    const name = source[`GIT_CONFIG_KEY_${index}`]
    const value = source[`GIT_CONFIG_VALUE_${index}`]
    if (name === undefined || value === undefined) return null
    if (GIT_CONFIG_CREDENTIAL_NAME.test(name) || GIT_CONFIG_CREDENTIAL_VALUE.test(value)) return null
    forwarded[`GIT_CONFIG_KEY_${index}`] = name
    forwarded[`GIT_CONFIG_VALUE_${index}`] = value
  }
  return forwarded
}

In scrubbedParentEnv, the loop gains one exclusion and the decision lands after it:

if (value !== undefined && !GIT_CONFIG_ENV_FAMILY.test(key) && !SENSITIVE_ENV_PATTERN.test(key) && !key.toUpperCase().startsWith(DSH_ENV_PREFIX)) env[key] = value
…
const gitConfig = gitConfigEnvFamily(process.env)
if (gitConfig !== null) Object.assign(env, gitConfig)

Why drop the group rather than repair it

Measured against git 2.56.0 on macOS, real repository, child environment taken from
scrubbedParentEnv():

child environment	exit	outcome
COUNT + VALUE_n, no KEY_n (today)	128	missing config key GIT_CONFIG_KEY_0
group removed whole	1	ordinary "not set"; git runs normally
KEY_n re-supplied via the spec's explicit env	0	value reads back

Keeping COUNT/KEY_n and dropping only VALUE_n also parses, but silently changes configuration
the parent deliberately set. Dropping the group is the only option that both parses and never
forwards the value, and its cost is zero — the child behaves exactly as it did before any host
added the group.

Tradeoff worth stating: the value-side check is a heuristic too, so a legitimate value that
merely contains token or secret (a path like ~/.config/token-cache) drops the group. That
fails safe — git still runs, just without that setting — and the alternative (forwarding) is the
leak. COUNT=0 is forwarded as-is; a non-numeric COUNT, stray indices, and KEY/VALUE without
COUNT all drop the group, since git rejects each of those anyway (bogus count in GIT_CONFIG_COUNT, too many entries in GIT_CONFIG_COUNT) and forwarding them would only move the
failure into the child.

Verification

case	expected	observed
complete Orca pair (credential.interactive, credential.guiPrompt)	forwarded whole	git rev-parse --git-dir exit 0; git config --get credential.interactive → false
COUNT + values, no keys	group dropped	exit 1, no parse error
http.<url>.extraheader = AUTHORIZATION: basic …	group dropped	no GIT_CONFIG_* reaches the child
core.abbrev=12	forwarded	value reads back
COUNT=0	forwarded empty	accepted
COUNT=abc / stray index / keys without count	group dropped	git never sees them
DEEPSEEK_API_KEY	still scrubbed	unchanged

The existing scrubbedParentEnv drops credential-shaped and DSH_ names test passes unchanged
(replayed against the patched function: all six assertions hold), so the credential rule itself is
not weakened — the family is simply classified by group instead of by member.

Scope

Two files: src/index.ts (~45 lines with comments) and tests/service.spec.ts (three regression
tests: complete group forwarded whole, incomplete group dropped, credential value dropped). Nothing
else changes — SENSITIVE_ENV_PATTERN, the DSH_* rule and the proxy overlay are untouched.

I am running this locally as a patched node_modules, since CONTRIBUTING.md says external PRs
are not accepted at the moment. Offered as a tested starting point, not as a request for someone
else to do the work.

Patch — applies to master with patch -p1

--- a/packages/subprocess/subprocess/src/index.ts
+++ b/packages/subprocess/subprocess/src/index.ts
@@ -47,6 +47,63 @@
 export const SENSITIVE_ENV_PATTERN = /KEY|PASSWORD|SECRET|TOKEN/i
 
 /**
+ * One environment *group*: several names that only mean something together.
+ * `SENSITIVE_ENV_PATTERN` classifies names one at a time, which is right for
+ * `DEEPSEEK_API_KEY` and wrong for a family like Git's indexed config, where
+ * `KEY_<n>` names a setting rather than a secret while `VALUE_<n>` is where a
+ * credential can actually ride. Git's protocol is
+ * `GIT_CONFIG_COUNT=<n>` plus `GIT_CONFIG_KEY_<i>` / `GIT_CONFIG_VALUE_<i>` for
+ * each `i < n`; given a partial set it refuses to run at all:
+ *
+ *     error: missing config key GIT_CONFIG_KEY_0
+ *     fatal: unable to parse command-line config
+ *
+ * A host that exports a config pair through the environment (IDE, agent host,
+ * CI runner) therefore breaks every `git` call in every child, and the same
+ * blind spot forwards `http.<url>.extraheader` values — the documented carrier
+ * for `AUTHORIZATION: basic …` — to a child the scrub exists to protect.
+ * Groups are decided whole: forwarded only when complete and credential-free,
+ * otherwise dropped whole. Dropping is always safe — the child then runs with
+ * no extra `-c` settings, which is the behaviour before any host added them.
+ */
+export const GIT_CONFIG_ENV_FAMILY = /^GIT_CONFIG_(?:COUNT|KEY_\d+|VALUE_\d+)$/u
+
+/** A config value carrying a credential, in the shapes Git itself documents. */
+const GIT_CONFIG_CREDENTIAL_VALUE = /AUTHORIZATION|BEARER|PRIVATE[_-]?KEY|PASSWORD|SECRET|TOKEN|API[_-]?KEY|:\/\/[^/\s]*:[^/\s]*@/iu
+
+/** Config names that exist to carry an authorization header. */
+const GIT_CONFIG_CREDENTIAL_NAME = /(?:^|\.)extraheader$/iu
+
+/**
+ * Decide Git's config group for one environment, as a unit.
+ * @param source - the environment being scrubbed.
+ * @returns the complete group to forward, or `null` when it must be dropped whole.
+ */
+export function gitConfigEnvFamily(source: NodeJS.ProcessEnv): Record<string, string> | null {
+  if (!Object.keys(source).some((name) => GIT_CONFIG_ENV_FAMILY.test(name))) return null
+  const count = source.GIT_CONFIG_COUNT
+  // Git ignores KEY/VALUE without COUNT, and rejects a count it cannot parse
+  // (`bogus count in GIT_CONFIG_COUNT`) or one beyond its entry limit.
+  if (count === undefined || !/^(?:0|[1-9]\d*)$/u.test(count)) return null
+  const total = Number(count)
+  if (!Number.isSafeInteger(total)) return null
+  // Exactly one KEY and one VALUE per pair: a stray index makes Git reject the
+  // whole set, so an unexpected member drops the group instead of forwarding it.
+  const members = Object.keys(source).filter((name) => GIT_CONFIG_ENV_FAMILY.test(name))
+  if (members.length !== total * 2 + 1) return null
+  const forwarded: Record<string, string> = { GIT_CONFIG_COUNT: String(total) }
+  for (let index = 0; index < total; index += 1) {
+    const name = source[`GIT_CONFIG_KEY_${index}`]
+    const value = source[`GIT_CONFIG_VALUE_${index}`]
+    if (name === undefined || value === undefined) return null
+    if (GIT_CONFIG_CREDENTIAL_NAME.test(name) || GIT_CONFIG_CREDENTIAL_VALUE.test(value)) return null
+    forwarded[`GIT_CONFIG_KEY_${index}`] = name
+    forwarded[`GIT_CONFIG_VALUE_${index}`] = value
+  }
+  return forwarded
+}
+
+/**
  * The ambient parent environment minus credential-shaped names and minus all
  * `DSH_*` names — the canonical base every harness child starts from. `PATH`,
  * `HOME`, locale, and proxy variables survive, so child CLIs run normally;
@@ -61,13 +118,21 @@
  *
  * When a proxy is active the result also carries the resolved proxy names and the flag a child Node
  * needs to honor them, so a child inherits the same routing as its parent.
+ *
+ * Group-structured names are decided by {@link gitConfigEnvFamily} rather than
+ * name by name, so a half-copied group never reaches the child.
  * @returns a fresh environment object safe to hand to a child spawn.
  */
 export function scrubbedParentEnv(): Record<string, string> {
   const env: Record<string, string> = {}
   for (const [key, value] of Object.entries(process.env)) {
-    if (value !== undefined && !SENSITIVE_ENV_PATTERN.test(key) && !key.toUpperCase().startsWith(DSH_ENV_PREFIX)) env[key] = value
+    // The Git config family is skipped here and decided as one unit below: its
+    // `KEY_<n>` names would otherwise match `SENSITIVE_ENV_PATTERN` and be
+    // dropped while `COUNT`/`VALUE_<n>` survive, which is what breaks `git`.
+    if (value !== undefined && !GIT_CONFIG_ENV_FAMILY.test(key) && !SENSITIVE_ENV_PATTERN.test(key) && !key.toUpperCase().startsWith(DSH_ENV_PREFIX)) env[key] = value
   }
+  const gitConfig = gitConfigEnvFamily(process.env)
+  if (gitConfig !== null) Object.assign(env, gitConfig)
   // A child Node ignores the inherited proxy variables unless the flag this adds is set, so an MCP
   // stdio server or subagent CLI would connect directly while its parent proxies. The same overlay
   // restores each proxy name to what the user exported, undoing this process's own normalization —
--- a/packages/subprocess/subprocess/tests/service.spec.ts
+++ b/packages/subprocess/subprocess/tests/service.spec.ts
@@ -100,4 +100,60 @@
       delete process.env.SCRUB_PROBE_PLAIN
     }
   })
+
+  // A host that exports Git config through the environment (IDE, agent host, CI
+  // runner) writes a *group*: COUNT plus one KEY/VALUE pair per entry. The
+  // name-only heuristic dropped `KEY_<n>` (it matches KEY) and kept COUNT and
+  // VALUE_<n>, which makes git refuse to run at all:
+  //   error: missing config key GIT_CONFIG_KEY_0
+  //   fatal: unable to parse command-line config
+  it('scrubbedParentEnv forwards a complete Git config group, and forwards it whole', () => {
+    process.env.GIT_CONFIG_COUNT = '2'
+    process.env.GIT_CONFIG_KEY_0 = 'credential.interactive'
+    process.env.GIT_CONFIG_VALUE_0 = 'false'
+    process.env.GIT_CONFIG_KEY_1 = 'credential.guiPrompt'
+    process.env.GIT_CONFIG_VALUE_1 = 'false'
+    try {
+      const env = scrubbedParentEnv()
+      // Dropping KEY_n alone is the regression: COUNT and VALUE_n survive without it.
+      expect(env.GIT_CONFIG_KEY_0).toBe('credential.interactive')
+      expect(env.GIT_CONFIG_KEY_1).toBe('credential.guiPrompt')
+      expect(env.GIT_CONFIG_VALUE_0).toBe('false')
+      expect(env.GIT_CONFIG_COUNT).toBe('2')
+    } finally {
+      for (const name of Object.keys(process.env)) if (name.startsWith('GIT_CONFIG_')) delete process.env[name]
+    }
+  })
+
+  it('scrubbedParentEnv drops an incomplete Git config group instead of forwarding half of it', () => {
+    // COUNT promises two pairs; only the values are present, so the keys were lost.
+    process.env.GIT_CONFIG_COUNT = '2'
+    process.env.GIT_CONFIG_VALUE_0 = 'false'
+    process.env.GIT_CONFIG_VALUE_1 = 'false'
+    try {
+      const env = scrubbedParentEnv()
+      expect(env.GIT_CONFIG_COUNT).toBeUndefined()
+      expect(env.GIT_CONFIG_VALUE_0).toBeUndefined()
+      expect(env.GIT_CONFIG_VALUE_1).toBeUndefined()
+    } finally {
+      for (const name of Object.keys(process.env)) if (name.startsWith('GIT_CONFIG_')) delete process.env[name]
+    }
+  })
+
+  // The name-only heuristic cannot see this: the config name is ordinary and the
+  // credential rides in the value, in the shape Git itself documents for
+  // `AUTHORIZATION: basic …`. Forwarding it would hand the secret to the child.
+  it('scrubbedParentEnv drops a Git config group whose value carries a credential', () => {
+    process.env.GIT_CONFIG_COUNT = '1'
+    process.env.GIT_CONFIG_KEY_0 = 'http.https://example.test/.extraheader'
+    process.env.GIT_CONFIG_VALUE_0 = 'AUTHORIZATION: basic dXNlcjpwYXNzd29yZA=='
+    try {
+      const env = scrubbedParentEnv()
+      expect(env.GIT_CONFIG_COUNT).toBeUndefined()
+      expect(env.GIT_CONFIG_KEY_0).toBeUndefined()
+      expect(env.GIT_CONFIG_VALUE_0).toBeUndefined()
+    } finally {
+      for (const name of Object.keys(process.env)) if (name.startsWith('GIT_CONFIG_')) delete process.env[name]
+    }
+  })
 })

[danielchanintereras]

Still reproducible on @deepseek-ai/dsh@0.2.1-rc.2 / @deepseek-ai/dsh-subprocess@0.2.1-rc.2 (macOS, npm global install).

Real-world trigger for us: the Orca agent launcher injects GIT_CONFIG_COUNT=2 with credential.interactive=false and credential.guiPrompt=false into every agent terminal, so every git call inside a DSH session dies with missing config key GIT_CONFIG_KEY_0 — DSH can't commit from a git worktree at all.

We're carrying a local patch that exempts only ^GIT_CONFIG_KEY_\d+$ from the scrub (other KEY/TOKEN/SECRET/PASSWORD names still scrubbed); after it, git works in fresh DSH sessions under the Orca environment. +1 for a fix upstream — the tuple-aware approach in the previous comment looks right to us.