fix: ${VAR} env-var interpolation for configs (closes #635) (#655)

anandgupta42 · web-flow · commit b724e1c2e7bd · 2026-04-04T20:59:44.000-07:00
* fix: [#635] accept ${VAR} shell/dotenv syntax for env interpolation Config substitution previously only accepted {env:VAR}. Users arriving from Claude Code, VS Code, dotenv, or docker-compose naturally write ${VAR} and hit silent failures — the literal string passes through to MCP servers as the env value, shadowing the forwarded parent env. This adds ${VAR} as an alias for {env:VAR}. Regex matches POSIX identifier names only ([A-Za-z_][A-Za-z0-9_]*) to avoid collisions with random ${...} content in URLs or paths. Bare $VAR (without braces) is intentionally NOT interpolated — too collision-prone. - paths.ts: add second regex replace after the existing {env:VAR} pass - paths-parsetext.test.ts: 6 new tests covering shell syntax, mixed use, invalid identifier names, bare $VAR rejection, and MCP env regression scenario - mcp-servers.md: document both syntaxes with a table Closes #635 * fix: address consensus review findings for PR #655 Applies fixes from multi-model consensus review (Claude + GPT 5.4 + Gemini 3.1 Pro). - C1 (CRITICAL — JSON injection): ${VAR} substitution is now JSON-safe via JSON.stringify(value).slice(1, -1). Env values with quotes/commas/newlines can no longer break out of the enclosing JSON string. Existing {env:VAR} keeps raw-injection semantics for backward compat (documented as the opt-in power-user syntax). - M2 (MAJOR — escape hatch): $${VAR} now preserves a literal ${VAR} in the output (docker-compose convention). Negative lookbehind in the match regex prevents substitution when preceded by $. - m3 (MINOR — documentation): docs expanded with a 3-row syntax comparison table explaining string-safe vs raw-injection modes. - m4 (MINOR — tests): added 3 edge-case tests covering JSON injection attempt, multiline/backslash values, and the new $${VAR} escape hatch. - n5 (NIT — stale tip): TUI tip at tips.tsx:150 now mentions both ${VAR} and {env:VAR} syntaxes. Deferred (larger refactor, scope discipline): - M1 (comment handling): substitutions still run inside // comments. Same pre-existing behavior for {env:VAR}. Would require unified substitution pass. Can be a follow-up PR. * feat: add ${VAR:-default} syntax for fallback values Extends the ${VAR} substitution to support POSIX/docker-compose-style defaults. Matches user expectations from dotenv, docker-compose, and shell: default value is used when the variable is unset OR empty (matching :- semantics rather than bare -). - ${VAR:-default} uses 'default' when VAR is unset or empty string - ${VAR:-} (empty default) resolves to empty string - Defaults with spaces/special chars supported (${VAR:-Hello World}) - Default values are JSON-escaped (same security properties as ${VAR}) - $${VAR:-default} escape hatch works for literal preservation Per research, ${VAR:-default} is the de facto standard across docker-compose, dotenv, POSIX shell, GitHub Actions, and Terraform — users arrive from these tools expecting this syntax. Added 7 tests covering unset/set/empty cases, empty default, spaces, JSON-injection attempt, and escape hatch. * feat: add config_env_interpolation telemetry event Collect signals on env-var interpolation usage to detect footguns and guide future improvements. Tracks per config-load: - dollar_refs: count of ${VAR} / ${VAR:-default} references - dollar_unresolved: ${VAR} with no value and no default → empty string (signal: users writing fragile configs without defaults) - dollar_defaulted: ${VAR:-default} where the fallback was used (signal: whether defaults syntax is actually being used) - dollar_escaped: $${VAR} literal escapes used - legacy_brace_refs: {env:VAR} references (raw-injection syntax) - legacy_brace_unresolved: {env:VAR} with no value Emitted via dynamic import to avoid circular dep with @/altimate/telemetry (which imports @/config/config). Event fires only when interpolation actually happens, so no-env-ref configs don't generate noise. What this lets us answer after shipping: - How many users hit 'unresolved' unintentionally? → consider failing loud - Is ${VAR:-default} getting adopted? → iterate on docs if not - Are users still writing the legacy {env:VAR}? → plan deprecation - Is the $${VAR} escape rare? → simplify docs if so Adds event type to ALL_EVENT_TYPES completeness list (43 → 44). * fix: address P1 double-substitution from cubic/coderabbit review Both bots flagged a backward-compat regression: ${VAR} pass runs after {env:VAR} pass, so an env value containing literal ${X} got expanded in the second pass. Reordering only fixed one direction (reverse cascade still possible when ${VAR}'s output contains {env:Y}). Correct fix: single-pass substitution with one regex alternation that evaluates all three patterns against the ORIGINAL text. Output of any pattern cannot be re-matched by another. Single regex handles: $${VAR} or $${VAR:-default} → literal escape (?<!$)${VAR}[:-default] → JSON-safe substitution {env:VAR} → raw text injection Regression tests added for both cascade directions: - env.A="${B}" + {env:A} → literal ${B} stays - env.A="{env:B}" + ${A} → literal {env:B} stays All 34 tests pass. Typecheck + marker guard clean.
diff --git a/docs/docs/configure/mcp-servers.md b/docs/docs/configure/mcp-servers.md
@@ -20,6 +20,21 @@ Run an MCP server as a local subprocess:
 }
 ```
 
+### Environment variable interpolation
+
+Both syntaxes work anywhere in the config:
+
+| Syntax | Injection mode | Example |
+|--------|----------------|---------|
+| `${VAR}` | String-safe (JSON-escaped) | `"API_KEY": "${MY_API_KEY}"` — shell / dotenv style |
+| `${VAR:-default}` | String-safe with fallback | `"MODE": "${APP_MODE:-production}"` — used when `VAR` is unset or empty |
+| `{env:VAR}` | Raw text | `"count": {env:NUM}` — use for unquoted structural injection |
+| `$${VAR}` | Escape hatch | `"template": "$${VAR}"` — preserves literal `${VAR}` (docker-compose style) |
+
+If the variable is not set and no default is given, it resolves to an empty string. Bare `$VAR` (without braces) is **not** interpolated — use `${VAR}` or `{env:VAR}`.
+
+**Why two syntaxes?** `${VAR}` JSON-escapes the value so tokens containing quotes or braces can't break the config structure — the safe default for secrets. `{env:VAR}` does raw text injection for the rare case where you need to inject numbers or structure into unquoted JSON positions.
+
 | Field | Type | Description |
 |-------|------|-------------|
 | `type` | `"local"` | Local subprocess server |
diff --git a/packages/opencode/src/altimate/telemetry/index.ts b/packages/opencode/src/altimate/telemetry/index.ts
@@ -659,6 +659,25 @@ export namespace Telemetry {
         error_message?: string
       }
     // altimate_change end
+    // altimate_change start — config env-var interpolation telemetry
+    | {
+        type: "config_env_interpolation"
+        timestamp: number
+        session_id: string
+        /** ${VAR} / ${VAR:-default} references encountered */
+        dollar_refs: number
+        /** ${VAR} with no value and no default → resolved to empty string (footgun signal) */
+        dollar_unresolved: number
+        /** ${VAR:-default} where default was used */
+        dollar_defaulted: number
+        /** $${VAR} literal escape sequences found */
+        dollar_escaped: number
+        /** legacy {env:VAR} references (raw injection syntax) */
+        legacy_brace_refs: number
+        /** {env:VAR} with no value → empty string */
+        legacy_brace_unresolved: number
+      }
+    // altimate_change end
     // altimate_change start — plan-agent model tool-call refusal detection
     | {
         type: "plan_no_tool_generation"
diff --git a/packages/opencode/src/cli/cmd/tui/component/tips.tsx b/packages/opencode/src/cli/cmd/tui/component/tips.tsx
@@ -147,7 +147,7 @@ const TIPS = [
   "Create JSON theme files in {highlight}.altimate-code/themes/{/highlight} directory",
   "Themes support dark/light variants for both modes",
   "Reference ANSI colors 0-255 in custom themes",
-  "Use {highlight}{env:VAR_NAME}{/highlight} syntax to reference environment variables in config",
+  "Use {highlight}${VAR_NAME}{/highlight} or {highlight}{env:VAR_NAME}{/highlight} to reference environment variables in config",
   "Use {highlight}{file:path}{/highlight} to include file contents in config values",
   "Use {highlight}instructions{/highlight} in config to load additional rules files",
   "Set agent {highlight}temperature{/highlight} from 0.0 (focused) to 1.0 (creative)",
diff --git a/packages/opencode/src/config/paths.ts b/packages/opencode/src/config/paths.ts
@@ -86,9 +86,69 @@ export namespace ConfigPaths {
 
   /** Apply {env:VAR} and {file:path} substitutions to config text. */
   async function substitute(text: string, input: ParseSource, missing: "error" | "empty" = "error") {
-    text = text.replace(/\{env:([^}]+)\}/g, (_, varName) => {
-      return process.env[varName] || ""
-    })
+    // altimate_change start — unified env-var interpolation
+    // Single-pass substitution against the ORIGINAL text prevents output of one
+    // pattern being re-matched by another (e.g. {env:A}="${B}" expanding B).
+    // Syntaxes (order tried, in one regex via alternation):
+    //   1. $${VAR} or $${VAR:-default} — literal escape (docker-compose style)
+    //   2. ${VAR} or ${VAR:-default}   — string-safe, JSON-escaped (shell/dotenv)
+    //   3. {env:VAR}                    — raw text injection (backward compat)
+    // Users arriving from Claude Code / VS Code / dotenv / docker-compose expect
+    // ${VAR}. Use {env:VAR} for raw unquoted injection. See issue #635.
+    let dollarRefs = 0
+    let dollarUnresolved = 0
+    let dollarDefaulted = 0
+    let dollarEscaped = 0
+    let legacyBraceRefs = 0
+    let legacyBraceUnresolved = 0
+    text = text.replace(
+      /\$\$(\{[A-Za-z_][A-Za-z0-9_]*(?::-[^}]*)?\})|(?<!\$)\$\{([A-Za-z_][A-Za-z0-9_]*)(?::-([^}]*))?\}|\{env:([^}]+)\}/g,
+      (match, escaped, dollarVar, dollarDefault, braceVar) => {
+        if (escaped !== undefined) {
+          // $${VAR} → literal ${VAR}
+          dollarEscaped++
+          return "$" + escaped
+        }
+        if (dollarVar !== undefined) {
+          // ${VAR} / ${VAR:-default} → JSON-escaped string-safe substitution
+          dollarRefs++
+          const envValue = process.env[dollarVar]
+          const resolved = envValue !== undefined && envValue !== ""
+          if (!resolved && dollarDefault !== undefined) dollarDefaulted++
+          if (!resolved && dollarDefault === undefined) dollarUnresolved++
+          const value = resolved ? envValue : (dollarDefault ?? "")
+          return JSON.stringify(value).slice(1, -1)
+        }
+        if (braceVar !== undefined) {
+          // {env:VAR} → raw text injection
+          legacyBraceRefs++
+          const v = process.env[braceVar]
+          if (v === undefined || v === "") legacyBraceUnresolved++
+          return v || ""
+        }
+        return match
+      },
+    )
+    // Emit telemetry if any env interpolation happened. Dynamic import avoids a
+    // circular dep with @/altimate/telemetry (which imports @/config/config).
+    if (dollarRefs > 0 || legacyBraceRefs > 0 || dollarEscaped > 0) {
+      import("@/altimate/telemetry")
+        .then(({ Telemetry }) => {
+          Telemetry.track({
+            type: "config_env_interpolation",
+            timestamp: Date.now(),
+            session_id: Telemetry.getContext().sessionId,
+            dollar_refs: dollarRefs,
+            dollar_unresolved: dollarUnresolved,
+            dollar_defaulted: dollarDefaulted,
+            dollar_escaped: dollarEscaped,
+            legacy_brace_refs: legacyBraceRefs,
+            legacy_brace_unresolved: legacyBraceUnresolved,
+          })
+        })
+        .catch(() => {})
+    }
+    // altimate_change end
 
     const fileMatches = Array.from(text.matchAll(/\{file:[^}]+\}/g))
     if (!fileMatches.length) return text
diff --git a/packages/opencode/test/config/paths-parsetext.test.ts b/packages/opencode/test/config/paths-parsetext.test.ts
@@ -97,6 +97,210 @@ describe("ConfigPaths.parseText: {env:VAR} substitution", () => {
   })
 })
 
+describe("ConfigPaths.parseText: ${VAR} substitution (shell/dotenv alias)", () => {
+  const envKey = "OPENCODE_TEST_SHELL_SYNTAX_KEY"
+
+  beforeEach(() => {
+    process.env[envKey] = "shell-style-value"
+  })
+
+  afterEach(() => {
+    delete process.env[envKey]
+  })
+
+  test("substitutes ${VAR} with environment variable value", async () => {
+    const text = `{"apiKey": "\${${envKey}}"}`
+    const result = await ConfigPaths.parseText(text, "/fake/config.json")
+    expect(result).toEqual({ apiKey: "shell-style-value" })
+  })
+
+  test("substitutes to empty string when env var is not set", async () => {
+    const text = '{"apiKey": "${OPENCODE_TEST_SHELL_NONEXISTENT_XYZ}"}'
+    const result = await ConfigPaths.parseText(text, "/fake/config.json")
+    expect(result).toEqual({ apiKey: "" })
+  })
+
+  test("${VAR} and {env:VAR} both work in same config", async () => {
+    process.env.OPENCODE_TEST_MIXED_A = "alpha"
+    process.env.OPENCODE_TEST_MIXED_B = "beta"
+    try {
+      const text = '{"a": "${OPENCODE_TEST_MIXED_A}", "b": "{env:OPENCODE_TEST_MIXED_B}"}'
+      const result = await ConfigPaths.parseText(text, "/fake/config.json")
+      expect(result).toEqual({ a: "alpha", b: "beta" })
+    } finally {
+      delete process.env.OPENCODE_TEST_MIXED_A
+      delete process.env.OPENCODE_TEST_MIXED_B
+    }
+  })
+
+  test("ignores ${...} with non-identifier names (spaces, special chars)", async () => {
+    // These should pass through unmodified — not valid POSIX identifiers
+    const text = '{"a": "${FOO BAR}", "b": "${foo-bar}", "c": "${foo.bar}"}'
+    const result = await ConfigPaths.parseText(text, "/fake/config.json")
+    expect(result).toEqual({ a: "${FOO BAR}", b: "${foo-bar}", c: "${foo.bar}" })
+  })
+
+  test("does not match bare $VAR (without braces)", async () => {
+    process.env.OPENCODE_TEST_BARE = "should-not-match"
+    try {
+      const text = '{"value": "$OPENCODE_TEST_BARE"}'
+      const result = await ConfigPaths.parseText(text, "/fake/config.json")
+      // Bare $VAR stays literal — only ${VAR} is interpolated
+      expect(result).toEqual({ value: "$OPENCODE_TEST_BARE" })
+    } finally {
+      delete process.env.OPENCODE_TEST_BARE
+    }
+  })
+
+  test("JSON-safe: env value with quotes cannot inject JSON structure", async () => {
+    // Security regression test for C1 in consensus review of PR #655.
+    // {env:VAR} is raw injection (backward compat); ${VAR} is string-safe.
+    process.env.OPENCODE_TEST_INJECT = 'pwned", "isAdmin": true, "x": "y'
+    try {
+      const text = '{"token": "${OPENCODE_TEST_INJECT}"}'
+      const result = await ConfigPaths.parseText(text, "/fake/config.json")
+      // Value stays inside the "token" string — no injection into sibling keys
+      expect(result).toEqual({ token: 'pwned", "isAdmin": true, "x": "y' })
+      expect(result.isAdmin).toBeUndefined()
+    } finally {
+      delete process.env.OPENCODE_TEST_INJECT
+    }
+  })
+
+  test("JSON-safe: env value with backslash and newline escaped properly", async () => {
+    process.env.OPENCODE_TEST_MULTILINE = 'line1\nline2\tpath\\to\\file'
+    try {
+      const text = '{"value": "${OPENCODE_TEST_MULTILINE}"}'
+      const result = await ConfigPaths.parseText(text, "/fake/config.json")
+      expect(result).toEqual({ value: "line1\nline2\tpath\\to\\file" })
+    } finally {
+      delete process.env.OPENCODE_TEST_MULTILINE
+    }
+  })
+
+  test("default: ${VAR:-default} uses default when var unset", async () => {
+    // Variable is not set — default value should be used
+    const text = '{"mode": "${OPENCODE_TEST_UNSET_VAR:-production}"}'
+    const result = await ConfigPaths.parseText(text, "/fake/config.json")
+    expect(result).toEqual({ mode: "production" })
+  })
+
+  test("default: ${VAR:-default} uses env value when var set", async () => {
+    process.env.OPENCODE_TEST_DEFAULT_OVERRIDE = "staging"
+    try {
+      const text = '{"mode": "${OPENCODE_TEST_DEFAULT_OVERRIDE:-production}"}'
+      const result = await ConfigPaths.parseText(text, "/fake/config.json")
+      expect(result).toEqual({ mode: "staging" })
+    } finally {
+      delete process.env.OPENCODE_TEST_DEFAULT_OVERRIDE
+    }
+  })
+
+  test("default: ${VAR:-default} uses default when var is empty string", async () => {
+    // POSIX :- uses default for both unset AND empty (matches docker-compose)
+    process.env.OPENCODE_TEST_EMPTY_VAR = ""
+    try {
+      const text = '{"mode": "${OPENCODE_TEST_EMPTY_VAR:-fallback}"}'
+      const result = await ConfigPaths.parseText(text, "/fake/config.json")
+      expect(result).toEqual({ mode: "fallback" })
+    } finally {
+      delete process.env.OPENCODE_TEST_EMPTY_VAR
+    }
+  })
+
+  test("default: empty default ${VAR:-} resolves to empty string", async () => {
+    const text = '{"value": "${OPENCODE_TEST_EMPTY_DEFAULT:-}"}'
+    const result = await ConfigPaths.parseText(text, "/fake/config.json")
+    expect(result).toEqual({ value: "" })
+  })
+
+  test("default: default value with spaces and special chars", async () => {
+    const text = '{"msg": "${OPENCODE_TEST_MISSING:-Hello World 123}"}'
+    const result = await ConfigPaths.parseText(text, "/fake/config.json")
+    expect(result).toEqual({ msg: "Hello World 123" })
+  })
+
+  test("default: default value is JSON-escaped (security)", async () => {
+    const text = '{"token": "${OPENCODE_TEST_MISSING:-pwned\\", \\"isAdmin\\": true, \\"x\\": \\"y}"}'
+    const result = await ConfigPaths.parseText(text, "/fake/config.json")
+    expect(result.token).toContain("pwned")
+    expect(result.isAdmin).toBeUndefined()
+  })
+
+  test("escape hatch: $${VAR:-default} stays literal", async () => {
+    process.env.OPENCODE_TEST_ESCAPED_DEFAULT = "should-not-be-used"
+    try {
+      const text = '{"template": "$${OPENCODE_TEST_ESCAPED_DEFAULT:-my-default}"}'
+      const result = await ConfigPaths.parseText(text, "/fake/config.json")
+      expect(result).toEqual({ template: "${OPENCODE_TEST_ESCAPED_DEFAULT:-my-default}" })
+    } finally {
+      delete process.env.OPENCODE_TEST_ESCAPED_DEFAULT
+    }
+  })
+
+  test("escape hatch: $${VAR} stays literal (docker-compose convention)", async () => {
+    process.env.OPENCODE_TEST_SHOULD_NOT_SUB = "interpolated"
+    try {
+      const text = '{"template": "$${OPENCODE_TEST_SHOULD_NOT_SUB}"}'
+      const result = await ConfigPaths.parseText(text, "/fake/config.json")
+      // $${VAR} → literal ${VAR}, env value is NOT substituted
+      expect(result).toEqual({ template: "${OPENCODE_TEST_SHOULD_NOT_SUB}" })
+    } finally {
+      delete process.env.OPENCODE_TEST_SHOULD_NOT_SUB
+    }
+  })
+
+  test("single-pass: {env:A} value containing ${B} stays literal (no cascade)", async () => {
+    // Regression test for cubic/coderabbit P1: previously the {env:VAR} pass ran
+    // first, then the ${VAR} pass expanded any ${...} in its output. Single-pass
+    // substitution evaluates both patterns against the ORIGINAL text only.
+    process.env.OPENCODE_TEST_CASCADE_A = "${OPENCODE_TEST_CASCADE_B}"
+    process.env.OPENCODE_TEST_CASCADE_B = "should-not-expand"
+    try {
+      const text = '{"value": "{env:OPENCODE_TEST_CASCADE_A}"}'
+      const result = await ConfigPaths.parseText(text, "/fake/config.json")
+      // {env:VAR} is raw injection — its output is NOT re-interpolated
+      expect(result.value).toBe("${OPENCODE_TEST_CASCADE_B}")
+    } finally {
+      delete process.env.OPENCODE_TEST_CASCADE_A
+      delete process.env.OPENCODE_TEST_CASCADE_B
+    }
+  })
+
+  test("single-pass: ${A} value containing {env:B} stays literal (no cascade)", async () => {
+    // Reverse direction: ${VAR} output must not be matched by {env:VAR} pass.
+    process.env.OPENCODE_TEST_CASCADE_C = "{env:OPENCODE_TEST_CASCADE_D}"
+    process.env.OPENCODE_TEST_CASCADE_D = "should-not-expand"
+    try {
+      const text = '{"value": "${OPENCODE_TEST_CASCADE_C}"}'
+      const result = await ConfigPaths.parseText(text, "/fake/config.json")
+      expect(result.value).toBe("{env:OPENCODE_TEST_CASCADE_D}")
+    } finally {
+      delete process.env.OPENCODE_TEST_CASCADE_C
+      delete process.env.OPENCODE_TEST_CASCADE_D
+    }
+  })
+
+  test("works inside MCP environment config (issue #635 regression)", async () => {
+    process.env.OPENCODE_TEST_GITLAB_TOKEN = "glpat-xxxxx"
+    try {
+      const text = `{
+        "mcp": {
+          "gitlab": {
+            "type": "local",
+            "command": ["npx", "-y", "@modelcontextprotocol/server-gitlab"],
+            "environment": { "GITLAB_TOKEN": "\${OPENCODE_TEST_GITLAB_TOKEN}" }
+          }
+        }
+      }`
+      const result = await ConfigPaths.parseText(text, "/fake/config.json")
+      expect(result.mcp.gitlab.environment.GITLAB_TOKEN).toBe("glpat-xxxxx")
+    } finally {
+      delete process.env.OPENCODE_TEST_GITLAB_TOKEN
+    }
+  })
+})
+
 describe("ConfigPaths.parseText: {file:path} substitution", () => {
   test("substitutes {file:path} with file contents (trimmed)", async () => {
     await using tmp = await tmpdir()
diff --git a/packages/opencode/test/telemetry/telemetry.test.ts b/packages/opencode/test/telemetry/telemetry.test.ts
@@ -246,11 +246,12 @@ const ALL_EVENT_TYPES: Telemetry.Event["type"][] = [
   "feature_suggestion",
   "core_failure",
   "sql_pre_validation",
+  "config_env_interpolation",
 ]
 
 describe("telemetry.event-types", () => {
   test("all event types are valid", () => {
-    expect(ALL_EVENT_TYPES.length).toBe(43)
+    expect(ALL_EVENT_TYPES.length).toBe(44)
   })
 })
 
